[PATCH 0/1] Fix XhciDxe Timeouts

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH 0/1] Fix XhciDxe Timeouts
@ 2020-09-01 18:55 patrick.henz
  2020-09-01 18:55 ` [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts patrick.henz
  2020-09-02  5:08 ` [PATCH 0/1] Fix XhciDxe Timeouts Wu, Hao A
  0 siblings, 2 replies; 10+ messages in thread
From: patrick.henz @ 2020-09-01 18:55 UTC (permalink / raw)
  To: devel; +Cc: henz, Jian J Wang, Hao A Wu, Ray Ni

From: henz <patrick.henz@hpe.com>

Timeouts in the XhciDxe driver are taking longer than
expected due to the timeout loops not accounting for
code execution time. As en example, 5 second timeouts
have been observed to take around 36 seconds to complete.
Use SetTimer and Create/CheckEvent from Boot Services to
determine when timeout occurred. This patch was tested
using forced timeouts and print statements with QEmu as
well as phycial hardware. The forced timeouts were
implemented in code via static variables that guaranteed
a timeout the first time the function with the broken
timeout was called.

Example:

XhcExecTransfer (
  .
  .
 )
{
  .
  .
  static int do_once = 1;  // test line
  .
  .
  do {
    Finished = XhcCheckUrbResult (Xhc, Urb);
    if (do_once) Finished = 0; // test line
    if (Finished) {
      break;
    }
    gBS->Stall (XHC_1_MICROSECOND);
  } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));

  do_once = 0; // test line

Using this forced timeout approach the correct timeouts
were observed on both hardware and in QEmu.

Similar broken timeout loops have been found in the Uhci
and Ehci drivers. This patch does not fix those issues.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Patrick Henz <patrick.henz@hpe.com>

Patrick Henz (1):
  MdeModulePkg/XhciDxe: Fix Broken Timeouts

 MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 28 ++++++++++++++++---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 34 +++++++++++++++++-------
 2 files changed, 49 insertions(+), 13 deletions(-)

-- 
2.27.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
  2020-09-01 18:55 [PATCH 0/1] Fix XhciDxe Timeouts patrick.henz
@ 2020-09-01 18:55 ` patrick.henz
  2020-09-02  3:21   ` Ni, Ray
  2020-09-02  6:45   ` [edk2-devel] " Laszlo Ersek
  2020-09-02  5:08 ` [PATCH 0/1] Fix XhciDxe Timeouts Wu, Hao A
  1 sibling, 2 replies; 10+ messages in thread
From: patrick.henz @ 2020-09-01 18:55 UTC (permalink / raw)
  To: devel; +Cc: henz, Jian J Wang, Hao A Wu, Ray Ni

From: henz <patrick.henz@hpe.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2948

Timeouts in the XhciDxe driver are taking longer than
expected due to the timeout loops not accounting for
code execution time. As en example, 5 second timeouts
have been observed to take around 36 seconds to complete.
Use SetTimer and Create/CheckEvent from Boot Services to
determine when timeout occurred.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 28 ++++++++++++++++---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 34 +++++++++++++++++-------
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
index 42b773ab31..5f7507f0a5 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
@@ -442,17 +442,37 @@ XhcWaitOpRegBit (
   IN UINT32               Timeout
   )
 {
-  UINT32                  Index;
-  UINT64                  Loop;
+  EFI_STATUS Status;
+  EFI_EVENT  TimeoutEvent;
 
-  Loop   = Timeout * XHC_1_MILLISECOND;
+  if (Timeout == 0) {
+    return EFI_TIMEOUT;
+  }
+
+  Status = gBS->CreateEvent (
+                  EVT_TIMER,
+                  TPL_CALLBACK,
+                  NULL,
+                  NULL,
+                  &TimeoutEvent
+                  );
 
-  for (Index = 0; Index < Loop; Index++) {
+  if (!EFI_ERROR (Status)) {
+    Status = gBS->SetTimer (TimeoutEvent,
+                            TimerRelative,
+                            EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
+  }
+
+  do {
     if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
       return EFI_SUCCESS;
     }
 
     gBS->Stall (XHC_1_MICROSECOND);
+  } while (!EFI_ERROR(Status) && EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
+
+  if (TimeoutEvent != NULL) {
+    gBS->CloseEvent (TimeoutEvent);
   }
 
   return EFI_TIMEOUT;
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index ab8957c546..cf271f0493 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -1273,11 +1273,11 @@ XhcExecTransfer (
   )
 {
   EFI_STATUS              Status;
-  UINTN                   Index;
-  UINT64                  Loop;
   UINT8                   SlotId;
   UINT8                   Dci;
   BOOLEAN                 Finished;
+  EFI_EVENT               TimeoutEvent;
+  EFI_STATUS              TimerStatus;
 
   if (CmdTransfer) {
     SlotId = 0;
@@ -1292,28 +1292,44 @@ XhcExecTransfer (
   }
 
   Status = EFI_SUCCESS;
-  Loop   = Timeout * XHC_1_MILLISECOND;
-  if (Timeout == 0) {
-    Loop = 0xFFFFFFFF;
-  }
+
+  TimerStatus = gBS->CreateEvent (
+                       EVT_TIMER,
+                       TPL_CALLBACK,
+                       NULL,
+                       NULL,
+                       &TimeoutEvent
+                       );
 
   XhcRingDoorBell (Xhc, SlotId, Dci);
 
-  for (Index = 0; Index < Loop; Index++) {
+  if (!EFI_ERROR (TimerStatus)) {
+    TimerStatus = gBS->SetTimer (TimeoutEvent,
+                                 TimerRelative,
+                                 (0 == Timeout)?
+                                 (EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):
+                                 (EFI_TIMER_PERIOD_MILLISECONDS(Timeout)));
+  }
+
+  do {
     Finished = XhcCheckUrbResult (Xhc, Urb);
     if (Finished) {
       break;
     }
     gBS->Stall (XHC_1_MICROSECOND);
-  }
+  } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
 
-  if (Index == Loop) {
+  if (!Finished) {
     Urb->Result = EFI_USB_ERR_TIMEOUT;
     Status      = EFI_TIMEOUT;
   } else if (Urb->Result != EFI_USB_NOERROR) {
     Status      = EFI_DEVICE_ERROR;
   }
 
+  if (TimeoutEvent != NULL) {
+    gBS->CloseEvent (TimeoutEvent);
+  }
+
   return Status;
 }
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
  2020-09-01 18:55 ` [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts patrick.henz
@ 2020-09-02  3:21   ` Ni, Ray
  2020-09-02  6:45   ` [edk2-devel] " Laszlo Ersek
  1 sibling, 0 replies; 10+ messages in thread
From: Ni, Ray @ 2020-09-02  3:21 UTC (permalink / raw)
  To: patrick.henz@hpe.com, devel@edk2.groups.io; +Cc: Wang, Jian J, Wu, Hao A

In general, thanks for enhancing the logic to take the cmd execution duration into the timeout.
2 minor comments:

> 
> +                                 (EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):
1. Can you enhance the patch to avoid creating the timer event the Timeout is 0.
> 
> +  } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> 
2. Can you please move the !EFI_ERROR (TimerStatus) check from the while() to outside of while()?
Because this status value never changes in while loop.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
  2020-09-01 18:55 ` [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts patrick.henz
  2020-09-02  3:21   ` Ni, Ray
@ 2020-09-02  6:45   ` Laszlo Ersek
  1 sibling, 0 replies; 10+ messages in thread
From: Laszlo Ersek @ 2020-09-02  6:45 UTC (permalink / raw)
  To: devel, patrick.henz; +Cc: Jian J Wang, Hao A Wu, Ray Ni

Hi Patrick,

On 09/01/20 20:55, patrick.henz@hpe.com wrote:
> From: henz <patrick.henz@hpe.com>

some meta-comments:

(1) Please consider setting the "user.name" item in your git config to
your full name, "Patrick Henz". Because right now it seems to be just
"henz", and that doesn't look very nice in the git commit history.

(2) When you update this patch for v2, point (1) will not fix this very
patch however. So please update this particular patch with

  git commit --amend --author='Patrick Henz <patrick.henz@hpe.com>'

(3) Please run

  python3 BaseTools/Scripts/SetupGit.py

in your edk2 clone -- it automates a bunch of the settings listed at
<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05>,
and more. For example, it sets 8bit content-transfer-encoding for
mailing out your edk2 patches, which is helpful because quoted-printable
doesn't play nice with the hard CRLF line endings that edk2 uses.

Thank you!
Laszlo


> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2948
> 
> Timeouts in the XhciDxe driver are taking longer than
> expected due to the timeout loops not accounting for
> code execution time. As en example, 5 second timeouts
> have been observed to take around 36 seconds to complete.
> Use SetTimer and Create/CheckEvent from Boot Services to
> determine when timeout occurred.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 28 ++++++++++++++++---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 34 +++++++++++++++++-------
>  2 files changed, 49 insertions(+), 13 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 42b773ab31..5f7507f0a5 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -442,17 +442,37 @@ XhcWaitOpRegBit (
>    IN UINT32               Timeout
> 
>    )
> 
>  {
> 
> -  UINT32                  Index;
> 
> -  UINT64                  Loop;
> 
> +  EFI_STATUS Status;
> 
> +  EFI_EVENT  TimeoutEvent;
> 
>  
> 
> -  Loop   = Timeout * XHC_1_MILLISECOND;
> 
> +  if (Timeout == 0) {
> 
> +    return EFI_TIMEOUT;
> 
> +  }
> 
> +
> 
> +  Status = gBS->CreateEvent (
> 
> +                  EVT_TIMER,
> 
> +                  TPL_CALLBACK,
> 
> +                  NULL,
> 
> +                  NULL,
> 
> +                  &TimeoutEvent
> 
> +                  );
> 
>  
> 
> -  for (Index = 0; Index < Loop; Index++) {
> 
> +  if (!EFI_ERROR (Status)) {
> 
> +    Status = gBS->SetTimer (TimeoutEvent,
> 
> +                            TimerRelative,
> 
> +                            EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> 
> +  }
> 
> +
> 
> +  do {
> 
>      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> 
>        return EFI_SUCCESS;
> 
>      }
> 
>  
> 
>      gBS->Stall (XHC_1_MICROSECOND);
> 
> +  } while (!EFI_ERROR(Status) && EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> 
> +
> 
> +  if (TimeoutEvent != NULL) {
> 
> +    gBS->CloseEvent (TimeoutEvent);
> 
>    }
> 
>  
> 
>    return EFI_TIMEOUT;
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index ab8957c546..cf271f0493 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -1273,11 +1273,11 @@ XhcExecTransfer (
>    )
> 
>  {
> 
>    EFI_STATUS              Status;
> 
> -  UINTN                   Index;
> 
> -  UINT64                  Loop;
> 
>    UINT8                   SlotId;
> 
>    UINT8                   Dci;
> 
>    BOOLEAN                 Finished;
> 
> +  EFI_EVENT               TimeoutEvent;
> 
> +  EFI_STATUS              TimerStatus;
> 
>  
> 
>    if (CmdTransfer) {
> 
>      SlotId = 0;
> 
> @@ -1292,28 +1292,44 @@ XhcExecTransfer (
>    }
> 
>  
> 
>    Status = EFI_SUCCESS;
> 
> -  Loop   = Timeout * XHC_1_MILLISECOND;
> 
> -  if (Timeout == 0) {
> 
> -    Loop = 0xFFFFFFFF;
> 
> -  }
> 
> +
> 
> +  TimerStatus = gBS->CreateEvent (
> 
> +                       EVT_TIMER,
> 
> +                       TPL_CALLBACK,
> 
> +                       NULL,
> 
> +                       NULL,
> 
> +                       &TimeoutEvent
> 
> +                       );
> 
>  
> 
>    XhcRingDoorBell (Xhc, SlotId, Dci);
> 
>  
> 
> -  for (Index = 0; Index < Loop; Index++) {
> 
> +  if (!EFI_ERROR (TimerStatus)) {
> 
> +    TimerStatus = gBS->SetTimer (TimeoutEvent,
> 
> +                                 TimerRelative,
> 
> +                                 (0 == Timeout)?
> 
> +                                 (EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):
> 
> +                                 (EFI_TIMER_PERIOD_MILLISECONDS(Timeout)));
> 
> +  }
> 
> +
> 
> +  do {
> 
>      Finished = XhcCheckUrbResult (Xhc, Urb);
> 
>      if (Finished) {
> 
>        break;
> 
>      }
> 
>      gBS->Stall (XHC_1_MICROSECOND);
> 
> -  }
> 
> +  } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> 
>  
> 
> -  if (Index == Loop) {
> 
> +  if (!Finished) {
> 
>      Urb->Result = EFI_USB_ERR_TIMEOUT;
> 
>      Status      = EFI_TIMEOUT;
> 
>    } else if (Urb->Result != EFI_USB_NOERROR) {
> 
>      Status      = EFI_DEVICE_ERROR;
> 
>    }
> 
>  
> 
> +  if (TimeoutEvent != NULL) {
> 
> +    gBS->CloseEvent (TimeoutEvent);
> 
> +  }
> 
> +
> 
>    return Status;
> 
>  }
> 
>  
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/1] Fix XhciDxe Timeouts
  2020-09-01 18:55 [PATCH 0/1] Fix XhciDxe Timeouts patrick.henz
  2020-09-01 18:55 ` [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts patrick.henz
@ 2020-09-02  5:08 ` Wu, Hao A
  2020-09-02  6:49   ` [edk2-devel] " Laszlo Ersek
  2020-09-02 23:03   ` patrick.henz
  1 sibling, 2 replies; 10+ messages in thread
From: Wu, Hao A @ 2020-09-02  5:08 UTC (permalink / raw)
  To: patrick.henz@hpe.com, devel@edk2.groups.io; +Cc: Wang, Jian J, Ni, Ray

> -----Original Message-----
> From: patrick.henz@hpe.com <patrick.henz@hpe.com>
> Sent: Wednesday, September 2, 2020 2:55 AM
> To: devel@edk2.groups.io
> Cc: henz <patrick.henz@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
> Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH 0/1] Fix XhciDxe Timeouts
> 
> From: henz <patrick.henz@hpe.com>
> 
> Timeouts in the XhciDxe driver are taking longer than expected due to the
> timeout loops not accounting for code execution time. As en example, 5 second
> timeouts have been observed to take around 36 seconds to complete.
> Use SetTimer and Create/CheckEvent from Boot Services to determine when
> timeout occurred. This patch was tested using forced timeouts and print
> statements with QEmu as well as phycial hardware. The forced timeouts were
> implemented in code via static variables that guaranteed a timeout the first time
> the function with the broken timeout was called.
> 
> Example:
> 
> XhcExecTransfer (
>   .
>   .
>  )
> {
>   .
>   .
>   static int do_once = 1;  // test line
>   .
>   .
>   do {
>     Finished = XhcCheckUrbResult (Xhc, Urb);
>     if (do_once) Finished = 0; // test line
>     if (Finished) {
>       break;
>     }
>     gBS->Stall (XHC_1_MICROSECOND);
>   } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent
> (TimeoutEvent)));
> 
>   do_once = 0; // test line
> 
> Using this forced timeout approach the correct timeouts were observed on both
> hardware and in QEmu.
> 
> Similar broken timeout loops have been found in the Uhci and Ehci drivers. This
> patch does not fix those issues.


Hello Patrick,

Besides the comments made by Ray in patch 1, could you help to provide 2 more patches for UHCI and EHCI drivers as well for complete enhancement?
Thanks in advance.

Best Regards,
Hao Wu


> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> 
> Patrick Henz (1):
>   MdeModulePkg/XhciDxe: Fix Broken Timeouts
> 
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 28 ++++++++++++++++---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 34 +++++++++++++++++-------
>  2 files changed, 49 insertions(+), 13 deletions(-)
> 
> --
> 2.27.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] [PATCH 0/1] Fix XhciDxe Timeouts
  2020-09-02  5:08 ` [PATCH 0/1] Fix XhciDxe Timeouts Wu, Hao A
@ 2020-09-02  6:49   ` Laszlo Ersek
  2020-09-02  6:58     ` Wu, Hao A
  2020-09-02 23:03   ` patrick.henz
  1 sibling, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2020-09-02  6:49 UTC (permalink / raw)
  To: devel, hao.a.wu, patrick.henz@hpe.com
  Cc: Wang, Jian J, Ni, Ray, Liming Gao (Byosoft address)

On 09/02/20 07:08, Wu, Hao A wrote:
>> -----Original Message-----
>> From: patrick.henz@hpe.com <patrick.henz@hpe.com>
>> Sent: Wednesday, September 2, 2020 2:55 AM
>> To: devel@edk2.groups.io
>> Cc: henz <patrick.henz@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
>> Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: [PATCH 0/1] Fix XhciDxe Timeouts
>>
>> From: henz <patrick.henz@hpe.com>
>>
>> Timeouts in the XhciDxe driver are taking longer than expected due to the
>> timeout loops not accounting for code execution time. As en example, 5 second
>> timeouts have been observed to take around 36 seconds to complete.
>> Use SetTimer and Create/CheckEvent from Boot Services to determine when
>> timeout occurred. This patch was tested using forced timeouts and print
>> statements with QEmu as well as phycial hardware. The forced timeouts were
>> implemented in code via static variables that guaranteed a timeout the first time
>> the function with the broken timeout was called.
>>
>> Example:
>>
>> XhcExecTransfer (
>>   .
>>   .
>>  )
>> {
>>   .
>>   .
>>   static int do_once = 1;  // test line
>>   .
>>   .
>>   do {
>>     Finished = XhcCheckUrbResult (Xhc, Urb);
>>     if (do_once) Finished = 0; // test line
>>     if (Finished) {
>>       break;
>>     }
>>     gBS->Stall (XHC_1_MICROSECOND);
>>   } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent
>> (TimeoutEvent)));
>>
>>   do_once = 0; // test line
>>
>> Using this forced timeout approach the correct timeouts were observed on both
>> hardware and in QEmu.
>>
>> Similar broken timeout loops have been found in the Uhci and Ehci drivers. This
>> patch does not fix those issues.
> 
> 
> Hello Patrick,
> 
> Besides the comments made by Ray in patch 1, could you help to provide 2 more patches for UHCI and EHCI drivers as well for complete enhancement?
> Thanks in advance.

We're very close to the edk2-stable202008 tag deadline.

This patch -- which in v2 is going to be a patch series -- is not aiming
at the stable tag, does it?

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] [PATCH 0/1] Fix XhciDxe Timeouts
  2020-09-02  6:49   ` [edk2-devel] " Laszlo Ersek
@ 2020-09-02  6:58     ` Wu, Hao A
  0 siblings, 0 replies; 10+ messages in thread
From: Wu, Hao A @ 2020-09-02  6:58 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io, patrick.henz@hpe.com
  Cc: Wang, Jian J, Ni, Ray, Liming Gao (Byosoft address)

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, September 2, 2020 2:49 PM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>;
> patrick.henz@hpe.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Liming
> Gao (Byosoft address) <gaoliming@byosoft.com.cn>
> Subject: Re: [edk2-devel] [PATCH 0/1] Fix XhciDxe Timeouts
> 
> On 09/02/20 07:08, Wu, Hao A wrote:
> >> -----Original Message-----
> >> From: patrick.henz@hpe.com <patrick.henz@hpe.com>
> >> Sent: Wednesday, September 2, 2020 2:55 AM
> >> To: devel@edk2.groups.io
> >> Cc: henz <patrick.henz@hpe.com>; Wang, Jian J
> >> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> >> <ray.ni@intel.com>
> >> Subject: [PATCH 0/1] Fix XhciDxe Timeouts
> >>
> >> From: henz <patrick.henz@hpe.com>
> >>
> >> Timeouts in the XhciDxe driver are taking longer than expected due to
> >> the timeout loops not accounting for code execution time. As en
> >> example, 5 second timeouts have been observed to take around 36 seconds
> to complete.
> >> Use SetTimer and Create/CheckEvent from Boot Services to determine
> >> when timeout occurred. This patch was tested using forced timeouts
> >> and print statements with QEmu as well as phycial hardware. The
> >> forced timeouts were implemented in code via static variables that
> >> guaranteed a timeout the first time the function with the broken timeout was
> called.
> >>
> >> Example:
> >>
> >> XhcExecTransfer (
> >>   .
> >>   .
> >>  )
> >> {
> >>   .
> >>   .
> >>   static int do_once = 1;  // test line
> >>   .
> >>   .
> >>   do {
> >>     Finished = XhcCheckUrbResult (Xhc, Urb);
> >>     if (do_once) Finished = 0; // test line
> >>     if (Finished) {
> >>       break;
> >>     }
> >>     gBS->Stall (XHC_1_MICROSECOND);
> >>   } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent
> >> (TimeoutEvent)));
> >>
> >>   do_once = 0; // test line
> >>
> >> Using this forced timeout approach the correct timeouts were observed
> >> on both hardware and in QEmu.
> >>
> >> Similar broken timeout loops have been found in the Uhci and Ehci
> >> drivers. This patch does not fix those issues.
> >
> >
> > Hello Patrick,
> >
> > Besides the comments made by Ray in patch 1, could you help to provide 2
> more patches for UHCI and EHCI drivers as well for complete enhancement?
> > Thanks in advance.
> 
> We're very close to the edk2-stable202008 tag deadline.
> 
> This patch -- which in v2 is going to be a patch series -- is not aiming at the
> stable tag, does it?


Hello Laszlo,

The patch (series) seems more like an enhancement to me. So I think it is not aiming the upcoming tag.

Best Regards,
Hao Wu


> 
> Thanks!
> Laszlo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/1] Fix XhciDxe Timeouts
  2020-09-02  5:08 ` [PATCH 0/1] Fix XhciDxe Timeouts Wu, Hao A
  2020-09-02  6:49   ` [edk2-devel] " Laszlo Ersek
@ 2020-09-02 23:03   ` patrick.henz
  2020-09-03  3:54     ` [edk2-devel] " Wu, Hao A
  1 sibling, 1 reply; 10+ messages in thread
From: patrick.henz @ 2020-09-02 23:03 UTC (permalink / raw)
  To: Wu, Hao A, devel@edk2.groups.io; +Cc: Wang, Jian J, Ni, Ray

Hello Hao,

Yes, I can provide patches for the UHCI and EHCI drivers. I haven't done any of the work on either of these yet but will hopefully get to them in the next day or so.

Thanks,
Patrick Henz

-----Original Message-----
From: Wu, Hao A [mailto:hao.a.wu@intel.com] 
Sent: Wednesday, September 2, 2020 12:09 AM
To: Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [PATCH 0/1] Fix XhciDxe Timeouts

> -----Original Message-----
> From: patrick.henz@hpe.com <patrick.henz@hpe.com>
> Sent: Wednesday, September 2, 2020 2:55 AM
> To: devel@edk2.groups.io
> Cc: henz <patrick.henz@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>; 
> Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH 0/1] Fix XhciDxe Timeouts
> 
> From: henz <patrick.henz@hpe.com>
> 
> Timeouts in the XhciDxe driver are taking longer than expected due to 
> the timeout loops not accounting for code execution time. As en 
> example, 5 second timeouts have been observed to take around 36 seconds to complete.
> Use SetTimer and Create/CheckEvent from Boot Services to determine 
> when timeout occurred. This patch was tested using forced timeouts and 
> print statements with QEmu as well as phycial hardware. The forced 
> timeouts were implemented in code via static variables that guaranteed 
> a timeout the first time the function with the broken timeout was called.
> 
> Example:
> 
> XhcExecTransfer (
>   .
>   .
>  )
> {
>   .
>   .
>   static int do_once = 1;  // test line
>   .
>   .
>   do {
>     Finished = XhcCheckUrbResult (Xhc, Urb);
>     if (do_once) Finished = 0; // test line
>     if (Finished) {
>       break;
>     }
>     gBS->Stall (XHC_1_MICROSECOND);
>   } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent 
> (TimeoutEvent)));
> 
>   do_once = 0; // test line
> 
> Using this forced timeout approach the correct timeouts were observed 
> on both hardware and in QEmu.
> 
> Similar broken timeout loops have been found in the Uhci and Ehci 
> drivers. This patch does not fix those issues.


Hello Patrick,

Besides the comments made by Ray in patch 1, could you help to provide 2 more patches for UHCI and EHCI drivers as well for complete enhancement?
Thanks in advance.

Best Regards,
Hao Wu


> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> 
> Patrick Henz (1):
>   MdeModulePkg/XhciDxe: Fix Broken Timeouts
> 
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 28 ++++++++++++++++---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 34 
> +++++++++++++++++-------
>  2 files changed, 49 insertions(+), 13 deletions(-)
> 
> --
> 2.27.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] [PATCH 0/1] Fix XhciDxe Timeouts
  2020-09-02 23:03   ` patrick.henz
@ 2020-09-03  3:54     ` Wu, Hao A
  2020-09-03  8:53       ` Ni, Ray
  0 siblings, 1 reply; 10+ messages in thread
From: Wu, Hao A @ 2020-09-03  3:54 UTC (permalink / raw)
  To: devel@edk2.groups.io, patrick.henz@hpe.com; +Cc: Wang, Jian J, Ni, Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> Patrick
> Sent: Thursday, September 3, 2020 7:03 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/1] Fix XhciDxe Timeouts
> 
> Hello Hao,
> 
> Yes, I can provide patches for the UHCI and EHCI drivers. I haven't done any
> of the work on either of these yet but will hopefully get to them in the next
> day or so.


Thanks Patrick,

Could you help to add the UHCI and EHCI changes together with the existing XHCI patch into one series for the next version?
That will make a series which include 3 patches.

Best Regards,
Hao Wu


> 
> Thanks,
> Patrick Henz
> 
> -----Original Message-----
> From: Wu, Hao A [mailto:hao.a.wu@intel.com]
> Sent: Wednesday, September 2, 2020 12:09 AM
> To: Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH 0/1] Fix XhciDxe Timeouts
> 
> > -----Original Message-----
> > From: patrick.henz@hpe.com <patrick.henz@hpe.com>
> > Sent: Wednesday, September 2, 2020 2:55 AM
> > To: devel@edk2.groups.io
> > Cc: henz <patrick.henz@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>;
> > Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: [PATCH 0/1] Fix XhciDxe Timeouts
> >
> > From: henz <patrick.henz@hpe.com>
> >
> > Timeouts in the XhciDxe driver are taking longer than expected due to
> > the timeout loops not accounting for code execution time. As en
> > example, 5 second timeouts have been observed to take around 36
> seconds to complete.
> > Use SetTimer and Create/CheckEvent from Boot Services to determine
> > when timeout occurred. This patch was tested using forced timeouts and
> > print statements with QEmu as well as phycial hardware. The forced
> > timeouts were implemented in code via static variables that guaranteed
> > a timeout the first time the function with the broken timeout was called.
> >
> > Example:
> >
> > XhcExecTransfer (
> >   .
> >   .
> >  )
> > {
> >   .
> >   .
> >   static int do_once = 1;  // test line
> >   .
> >   .
> >   do {
> >     Finished = XhcCheckUrbResult (Xhc, Urb);
> >     if (do_once) Finished = 0; // test line
> >     if (Finished) {
> >       break;
> >     }
> >     gBS->Stall (XHC_1_MICROSECOND);
> >   } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent
> > (TimeoutEvent)));
> >
> >   do_once = 0; // test line
> >
> > Using this forced timeout approach the correct timeouts were observed
> > on both hardware and in QEmu.
> >
> > Similar broken timeout loops have been found in the Uhci and Ehci
> > drivers. This patch does not fix those issues.
> 
> 
> Hello Patrick,
> 
> Besides the comments made by Ray in patch 1, could you help to provide 2
> more patches for UHCI and EHCI drivers as well for complete enhancement?
> Thanks in advance.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> >
> > Patrick Henz (1):
> >   MdeModulePkg/XhciDxe: Fix Broken Timeouts
> >
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 28 ++++++++++++++++---
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 34
> > +++++++++++++++++-------
> >  2 files changed, 49 insertions(+), 13 deletions(-)
> >
> > --
> > 2.27.0
> 
> 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] [PATCH 0/1] Fix XhciDxe Timeouts
  2020-09-03  3:54     ` [edk2-devel] " Wu, Hao A
@ 2020-09-03  8:53       ` Ni, Ray
  0 siblings, 0 replies; 10+ messages in thread
From: Ni, Ray @ 2020-09-03  8:53 UTC (permalink / raw)
  To: Wu, Hao A, devel@edk2.groups.io, patrick.henz@hpe.com; +Cc: Wang, Jian J

Patrick,
Please make sure unit test is done for the code you changed.
If you cannot find environment to test the changes, I prefer to not make the change.

Thanks,
Ray

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, September 3, 2020 11:55 AM
> To: devel@edk2.groups.io; patrick.henz@hpe.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/1] Fix XhciDxe Timeouts
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > Patrick
> > Sent: Thursday, September 3, 2020 7:03 AM
> > To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 0/1] Fix XhciDxe Timeouts
> >
> > Hello Hao,
> >
> > Yes, I can provide patches for the UHCI and EHCI drivers. I haven't done any
> > of the work on either of these yet but will hopefully get to them in the next
> > day or so.
> 
> 
> Thanks Patrick,
> 
> Could you help to add the UHCI and EHCI changes together with the existing
> XHCI patch into one series for the next version?
> That will make a series which include 3 patches.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Thanks,
> > Patrick Henz
> >
> > -----Original Message-----
> > From: Wu, Hao A [mailto:hao.a.wu@intel.com]
> > Sent: Wednesday, September 2, 2020 12:09 AM
> > To: Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: RE: [PATCH 0/1] Fix XhciDxe Timeouts
> >
> > > -----Original Message-----
> > > From: patrick.henz@hpe.com <patrick.henz@hpe.com>
> > > Sent: Wednesday, September 2, 2020 2:55 AM
> > > To: devel@edk2.groups.io
> > > Cc: henz <patrick.henz@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>;
> > > Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> > > Subject: [PATCH 0/1] Fix XhciDxe Timeouts
> > >
> > > From: henz <patrick.henz@hpe.com>
> > >
> > > Timeouts in the XhciDxe driver are taking longer than expected due to
> > > the timeout loops not accounting for code execution time. As en
> > > example, 5 second timeouts have been observed to take around 36
> > seconds to complete.
> > > Use SetTimer and Create/CheckEvent from Boot Services to determine
> > > when timeout occurred. This patch was tested using forced timeouts and
> > > print statements with QEmu as well as phycial hardware. The forced
> > > timeouts were implemented in code via static variables that guaranteed
> > > a timeout the first time the function with the broken timeout was called.
> > >
> > > Example:
> > >
> > > XhcExecTransfer (
> > >   .
> > >   .
> > >  )
> > > {
> > >   .
> > >   .
> > >   static int do_once = 1;  // test line
> > >   .
> > >   .
> > >   do {
> > >     Finished = XhcCheckUrbResult (Xhc, Urb);
> > >     if (do_once) Finished = 0; // test line
> > >     if (Finished) {
> > >       break;
> > >     }
> > >     gBS->Stall (XHC_1_MICROSECOND);
> > >   } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent
> > > (TimeoutEvent)));
> > >
> > >   do_once = 0; // test line
> > >
> > > Using this forced timeout approach the correct timeouts were observed
> > > on both hardware and in QEmu.
> > >
> > > Similar broken timeout loops have been found in the Uhci and Ehci
> > > drivers. This patch does not fix those issues.
> >
> >
> > Hello Patrick,
> >
> > Besides the comments made by Ray in patch 1, could you help to provide 2
> > more patches for UHCI and EHCI drivers as well for complete enhancement?
> > Thanks in advance.
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > >
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> > >
> > > Patrick Henz (1):
> > >   MdeModulePkg/XhciDxe: Fix Broken Timeouts
> > >
> > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 28 ++++++++++++++++---
> > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 34
> > > +++++++++++++++++-------
> > >  2 files changed, 49 insertions(+), 13 deletions(-)
> > >
> > > --
> > > 2.27.0
> >
> >
> > 


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-09-03  8:53 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-01 18:55 [PATCH 0/1] Fix XhciDxe Timeouts patrick.henz
2020-09-01 18:55 ` [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts patrick.henz
2020-09-02  3:21   ` Ni, Ray
2020-09-02  6:45   ` [edk2-devel] " Laszlo Ersek
2020-09-02  5:08 ` [PATCH 0/1] Fix XhciDxe Timeouts Wu, Hao A
2020-09-02  6:49   ` [edk2-devel] " Laszlo Ersek
2020-09-02  6:58     ` Wu, Hao A
2020-09-02 23:03   ` patrick.henz
2020-09-03  3:54     ` [edk2-devel] " Wu, Hao A
2020-09-03  8:53       ` Ni, Ray

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox