[PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Henz, Patrick]

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

XhciDxe uses the timer functionality provided by the
boot services table to detect timeout conditions. This
breaks the driver's ExitBootServices call back, as
CoreExitBootServices halts the timer before signaling
the ExitBootServices event. If the host controller
fails to halt in the call back, the timeout condition
will never occur and the boot gets stuck in an indefinite
spin loop. Use the free running timer provided by
TimerLib to calculate timeouts, avoiding the potential
hang.

Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
Reviewed-by:
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
 MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
 MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68 +++++++++---------------
 5 files changed, 129 insertions(+), 86 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index 62682dd27c..1dcbe20512 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -1,6 +1,7 @@
 /** @file
   The XHCI controller driver.
 
+(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
 Copyright (c) 2011 - 2022, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
 
   return EFI_SUCCESS;
 }
+
+/**
+  Computes and returns the elapsed time in nanoseconds since
+  PreviousTick. The value of PreviousTick is overwritten with the
+  current performance counter value.
+
+  @param  PreviousTick    Pointer to PreviousTick count.
+  @return The elapsed time in nanoseconds since PreviousCount.
+          PreviousCount is overwritten with the current performance
+          counter value.
+**/
+UINT64
+XhcGetElapsedTime (
+  IN OUT UINT64  *PreviousTick
+  )
+{
+  UINT64  StartValue;
+  UINT64  EndValue;
+  UINT64  CurrentTick;
+  UINT64  Delta;
+
+  GetPerformanceCounterProperties (&StartValue, &EndValue);
+
+  CurrentTick = GetPerformanceCounter ();
+
+  //
+  // Determine if the counter is counting up or down
+  //
+  if (StartValue < EndValue) {
+    //
+    // Counter counts upwards, check for an overflow condition
+    //
+    if (*PreviousTick > CurrentTick) {
+      Delta = (EndValue - *PreviousTick) + CurrentTick;
+    } else {
+      Delta = CurrentTick - *PreviousTick;
+    }
+  } else {
+    //
+    // Counter counts downwards, check for an underflow condition
+    //
+    if (*PreviousTick < CurrentTick) {
+      Delta = (StartValue - CurrentTick) + *PreviousTick;
+    } else {
+      Delta = *PreviousTick - CurrentTick;
+    }
+  }
+
+  //
+  // Set PreviousTick to CurrentTick
+  //
+  *PreviousTick = CurrentTick;
+
+  return GetTimeInNanoSecond (Delta);
+}
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
index ca223bd20c..77feb66647 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
@@ -2,6 +2,7 @@
 
   Provides some data structure definitions used by the XHCI host controller driver.
 
+(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
 Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.<BR>
 Copyright (c) Microsoft Corporation.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/UefiLib.h>
 #include <Library/DebugLib.h>
 #include <Library/ReportStatusCodeLib.h>
+#include <Library/TimerLib.h>
 
 #include <IndustryStandard/Pci.h>
 
@@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT    USB_DEV_CONTEXT;
 #include "ComponentName.h"
 #include "UsbHcMem.h"
 
+//
+// Converts a count from microseconds to nanoseconds
+//
+#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time) * 1000)
 //
 // The unit is microsecond, setting it as 1us.
 //
@@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
   IN     VOID                                *Context
   );
 
+/**
+  Computes and returns the elapsed time in nanoseconds since
+  PreviousTick. The value of PreviousTick is overwritten with the
+  current performance counter value.
+
+  @param  PreviousTick    Pointer to PreviousTick count.
+                          before calling this function.
+  @return The elapsed time in nanoseconds since PreviousCount.
+          PreviousCount is overwritten with the current performance
+          counter value.
+**/
+UINT64
+XhcGetElapsedTime (
+  IN OUT UINT64  *PreviousTick
+  );
+
 #endif
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
index 5865d86822..18ef87916a 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
@@ -3,6 +3,7 @@
 #  It implements the interfaces of monitoring the status of all ports and transferring
 #  Control, Bulk, Interrupt and Isochronous requests to those attached usb LS/FS/HS/SS devices.
 #
+#  (C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
 #  Copyright (c) 2011 - 2018, Intel Corporation. All rights reserved.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -54,6 +55,7 @@
   BaseMemoryLib
   DebugLib
   ReportStatusCodeLib
+  TimerLib
 
 [Guids]
   gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ## Event
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
index 5700fc5fb8..2499698715 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
@@ -2,6 +2,7 @@
 
   The XHCI register operation routines.
 
+(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
 Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -417,15 +418,14 @@ XhcClearOpRegBit (
   Wait the operation register's bit as specified by Bit
   to become set (or clear).
 
-  @param  Xhc                    The XHCI Instance.
-  @param  Offset                 The offset of the operation register.
-  @param  Bit                    The bit of the register to wait for.
-  @param  WaitToSet              Wait the bit to set or clear.
-  @param  Timeout                The time to wait before abort (in millisecond, ms).
+  @param  Xhc          The XHCI Instance.
+  @param  Offset       The offset of the operation register.
+  @param  Bit          The bit of the register to wait for.
+  @param  WaitToSet    Wait the bit to set or clear.
+  @param  Timeout      The time to wait before abort (in millisecond, ms).
 
-  @retval EFI_SUCCESS            The bit successfully changed by host controller.
-  @retval EFI_TIMEOUT            The time out occurred.
-  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not be allocated.
+  @retval EFI_SUCCESS  The bit successfully changed by host controller.
+  @retval EFI_TIMEOUT  The time out occurred.
 
 **/
 EFI_STATUS
@@ -437,54 +437,35 @@ XhcWaitOpRegBit (
   IN UINT32             Timeout
   )
 {
-  EFI_STATUS  Status;
-  EFI_EVENT   TimeoutEvent;
-
-  TimeoutEvent = NULL;
+  UINT64  TimeoutTime;
+  UINT64  ElapsedTime;
+  UINT64  TimeDelta;
+  UINT64  CurrentTick;
 
   if (Timeout == 0) {
     return EFI_TIMEOUT;
   }
 
-  Status = gBS->CreateEvent (
-                  EVT_TIMER,
-                  TPL_CALLBACK,
-                  NULL,
-                  NULL,
-                  &TimeoutEvent
-                  );
-
-  if (EFI_ERROR (Status)) {
-    goto DONE;
-  }
-
-  Status = gBS->SetTimer (
-                  TimeoutEvent,
-                  TimerRelative,
-                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
-                  );
-
-  if (EFI_ERROR (Status)) {
-    goto DONE;
-  }
+  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout * XHC_1_MILLISECOND);
+  ElapsedTime = 0;
+  CurrentTick = GetPerformanceCounter ();
 
   do {
     if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
-      Status = EFI_SUCCESS;
-      goto DONE;
+      return EFI_SUCCESS;
     }
 
     gBS->Stall (XHC_1_MICROSECOND);
-  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
-
-  Status = EFI_TIMEOUT;
+    TimeDelta = XhcGetElapsedTime (&CurrentTick);
+    // Ensure that ElapsedTime is always incremented to avoid indefinite hangs
+    if (TimeDelta == 0) {
+      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND (XHC_1_MICROSECOND);
+    }
 
-DONE:
-  if (TimeoutEvent != NULL) {
-    gBS->CloseEvent (TimeoutEvent);
-  }
+    ElapsedTime += TimeDelta;
+  } while (ElapsedTime < TimeoutTime);
 
-  return Status;
+  return EFI_TIMEOUT;
 }
 
 /**
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 298fb88b81..5177886e52 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -2,6 +2,7 @@
 
   XHCI transfer scheduling routines.
 
+(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
 Copyright (c) 2011 - 2020, Intel Corporation. All rights reserved.<BR>
 Copyright (c) Microsoft Corporation.<BR>
 Copyright (C) 2022 Advanced Micro Devices, Inc. All rights reserved.<BR>
@@ -1273,15 +1274,14 @@ EXIT:
 /**
   Execute the transfer by polling the URB. This is a synchronous operation.
 
-  @param  Xhc                    The XHCI Instance.
-  @param  CmdTransfer            The executed URB is for cmd transfer or not.
-  @param  Urb                    The URB to execute.
-  @param  Timeout                The time to wait before abort, in millisecond.
+  @param  Xhc               The XHCI Instance.
+  @param  CmdTransfer       The executed URB is for cmd transfer or not.
+  @param  Urb               The URB to execute.
+  @param  Timeout           The time to wait before abort, in millisecond.
 
-  @return EFI_DEVICE_ERROR       The transfer failed due to transfer error.
-  @return EFI_TIMEOUT            The transfer failed due to time out.
-  @return EFI_SUCCESS            The transfer finished OK.
-  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not be allocated.
+  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
+  @return EFI_TIMEOUT       The transfer failed due to time out.
+  @return EFI_SUCCESS       The transfer finished OK.
 
 **/
 EFI_STATUS
@@ -1296,12 +1296,14 @@ XhcExecTransfer (
   UINT8       SlotId;
   UINT8       Dci;
   BOOLEAN     Finished;
-  EFI_EVENT   TimeoutEvent;
+  UINT64      TimeoutTime;
+  UINT64      ElapsedTime;
+  UINT64      TimeDelta;
+  UINT64      CurrentTick;
   BOOLEAN     IndefiniteTimeout;
 
   Status            = EFI_SUCCESS;
   Finished          = FALSE;
-  TimeoutEvent      = NULL;
   IndefiniteTimeout = FALSE;
 
   if (CmdTransfer) {
@@ -1319,34 +1321,14 @@ XhcExecTransfer (
 
   if (Timeout == 0) {
     IndefiniteTimeout = TRUE;
-    goto RINGDOORBELL;
-  }
-
-  Status = gBS->CreateEvent (
-                  EVT_TIMER,
-                  TPL_CALLBACK,
-                  NULL,
-                  NULL,
-                  &TimeoutEvent
-                  );
-
-  if (EFI_ERROR (Status)) {
-    goto DONE;
   }
 
-  Status = gBS->SetTimer (
-                  TimeoutEvent,
-                  TimerRelative,
-                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
-                  );
-
-  if (EFI_ERROR (Status)) {
-    goto DONE;
-  }
-
-RINGDOORBELL:
   XhcRingDoorBell (Xhc, SlotId, Dci);
 
+  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout * XHC_1_MILLISECOND);
+  ElapsedTime = 0;
+  CurrentTick = GetPerformanceCounter ();
+
   do {
     Finished = XhcCheckUrbResult (Xhc, Urb);
     if (Finished) {
@@ -1354,22 +1336,22 @@ RINGDOORBELL:
     }
 
     gBS->Stall (XHC_1_MICROSECOND);
-  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
+    TimeDelta = XhcGetElapsedTime (&CurrentTick);
+    // Ensure that ElapsedTime is always incremented to avoid indefinite hangs
+    if (TimeDelta == 0) {
+      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND (XHC_1_MICROSECOND);
+    }
 
-DONE:
-  if (EFI_ERROR (Status)) {
-    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
-  } else if (!Finished) {
+    ElapsedTime += TimeDelta;
+  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
+
+  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.34.1

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Michael Brown]

On 05/07/2023 21:15, Henz, Patrick wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2948
> 
> XhciDxe uses the timer functionality provided by the
> boot services table to detect timeout conditions. This
> breaks the driver's ExitBootServices call back, as
> CoreExitBootServices halts the timer before signaling
> the ExitBootServices event. If the host controller
> fails to halt in the call back, the timeout condition
> will never occur and the boot gets stuck in an indefinite
> spin loop. Use the free running timer provided by
> TimerLib to calculate timeouts, avoiding the potential
> hang.

Two points:

1. The XhcGetElapsedTime() function feels like a generally reusable 
abstraction that should exist within TimerLib, rather than being 
open-coded within XhciDxe.

2. Is the performance counter guaranteed to exist and work as expected 
on all platforms and CPU architectures?  (For example: what if the CPU 
does not support a constant TSC?)

Thanks,

Michael

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Henz, Patrick]

Hi Michael,

I agree that XhcGetElapsedTime() would be better off in TimerLib, but I wasn't sure how the community would feel about adding to the interface.

As for your question, I was wondering the same thing, I'm not sure if there are any platforms that do not have a free-running timer that would be accessed through TimerLib. Because of this unknown I included a check on the return value from XhcGetElapsedTime(), if that value is 0 ElapsedTime is incremented regardless to avoid a potential hang. The behavior of the function essentially reverts to the pre "MdeModulePkg/XhciDxe: Fix Broken Timeouts" commit implementation.

Thanks,
Patrick Henz

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Brown
Sent: Thursday, July 6, 2023 8:02 AM
To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

On 05/07/2023 21:15, Henz, Patrick wrote:
> REF:INVALID URI REMOVED
> g.cgi?id=2948__;!!NpxR!mnYVFUERkrUcRiNbM7pejt6qy6h5tilkuGydeM0skcGMAzS
> 1oEy1DSDT720vj4Le2cGtf_kmPYFIng$
> 
> XhciDxe uses the timer functionality provided by the boot services 
> table to detect timeout conditions. This breaks the driver's 
> ExitBootServices call back, as CoreExitBootServices halts the timer 
> before signaling the ExitBootServices event. If the host controller 
> fails to halt in the call back, the timeout condition will never occur 
> and the boot gets stuck in an indefinite spin loop. Use the free 
> running timer provided by TimerLib to calculate timeouts, avoiding the 
> potential hang.

Two points:

1. The XhcGetElapsedTime() function feels like a generally reusable abstraction that should exist within TimerLib, rather than being open-coded within XhciDxe.

2. Is the performance counter guaranteed to exist and work as expected on all platforms and CPU architectures?  (For example: what if the CPU does not support a constant TSC?)

Thanks,

Michael

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Michael Brown]

On 06/07/2023 15:19, Henz, Patrick wrote:
> I agree that XhcGetElapsedTime() would be better off in TimerLib, but I wasn't sure how the community would feel about adding to the interface.

My understanding is that the TimerLib API is not prescribed by any 
standards document, and that this change would add a new function 
without changing any existing functions and so would not break any 
existing users of TimerLib.  On that basis, I would be firmly in favour 
of extending TimerLib to include this functionality.

I will defer to actual EDK2 maintainers on this, however.  :)

Thanks,

Michael

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Michael D Kinney]

There is a similar lib API for SMI handlers:

https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SmmPeriodicSmiLib.h

/**
  This function returns the time in 100ns units since the periodic SMI
  handler function was called.  If the periodic SMI handler was resumed
  through PeriodicSmiYield(), then the time returned is the time in
  100ns units since PeriodicSmiYield() returned.

  @return  The actual time in 100ns units that the periodic SMI handler
           has been executing.  If this function is not called from within
           an enabled periodic SMI handler, then 0 is returned.

**/
UINT64
EFIAPI
PeriodicSmiExecutionTime (
  VOID
  );

https://github.com/tianocore/edk2/blob/96d691166f07b7ed422f9536832edadc0aea35c9/MdePkg/Library/SmmPeriodicSmiLib/SmmPeriodicSmiLib.c#L422


I agree that an elapsed time API based on the performance counter in the TimerLib
would be a good addition to the TimerLib and would provide a common implementation
of the feature.  May need an input parameter to pass in the start time to measure
from so it can work in BASE implementations that can not assume writable globals.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> Brown
> Sent: Thursday, July 6, 2023 8:47 AM
> To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
> Timer for XHCI Timeouts
> 
> On 06/07/2023 15:19, Henz, Patrick wrote:
> > I agree that XhcGetElapsedTime() would be better off in TimerLib, but I
> wasn't sure how the community would feel about adding to the interface.
> 
> My understanding is that the TimerLib API is not prescribed by any
> standards document, and that this change would add a new function
> without changing any existing functions and so would not break any
> existing users of TimerLib.  On that basis, I would be firmly in favour
> of extending TimerLib to include this functionality.
> 
> I will defer to actual EDK2 maintainers on this, however.  :)
> 
> Thanks,
> 
> Michael
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Wu, Hao A]

For the 2 occurrences of in XhcWaitOpRegBit & XhcExecTransfer:
  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout * XHC_1_MILLISECOND);
How about changing them to:
  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND ((UINT64) Timeout * XHC_1_MILLISECOND);
to address possible overflow during "Timeout * XHC_1_MILLISECOND"?

For extending XhcGetElapsedTime as a TimerLib API, I am fine to put it in XhciDxe at this moment.
If package maintainers suggest to make it as a public library API, my take is that this should be done in a separate commit.

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> Patrick
> Sent: Thursday, July 6, 2023 4:16 AM
> To: devel@edk2.groups.io
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
> Timer for XHCI Timeouts
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2948
> 
> XhciDxe uses the timer functionality provided by the
> boot services table to detect timeout conditions. This
> breaks the driver's ExitBootServices call back, as
> CoreExitBootServices halts the timer before signaling
> the ExitBootServices event. If the host controller
> fails to halt in the call back, the timeout condition
> will never occur and the boot gets stuck in an indefinite
> spin loop. Use the free running timer provided by
> TimerLib to calculate timeouts, avoiding the potential
> hang.
> 
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> Reviewed-by:
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68 +++++++++---------------
>  5 files changed, 129 insertions(+), 86 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index 62682dd27c..1dcbe20512 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -1,6 +1,7 @@
>  /** @file
> 
>    The XHCI controller driver.
> 
> 
> 
> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  Copyright (c) 2011 - 2022, Intel Corporation. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
> @@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
> 
> 
>    return EFI_SUCCESS;
> 
>  }
> 
> +
> 
> +/**
> 
> +  Computes and returns the elapsed time in nanoseconds since
> 
> +  PreviousTick. The value of PreviousTick is overwritten with the
> 
> +  current performance counter value.
> 
> +
> 
> +  @param  PreviousTick    Pointer to PreviousTick count.
> 
> +  @return The elapsed time in nanoseconds since PreviousCount.
> 
> +          PreviousCount is overwritten with the current performance
> 
> +          counter value.
> 
> +**/
> 
> +UINT64
> 
> +XhcGetElapsedTime (
> 
> +  IN OUT UINT64  *PreviousTick
> 
> +  )
> 
> +{
> 
> +  UINT64  StartValue;
> 
> +  UINT64  EndValue;
> 
> +  UINT64  CurrentTick;
> 
> +  UINT64  Delta;
> 
> +
> 
> +  GetPerformanceCounterProperties (&StartValue, &EndValue);
> 
> +
> 
> +  CurrentTick = GetPerformanceCounter ();
> 
> +
> 
> +  //
> 
> +  // Determine if the counter is counting up or down
> 
> +  //
> 
> +  if (StartValue < EndValue) {
> 
> +    //
> 
> +    // Counter counts upwards, check for an overflow condition
> 
> +    //
> 
> +    if (*PreviousTick > CurrentTick) {
> 
> +      Delta = (EndValue - *PreviousTick) + CurrentTick;
> 
> +    } else {
> 
> +      Delta = CurrentTick - *PreviousTick;
> 
> +    }
> 
> +  } else {
> 
> +    //
> 
> +    // Counter counts downwards, check for an underflow condition
> 
> +    //
> 
> +    if (*PreviousTick < CurrentTick) {
> 
> +      Delta = (StartValue - CurrentTick) + *PreviousTick;
> 
> +    } else {
> 
> +      Delta = *PreviousTick - CurrentTick;
> 
> +    }
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // Set PreviousTick to CurrentTick
> 
> +  //
> 
> +  *PreviousTick = CurrentTick;
> 
> +
> 
> +  return GetTimeInNanoSecond (Delta);
> 
> +}
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> index ca223bd20c..77feb66647 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> @@ -2,6 +2,7 @@
> 
> 
>    Provides some data structure definitions used by the XHCI host controller
> driver.
> 
> 
> 
> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.<BR>
> 
>  Copyright (c) Microsoft Corporation.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Library/UefiLib.h>
> 
>  #include <Library/DebugLib.h>
> 
>  #include <Library/ReportStatusCodeLib.h>
> 
> +#include <Library/TimerLib.h>
> 
> 
> 
>  #include <IndustryStandard/Pci.h>
> 
> 
> 
> @@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT
> USB_DEV_CONTEXT;
>  #include "ComponentName.h"
> 
>  #include "UsbHcMem.h"
> 
> 
> 
> +//
> 
> +// Converts a count from microseconds to nanoseconds
> 
> +//
> 
> +#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time) *
> 1000)
> 
>  //
> 
>  // The unit is microsecond, setting it as 1us.
> 
>  //
> 
> @@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
>    IN     VOID                                *Context
> 
>    );
> 
> 
> 
> +/**
> 
> +  Computes and returns the elapsed time in nanoseconds since
> 
> +  PreviousTick. The value of PreviousTick is overwritten with the
> 
> +  current performance counter value.
> 
> +
> 
> +  @param  PreviousTick    Pointer to PreviousTick count.
> 
> +                          before calling this function.
> 
> +  @return The elapsed time in nanoseconds since PreviousCount.
> 
> +          PreviousCount is overwritten with the current performance
> 
> +          counter value.
> 
> +**/
> 
> +UINT64
> 
> +XhcGetElapsedTime (
> 
> +  IN OUT UINT64  *PreviousTick
> 
> +  );
> 
> +
> 
>  #endif
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> index 5865d86822..18ef87916a 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> @@ -3,6 +3,7 @@
>  #  It implements the interfaces of monitoring the status of all ports and
> transferring
> 
>  #  Control, Bulk, Interrupt and Isochronous requests to those attached usb
> LS/FS/HS/SS devices.
> 
>  #
> 
> +#  (C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  #  Copyright (c) 2011 - 2018, Intel Corporation. All rights reserved.<BR>
> 
>  #
> 
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -54,6 +55,7 @@
>    BaseMemoryLib
> 
>    DebugLib
> 
>    ReportStatusCodeLib
> 
> +  TimerLib
> 
> 
> 
>  [Guids]
> 
>    gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
> Event
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 5700fc5fb8..2499698715 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -2,6 +2,7 @@
> 
> 
>    The XHCI register operation routines.
> 
> 
> 
> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
> @@ -417,15 +418,14 @@ XhcClearOpRegBit (
>    Wait the operation register's bit as specified by Bit
> 
>    to become set (or clear).
> 
> 
> 
> -  @param  Xhc                    The XHCI Instance.
> 
> -  @param  Offset                 The offset of the operation register.
> 
> -  @param  Bit                    The bit of the register to wait for.
> 
> -  @param  WaitToSet              Wait the bit to set or clear.
> 
> -  @param  Timeout                The time to wait before abort (in millisecond,
> ms).
> 
> +  @param  Xhc          The XHCI Instance.
> 
> +  @param  Offset       The offset of the operation register.
> 
> +  @param  Bit          The bit of the register to wait for.
> 
> +  @param  WaitToSet    Wait the bit to set or clear.
> 
> +  @param  Timeout      The time to wait before abort (in millisecond, ms).
> 
> 
> 
> -  @retval EFI_SUCCESS            The bit successfully changed by host controller.
> 
> -  @retval EFI_TIMEOUT            The time out occurred.
> 
> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not
> be allocated.
> 
> +  @retval EFI_SUCCESS  The bit successfully changed by host controller.
> 
> +  @retval EFI_TIMEOUT  The time out occurred.
> 
> 
> 
>  **/
> 
>  EFI_STATUS
> 
> @@ -437,54 +437,35 @@ XhcWaitOpRegBit (
>    IN UINT32             Timeout
> 
>    )
> 
>  {
> 
> -  EFI_STATUS  Status;
> 
> -  EFI_EVENT   TimeoutEvent;
> 
> -
> 
> -  TimeoutEvent = NULL;
> 
> +  UINT64  TimeoutTime;
> 
> +  UINT64  ElapsedTime;
> 
> +  UINT64  TimeDelta;
> 
> +  UINT64  CurrentTick;
> 
> 
> 
>    if (Timeout == 0) {
> 
>      return EFI_TIMEOUT;
> 
>    }
> 
> 
> 
> -  Status = gBS->CreateEvent (
> 
> -                  EVT_TIMER,
> 
> -                  TPL_CALLBACK,
> 
> -                  NULL,
> 
> -                  NULL,
> 
> -                  &TimeoutEvent
> 
> -                  );
> 
> -
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    goto DONE;
> 
> -  }
> 
> -
> 
> -  Status = gBS->SetTimer (
> 
> -                  TimeoutEvent,
> 
> -                  TimerRelative,
> 
> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> 
> -                  );
> 
> -
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    goto DONE;
> 
> -  }
> 
> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> XHC_1_MILLISECOND);
> 
> +  ElapsedTime = 0;
> 
> +  CurrentTick = GetPerformanceCounter ();
> 
> 
> 
>    do {
> 
>      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> 
> -      Status = EFI_SUCCESS;
> 
> -      goto DONE;
> 
> +      return EFI_SUCCESS;
> 
>      }
> 
> 
> 
>      gBS->Stall (XHC_1_MICROSECOND);
> 
> -  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
> 
> -
> 
> -  Status = EFI_TIMEOUT;
> 
> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> 
> +    // Ensure that ElapsedTime is always incremented to avoid indefinite
> hangs
> 
> +    if (TimeDelta == 0) {
> 
> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> (XHC_1_MICROSECOND);
> 
> +    }
> 
> 
> 
> -DONE:
> 
> -  if (TimeoutEvent != NULL) {
> 
> -    gBS->CloseEvent (TimeoutEvent);
> 
> -  }
> 
> +    ElapsedTime += TimeDelta;
> 
> +  } while (ElapsedTime < TimeoutTime);
> 
> 
> 
> -  return Status;
> 
> +  return EFI_TIMEOUT;
> 
>  }
> 
> 
> 
>  /**
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 298fb88b81..5177886e52 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -2,6 +2,7 @@
> 
> 
>    XHCI transfer scheduling routines.
> 
> 
> 
> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  Copyright (c) 2011 - 2020, Intel Corporation. All rights reserved.<BR>
> 
>  Copyright (c) Microsoft Corporation.<BR>
> 
>  Copyright (C) 2022 Advanced Micro Devices, Inc. All rights reserved.<BR>
> 
> @@ -1273,15 +1274,14 @@ EXIT:
>  /**
> 
>    Execute the transfer by polling the URB. This is a synchronous operation.
> 
> 
> 
> -  @param  Xhc                    The XHCI Instance.
> 
> -  @param  CmdTransfer            The executed URB is for cmd transfer or not.
> 
> -  @param  Urb                    The URB to execute.
> 
> -  @param  Timeout                The time to wait before abort, in millisecond.
> 
> +  @param  Xhc               The XHCI Instance.
> 
> +  @param  CmdTransfer       The executed URB is for cmd transfer or not.
> 
> +  @param  Urb               The URB to execute.
> 
> +  @param  Timeout           The time to wait before abort, in millisecond.
> 
> 
> 
> -  @return EFI_DEVICE_ERROR       The transfer failed due to transfer error.
> 
> -  @return EFI_TIMEOUT            The transfer failed due to time out.
> 
> -  @return EFI_SUCCESS            The transfer finished OK.
> 
> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not
> be allocated.
> 
> +  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
> 
> +  @return EFI_TIMEOUT       The transfer failed due to time out.
> 
> +  @return EFI_SUCCESS       The transfer finished OK.
> 
> 
> 
>  **/
> 
>  EFI_STATUS
> 
> @@ -1296,12 +1296,14 @@ XhcExecTransfer (
>    UINT8       SlotId;
> 
>    UINT8       Dci;
> 
>    BOOLEAN     Finished;
> 
> -  EFI_EVENT   TimeoutEvent;
> 
> +  UINT64      TimeoutTime;
> 
> +  UINT64      ElapsedTime;
> 
> +  UINT64      TimeDelta;
> 
> +  UINT64      CurrentTick;
> 
>    BOOLEAN     IndefiniteTimeout;
> 
> 
> 
>    Status            = EFI_SUCCESS;
> 
>    Finished          = FALSE;
> 
> -  TimeoutEvent      = NULL;
> 
>    IndefiniteTimeout = FALSE;
> 
> 
> 
>    if (CmdTransfer) {
> 
> @@ -1319,34 +1321,14 @@ XhcExecTransfer (
> 
> 
>    if (Timeout == 0) {
> 
>      IndefiniteTimeout = TRUE;
> 
> -    goto RINGDOORBELL;
> 
> -  }
> 
> -
> 
> -  Status = gBS->CreateEvent (
> 
> -                  EVT_TIMER,
> 
> -                  TPL_CALLBACK,
> 
> -                  NULL,
> 
> -                  NULL,
> 
> -                  &TimeoutEvent
> 
> -                  );
> 
> -
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    goto DONE;
> 
>    }
> 
> 
> 
> -  Status = gBS->SetTimer (
> 
> -                  TimeoutEvent,
> 
> -                  TimerRelative,
> 
> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> 
> -                  );
> 
> -
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    goto DONE;
> 
> -  }
> 
> -
> 
> -RINGDOORBELL:
> 
>    XhcRingDoorBell (Xhc, SlotId, Dci);
> 
> 
> 
> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> XHC_1_MILLISECOND);
> 
> +  ElapsedTime = 0;
> 
> +  CurrentTick = GetPerformanceCounter ();
> 
> +
> 
>    do {
> 
>      Finished = XhcCheckUrbResult (Xhc, Urb);
> 
>      if (Finished) {
> 
> @@ -1354,22 +1336,22 @@ RINGDOORBELL:
>      }
> 
> 
> 
>      gBS->Stall (XHC_1_MICROSECOND);
> 
> -  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent
> (TimeoutEvent)));
> 
> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> 
> +    // Ensure that ElapsedTime is always incremented to avoid indefinite
> hangs
> 
> +    if (TimeDelta == 0) {
> 
> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> (XHC_1_MICROSECOND);
> 
> +    }
> 
> 
> 
> -DONE:
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
> 
> -  } else if (!Finished) {
> 
> +    ElapsedTime += TimeDelta;
> 
> +  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
> 
> +
> 
> +  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.34.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#106668):
> https://edk2.groups.io/g/devel/message/106668
> Mute This Topic: https://groups.io/mt/99972791/1768737
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107390): https://edk2.groups.io/g/devel/message/107390
Mute This Topic: https://groups.io/mt/99972791/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Henz, Patrick]

I can certainly make that change.

For what it's worth I have been working on adding the new function, GetElapsedTicks, to the various implementations of TimerLib. I've finished up testing, I would just need to clean up the commits for a patch. Should I move forward with that, or would we rather I just add XhcGetElapsedTime to XhciDxe for the time being?

Thanks,
Patrick Henz

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
Sent: Sunday, July 30, 2023 9:57 PM
To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>; Michael Brown <mcb30@ipxe.org>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

For the 2 occurrences of in XhcWaitOpRegBit & XhcExecTransfer:
  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout * XHC_1_MILLISECOND); How about changing them to:
  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND ((UINT64) Timeout * XHC_1_MILLISECOND); to address possible overflow during "Timeout * XHC_1_MILLISECOND"?

For extending XhcGetElapsedTime as a TimerLib API, I am fine to put it in XhciDxe at this moment.
If package maintainers suggest to make it as a public library API, my take is that this should be done in a separate commit.

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz, 
> Patrick
> Sent: Thursday, July 6, 2023 4:16 AM
> To: devel@edk2.groups.io
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance 
> Timer for XHCI Timeouts
> 
> REF:INVALID URI REMOVED
> g.cgi?id=2948__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-lctg5
> _B3K1Lcta452Gx-1twRt8At3cueAYDq_n$
> 
> XhciDxe uses the timer functionality provided by the boot services 
> table to detect timeout conditions. This breaks the driver's 
> ExitBootServices call back, as CoreExitBootServices halts the timer 
> before signaling the ExitBootServices event. If the host controller 
> fails to halt in the call back, the timeout condition will never occur 
> and the boot gets stuck in an indefinite spin loop. Use the free 
> running timer provided by TimerLib to calculate timeouts, avoiding the 
> potential hang.
> 
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> Reviewed-by:
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68 
> +++++++++---------------
>  5 files changed, 129 insertions(+), 86 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index 62682dd27c..1dcbe20512 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -1,6 +1,7 @@
>  /** @file
> 
>    The XHCI controller driver.
> 
> 
> 
> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  Copyright (c) 2011 - 2022, Intel Corporation. All rights 
> reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
> @@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
> 
> 
>    return EFI_SUCCESS;
> 
>  }
> 
> +
> 
> +/**
> 
> +  Computes and returns the elapsed time in nanoseconds since
> 
> +  PreviousTick. The value of PreviousTick is overwritten with the
> 
> +  current performance counter value.
> 
> +
> 
> +  @param  PreviousTick    Pointer to PreviousTick count.
> 
> +  @return The elapsed time in nanoseconds since PreviousCount.
> 
> +          PreviousCount is overwritten with the current performance
> 
> +          counter value.
> 
> +**/
> 
> +UINT64
> 
> +XhcGetElapsedTime (
> 
> +  IN OUT UINT64  *PreviousTick
> 
> +  )
> 
> +{
> 
> +  UINT64  StartValue;
> 
> +  UINT64  EndValue;
> 
> +  UINT64  CurrentTick;
> 
> +  UINT64  Delta;
> 
> +
> 
> +  GetPerformanceCounterProperties (&StartValue, &EndValue);
> 
> +
> 
> +  CurrentTick = GetPerformanceCounter ();
> 
> +
> 
> +  //
> 
> +  // Determine if the counter is counting up or down
> 
> +  //
> 
> +  if (StartValue < EndValue) {
> 
> +    //
> 
> +    // Counter counts upwards, check for an overflow condition
> 
> +    //
> 
> +    if (*PreviousTick > CurrentTick) {
> 
> +      Delta = (EndValue - *PreviousTick) + CurrentTick;
> 
> +    } else {
> 
> +      Delta = CurrentTick - *PreviousTick;
> 
> +    }
> 
> +  } else {
> 
> +    //
> 
> +    // Counter counts downwards, check for an underflow condition
> 
> +    //
> 
> +    if (*PreviousTick < CurrentTick) {
> 
> +      Delta = (StartValue - CurrentTick) + *PreviousTick;
> 
> +    } else {
> 
> +      Delta = *PreviousTick - CurrentTick;
> 
> +    }
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // Set PreviousTick to CurrentTick
> 
> +  //
> 
> +  *PreviousTick = CurrentTick;
> 
> +
> 
> +  return GetTimeInNanoSecond (Delta);
> 
> +}
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> index ca223bd20c..77feb66647 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> @@ -2,6 +2,7 @@
> 
> 
>    Provides some data structure definitions used by the XHCI host 
> controller driver.
> 
> 
> 
> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  Copyright (c) 2011 - 2017, Intel Corporation. All rights 
> reserved.<BR>
> 
>  Copyright (c) Microsoft Corporation.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  
> #include <Library/UefiLib.h>
> 
>  #include <Library/DebugLib.h>
> 
>  #include <Library/ReportStatusCodeLib.h>
> 
> +#include <Library/TimerLib.h>
> 
> 
> 
>  #include <IndustryStandard/Pci.h>
> 
> 
> 
> @@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT USB_DEV_CONTEXT;  
> #include "ComponentName.h"
> 
>  #include "UsbHcMem.h"
> 
> 
> 
> +//
> 
> +// Converts a count from microseconds to nanoseconds
> 
> +//
> 
> +#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time) *
> 1000)
> 
>  //
> 
>  // The unit is microsecond, setting it as 1us.
> 
>  //
> 
> @@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
>    IN     VOID                                *Context
> 
>    );
> 
> 
> 
> +/**
> 
> +  Computes and returns the elapsed time in nanoseconds since
> 
> +  PreviousTick. The value of PreviousTick is overwritten with the
> 
> +  current performance counter value.
> 
> +
> 
> +  @param  PreviousTick    Pointer to PreviousTick count.
> 
> +                          before calling this function.
> 
> +  @return The elapsed time in nanoseconds since PreviousCount.
> 
> +          PreviousCount is overwritten with the current performance
> 
> +          counter value.
> 
> +**/
> 
> +UINT64
> 
> +XhcGetElapsedTime (
> 
> +  IN OUT UINT64  *PreviousTick
> 
> +  );
> 
> +
> 
>  #endif
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> index 5865d86822..18ef87916a 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> @@ -3,6 +3,7 @@
>  #  It implements the interfaces of monitoring the status of all ports 
> and transferring
> 
>  #  Control, Bulk, Interrupt and Isochronous requests to those 
> attached usb LS/FS/HS/SS devices.
> 
>  #
> 
> +#  (C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  #  Copyright (c) 2011 - 2018, Intel Corporation. All rights 
> reserved.<BR>
> 
>  #
> 
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -54,6 +55,7 @@
>    BaseMemoryLib
> 
>    DebugLib
> 
>    ReportStatusCodeLib
> 
> +  TimerLib
> 
> 
> 
>  [Guids]
> 
>    gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
> Event
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 5700fc5fb8..2499698715 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -2,6 +2,7 @@
> 
> 
>    The XHCI register operation routines.
> 
> 
> 
> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  Copyright (c) 2011 - 2017, Intel Corporation. All rights 
> reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
> @@ -417,15 +418,14 @@ XhcClearOpRegBit (
>    Wait the operation register's bit as specified by Bit
> 
>    to become set (or clear).
> 
> 
> 
> -  @param  Xhc                    The XHCI Instance.
> 
> -  @param  Offset                 The offset of the operation register.
> 
> -  @param  Bit                    The bit of the register to wait for.
> 
> -  @param  WaitToSet              Wait the bit to set or clear.
> 
> -  @param  Timeout                The time to wait before abort (in millisecond,
> ms).
> 
> +  @param  Xhc          The XHCI Instance.
> 
> +  @param  Offset       The offset of the operation register.
> 
> +  @param  Bit          The bit of the register to wait for.
> 
> +  @param  WaitToSet    Wait the bit to set or clear.
> 
> +  @param  Timeout      The time to wait before abort (in millisecond, ms).
> 
> 
> 
> -  @retval EFI_SUCCESS            The bit successfully changed by host controller.
> 
> -  @retval EFI_TIMEOUT            The time out occurred.
> 
> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not
> be allocated.
> 
> +  @retval EFI_SUCCESS  The bit successfully changed by host controller.
> 
> +  @retval EFI_TIMEOUT  The time out occurred.
> 
> 
> 
>  **/
> 
>  EFI_STATUS
> 
> @@ -437,54 +437,35 @@ XhcWaitOpRegBit (
>    IN UINT32             Timeout
> 
>    )
> 
>  {
> 
> -  EFI_STATUS  Status;
> 
> -  EFI_EVENT   TimeoutEvent;
> 
> -
> 
> -  TimeoutEvent = NULL;
> 
> +  UINT64  TimeoutTime;
> 
> +  UINT64  ElapsedTime;
> 
> +  UINT64  TimeDelta;
> 
> +  UINT64  CurrentTick;
> 
> 
> 
>    if (Timeout == 0) {
> 
>      return EFI_TIMEOUT;
> 
>    }
> 
> 
> 
> -  Status = gBS->CreateEvent (
> 
> -                  EVT_TIMER,
> 
> -                  TPL_CALLBACK,
> 
> -                  NULL,
> 
> -                  NULL,
> 
> -                  &TimeoutEvent
> 
> -                  );
> 
> -
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    goto DONE;
> 
> -  }
> 
> -
> 
> -  Status = gBS->SetTimer (
> 
> -                  TimeoutEvent,
> 
> -                  TimerRelative,
> 
> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> 
> -                  );
> 
> -
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    goto DONE;
> 
> -  }
> 
> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> XHC_1_MILLISECOND);
> 
> +  ElapsedTime = 0;
> 
> +  CurrentTick = GetPerformanceCounter ();
> 
> 
> 
>    do {
> 
>      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> 
> -      Status = EFI_SUCCESS;
> 
> -      goto DONE;
> 
> +      return EFI_SUCCESS;
> 
>      }
> 
> 
> 
>      gBS->Stall (XHC_1_MICROSECOND);
> 
> -  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
> 
> -
> 
> -  Status = EFI_TIMEOUT;
> 
> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> 
> +    // Ensure that ElapsedTime is always incremented to avoid 
> + indefinite
> hangs
> 
> +    if (TimeDelta == 0) {
> 
> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> (XHC_1_MICROSECOND);
> 
> +    }
> 
> 
> 
> -DONE:
> 
> -  if (TimeoutEvent != NULL) {
> 
> -    gBS->CloseEvent (TimeoutEvent);
> 
> -  }
> 
> +    ElapsedTime += TimeDelta;
> 
> +  } while (ElapsedTime < TimeoutTime);
> 
> 
> 
> -  return Status;
> 
> +  return EFI_TIMEOUT;
> 
>  }
> 
> 
> 
>  /**
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 298fb88b81..5177886e52 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -2,6 +2,7 @@
> 
> 
>    XHCI transfer scheduling routines.
> 
> 
> 
> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> 
>  Copyright (c) 2011 - 2020, Intel Corporation. All rights 
> reserved.<BR>
> 
>  Copyright (c) Microsoft Corporation.<BR>
> 
>  Copyright (C) 2022 Advanced Micro Devices, Inc. All rights 
> reserved.<BR>
> 
> @@ -1273,15 +1274,14 @@ EXIT:
>  /**
> 
>    Execute the transfer by polling the URB. This is a synchronous operation.
> 
> 
> 
> -  @param  Xhc                    The XHCI Instance.
> 
> -  @param  CmdTransfer            The executed URB is for cmd transfer or not.
> 
> -  @param  Urb                    The URB to execute.
> 
> -  @param  Timeout                The time to wait before abort, in millisecond.
> 
> +  @param  Xhc               The XHCI Instance.
> 
> +  @param  CmdTransfer       The executed URB is for cmd transfer or not.
> 
> +  @param  Urb               The URB to execute.
> 
> +  @param  Timeout           The time to wait before abort, in millisecond.
> 
> 
> 
> -  @return EFI_DEVICE_ERROR       The transfer failed due to transfer error.
> 
> -  @return EFI_TIMEOUT            The transfer failed due to time out.
> 
> -  @return EFI_SUCCESS            The transfer finished OK.
> 
> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not
> be allocated.
> 
> +  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
> 
> +  @return EFI_TIMEOUT       The transfer failed due to time out.
> 
> +  @return EFI_SUCCESS       The transfer finished OK.
> 
> 
> 
>  **/
> 
>  EFI_STATUS
> 
> @@ -1296,12 +1296,14 @@ XhcExecTransfer (
>    UINT8       SlotId;
> 
>    UINT8       Dci;
> 
>    BOOLEAN     Finished;
> 
> -  EFI_EVENT   TimeoutEvent;
> 
> +  UINT64      TimeoutTime;
> 
> +  UINT64      ElapsedTime;
> 
> +  UINT64      TimeDelta;
> 
> +  UINT64      CurrentTick;
> 
>    BOOLEAN     IndefiniteTimeout;
> 
> 
> 
>    Status            = EFI_SUCCESS;
> 
>    Finished          = FALSE;
> 
> -  TimeoutEvent      = NULL;
> 
>    IndefiniteTimeout = FALSE;
> 
> 
> 
>    if (CmdTransfer) {
> 
> @@ -1319,34 +1321,14 @@ XhcExecTransfer (
> 
> 
>    if (Timeout == 0) {
> 
>      IndefiniteTimeout = TRUE;
> 
> -    goto RINGDOORBELL;
> 
> -  }
> 
> -
> 
> -  Status = gBS->CreateEvent (
> 
> -                  EVT_TIMER,
> 
> -                  TPL_CALLBACK,
> 
> -                  NULL,
> 
> -                  NULL,
> 
> -                  &TimeoutEvent
> 
> -                  );
> 
> -
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    goto DONE;
> 
>    }
> 
> 
> 
> -  Status = gBS->SetTimer (
> 
> -                  TimeoutEvent,
> 
> -                  TimerRelative,
> 
> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> 
> -                  );
> 
> -
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    goto DONE;
> 
> -  }
> 
> -
> 
> -RINGDOORBELL:
> 
>    XhcRingDoorBell (Xhc, SlotId, Dci);
> 
> 
> 
> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> XHC_1_MILLISECOND);
> 
> +  ElapsedTime = 0;
> 
> +  CurrentTick = GetPerformanceCounter ();
> 
> +
> 
>    do {
> 
>      Finished = XhcCheckUrbResult (Xhc, Urb);
> 
>      if (Finished) {
> 
> @@ -1354,22 +1336,22 @@ RINGDOORBELL:
>      }
> 
> 
> 
>      gBS->Stall (XHC_1_MICROSECOND);
> 
> -  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent 
> (TimeoutEvent)));
> 
> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> 
> +    // Ensure that ElapsedTime is always incremented to avoid 
> + indefinite
> hangs
> 
> +    if (TimeDelta == 0) {
> 
> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> (XHC_1_MICROSECOND);
> 
> +    }
> 
> 
> 
> -DONE:
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
> 
> -  } else if (!Finished) {
> 
> +    ElapsedTime += TimeDelta;
> 
> +  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
> 
> +
> 
> +  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.34.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#106668):
> INVALID URI REMOVED
> 668__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-lctg5_B3K1Lcta4
> 52Gx-1twRt8At3cuWxQsO18$ Mute This Topic: 
> https://groups.io/mt/99972791/1768737
> NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-lctg5_B3K1Lcta452Gx-1tw
> Rt8At3cubh2HDyk$ Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: 
> https://edk2.groups.io/g/devel/unsub
> pxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-lctg5_B3K1Lcta452Gx-1twR
> t8At3cuWYMMXVU$  [hao.a.wu@intel.com] -=-=-=-=-=-=
> 








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107702): https://edk2.groups.io/g/devel/message/107702
Mute This Topic: https://groups.io/mt/99972791/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Wu, Hao A]

My personal preference is to do this as two steps.
Address the issue in XhciDxe first. Then add new TimerLib API to replace all driver/library internal implementations.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Henz, Patrick <patrick.henz@hpe.com>
> Sent: Friday, August 11, 2023 6:45 AM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Michael Brown
> <mcb30@ipxe.org>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
> Timer for XHCI Timeouts
> 
> I can certainly make that change.
> 
> For what it's worth I have been working on adding the new function,
> GetElapsedTicks, to the various implementations of TimerLib. I've finished up
> testing, I would just need to clean up the commits for a patch. Should I move
> forward with that, or would we rather I just add XhcGetElapsedTime to
> XhciDxe for the time being?
> 
> Thanks,
> Patrick Henz
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Sunday, July 30, 2023 9:57 PM
> To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>; Michael
> Brown <mcb30@ipxe.org>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
> Timer for XHCI Timeouts
> 
> For the 2 occurrences of in XhcWaitOpRegBit & XhcExecTransfer:
>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> XHC_1_MILLISECOND); How about changing them to:
>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND ((UINT64) Timeout
> * XHC_1_MILLISECOND); to address possible overflow during "Timeout *
> XHC_1_MILLISECOND"?
> 
> For extending XhcGetElapsedTime as a TimerLib API, I am fine to put it in
> XhciDxe at this moment.
> If package maintainers suggest to make it as a public library API, my take is
> that this should be done in a separate commit.
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > Patrick
> > Sent: Thursday, July 6, 2023 4:16 AM
> > To: devel@edk2.groups.io
> > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
> > Timer for XHCI Timeouts
> >
> > REF:https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bu
> > g.cgi?id=2948__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-
> NKbMjOV-lctg5
> > _B3K1Lcta452Gx-1twRt8At3cueAYDq_n$
> >
> > XhciDxe uses the timer functionality provided by the boot services
> > table to detect timeout conditions. This breaks the driver's
> > ExitBootServices call back, as CoreExitBootServices halts the timer
> > before signaling the ExitBootServices event. If the host controller
> > fails to halt in the call back, the timeout condition will never occur
> > and the boot gets stuck in an indefinite spin loop. Use the free
> > running timer provided by TimerLib to calculate timeouts, avoiding the
> > potential hang.
> >
> > Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> > Reviewed-by:
> > ---
> >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
> >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68
> > +++++++++---------------
> >  5 files changed, 129 insertions(+), 86 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > index 62682dd27c..1dcbe20512 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > @@ -1,6 +1,7 @@
> >  /** @file
> >
> >    The XHCI controller driver.
> >
> >
> >
> > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  Copyright (c) 2011 - 2022, Intel Corporation. All rights
> > reserved.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> > @@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
> >
> >
> >    return EFI_SUCCESS;
> >
> >  }
> >
> > +
> >
> > +/**
> >
> > +  Computes and returns the elapsed time in nanoseconds since
> >
> > +  PreviousTick. The value of PreviousTick is overwritten with the
> >
> > +  current performance counter value.
> >
> > +
> >
> > +  @param  PreviousTick    Pointer to PreviousTick count.
> >
> > +  @return The elapsed time in nanoseconds since PreviousCount.
> >
> > +          PreviousCount is overwritten with the current performance
> >
> > +          counter value.
> >
> > +**/
> >
> > +UINT64
> >
> > +XhcGetElapsedTime (
> >
> > +  IN OUT UINT64  *PreviousTick
> >
> > +  )
> >
> > +{
> >
> > +  UINT64  StartValue;
> >
> > +  UINT64  EndValue;
> >
> > +  UINT64  CurrentTick;
> >
> > +  UINT64  Delta;
> >
> > +
> >
> > +  GetPerformanceCounterProperties (&StartValue, &EndValue);
> >
> > +
> >
> > +  CurrentTick = GetPerformanceCounter ();
> >
> > +
> >
> > +  //
> >
> > +  // Determine if the counter is counting up or down
> >
> > +  //
> >
> > +  if (StartValue < EndValue) {
> >
> > +    //
> >
> > +    // Counter counts upwards, check for an overflow condition
> >
> > +    //
> >
> > +    if (*PreviousTick > CurrentTick) {
> >
> > +      Delta = (EndValue - *PreviousTick) + CurrentTick;
> >
> > +    } else {
> >
> > +      Delta = CurrentTick - *PreviousTick;
> >
> > +    }
> >
> > +  } else {
> >
> > +    //
> >
> > +    // Counter counts downwards, check for an underflow condition
> >
> > +    //
> >
> > +    if (*PreviousTick < CurrentTick) {
> >
> > +      Delta = (StartValue - CurrentTick) + *PreviousTick;
> >
> > +    } else {
> >
> > +      Delta = *PreviousTick - CurrentTick;
> >
> > +    }
> >
> > +  }
> >
> > +
> >
> > +  //
> >
> > +  // Set PreviousTick to CurrentTick
> >
> > +  //
> >
> > +  *PreviousTick = CurrentTick;
> >
> > +
> >
> > +  return GetTimeInNanoSecond (Delta);
> >
> > +}
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > index ca223bd20c..77feb66647 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > @@ -2,6 +2,7 @@
> >
> >
> >    Provides some data structure definitions used by the XHCI host
> > controller driver.
> >
> >
> >
> > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  Copyright (c) 2011 - 2017, Intel Corporation. All rights
> > reserved.<BR>
> >
> >  Copyright (c) Microsoft Corporation.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > #include <Library/UefiLib.h>
> >
> >  #include <Library/DebugLib.h>
> >
> >  #include <Library/ReportStatusCodeLib.h>
> >
> > +#include <Library/TimerLib.h>
> >
> >
> >
> >  #include <IndustryStandard/Pci.h>
> >
> >
> >
> > @@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT
> USB_DEV_CONTEXT;
> > #include "ComponentName.h"
> >
> >  #include "UsbHcMem.h"
> >
> >
> >
> > +//
> >
> > +// Converts a count from microseconds to nanoseconds
> >
> > +//
> >
> > +#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time) *
> > 1000)
> >
> >  //
> >
> >  // The unit is microsecond, setting it as 1us.
> >
> >  //
> >
> > @@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
> >    IN     VOID                                *Context
> >
> >    );
> >
> >
> >
> > +/**
> >
> > +  Computes and returns the elapsed time in nanoseconds since
> >
> > +  PreviousTick. The value of PreviousTick is overwritten with the
> >
> > +  current performance counter value.
> >
> > +
> >
> > +  @param  PreviousTick    Pointer to PreviousTick count.
> >
> > +                          before calling this function.
> >
> > +  @return The elapsed time in nanoseconds since PreviousCount.
> >
> > +          PreviousCount is overwritten with the current performance
> >
> > +          counter value.
> >
> > +**/
> >
> > +UINT64
> >
> > +XhcGetElapsedTime (
> >
> > +  IN OUT UINT64  *PreviousTick
> >
> > +  );
> >
> > +
> >
> >  #endif
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > index 5865d86822..18ef87916a 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > @@ -3,6 +3,7 @@
> >  #  It implements the interfaces of monitoring the status of all ports
> > and transferring
> >
> >  #  Control, Bulk, Interrupt and Isochronous requests to those
> > attached usb LS/FS/HS/SS devices.
> >
> >  #
> >
> > +#  (C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  #  Copyright (c) 2011 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> >
> >  #
> >
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -54,6 +55,7 @@
> >    BaseMemoryLib
> >
> >    DebugLib
> >
> >    ReportStatusCodeLib
> >
> > +  TimerLib
> >
> >
> >
> >  [Guids]
> >
> >    gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
> > Event
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > index 5700fc5fb8..2499698715 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > @@ -2,6 +2,7 @@
> >
> >
> >    The XHCI register operation routines.
> >
> >
> >
> > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  Copyright (c) 2011 - 2017, Intel Corporation. All rights
> > reserved.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> > @@ -417,15 +418,14 @@ XhcClearOpRegBit (
> >    Wait the operation register's bit as specified by Bit
> >
> >    to become set (or clear).
> >
> >
> >
> > -  @param  Xhc                    The XHCI Instance.
> >
> > -  @param  Offset                 The offset of the operation register.
> >
> > -  @param  Bit                    The bit of the register to wait for.
> >
> > -  @param  WaitToSet              Wait the bit to set or clear.
> >
> > -  @param  Timeout                The time to wait before abort (in millisecond,
> > ms).
> >
> > +  @param  Xhc          The XHCI Instance.
> >
> > +  @param  Offset       The offset of the operation register.
> >
> > +  @param  Bit          The bit of the register to wait for.
> >
> > +  @param  WaitToSet    Wait the bit to set or clear.
> >
> > +  @param  Timeout      The time to wait before abort (in millisecond, ms).
> >
> >
> >
> > -  @retval EFI_SUCCESS            The bit successfully changed by host
> controller.
> >
> > -  @retval EFI_TIMEOUT            The time out occurred.
> >
> > -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not
> > be allocated.
> >
> > +  @retval EFI_SUCCESS  The bit successfully changed by host controller.
> >
> > +  @retval EFI_TIMEOUT  The time out occurred.
> >
> >
> >
> >  **/
> >
> >  EFI_STATUS
> >
> > @@ -437,54 +437,35 @@ XhcWaitOpRegBit (
> >    IN UINT32             Timeout
> >
> >    )
> >
> >  {
> >
> > -  EFI_STATUS  Status;
> >
> > -  EFI_EVENT   TimeoutEvent;
> >
> > -
> >
> > -  TimeoutEvent = NULL;
> >
> > +  UINT64  TimeoutTime;
> >
> > +  UINT64  ElapsedTime;
> >
> > +  UINT64  TimeDelta;
> >
> > +  UINT64  CurrentTick;
> >
> >
> >
> >    if (Timeout == 0) {
> >
> >      return EFI_TIMEOUT;
> >
> >    }
> >
> >
> >
> > -  Status = gBS->CreateEvent (
> >
> > -                  EVT_TIMER,
> >
> > -                  TPL_CALLBACK,
> >
> > -                  NULL,
> >
> > -                  NULL,
> >
> > -                  &TimeoutEvent
> >
> > -                  );
> >
> > -
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    goto DONE;
> >
> > -  }
> >
> > -
> >
> > -  Status = gBS->SetTimer (
> >
> > -                  TimeoutEvent,
> >
> > -                  TimerRelative,
> >
> > -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> >
> > -                  );
> >
> > -
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    goto DONE;
> >
> > -  }
> >
> > +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> > XHC_1_MILLISECOND);
> >
> > +  ElapsedTime = 0;
> >
> > +  CurrentTick = GetPerformanceCounter ();
> >
> >
> >
> >    do {
> >
> >      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> >
> > -      Status = EFI_SUCCESS;
> >
> > -      goto DONE;
> >
> > +      return EFI_SUCCESS;
> >
> >      }
> >
> >
> >
> >      gBS->Stall (XHC_1_MICROSECOND);
> >
> > -  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
> >
> > -
> >
> > -  Status = EFI_TIMEOUT;
> >
> > +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> >
> > +    // Ensure that ElapsedTime is always incremented to avoid
> > + indefinite
> > hangs
> >
> > +    if (TimeDelta == 0) {
> >
> > +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> > (XHC_1_MICROSECOND);
> >
> > +    }
> >
> >
> >
> > -DONE:
> >
> > -  if (TimeoutEvent != NULL) {
> >
> > -    gBS->CloseEvent (TimeoutEvent);
> >
> > -  }
> >
> > +    ElapsedTime += TimeDelta;
> >
> > +  } while (ElapsedTime < TimeoutTime);
> >
> >
> >
> > -  return Status;
> >
> > +  return EFI_TIMEOUT;
> >
> >  }
> >
> >
> >
> >  /**
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > index 298fb88b81..5177886e52 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > @@ -2,6 +2,7 @@
> >
> >
> >    XHCI transfer scheduling routines.
> >
> >
> >
> > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  Copyright (c) 2011 - 2020, Intel Corporation. All rights
> > reserved.<BR>
> >
> >  Copyright (c) Microsoft Corporation.<BR>
> >
> >  Copyright (C) 2022 Advanced Micro Devices, Inc. All rights
> > reserved.<BR>
> >
> > @@ -1273,15 +1274,14 @@ EXIT:
> >  /**
> >
> >    Execute the transfer by polling the URB. This is a synchronous operation.
> >
> >
> >
> > -  @param  Xhc                    The XHCI Instance.
> >
> > -  @param  CmdTransfer            The executed URB is for cmd transfer or not.
> >
> > -  @param  Urb                    The URB to execute.
> >
> > -  @param  Timeout                The time to wait before abort, in millisecond.
> >
> > +  @param  Xhc               The XHCI Instance.
> >
> > +  @param  CmdTransfer       The executed URB is for cmd transfer or not.
> >
> > +  @param  Urb               The URB to execute.
> >
> > +  @param  Timeout           The time to wait before abort, in millisecond.
> >
> >
> >
> > -  @return EFI_DEVICE_ERROR       The transfer failed due to transfer error.
> >
> > -  @return EFI_TIMEOUT            The transfer failed due to time out.
> >
> > -  @return EFI_SUCCESS            The transfer finished OK.
> >
> > -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not
> > be allocated.
> >
> > +  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
> >
> > +  @return EFI_TIMEOUT       The transfer failed due to time out.
> >
> > +  @return EFI_SUCCESS       The transfer finished OK.
> >
> >
> >
> >  **/
> >
> >  EFI_STATUS
> >
> > @@ -1296,12 +1296,14 @@ XhcExecTransfer (
> >    UINT8       SlotId;
> >
> >    UINT8       Dci;
> >
> >    BOOLEAN     Finished;
> >
> > -  EFI_EVENT   TimeoutEvent;
> >
> > +  UINT64      TimeoutTime;
> >
> > +  UINT64      ElapsedTime;
> >
> > +  UINT64      TimeDelta;
> >
> > +  UINT64      CurrentTick;
> >
> >    BOOLEAN     IndefiniteTimeout;
> >
> >
> >
> >    Status            = EFI_SUCCESS;
> >
> >    Finished          = FALSE;
> >
> > -  TimeoutEvent      = NULL;
> >
> >    IndefiniteTimeout = FALSE;
> >
> >
> >
> >    if (CmdTransfer) {
> >
> > @@ -1319,34 +1321,14 @@ XhcExecTransfer (
> >
> >
> >    if (Timeout == 0) {
> >
> >      IndefiniteTimeout = TRUE;
> >
> > -    goto RINGDOORBELL;
> >
> > -  }
> >
> > -
> >
> > -  Status = gBS->CreateEvent (
> >
> > -                  EVT_TIMER,
> >
> > -                  TPL_CALLBACK,
> >
> > -                  NULL,
> >
> > -                  NULL,
> >
> > -                  &TimeoutEvent
> >
> > -                  );
> >
> > -
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    goto DONE;
> >
> >    }
> >
> >
> >
> > -  Status = gBS->SetTimer (
> >
> > -                  TimeoutEvent,
> >
> > -                  TimerRelative,
> >
> > -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> >
> > -                  );
> >
> > -
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    goto DONE;
> >
> > -  }
> >
> > -
> >
> > -RINGDOORBELL:
> >
> >    XhcRingDoorBell (Xhc, SlotId, Dci);
> >
> >
> >
> > +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> > XHC_1_MILLISECOND);
> >
> > +  ElapsedTime = 0;
> >
> > +  CurrentTick = GetPerformanceCounter ();
> >
> > +
> >
> >    do {
> >
> >      Finished = XhcCheckUrbResult (Xhc, Urb);
> >
> >      if (Finished) {
> >
> > @@ -1354,22 +1336,22 @@ RINGDOORBELL:
> >      }
> >
> >
> >
> >      gBS->Stall (XHC_1_MICROSECOND);
> >
> > -  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent
> > (TimeoutEvent)));
> >
> > +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> >
> > +    // Ensure that ElapsedTime is always incremented to avoid
> > + indefinite
> > hangs
> >
> > +    if (TimeDelta == 0) {
> >
> > +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> > (XHC_1_MICROSECOND);
> >
> > +    }
> >
> >
> >
> > -DONE:
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
> >
> > -  } else if (!Finished) {
> >
> > +    ElapsedTime += TimeDelta;
> >
> > +  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
> >
> > +
> >
> > +  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.34.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#106668):
> > https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/106
> > 668__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
> lctg5_B3K1Lcta4
> > 52Gx-1twRt8At3cuWxQsO18$ Mute This Topic:
> > https://urldefense.com/v3/__https://groups.io/mt/99972791/1768737__;!!
> > NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
> lctg5_B3K1Lcta452Gx-1tw
> > Rt8At3cubh2HDyk$ Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe:
> > https://urldefense.com/v3/__https://edk2.groups.io/g/devel/unsub__;!!N
> > pxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
> lctg5_B3K1Lcta452Gx-1twR
> > t8At3cuWYMMXVU$  [hao.a.wu@intel.com] -=-=-=-=-=-=
> >
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107704): https://edk2.groups.io/g/devel/message/107704
Mute This Topic: https://groups.io/mt/99972791/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Henz, Patrick]

My changes have been in for a little over a month now, I've been looking into updating the TimerLib API but I'm questioning if it still makes sense to go down that path.  The functions I had implemented, XhcGetElapsedTicks () and XhcConvertTimeToTicks (), now rely heavily on the use of global variables for caching return values from GetPerformanceCounterProperties. As-is these functions won't work for all instances of TimerLib, specifically if the instance is used before memory (RAM) is available. I could implement the functions as they originally were, but I wouldn't be able to replace the Xhc functions without adding additional parameters to said TimerLib functions (e.g. adding Frequency, StartValue, StopValue to ConvertTimeToTicks), which feels clunky to me. Would a new library make sense here? Something that sits between the driver and the TimerLib library? Or do we just leave things as they currently are?

Thanks,
Patrick Henz

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com> 
Sent: Thursday, August 10, 2023 8:43 PM
To: Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io; Michael Brown <mcb30@ipxe.org>
Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

My personal preference is to do this as two steps.
Address the issue in XhciDxe first. Then add new TimerLib API to replace all driver/library internal implementations.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Henz, Patrick <patrick.henz@hpe.com>
> Sent: Friday, August 11, 2023 6:45 AM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Michael 
> Brown <mcb30@ipxe.org>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use 
> Performance Timer for XHCI Timeouts
> 
> I can certainly make that change.
> 
> For what it's worth I have been working on adding the new function, 
> GetElapsedTicks, to the various implementations of TimerLib. I've 
> finished up testing, I would just need to clean up the commits for a 
> patch. Should I move forward with that, or would we rather I just add 
> XhcGetElapsedTime to XhciDxe for the time being?
> 
> Thanks,
> Patrick Henz
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao 
> A
> Sent: Sunday, July 30, 2023 9:57 PM
> To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>; 
> Michael Brown <mcb30@ipxe.org>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use 
> Performance Timer for XHCI Timeouts
> 
> For the 2 occurrences of in XhcWaitOpRegBit & XhcExecTransfer:
>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout * 
> XHC_1_MILLISECOND); How about changing them to:
>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND ((UINT64) Timeout
> * XHC_1_MILLISECOND); to address possible overflow during "Timeout * 
> XHC_1_MILLISECOND"?
> 
> For extending XhcGetElapsedTime as a TimerLib API, I am fine to put it 
> in XhciDxe at this moment.
> If package maintainers suggest to make it as a public library API, my 
> take is that this should be done in a separate commit.
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz, 
> > Patrick
> > Sent: Thursday, July 6, 2023 4:16 AM
> > To: devel@edk2.groups.io
> > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance 
> > Timer for XHCI Timeouts
> >
> > REF:INVALID URI REMOVED
> > bu
> > g.cgi?id=2948__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-
> NKbMjOV-lctg5
> > _B3K1Lcta452Gx-1twRt8At3cueAYDq_n$
> >
> > XhciDxe uses the timer functionality provided by the boot services 
> > table to detect timeout conditions. This breaks the driver's 
> > ExitBootServices call back, as CoreExitBootServices halts the timer 
> > before signaling the ExitBootServices event. If the host controller 
> > fails to halt in the call back, the timeout condition will never 
> > occur and the boot gets stuck in an indefinite spin loop. Use the 
> > free running timer provided by TimerLib to calculate timeouts, 
> > avoiding the potential hang.
> >
> > Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> > Reviewed-by:
> > ---
> >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
> >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68
> > +++++++++---------------
> >  5 files changed, 129 insertions(+), 86 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > index 62682dd27c..1dcbe20512 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > @@ -1,6 +1,7 @@
> >  /** @file
> >
> >    The XHCI controller driver.
> >
> >
> >
> > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  Copyright (c) 2011 - 2022, Intel Corporation. All rights 
> > reserved.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> > @@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
> >
> >
> >    return EFI_SUCCESS;
> >
> >  }
> >
> > +
> >
> > +/**
> >
> > +  Computes and returns the elapsed time in nanoseconds since
> >
> > +  PreviousTick. The value of PreviousTick is overwritten with the
> >
> > +  current performance counter value.
> >
> > +
> >
> > +  @param  PreviousTick    Pointer to PreviousTick count.
> >
> > +  @return The elapsed time in nanoseconds since PreviousCount.
> >
> > +          PreviousCount is overwritten with the current performance
> >
> > +          counter value.
> >
> > +**/
> >
> > +UINT64
> >
> > +XhcGetElapsedTime (
> >
> > +  IN OUT UINT64  *PreviousTick
> >
> > +  )
> >
> > +{
> >
> > +  UINT64  StartValue;
> >
> > +  UINT64  EndValue;
> >
> > +  UINT64  CurrentTick;
> >
> > +  UINT64  Delta;
> >
> > +
> >
> > +  GetPerformanceCounterProperties (&StartValue, &EndValue);
> >
> > +
> >
> > +  CurrentTick = GetPerformanceCounter ();
> >
> > +
> >
> > +  //
> >
> > +  // Determine if the counter is counting up or down
> >
> > +  //
> >
> > +  if (StartValue < EndValue) {
> >
> > +    //
> >
> > +    // Counter counts upwards, check for an overflow condition
> >
> > +    //
> >
> > +    if (*PreviousTick > CurrentTick) {
> >
> > +      Delta = (EndValue - *PreviousTick) + CurrentTick;
> >
> > +    } else {
> >
> > +      Delta = CurrentTick - *PreviousTick;
> >
> > +    }
> >
> > +  } else {
> >
> > +    //
> >
> > +    // Counter counts downwards, check for an underflow condition
> >
> > +    //
> >
> > +    if (*PreviousTick < CurrentTick) {
> >
> > +      Delta = (StartValue - CurrentTick) + *PreviousTick;
> >
> > +    } else {
> >
> > +      Delta = *PreviousTick - CurrentTick;
> >
> > +    }
> >
> > +  }
> >
> > +
> >
> > +  //
> >
> > +  // Set PreviousTick to CurrentTick
> >
> > +  //
> >
> > +  *PreviousTick = CurrentTick;
> >
> > +
> >
> > +  return GetTimeInNanoSecond (Delta);
> >
> > +}
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > index ca223bd20c..77feb66647 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > @@ -2,6 +2,7 @@
> >
> >
> >    Provides some data structure definitions used by the XHCI host 
> > controller driver.
> >
> >
> >
> > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  Copyright (c) 2011 - 2017, Intel Corporation. All rights 
> > reserved.<BR>
> >
> >  Copyright (c) Microsoft Corporation.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent 
> > #include <Library/UefiLib.h>
> >
> >  #include <Library/DebugLib.h>
> >
> >  #include <Library/ReportStatusCodeLib.h>
> >
> > +#include <Library/TimerLib.h>
> >
> >
> >
> >  #include <IndustryStandard/Pci.h>
> >
> >
> >
> > @@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT
> USB_DEV_CONTEXT;
> > #include "ComponentName.h"
> >
> >  #include "UsbHcMem.h"
> >
> >
> >
> > +//
> >
> > +// Converts a count from microseconds to nanoseconds
> >
> > +//
> >
> > +#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time) *
> > 1000)
> >
> >  //
> >
> >  // The unit is microsecond, setting it as 1us.
> >
> >  //
> >
> > @@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
> >    IN     VOID                                *Context
> >
> >    );
> >
> >
> >
> > +/**
> >
> > +  Computes and returns the elapsed time in nanoseconds since
> >
> > +  PreviousTick. The value of PreviousTick is overwritten with the
> >
> > +  current performance counter value.
> >
> > +
> >
> > +  @param  PreviousTick    Pointer to PreviousTick count.
> >
> > +                          before calling this function.
> >
> > +  @return The elapsed time in nanoseconds since PreviousCount.
> >
> > +          PreviousCount is overwritten with the current performance
> >
> > +          counter value.
> >
> > +**/
> >
> > +UINT64
> >
> > +XhcGetElapsedTime (
> >
> > +  IN OUT UINT64  *PreviousTick
> >
> > +  );
> >
> > +
> >
> >  #endif
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > index 5865d86822..18ef87916a 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > @@ -3,6 +3,7 @@
> >  #  It implements the interfaces of monitoring the status of all 
> > ports and transferring
> >
> >  #  Control, Bulk, Interrupt and Isochronous requests to those 
> > attached usb LS/FS/HS/SS devices.
> >
> >  #
> >
> > +#  (C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  #  Copyright (c) 2011 - 2018, Intel Corporation. All rights 
> > reserved.<BR>
> >
> >  #
> >
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -54,6 +55,7 @@
> >    BaseMemoryLib
> >
> >    DebugLib
> >
> >    ReportStatusCodeLib
> >
> > +  TimerLib
> >
> >
> >
> >  [Guids]
> >
> >    gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
> > Event
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > index 5700fc5fb8..2499698715 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > @@ -2,6 +2,7 @@
> >
> >
> >    The XHCI register operation routines.
> >
> >
> >
> > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  Copyright (c) 2011 - 2017, Intel Corporation. All rights 
> > reserved.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> > @@ -417,15 +418,14 @@ XhcClearOpRegBit (
> >    Wait the operation register's bit as specified by Bit
> >
> >    to become set (or clear).
> >
> >
> >
> > -  @param  Xhc                    The XHCI Instance.
> >
> > -  @param  Offset                 The offset of the operation register.
> >
> > -  @param  Bit                    The bit of the register to wait for.
> >
> > -  @param  WaitToSet              Wait the bit to set or clear.
> >
> > -  @param  Timeout                The time to wait before abort (in millisecond,
> > ms).
> >
> > +  @param  Xhc          The XHCI Instance.
> >
> > +  @param  Offset       The offset of the operation register.
> >
> > +  @param  Bit          The bit of the register to wait for.
> >
> > +  @param  WaitToSet    Wait the bit to set or clear.
> >
> > +  @param  Timeout      The time to wait before abort (in millisecond, ms).
> >
> >
> >
> > -  @retval EFI_SUCCESS            The bit successfully changed by host
> controller.
> >
> > -  @retval EFI_TIMEOUT            The time out occurred.
> >
> > -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not
> > be allocated.
> >
> > +  @retval EFI_SUCCESS  The bit successfully changed by host controller.
> >
> > +  @retval EFI_TIMEOUT  The time out occurred.
> >
> >
> >
> >  **/
> >
> >  EFI_STATUS
> >
> > @@ -437,54 +437,35 @@ XhcWaitOpRegBit (
> >    IN UINT32             Timeout
> >
> >    )
> >
> >  {
> >
> > -  EFI_STATUS  Status;
> >
> > -  EFI_EVENT   TimeoutEvent;
> >
> > -
> >
> > -  TimeoutEvent = NULL;
> >
> > +  UINT64  TimeoutTime;
> >
> > +  UINT64  ElapsedTime;
> >
> > +  UINT64  TimeDelta;
> >
> > +  UINT64  CurrentTick;
> >
> >
> >
> >    if (Timeout == 0) {
> >
> >      return EFI_TIMEOUT;
> >
> >    }
> >
> >
> >
> > -  Status = gBS->CreateEvent (
> >
> > -                  EVT_TIMER,
> >
> > -                  TPL_CALLBACK,
> >
> > -                  NULL,
> >
> > -                  NULL,
> >
> > -                  &TimeoutEvent
> >
> > -                  );
> >
> > -
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    goto DONE;
> >
> > -  }
> >
> > -
> >
> > -  Status = gBS->SetTimer (
> >
> > -                  TimeoutEvent,
> >
> > -                  TimerRelative,
> >
> > -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> >
> > -                  );
> >
> > -
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    goto DONE;
> >
> > -  }
> >
> > +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> > XHC_1_MILLISECOND);
> >
> > +  ElapsedTime = 0;
> >
> > +  CurrentTick = GetPerformanceCounter ();
> >
> >
> >
> >    do {
> >
> >      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> >
> > -      Status = EFI_SUCCESS;
> >
> > -      goto DONE;
> >
> > +      return EFI_SUCCESS;
> >
> >      }
> >
> >
> >
> >      gBS->Stall (XHC_1_MICROSECOND);
> >
> > -  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
> >
> > -
> >
> > -  Status = EFI_TIMEOUT;
> >
> > +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> >
> > +    // Ensure that ElapsedTime is always incremented to avoid 
> > + indefinite
> > hangs
> >
> > +    if (TimeDelta == 0) {
> >
> > +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> > (XHC_1_MICROSECOND);
> >
> > +    }
> >
> >
> >
> > -DONE:
> >
> > -  if (TimeoutEvent != NULL) {
> >
> > -    gBS->CloseEvent (TimeoutEvent);
> >
> > -  }
> >
> > +    ElapsedTime += TimeDelta;
> >
> > +  } while (ElapsedTime < TimeoutTime);
> >
> >
> >
> > -  return Status;
> >
> > +  return EFI_TIMEOUT;
> >
> >  }
> >
> >
> >
> >  /**
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > index 298fb88b81..5177886e52 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > @@ -2,6 +2,7 @@
> >
> >
> >    XHCI transfer scheduling routines.
> >
> >
> >
> > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> >
> >  Copyright (c) 2011 - 2020, Intel Corporation. All rights 
> > reserved.<BR>
> >
> >  Copyright (c) Microsoft Corporation.<BR>
> >
> >  Copyright (C) 2022 Advanced Micro Devices, Inc. All rights 
> > reserved.<BR>
> >
> > @@ -1273,15 +1274,14 @@ EXIT:
> >  /**
> >
> >    Execute the transfer by polling the URB. This is a synchronous operation.
> >
> >
> >
> > -  @param  Xhc                    The XHCI Instance.
> >
> > -  @param  CmdTransfer            The executed URB is for cmd transfer or not.
> >
> > -  @param  Urb                    The URB to execute.
> >
> > -  @param  Timeout                The time to wait before abort, in millisecond.
> >
> > +  @param  Xhc               The XHCI Instance.
> >
> > +  @param  CmdTransfer       The executed URB is for cmd transfer or not.
> >
> > +  @param  Urb               The URB to execute.
> >
> > +  @param  Timeout           The time to wait before abort, in millisecond.
> >
> >
> >
> > -  @return EFI_DEVICE_ERROR       The transfer failed due to transfer error.
> >
> > -  @return EFI_TIMEOUT            The transfer failed due to time out.
> >
> > -  @return EFI_SUCCESS            The transfer finished OK.
> >
> > -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not
> > be allocated.
> >
> > +  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
> >
> > +  @return EFI_TIMEOUT       The transfer failed due to time out.
> >
> > +  @return EFI_SUCCESS       The transfer finished OK.
> >
> >
> >
> >  **/
> >
> >  EFI_STATUS
> >
> > @@ -1296,12 +1296,14 @@ XhcExecTransfer (
> >    UINT8       SlotId;
> >
> >    UINT8       Dci;
> >
> >    BOOLEAN     Finished;
> >
> > -  EFI_EVENT   TimeoutEvent;
> >
> > +  UINT64      TimeoutTime;
> >
> > +  UINT64      ElapsedTime;
> >
> > +  UINT64      TimeDelta;
> >
> > +  UINT64      CurrentTick;
> >
> >    BOOLEAN     IndefiniteTimeout;
> >
> >
> >
> >    Status            = EFI_SUCCESS;
> >
> >    Finished          = FALSE;
> >
> > -  TimeoutEvent      = NULL;
> >
> >    IndefiniteTimeout = FALSE;
> >
> >
> >
> >    if (CmdTransfer) {
> >
> > @@ -1319,34 +1321,14 @@ XhcExecTransfer (
> >
> >
> >    if (Timeout == 0) {
> >
> >      IndefiniteTimeout = TRUE;
> >
> > -    goto RINGDOORBELL;
> >
> > -  }
> >
> > -
> >
> > -  Status = gBS->CreateEvent (
> >
> > -                  EVT_TIMER,
> >
> > -                  TPL_CALLBACK,
> >
> > -                  NULL,
> >
> > -                  NULL,
> >
> > -                  &TimeoutEvent
> >
> > -                  );
> >
> > -
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    goto DONE;
> >
> >    }
> >
> >
> >
> > -  Status = gBS->SetTimer (
> >
> > -                  TimeoutEvent,
> >
> > -                  TimerRelative,
> >
> > -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> >
> > -                  );
> >
> > -
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    goto DONE;
> >
> > -  }
> >
> > -
> >
> > -RINGDOORBELL:
> >
> >    XhcRingDoorBell (Xhc, SlotId, Dci);
> >
> >
> >
> > +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> > XHC_1_MILLISECOND);
> >
> > +  ElapsedTime = 0;
> >
> > +  CurrentTick = GetPerformanceCounter ();
> >
> > +
> >
> >    do {
> >
> >      Finished = XhcCheckUrbResult (Xhc, Urb);
> >
> >      if (Finished) {
> >
> > @@ -1354,22 +1336,22 @@ RINGDOORBELL:
> >      }
> >
> >
> >
> >      gBS->Stall (XHC_1_MICROSECOND);
> >
> > -  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent 
> > (TimeoutEvent)));
> >
> > +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> >
> > +    // Ensure that ElapsedTime is always incremented to avoid 
> > + indefinite
> > hangs
> >
> > +    if (TimeDelta == 0) {
> >
> > +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> > (XHC_1_MICROSECOND);
> >
> > +    }
> >
> >
> >
> > -DONE:
> >
> > -  if (EFI_ERROR (Status)) {
> >
> > -    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
> >
> > -  } else if (!Finished) {
> >
> > +    ElapsedTime += TimeDelta;
> >
> > +  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
> >
> > +
> >
> > +  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.34.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#106668):
> > INVALID URI REMOVED
> > 06
> > 668__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
> lctg5_B3K1Lcta4
> > 52Gx-1twRt8At3cuWxQsO18$ Mute This Topic:
> > https://groups.io/mt/99972791/1768737
> > NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
> lctg5_B3K1Lcta452Gx-1tw
> > Rt8At3cubh2HDyk$ Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe:
> > https://edk2.groups.io/g/devel/unsub
> > !N
> > pxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
> lctg5_B3K1Lcta452Gx-1twR
> > t8At3cuWYMMXVU$  [hao.a.wu@intel.com] -=-=-=-=-=-=
> >
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110344): https://edk2.groups.io/g/devel/message/110344
Mute This Topic: https://groups.io/mt/99972791/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Wu, Hao A]

(Add MdePkg maintainers for their feedbacks)

Sorry that I do not have strong opinion on this one.

Some of my thoughts are:
* If you find the to-be-added APIs can be used in serveral places to reduce
  repetative codes, then it will be worthwhile to add new library APIs.

* TimerLib has many instance implementations, my take is that many post-mem
  instances use library level global variable to store information like timer
  frequency to avoid calculating it every time.
  For pre-mem instances, such caching is not feasible. I think it will be the
  API consumer's responsibility to limit the usage of these APIs for performance
  consideration.

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> Patrick
> Sent: Tuesday, October 31, 2023 4:36 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io; Michael Brown
> <mcb30@ipxe.org>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
> Timer for XHCI Timeouts
> 
> My changes have been in for a little over a month now, I've been looking into
> updating the TimerLib API but I'm questioning if it still makes sense to go down
> that path.  The functions I had implemented, XhcGetElapsedTicks () and
> XhcConvertTimeToTicks (), now rely heavily on the use of global variables for
> caching return values from GetPerformanceCounterProperties. As-is these
> functions won't work for all instances of TimerLib, specifically if the instance is
> used before memory (RAM) is available. I could implement the functions as
> they originally were, but I wouldn't be able to replace the Xhc functions
> without adding additional parameters to said TimerLib functions (e.g. adding
> Frequency, StartValue, StopValue to ConvertTimeToTicks), which feels clunky
> to me. Would a new library make sense here? Something that sits between
> the driver and the TimerLib library? Or do we just leave things as they
> currently are?
> 
> Thanks,
> Patrick Henz
> 
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, August 10, 2023 8:43 PM
> To: Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io; Michael
> Brown <mcb30@ipxe.org>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
> Timer for XHCI Timeouts
> 
> My personal preference is to do this as two steps.
> Address the issue in XhciDxe first. Then add new TimerLib API to replace all
> driver/library internal implementations.
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: Henz, Patrick <patrick.henz@hpe.com>
> > Sent: Friday, August 11, 2023 6:45 AM
> > To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Michael
> > Brown <mcb30@ipxe.org>
> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use
> > Performance Timer for XHCI Timeouts
> >
> > I can certainly make that change.
> >
> > For what it's worth I have been working on adding the new function,
> > GetElapsedTicks, to the various implementations of TimerLib. I've
> > finished up testing, I would just need to clean up the commits for a
> > patch. Should I move forward with that, or would we rather I just add
> > XhcGetElapsedTime to XhciDxe for the time being?
> >
> > Thanks,
> > Patrick Henz
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Hao
> > A
> > Sent: Sunday, July 30, 2023 9:57 PM
> > To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>;
> > Michael Brown <mcb30@ipxe.org>
> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use
> > Performance Timer for XHCI Timeouts
> >
> > For the 2 occurrences of in XhcWaitOpRegBit & XhcExecTransfer:
> >   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> > XHC_1_MILLISECOND); How about changing them to:
> >   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND ((UINT64)
> Timeout
> > * XHC_1_MILLISECOND); to address possible overflow during "Timeout *
> > XHC_1_MILLISECOND"?
> >
> > For extending XhcGetElapsedTime as a TimerLib API, I am fine to put it
> > in XhciDxe at this moment.
> > If package maintainers suggest to make it as a public library API, my
> > take is that this should be done in a separate commit.
> >
> > Best Regards,
> > Hao Wu
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > > Patrick
> > > Sent: Thursday, July 6, 2023 4:16 AM
> > > To: devel@edk2.groups.io
> > > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > > Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
> > > Timer for XHCI Timeouts
> > >
> > > REF:INVALID URI REMOVED
> > > bu
> > > g.cgi?id=2948__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-
> > NKbMjOV-lctg5
> > > _B3K1Lcta452Gx-1twRt8At3cueAYDq_n$
> > >
> > > XhciDxe uses the timer functionality provided by the boot services
> > > table to detect timeout conditions. This breaks the driver's
> > > ExitBootServices call back, as CoreExitBootServices halts the timer
> > > before signaling the ExitBootServices event. If the host controller
> > > fails to halt in the call back, the timeout condition will never
> > > occur and the boot gets stuck in an indefinite spin loop. Use the
> > > free running timer provided by TimerLib to calculate timeouts,
> > > avoiding the potential hang.
> > >
> > > Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> > > Reviewed-by:
> > > ---
> > >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
> > >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
> > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
> > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
> > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68
> > > +++++++++---------------
> > >  5 files changed, 129 insertions(+), 86 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > > index 62682dd27c..1dcbe20512 100644
> > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > > @@ -1,6 +1,7 @@
> > >  /** @file
> > >
> > >    The XHCI controller driver.
> > >
> > >
> > >
> > > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> > >
> > >  Copyright (c) 2011 - 2022, Intel Corporation. All rights
> > > reserved.<BR>
> > >
> > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >
> > >
> > > @@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
> > >
> > >
> > >    return EFI_SUCCESS;
> > >
> > >  }
> > >
> > > +
> > >
> > > +/**
> > >
> > > +  Computes and returns the elapsed time in nanoseconds since
> > >
> > > +  PreviousTick. The value of PreviousTick is overwritten with the
> > >
> > > +  current performance counter value.
> > >
> > > +
> > >
> > > +  @param  PreviousTick    Pointer to PreviousTick count.
> > >
> > > +  @return The elapsed time in nanoseconds since PreviousCount.
> > >
> > > +          PreviousCount is overwritten with the current performance
> > >
> > > +          counter value.
> > >
> > > +**/
> > >
> > > +UINT64
> > >
> > > +XhcGetElapsedTime (
> > >
> > > +  IN OUT UINT64  *PreviousTick
> > >
> > > +  )
> > >
> > > +{
> > >
> > > +  UINT64  StartValue;
> > >
> > > +  UINT64  EndValue;
> > >
> > > +  UINT64  CurrentTick;
> > >
> > > +  UINT64  Delta;
> > >
> > > +
> > >
> > > +  GetPerformanceCounterProperties (&StartValue, &EndValue);
> > >
> > > +
> > >
> > > +  CurrentTick = GetPerformanceCounter ();
> > >
> > > +
> > >
> > > +  //
> > >
> > > +  // Determine if the counter is counting up or down
> > >
> > > +  //
> > >
> > > +  if (StartValue < EndValue) {
> > >
> > > +    //
> > >
> > > +    // Counter counts upwards, check for an overflow condition
> > >
> > > +    //
> > >
> > > +    if (*PreviousTick > CurrentTick) {
> > >
> > > +      Delta = (EndValue - *PreviousTick) + CurrentTick;
> > >
> > > +    } else {
> > >
> > > +      Delta = CurrentTick - *PreviousTick;
> > >
> > > +    }
> > >
> > > +  } else {
> > >
> > > +    //
> > >
> > > +    // Counter counts downwards, check for an underflow condition
> > >
> > > +    //
> > >
> > > +    if (*PreviousTick < CurrentTick) {
> > >
> > > +      Delta = (StartValue - CurrentTick) + *PreviousTick;
> > >
> > > +    } else {
> > >
> > > +      Delta = *PreviousTick - CurrentTick;
> > >
> > > +    }
> > >
> > > +  }
> > >
> > > +
> > >
> > > +  //
> > >
> > > +  // Set PreviousTick to CurrentTick
> > >
> > > +  //
> > >
> > > +  *PreviousTick = CurrentTick;
> > >
> > > +
> > >
> > > +  return GetTimeInNanoSecond (Delta);
> > >
> > > +}
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > > index ca223bd20c..77feb66647 100644
> > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > > @@ -2,6 +2,7 @@
> > >
> > >
> > >    Provides some data structure definitions used by the XHCI host
> > > controller driver.
> > >
> > >
> > >
> > > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> > >
> > >  Copyright (c) 2011 - 2017, Intel Corporation. All rights
> > > reserved.<BR>
> > >
> > >  Copyright (c) Microsoft Corporation.<BR>
> > >
> > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > @@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > #include <Library/UefiLib.h>
> > >
> > >  #include <Library/DebugLib.h>
> > >
> > >  #include <Library/ReportStatusCodeLib.h>
> > >
> > > +#include <Library/TimerLib.h>
> > >
> > >
> > >
> > >  #include <IndustryStandard/Pci.h>
> > >
> > >
> > >
> > > @@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT
> > USB_DEV_CONTEXT;
> > > #include "ComponentName.h"
> > >
> > >  #include "UsbHcMem.h"
> > >
> > >
> > >
> > > +//
> > >
> > > +// Converts a count from microseconds to nanoseconds
> > >
> > > +//
> > >
> > > +#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time)
> *
> > > 1000)
> > >
> > >  //
> > >
> > >  // The unit is microsecond, setting it as 1us.
> > >
> > >  //
> > >
> > > @@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
> > >    IN     VOID                                *Context
> > >
> > >    );
> > >
> > >
> > >
> > > +/**
> > >
> > > +  Computes and returns the elapsed time in nanoseconds since
> > >
> > > +  PreviousTick. The value of PreviousTick is overwritten with the
> > >
> > > +  current performance counter value.
> > >
> > > +
> > >
> > > +  @param  PreviousTick    Pointer to PreviousTick count.
> > >
> > > +                          before calling this function.
> > >
> > > +  @return The elapsed time in nanoseconds since PreviousCount.
> > >
> > > +          PreviousCount is overwritten with the current performance
> > >
> > > +          counter value.
> > >
> > > +**/
> > >
> > > +UINT64
> > >
> > > +XhcGetElapsedTime (
> > >
> > > +  IN OUT UINT64  *PreviousTick
> > >
> > > +  );
> > >
> > > +
> > >
> > >  #endif
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > > index 5865d86822..18ef87916a 100644
> > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > > @@ -3,6 +3,7 @@
> > >  #  It implements the interfaces of monitoring the status of all
> > > ports and transferring
> > >
> > >  #  Control, Bulk, Interrupt and Isochronous requests to those
> > > attached usb LS/FS/HS/SS devices.
> > >
> > >  #
> > >
> > > +#  (C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> > >
> > >  #  Copyright (c) 2011 - 2018, Intel Corporation. All rights
> > > reserved.<BR>
> > >
> > >  #
> > >
> > >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > @@ -54,6 +55,7 @@
> > >    BaseMemoryLib
> > >
> > >    DebugLib
> > >
> > >    ReportStatusCodeLib
> > >
> > > +  TimerLib
> > >
> > >
> > >
> > >  [Guids]
> > >
> > >    gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
> > > Event
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > index 5700fc5fb8..2499698715 100644
> > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > @@ -2,6 +2,7 @@
> > >
> > >
> > >    The XHCI register operation routines.
> > >
> > >
> > >
> > > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> > >
> > >  Copyright (c) 2011 - 2017, Intel Corporation. All rights
> > > reserved.<BR>
> > >
> > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >
> > >
> > > @@ -417,15 +418,14 @@ XhcClearOpRegBit (
> > >    Wait the operation register's bit as specified by Bit
> > >
> > >    to become set (or clear).
> > >
> > >
> > >
> > > -  @param  Xhc                    The XHCI Instance.
> > >
> > > -  @param  Offset                 The offset of the operation register.
> > >
> > > -  @param  Bit                    The bit of the register to wait for.
> > >
> > > -  @param  WaitToSet              Wait the bit to set or clear.
> > >
> > > -  @param  Timeout                The time to wait before abort (in millisecond,
> > > ms).
> > >
> > > +  @param  Xhc          The XHCI Instance.
> > >
> > > +  @param  Offset       The offset of the operation register.
> > >
> > > +  @param  Bit          The bit of the register to wait for.
> > >
> > > +  @param  WaitToSet    Wait the bit to set or clear.
> > >
> > > +  @param  Timeout      The time to wait before abort (in millisecond, ms).
> > >
> > >
> > >
> > > -  @retval EFI_SUCCESS            The bit successfully changed by host
> > controller.
> > >
> > > -  @retval EFI_TIMEOUT            The time out occurred.
> > >
> > > -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
> not
> > > be allocated.
> > >
> > > +  @retval EFI_SUCCESS  The bit successfully changed by host controller.
> > >
> > > +  @retval EFI_TIMEOUT  The time out occurred.
> > >
> > >
> > >
> > >  **/
> > >
> > >  EFI_STATUS
> > >
> > > @@ -437,54 +437,35 @@ XhcWaitOpRegBit (
> > >    IN UINT32             Timeout
> > >
> > >    )
> > >
> > >  {
> > >
> > > -  EFI_STATUS  Status;
> > >
> > > -  EFI_EVENT   TimeoutEvent;
> > >
> > > -
> > >
> > > -  TimeoutEvent = NULL;
> > >
> > > +  UINT64  TimeoutTime;
> > >
> > > +  UINT64  ElapsedTime;
> > >
> > > +  UINT64  TimeDelta;
> > >
> > > +  UINT64  CurrentTick;
> > >
> > >
> > >
> > >    if (Timeout == 0) {
> > >
> > >      return EFI_TIMEOUT;
> > >
> > >    }
> > >
> > >
> > >
> > > -  Status = gBS->CreateEvent (
> > >
> > > -                  EVT_TIMER,
> > >
> > > -                  TPL_CALLBACK,
> > >
> > > -                  NULL,
> > >
> > > -                  NULL,
> > >
> > > -                  &TimeoutEvent
> > >
> > > -                  );
> > >
> > > -
> > >
> > > -  if (EFI_ERROR (Status)) {
> > >
> > > -    goto DONE;
> > >
> > > -  }
> > >
> > > -
> > >
> > > -  Status = gBS->SetTimer (
> > >
> > > -                  TimeoutEvent,
> > >
> > > -                  TimerRelative,
> > >
> > > -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> > >
> > > -                  );
> > >
> > > -
> > >
> > > -  if (EFI_ERROR (Status)) {
> > >
> > > -    goto DONE;
> > >
> > > -  }
> > >
> > > +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> > > XHC_1_MILLISECOND);
> > >
> > > +  ElapsedTime = 0;
> > >
> > > +  CurrentTick = GetPerformanceCounter ();
> > >
> > >
> > >
> > >    do {
> > >
> > >      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> > >
> > > -      Status = EFI_SUCCESS;
> > >
> > > -      goto DONE;
> > >
> > > +      return EFI_SUCCESS;
> > >
> > >      }
> > >
> > >
> > >
> > >      gBS->Stall (XHC_1_MICROSECOND);
> > >
> > > -  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
> > >
> > > -
> > >
> > > -  Status = EFI_TIMEOUT;
> > >
> > > +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> > >
> > > +    // Ensure that ElapsedTime is always incremented to avoid
> > > + indefinite
> > > hangs
> > >
> > > +    if (TimeDelta == 0) {
> > >
> > > +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> > > (XHC_1_MICROSECOND);
> > >
> > > +    }
> > >
> > >
> > >
> > > -DONE:
> > >
> > > -  if (TimeoutEvent != NULL) {
> > >
> > > -    gBS->CloseEvent (TimeoutEvent);
> > >
> > > -  }
> > >
> > > +    ElapsedTime += TimeDelta;
> > >
> > > +  } while (ElapsedTime < TimeoutTime);
> > >
> > >
> > >
> > > -  return Status;
> > >
> > > +  return EFI_TIMEOUT;
> > >
> > >  }
> > >
> > >
> > >
> > >  /**
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > index 298fb88b81..5177886e52 100644
> > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > @@ -2,6 +2,7 @@
> > >
> > >
> > >    XHCI transfer scheduling routines.
> > >
> > >
> > >
> > > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> > >
> > >  Copyright (c) 2011 - 2020, Intel Corporation. All rights
> > > reserved.<BR>
> > >
> > >  Copyright (c) Microsoft Corporation.<BR>
> > >
> > >  Copyright (C) 2022 Advanced Micro Devices, Inc. All rights
> > > reserved.<BR>
> > >
> > > @@ -1273,15 +1274,14 @@ EXIT:
> > >  /**
> > >
> > >    Execute the transfer by polling the URB. This is a synchronous operation.
> > >
> > >
> > >
> > > -  @param  Xhc                    The XHCI Instance.
> > >
> > > -  @param  CmdTransfer            The executed URB is for cmd transfer or
> not.
> > >
> > > -  @param  Urb                    The URB to execute.
> > >
> > > -  @param  Timeout                The time to wait before abort, in millisecond.
> > >
> > > +  @param  Xhc               The XHCI Instance.
> > >
> > > +  @param  CmdTransfer       The executed URB is for cmd transfer or not.
> > >
> > > +  @param  Urb               The URB to execute.
> > >
> > > +  @param  Timeout           The time to wait before abort, in millisecond.
> > >
> > >
> > >
> > > -  @return EFI_DEVICE_ERROR       The transfer failed due to transfer
> error.
> > >
> > > -  @return EFI_TIMEOUT            The transfer failed due to time out.
> > >
> > > -  @return EFI_SUCCESS            The transfer finished OK.
> > >
> > > -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
> not
> > > be allocated.
> > >
> > > +  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
> > >
> > > +  @return EFI_TIMEOUT       The transfer failed due to time out.
> > >
> > > +  @return EFI_SUCCESS       The transfer finished OK.
> > >
> > >
> > >
> > >  **/
> > >
> > >  EFI_STATUS
> > >
> > > @@ -1296,12 +1296,14 @@ XhcExecTransfer (
> > >    UINT8       SlotId;
> > >
> > >    UINT8       Dci;
> > >
> > >    BOOLEAN     Finished;
> > >
> > > -  EFI_EVENT   TimeoutEvent;
> > >
> > > +  UINT64      TimeoutTime;
> > >
> > > +  UINT64      ElapsedTime;
> > >
> > > +  UINT64      TimeDelta;
> > >
> > > +  UINT64      CurrentTick;
> > >
> > >    BOOLEAN     IndefiniteTimeout;
> > >
> > >
> > >
> > >    Status            = EFI_SUCCESS;
> > >
> > >    Finished          = FALSE;
> > >
> > > -  TimeoutEvent      = NULL;
> > >
> > >    IndefiniteTimeout = FALSE;
> > >
> > >
> > >
> > >    if (CmdTransfer) {
> > >
> > > @@ -1319,34 +1321,14 @@ XhcExecTransfer (
> > >
> > >
> > >    if (Timeout == 0) {
> > >
> > >      IndefiniteTimeout = TRUE;
> > >
> > > -    goto RINGDOORBELL;
> > >
> > > -  }
> > >
> > > -
> > >
> > > -  Status = gBS->CreateEvent (
> > >
> > > -                  EVT_TIMER,
> > >
> > > -                  TPL_CALLBACK,
> > >
> > > -                  NULL,
> > >
> > > -                  NULL,
> > >
> > > -                  &TimeoutEvent
> > >
> > > -                  );
> > >
> > > -
> > >
> > > -  if (EFI_ERROR (Status)) {
> > >
> > > -    goto DONE;
> > >
> > >    }
> > >
> > >
> > >
> > > -  Status = gBS->SetTimer (
> > >
> > > -                  TimeoutEvent,
> > >
> > > -                  TimerRelative,
> > >
> > > -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> > >
> > > -                  );
> > >
> > > -
> > >
> > > -  if (EFI_ERROR (Status)) {
> > >
> > > -    goto DONE;
> > >
> > > -  }
> > >
> > > -
> > >
> > > -RINGDOORBELL:
> > >
> > >    XhcRingDoorBell (Xhc, SlotId, Dci);
> > >
> > >
> > >
> > > +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
> > > XHC_1_MILLISECOND);
> > >
> > > +  ElapsedTime = 0;
> > >
> > > +  CurrentTick = GetPerformanceCounter ();
> > >
> > > +
> > >
> > >    do {
> > >
> > >      Finished = XhcCheckUrbResult (Xhc, Urb);
> > >
> > >      if (Finished) {
> > >
> > > @@ -1354,22 +1336,22 @@ RINGDOORBELL:
> > >      }
> > >
> > >
> > >
> > >      gBS->Stall (XHC_1_MICROSECOND);
> > >
> > > -  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent
> > > (TimeoutEvent)));
> > >
> > > +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> > >
> > > +    // Ensure that ElapsedTime is always incremented to avoid
> > > + indefinite
> > > hangs
> > >
> > > +    if (TimeDelta == 0) {
> > >
> > > +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> > > (XHC_1_MICROSECOND);
> > >
> > > +    }
> > >
> > >
> > >
> > > -DONE:
> > >
> > > -  if (EFI_ERROR (Status)) {
> > >
> > > -    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
> > >
> > > -  } else if (!Finished) {
> > >
> > > +    ElapsedTime += TimeDelta;
> > >
> > > +  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
> > >
> > > +
> > >
> > > +  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.34.1
> > >
> > >
> > >
> > > -=-=-=-=-=-=
> > > Groups.io Links: You receive all messages sent to this group.
> > > View/Reply Online (#106668):
> > > INVALID URI REMOVED
> > > 06
> > > 668__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
> > lctg5_B3K1Lcta4
> > > 52Gx-1twRt8At3cuWxQsO18$ Mute This Topic:
> > > https://groups.io/mt/99972791/1768737
> > > NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
> > lctg5_B3K1Lcta452Gx-1tw
> > > Rt8At3cubh2HDyk$ Group Owner: devel+owner@edk2.groups.io
> > > Unsubscribe:
> > > https://edk2.groups.io/g/devel/unsub
> > > !N
> > > pxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
> > lctg5_B3K1Lcta452Gx-1twR
> > > t8At3cuWYMMXVU$  [hao.a.wu@intel.com] -=-=-=-=-=-=
> > >
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110354): https://edk2.groups.io/g/devel/message/110354
Mute This Topic: https://groups.io/mt/99972791/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Laszlo Ersek]

On 10/31/23 02:41, Wu, Hao A wrote:
> (Add MdePkg maintainers for their feedbacks)
> 
> Sorry that I do not have strong opinion on this one.
> 
> Some of my thoughts are:
> * If you find the to-be-added APIs can be used in serveral places to reduce
>   repetative codes, then it will be worthwhile to add new library APIs.
> 
> * TimerLib has many instance implementations, my take is that many post-mem
>   instances use library level global variable to store information like timer
>   frequency to avoid calculating it every time.
>   For pre-mem instances, such caching is not feasible. I think it will be the
>   API consumer's responsibility to limit the usage of these APIs for performance
>   consideration.

I wouldn't recommend extending the TimerLib class. TimerLib has a huge
amount of instances (implementations), and adding the (effectively) same
calculation to each is a waste of developer effort, reviewer effort, and
long-term maintenance (lots of duplications).

Introducing a separate TimerTickDiffLib class is what I proposed in
2017. It can be small and simple; do one thing and do it well. It would
be widely used (*many* sites need tick differences). Yet keeping it
separate from TimerLib is absolutely justified, as TimerTickDiffLib
would be independent of both timer hardware specifics *and* of firmware
phase too (RAM or no RAM).

The functions -- or more likely, the single function -- in
TimerTickDiffLib can easily take the performance counter characteristics
as parameters. Then callers in RAM-based modules may pass those args
from global variables in which they cache the perf counter
characteristics themselves. No need to move the caching into a
particular (RAM-bound) TimerTickDiffLib instance. And callers that XIP
from flash can fill in those args from repeated / separate
GetPerformanceCounterProperties() calls, or -- in the luckier case --
cache the output of GetPerformanceCounterProperties() for each *call
site*, in local variables.

A futher API optimization may be to introduce a new (small) structure
type in TimerTickDiffLib, for collecting the outputs of
GetPerformanceCounterProperties(). Then the sole function in
TimerTickDiffLib would take a pointer to a *constant* structure of this
type.

Thanks
Laszlo

> 
> Best Regards,
> Hao Wu
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
>> Patrick
>> Sent: Tuesday, October 31, 2023 4:36 AM
>> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io; Michael Brown
>> <mcb30@ipxe.org>
>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
>> Timer for XHCI Timeouts
>>
>> My changes have been in for a little over a month now, I've been looking into
>> updating the TimerLib API but I'm questioning if it still makes sense to go down
>> that path.  The functions I had implemented, XhcGetElapsedTicks () and
>> XhcConvertTimeToTicks (), now rely heavily on the use of global variables for
>> caching return values from GetPerformanceCounterProperties. As-is these
>> functions won't work for all instances of TimerLib, specifically if the instance is
>> used before memory (RAM) is available. I could implement the functions as
>> they originally were, but I wouldn't be able to replace the Xhc functions
>> without adding additional parameters to said TimerLib functions (e.g. adding
>> Frequency, StartValue, StopValue to ConvertTimeToTicks), which feels clunky
>> to me. Would a new library make sense here? Something that sits between
>> the driver and the TimerLib library? Or do we just leave things as they
>> currently are?
>>
>> Thanks,
>> Patrick Henz
>>
>> -----Original Message-----
>> From: Wu, Hao A <hao.a.wu@intel.com>
>> Sent: Thursday, August 10, 2023 8:43 PM
>> To: Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io; Michael
>> Brown <mcb30@ipxe.org>
>> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
>> Timer for XHCI Timeouts
>>
>> My personal preference is to do this as two steps.
>> Address the issue in XhciDxe first. Then add new TimerLib API to replace all
>> driver/library internal implementations.
>>
>> Best Regards,
>> Hao Wu
>>
>>> -----Original Message-----
>>> From: Henz, Patrick <patrick.henz@hpe.com>
>>> Sent: Friday, August 11, 2023 6:45 AM
>>> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Michael
>>> Brown <mcb30@ipxe.org>
>>> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use
>>> Performance Timer for XHCI Timeouts
>>>
>>> I can certainly make that change.
>>>
>>> For what it's worth I have been working on adding the new function,
>>> GetElapsedTicks, to the various implementations of TimerLib. I've
>>> finished up testing, I would just need to clean up the commits for a
>>> patch. Should I move forward with that, or would we rather I just add
>>> XhcGetElapsedTime to XhciDxe for the time being?
>>>
>>> Thanks,
>>> Patrick Henz
>>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
>> Hao
>>> A
>>> Sent: Sunday, July 30, 2023 9:57 PM
>>> To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>;
>>> Michael Brown <mcb30@ipxe.org>
>>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use
>>> Performance Timer for XHCI Timeouts
>>>
>>> For the 2 occurrences of in XhcWaitOpRegBit & XhcExecTransfer:
>>>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
>>> XHC_1_MILLISECOND); How about changing them to:
>>>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND ((UINT64)
>> Timeout
>>> * XHC_1_MILLISECOND); to address possible overflow during "Timeout *
>>> XHC_1_MILLISECOND"?
>>>
>>> For extending XhcGetElapsedTime as a TimerLib API, I am fine to put it
>>> in XhciDxe at this moment.
>>> If package maintainers suggest to make it as a public library API, my
>>> take is that this should be done in a separate commit.
>>>
>>> Best Regards,
>>> Hao Wu
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
>>>> Patrick
>>>> Sent: Thursday, July 6, 2023 4:16 AM
>>>> To: devel@edk2.groups.io
>>>> Cc: Henz, Patrick <patrick.henz@hpe.com>
>>>> Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
>>>> Timer for XHCI Timeouts
>>>>
>>>> REF:INVALID URI REMOVED
>>>> bu
>>>> g.cgi?id=2948__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-
>>> NKbMjOV-lctg5
>>>> _B3K1Lcta452Gx-1twRt8At3cueAYDq_n$
>>>>
>>>> XhciDxe uses the timer functionality provided by the boot services
>>>> table to detect timeout conditions. This breaks the driver's
>>>> ExitBootServices call back, as CoreExitBootServices halts the timer
>>>> before signaling the ExitBootServices event. If the host controller
>>>> fails to halt in the call back, the timeout condition will never
>>>> occur and the boot gets stuck in an indefinite spin loop. Use the
>>>> free running timer provided by TimerLib to calculate timeouts,
>>>> avoiding the potential hang.
>>>>
>>>> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
>>>> Reviewed-by:
>>>> ---
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68
>>>> +++++++++---------------
>>>>  5 files changed, 129 insertions(+), 86 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> index 62682dd27c..1dcbe20512 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> @@ -1,6 +1,7 @@
>>>>  /** @file
>>>>
>>>>    The XHCI controller driver.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2022, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>>
>>>>
>>>> @@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
>>>>
>>>>
>>>>    return EFI_SUCCESS;
>>>>
>>>>  }
>>>>
>>>> +
>>>>
>>>> +/**
>>>>
>>>> +  Computes and returns the elapsed time in nanoseconds since
>>>>
>>>> +  PreviousTick. The value of PreviousTick is overwritten with the
>>>>
>>>> +  current performance counter value.
>>>>
>>>> +
>>>>
>>>> +  @param  PreviousTick    Pointer to PreviousTick count.
>>>>
>>>> +  @return The elapsed time in nanoseconds since PreviousCount.
>>>>
>>>> +          PreviousCount is overwritten with the current performance
>>>>
>>>> +          counter value.
>>>>
>>>> +**/
>>>>
>>>> +UINT64
>>>>
>>>> +XhcGetElapsedTime (
>>>>
>>>> +  IN OUT UINT64  *PreviousTick
>>>>
>>>> +  )
>>>>
>>>> +{
>>>>
>>>> +  UINT64  StartValue;
>>>>
>>>> +  UINT64  EndValue;
>>>>
>>>> +  UINT64  CurrentTick;
>>>>
>>>> +  UINT64  Delta;
>>>>
>>>> +
>>>>
>>>> +  GetPerformanceCounterProperties (&StartValue, &EndValue);
>>>>
>>>> +
>>>>
>>>> +  CurrentTick = GetPerformanceCounter ();
>>>>
>>>> +
>>>>
>>>> +  //
>>>>
>>>> +  // Determine if the counter is counting up or down
>>>>
>>>> +  //
>>>>
>>>> +  if (StartValue < EndValue) {
>>>>
>>>> +    //
>>>>
>>>> +    // Counter counts upwards, check for an overflow condition
>>>>
>>>> +    //
>>>>
>>>> +    if (*PreviousTick > CurrentTick) {
>>>>
>>>> +      Delta = (EndValue - *PreviousTick) + CurrentTick;
>>>>
>>>> +    } else {
>>>>
>>>> +      Delta = CurrentTick - *PreviousTick;
>>>>
>>>> +    }
>>>>
>>>> +  } else {
>>>>
>>>> +    //
>>>>
>>>> +    // Counter counts downwards, check for an underflow condition
>>>>
>>>> +    //
>>>>
>>>> +    if (*PreviousTick < CurrentTick) {
>>>>
>>>> +      Delta = (StartValue - CurrentTick) + *PreviousTick;
>>>>
>>>> +    } else {
>>>>
>>>> +      Delta = *PreviousTick - CurrentTick;
>>>>
>>>> +    }
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  //
>>>>
>>>> +  // Set PreviousTick to CurrentTick
>>>>
>>>> +  //
>>>>
>>>> +  *PreviousTick = CurrentTick;
>>>>
>>>> +
>>>>
>>>> +  return GetTimeInNanoSecond (Delta);
>>>>
>>>> +}
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> index ca223bd20c..77feb66647 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> @@ -2,6 +2,7 @@
>>>>
>>>>
>>>>    Provides some data structure definitions used by the XHCI host
>>>> controller driver.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2017, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  Copyright (c) Microsoft Corporation.<BR>
>>>>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>> @@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>> #include <Library/UefiLib.h>
>>>>
>>>>  #include <Library/DebugLib.h>
>>>>
>>>>  #include <Library/ReportStatusCodeLib.h>
>>>>
>>>> +#include <Library/TimerLib.h>
>>>>
>>>>
>>>>
>>>>  #include <IndustryStandard/Pci.h>
>>>>
>>>>
>>>>
>>>> @@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT
>>> USB_DEV_CONTEXT;
>>>> #include "ComponentName.h"
>>>>
>>>>  #include "UsbHcMem.h"
>>>>
>>>>
>>>>
>>>> +//
>>>>
>>>> +// Converts a count from microseconds to nanoseconds
>>>>
>>>> +//
>>>>
>>>> +#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time)
>> *
>>>> 1000)
>>>>
>>>>  //
>>>>
>>>>  // The unit is microsecond, setting it as 1us.
>>>>
>>>>  //
>>>>
>>>> @@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
>>>>    IN     VOID                                *Context
>>>>
>>>>    );
>>>>
>>>>
>>>>
>>>> +/**
>>>>
>>>> +  Computes and returns the elapsed time in nanoseconds since
>>>>
>>>> +  PreviousTick. The value of PreviousTick is overwritten with the
>>>>
>>>> +  current performance counter value.
>>>>
>>>> +
>>>>
>>>> +  @param  PreviousTick    Pointer to PreviousTick count.
>>>>
>>>> +                          before calling this function.
>>>>
>>>> +  @return The elapsed time in nanoseconds since PreviousCount.
>>>>
>>>> +          PreviousCount is overwritten with the current performance
>>>>
>>>> +          counter value.
>>>>
>>>> +**/
>>>>
>>>> +UINT64
>>>>
>>>> +XhcGetElapsedTime (
>>>>
>>>> +  IN OUT UINT64  *PreviousTick
>>>>
>>>> +  );
>>>>
>>>> +
>>>>
>>>>  #endif
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> index 5865d86822..18ef87916a 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> @@ -3,6 +3,7 @@
>>>>  #  It implements the interfaces of monitoring the status of all
>>>> ports and transferring
>>>>
>>>>  #  Control, Bulk, Interrupt and Isochronous requests to those
>>>> attached usb LS/FS/HS/SS devices.
>>>>
>>>>  #
>>>>
>>>> +#  (C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  #  Copyright (c) 2011 - 2018, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  #
>>>>
>>>>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>> @@ -54,6 +55,7 @@
>>>>    BaseMemoryLib
>>>>
>>>>    DebugLib
>>>>
>>>>    ReportStatusCodeLib
>>>>
>>>> +  TimerLib
>>>>
>>>>
>>>>
>>>>  [Guids]
>>>>
>>>>    gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
>>>> Event
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> index 5700fc5fb8..2499698715 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> @@ -2,6 +2,7 @@
>>>>
>>>>
>>>>    The XHCI register operation routines.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2017, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>>
>>>>
>>>> @@ -417,15 +418,14 @@ XhcClearOpRegBit (
>>>>    Wait the operation register's bit as specified by Bit
>>>>
>>>>    to become set (or clear).
>>>>
>>>>
>>>>
>>>> -  @param  Xhc                    The XHCI Instance.
>>>>
>>>> -  @param  Offset                 The offset of the operation register.
>>>>
>>>> -  @param  Bit                    The bit of the register to wait for.
>>>>
>>>> -  @param  WaitToSet              Wait the bit to set or clear.
>>>>
>>>> -  @param  Timeout                The time to wait before abort (in millisecond,
>>>> ms).
>>>>
>>>> +  @param  Xhc          The XHCI Instance.
>>>>
>>>> +  @param  Offset       The offset of the operation register.
>>>>
>>>> +  @param  Bit          The bit of the register to wait for.
>>>>
>>>> +  @param  WaitToSet    Wait the bit to set or clear.
>>>>
>>>> +  @param  Timeout      The time to wait before abort (in millisecond, ms).
>>>>
>>>>
>>>>
>>>> -  @retval EFI_SUCCESS            The bit successfully changed by host
>>> controller.
>>>>
>>>> -  @retval EFI_TIMEOUT            The time out occurred.
>>>>
>>>> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
>> not
>>>> be allocated.
>>>>
>>>> +  @retval EFI_SUCCESS  The bit successfully changed by host controller.
>>>>
>>>> +  @retval EFI_TIMEOUT  The time out occurred.
>>>>
>>>>
>>>>
>>>>  **/
>>>>
>>>>  EFI_STATUS
>>>>
>>>> @@ -437,54 +437,35 @@ XhcWaitOpRegBit (
>>>>    IN UINT32             Timeout
>>>>
>>>>    )
>>>>
>>>>  {
>>>>
>>>> -  EFI_STATUS  Status;
>>>>
>>>> -  EFI_EVENT   TimeoutEvent;
>>>>
>>>> -
>>>>
>>>> -  TimeoutEvent = NULL;
>>>>
>>>> +  UINT64  TimeoutTime;
>>>>
>>>> +  UINT64  ElapsedTime;
>>>>
>>>> +  UINT64  TimeDelta;
>>>>
>>>> +  UINT64  CurrentTick;
>>>>
>>>>
>>>>
>>>>    if (Timeout == 0) {
>>>>
>>>>      return EFI_TIMEOUT;
>>>>
>>>>    }
>>>>
>>>>
>>>>
>>>> -  Status = gBS->CreateEvent (
>>>>
>>>> -                  EVT_TIMER,
>>>>
>>>> -                  TPL_CALLBACK,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  &TimeoutEvent
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>> -  }
>>>>
>>>> -
>>>>
>>>> -  Status = gBS->SetTimer (
>>>>
>>>> -                  TimeoutEvent,
>>>>
>>>> -                  TimerRelative,
>>>>
>>>> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>> -  }
>>>>
>>>> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
>>>> XHC_1_MILLISECOND);
>>>>
>>>> +  ElapsedTime = 0;
>>>>
>>>> +  CurrentTick = GetPerformanceCounter ();
>>>>
>>>>
>>>>
>>>>    do {
>>>>
>>>>      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
>>>>
>>>> -      Status = EFI_SUCCESS;
>>>>
>>>> -      goto DONE;
>>>>
>>>> +      return EFI_SUCCESS;
>>>>
>>>>      }
>>>>
>>>>
>>>>
>>>>      gBS->Stall (XHC_1_MICROSECOND);
>>>>
>>>> -  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
>>>>
>>>> -
>>>>
>>>> -  Status = EFI_TIMEOUT;
>>>>
>>>> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
>>>>
>>>> +    // Ensure that ElapsedTime is always incremented to avoid
>>>> + indefinite
>>>> hangs
>>>>
>>>> +    if (TimeDelta == 0) {
>>>>
>>>> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
>>>> (XHC_1_MICROSECOND);
>>>>
>>>> +    }
>>>>
>>>>
>>>>
>>>> -DONE:
>>>>
>>>> -  if (TimeoutEvent != NULL) {
>>>>
>>>> -    gBS->CloseEvent (TimeoutEvent);
>>>>
>>>> -  }
>>>>
>>>> +    ElapsedTime += TimeDelta;
>>>>
>>>> +  } while (ElapsedTime < TimeoutTime);
>>>>
>>>>
>>>>
>>>> -  return Status;
>>>>
>>>> +  return EFI_TIMEOUT;
>>>>
>>>>  }
>>>>
>>>>
>>>>
>>>>  /**
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> index 298fb88b81..5177886e52 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> @@ -2,6 +2,7 @@
>>>>
>>>>
>>>>    XHCI transfer scheduling routines.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2020, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  Copyright (c) Microsoft Corporation.<BR>
>>>>
>>>>  Copyright (C) 2022 Advanced Micro Devices, Inc. All rights
>>>> reserved.<BR>
>>>>
>>>> @@ -1273,15 +1274,14 @@ EXIT:
>>>>  /**
>>>>
>>>>    Execute the transfer by polling the URB. This is a synchronous operation.
>>>>
>>>>
>>>>
>>>> -  @param  Xhc                    The XHCI Instance.
>>>>
>>>> -  @param  CmdTransfer            The executed URB is for cmd transfer or
>> not.
>>>>
>>>> -  @param  Urb                    The URB to execute.
>>>>
>>>> -  @param  Timeout                The time to wait before abort, in millisecond.
>>>>
>>>> +  @param  Xhc               The XHCI Instance.
>>>>
>>>> +  @param  CmdTransfer       The executed URB is for cmd transfer or not.
>>>>
>>>> +  @param  Urb               The URB to execute.
>>>>
>>>> +  @param  Timeout           The time to wait before abort, in millisecond.
>>>>
>>>>
>>>>
>>>> -  @return EFI_DEVICE_ERROR       The transfer failed due to transfer
>> error.
>>>>
>>>> -  @return EFI_TIMEOUT            The transfer failed due to time out.
>>>>
>>>> -  @return EFI_SUCCESS            The transfer finished OK.
>>>>
>>>> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
>> not
>>>> be allocated.
>>>>
>>>> +  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
>>>>
>>>> +  @return EFI_TIMEOUT       The transfer failed due to time out.
>>>>
>>>> +  @return EFI_SUCCESS       The transfer finished OK.
>>>>
>>>>
>>>>
>>>>  **/
>>>>
>>>>  EFI_STATUS
>>>>
>>>> @@ -1296,12 +1296,14 @@ XhcExecTransfer (
>>>>    UINT8       SlotId;
>>>>
>>>>    UINT8       Dci;
>>>>
>>>>    BOOLEAN     Finished;
>>>>
>>>> -  EFI_EVENT   TimeoutEvent;
>>>>
>>>> +  UINT64      TimeoutTime;
>>>>
>>>> +  UINT64      ElapsedTime;
>>>>
>>>> +  UINT64      TimeDelta;
>>>>
>>>> +  UINT64      CurrentTick;
>>>>
>>>>    BOOLEAN     IndefiniteTimeout;
>>>>
>>>>
>>>>
>>>>    Status            = EFI_SUCCESS;
>>>>
>>>>    Finished          = FALSE;
>>>>
>>>> -  TimeoutEvent      = NULL;
>>>>
>>>>    IndefiniteTimeout = FALSE;
>>>>
>>>>
>>>>
>>>>    if (CmdTransfer) {
>>>>
>>>> @@ -1319,34 +1321,14 @@ XhcExecTransfer (
>>>>
>>>>
>>>>    if (Timeout == 0) {
>>>>
>>>>      IndefiniteTimeout = TRUE;
>>>>
>>>> -    goto RINGDOORBELL;
>>>>
>>>> -  }
>>>>
>>>> -
>>>>
>>>> -  Status = gBS->CreateEvent (
>>>>
>>>> -                  EVT_TIMER,
>>>>
>>>> -                  TPL_CALLBACK,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  &TimeoutEvent
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>>    }
>>>>
>>>>
>>>>
>>>> -  Status = gBS->SetTimer (
>>>>
>>>> -                  TimeoutEvent,
>>>>
>>>> -                  TimerRelative,
>>>>
>>>> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>> -  }
>>>>
>>>> -
>>>>
>>>> -RINGDOORBELL:
>>>>
>>>>    XhcRingDoorBell (Xhc, SlotId, Dci);
>>>>
>>>>
>>>>
>>>> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
>>>> XHC_1_MILLISECOND);
>>>>
>>>> +  ElapsedTime = 0;
>>>>
>>>> +  CurrentTick = GetPerformanceCounter ();
>>>>
>>>> +
>>>>
>>>>    do {
>>>>
>>>>      Finished = XhcCheckUrbResult (Xhc, Urb);
>>>>
>>>>      if (Finished) {
>>>>
>>>> @@ -1354,22 +1336,22 @@ RINGDOORBELL:
>>>>      }
>>>>
>>>>
>>>>
>>>>      gBS->Stall (XHC_1_MICROSECOND);
>>>>
>>>> -  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent
>>>> (TimeoutEvent)));
>>>>
>>>> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
>>>>
>>>> +    // Ensure that ElapsedTime is always incremented to avoid
>>>> + indefinite
>>>> hangs
>>>>
>>>> +    if (TimeDelta == 0) {
>>>>
>>>> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
>>>> (XHC_1_MICROSECOND);
>>>>
>>>> +    }
>>>>
>>>>
>>>>
>>>> -DONE:
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
>>>>
>>>> -  } else if (!Finished) {
>>>>
>>>> +    ElapsedTime += TimeDelta;
>>>>
>>>> +  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
>>>>
>>>> +
>>>>
>>>> +  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.34.1
>>>>
>>>>
>>>>
>>>> -=-=-=-=-=-=
>>>> Groups.io Links: You receive all messages sent to this group.
>>>> View/Reply Online (#106668):
>>>> INVALID URI REMOVED
>>>> 06
>>>> 668__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
>>> lctg5_B3K1Lcta4
>>>> 52Gx-1twRt8At3cuWxQsO18$ Mute This Topic:
>>>> https://groups.io/mt/99972791/1768737
>>>> NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
>>> lctg5_B3K1Lcta452Gx-1tw
>>>> Rt8At3cubh2HDyk$ Group Owner: devel+owner@edk2.groups.io
>>>> Unsubscribe:
>>>> https://edk2.groups.io/g/devel/unsub
>>>> !N
>>>> pxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
>>> lctg5_B3K1Lcta452Gx-1twR
>>>> t8At3cuWYMMXVU$  [hao.a.wu@intel.com] -=-=-=-=-=-=
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110403): https://edk2.groups.io/g/devel/message/110403
Mute This Topic: https://groups.io/mt/99972791/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Laszlo Ersek]

On 10/31/23 02:41, Wu, Hao A wrote:
> (Add MdePkg maintainers for their feedbacks)
> 
> Sorry that I do not have strong opinion on this one.
> 
> Some of my thoughts are:
> * If you find the to-be-added APIs can be used in serveral places to reduce
>   repetative codes, then it will be worthwhile to add new library APIs.
> 
> * TimerLib has many instance implementations, my take is that many post-mem
>   instances use library level global variable to store information like timer
>   frequency to avoid calculating it every time.
>   For pre-mem instances, such caching is not feasible. I think it will be the
>   API consumer's responsibility to limit the usage of these APIs for performance
>   consideration.
> 
> Best Regards,
> Hao Wu

Patrick, while at it: I've recently filed the bug

  potentially invalid elapsed tick count calculation from commit 43dcf453fc15 

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

and assigned it to you (given that that commit is yours). I proposed the solution in the bugzilla too; please check it out if you can find the time.

Thanks
Laszlo

> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
>> Patrick
>> Sent: Tuesday, October 31, 2023 4:36 AM
>> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io; Michael Brown
>> <mcb30@ipxe.org>
>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
>> Timer for XHCI Timeouts
>>
>> My changes have been in for a little over a month now, I've been looking into
>> updating the TimerLib API but I'm questioning if it still makes sense to go down
>> that path.  The functions I had implemented, XhcGetElapsedTicks () and
>> XhcConvertTimeToTicks (), now rely heavily on the use of global variables for
>> caching return values from GetPerformanceCounterProperties. As-is these
>> functions won't work for all instances of TimerLib, specifically if the instance is
>> used before memory (RAM) is available. I could implement the functions as
>> they originally were, but I wouldn't be able to replace the Xhc functions
>> without adding additional parameters to said TimerLib functions (e.g. adding
>> Frequency, StartValue, StopValue to ConvertTimeToTicks), which feels clunky
>> to me. Would a new library make sense here? Something that sits between
>> the driver and the TimerLib library? Or do we just leave things as they
>> currently are?
>>
>> Thanks,
>> Patrick Henz
>>
>> -----Original Message-----
>> From: Wu, Hao A <hao.a.wu@intel.com>
>> Sent: Thursday, August 10, 2023 8:43 PM
>> To: Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io; Michael
>> Brown <mcb30@ipxe.org>
>> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
>> Timer for XHCI Timeouts
>>
>> My personal preference is to do this as two steps.
>> Address the issue in XhciDxe first. Then add new TimerLib API to replace all
>> driver/library internal implementations.
>>
>> Best Regards,
>> Hao Wu
>>
>>> -----Original Message-----
>>> From: Henz, Patrick <patrick.henz@hpe.com>
>>> Sent: Friday, August 11, 2023 6:45 AM
>>> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Michael
>>> Brown <mcb30@ipxe.org>
>>> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use
>>> Performance Timer for XHCI Timeouts
>>>
>>> I can certainly make that change.
>>>
>>> For what it's worth I have been working on adding the new function,
>>> GetElapsedTicks, to the various implementations of TimerLib. I've
>>> finished up testing, I would just need to clean up the commits for a
>>> patch. Should I move forward with that, or would we rather I just add
>>> XhcGetElapsedTime to XhciDxe for the time being?
>>>
>>> Thanks,
>>> Patrick Henz
>>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
>> Hao
>>> A
>>> Sent: Sunday, July 30, 2023 9:57 PM
>>> To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>;
>>> Michael Brown <mcb30@ipxe.org>
>>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use
>>> Performance Timer for XHCI Timeouts
>>>
>>> For the 2 occurrences of in XhcWaitOpRegBit & XhcExecTransfer:
>>>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
>>> XHC_1_MILLISECOND); How about changing them to:
>>>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND ((UINT64)
>> Timeout
>>> * XHC_1_MILLISECOND); to address possible overflow during "Timeout *
>>> XHC_1_MILLISECOND"?
>>>
>>> For extending XhcGetElapsedTime as a TimerLib API, I am fine to put it
>>> in XhciDxe at this moment.
>>> If package maintainers suggest to make it as a public library API, my
>>> take is that this should be done in a separate commit.
>>>
>>> Best Regards,
>>> Hao Wu
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
>>>> Patrick
>>>> Sent: Thursday, July 6, 2023 4:16 AM
>>>> To: devel@edk2.groups.io
>>>> Cc: Henz, Patrick <patrick.henz@hpe.com>
>>>> Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance
>>>> Timer for XHCI Timeouts
>>>>
>>>> REF:INVALID URI REMOVED
>>>> bu
>>>> g.cgi?id=2948__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-
>>> NKbMjOV-lctg5
>>>> _B3K1Lcta452Gx-1twRt8At3cueAYDq_n$
>>>>
>>>> XhciDxe uses the timer functionality provided by the boot services
>>>> table to detect timeout conditions. This breaks the driver's
>>>> ExitBootServices call back, as CoreExitBootServices halts the timer
>>>> before signaling the ExitBootServices event. If the host controller
>>>> fails to halt in the call back, the timeout condition will never
>>>> occur and the boot gets stuck in an indefinite spin loop. Use the
>>>> free running timer provided by TimerLib to calculate timeouts,
>>>> avoiding the potential hang.
>>>>
>>>> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
>>>> Reviewed-by:
>>>> ---
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68
>>>> +++++++++---------------
>>>>  5 files changed, 129 insertions(+), 86 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> index 62682dd27c..1dcbe20512 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> @@ -1,6 +1,7 @@
>>>>  /** @file
>>>>
>>>>    The XHCI controller driver.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2022, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>>
>>>>
>>>> @@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
>>>>
>>>>
>>>>    return EFI_SUCCESS;
>>>>
>>>>  }
>>>>
>>>> +
>>>>
>>>> +/**
>>>>
>>>> +  Computes and returns the elapsed time in nanoseconds since
>>>>
>>>> +  PreviousTick. The value of PreviousTick is overwritten with the
>>>>
>>>> +  current performance counter value.
>>>>
>>>> +
>>>>
>>>> +  @param  PreviousTick    Pointer to PreviousTick count.
>>>>
>>>> +  @return The elapsed time in nanoseconds since PreviousCount.
>>>>
>>>> +          PreviousCount is overwritten with the current performance
>>>>
>>>> +          counter value.
>>>>
>>>> +**/
>>>>
>>>> +UINT64
>>>>
>>>> +XhcGetElapsedTime (
>>>>
>>>> +  IN OUT UINT64  *PreviousTick
>>>>
>>>> +  )
>>>>
>>>> +{
>>>>
>>>> +  UINT64  StartValue;
>>>>
>>>> +  UINT64  EndValue;
>>>>
>>>> +  UINT64  CurrentTick;
>>>>
>>>> +  UINT64  Delta;
>>>>
>>>> +
>>>>
>>>> +  GetPerformanceCounterProperties (&StartValue, &EndValue);
>>>>
>>>> +
>>>>
>>>> +  CurrentTick = GetPerformanceCounter ();
>>>>
>>>> +
>>>>
>>>> +  //
>>>>
>>>> +  // Determine if the counter is counting up or down
>>>>
>>>> +  //
>>>>
>>>> +  if (StartValue < EndValue) {
>>>>
>>>> +    //
>>>>
>>>> +    // Counter counts upwards, check for an overflow condition
>>>>
>>>> +    //
>>>>
>>>> +    if (*PreviousTick > CurrentTick) {
>>>>
>>>> +      Delta = (EndValue - *PreviousTick) + CurrentTick;
>>>>
>>>> +    } else {
>>>>
>>>> +      Delta = CurrentTick - *PreviousTick;
>>>>
>>>> +    }
>>>>
>>>> +  } else {
>>>>
>>>> +    //
>>>>
>>>> +    // Counter counts downwards, check for an underflow condition
>>>>
>>>> +    //
>>>>
>>>> +    if (*PreviousTick < CurrentTick) {
>>>>
>>>> +      Delta = (StartValue - CurrentTick) + *PreviousTick;
>>>>
>>>> +    } else {
>>>>
>>>> +      Delta = *PreviousTick - CurrentTick;
>>>>
>>>> +    }
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  //
>>>>
>>>> +  // Set PreviousTick to CurrentTick
>>>>
>>>> +  //
>>>>
>>>> +  *PreviousTick = CurrentTick;
>>>>
>>>> +
>>>>
>>>> +  return GetTimeInNanoSecond (Delta);
>>>>
>>>> +}
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> index ca223bd20c..77feb66647 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> @@ -2,6 +2,7 @@
>>>>
>>>>
>>>>    Provides some data structure definitions used by the XHCI host
>>>> controller driver.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2017, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  Copyright (c) Microsoft Corporation.<BR>
>>>>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>> @@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>> #include <Library/UefiLib.h>
>>>>
>>>>  #include <Library/DebugLib.h>
>>>>
>>>>  #include <Library/ReportStatusCodeLib.h>
>>>>
>>>> +#include <Library/TimerLib.h>
>>>>
>>>>
>>>>
>>>>  #include <IndustryStandard/Pci.h>
>>>>
>>>>
>>>>
>>>> @@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT
>>> USB_DEV_CONTEXT;
>>>> #include "ComponentName.h"
>>>>
>>>>  #include "UsbHcMem.h"
>>>>
>>>>
>>>>
>>>> +//
>>>>
>>>> +// Converts a count from microseconds to nanoseconds
>>>>
>>>> +//
>>>>
>>>> +#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time)
>> *
>>>> 1000)
>>>>
>>>>  //
>>>>
>>>>  // The unit is microsecond, setting it as 1us.
>>>>
>>>>  //
>>>>
>>>> @@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
>>>>    IN     VOID                                *Context
>>>>
>>>>    );
>>>>
>>>>
>>>>
>>>> +/**
>>>>
>>>> +  Computes and returns the elapsed time in nanoseconds since
>>>>
>>>> +  PreviousTick. The value of PreviousTick is overwritten with the
>>>>
>>>> +  current performance counter value.
>>>>
>>>> +
>>>>
>>>> +  @param  PreviousTick    Pointer to PreviousTick count.
>>>>
>>>> +                          before calling this function.
>>>>
>>>> +  @return The elapsed time in nanoseconds since PreviousCount.
>>>>
>>>> +          PreviousCount is overwritten with the current performance
>>>>
>>>> +          counter value.
>>>>
>>>> +**/
>>>>
>>>> +UINT64
>>>>
>>>> +XhcGetElapsedTime (
>>>>
>>>> +  IN OUT UINT64  *PreviousTick
>>>>
>>>> +  );
>>>>
>>>> +
>>>>
>>>>  #endif
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> index 5865d86822..18ef87916a 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> @@ -3,6 +3,7 @@
>>>>  #  It implements the interfaces of monitoring the status of all
>>>> ports and transferring
>>>>
>>>>  #  Control, Bulk, Interrupt and Isochronous requests to those
>>>> attached usb LS/FS/HS/SS devices.
>>>>
>>>>  #
>>>>
>>>> +#  (C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  #  Copyright (c) 2011 - 2018, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  #
>>>>
>>>>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>> @@ -54,6 +55,7 @@
>>>>    BaseMemoryLib
>>>>
>>>>    DebugLib
>>>>
>>>>    ReportStatusCodeLib
>>>>
>>>> +  TimerLib
>>>>
>>>>
>>>>
>>>>  [Guids]
>>>>
>>>>    gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
>>>> Event
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> index 5700fc5fb8..2499698715 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> @@ -2,6 +2,7 @@
>>>>
>>>>
>>>>    The XHCI register operation routines.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2017, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>>
>>>>
>>>> @@ -417,15 +418,14 @@ XhcClearOpRegBit (
>>>>    Wait the operation register's bit as specified by Bit
>>>>
>>>>    to become set (or clear).
>>>>
>>>>
>>>>
>>>> -  @param  Xhc                    The XHCI Instance.
>>>>
>>>> -  @param  Offset                 The offset of the operation register.
>>>>
>>>> -  @param  Bit                    The bit of the register to wait for.
>>>>
>>>> -  @param  WaitToSet              Wait the bit to set or clear.
>>>>
>>>> -  @param  Timeout                The time to wait before abort (in millisecond,
>>>> ms).
>>>>
>>>> +  @param  Xhc          The XHCI Instance.
>>>>
>>>> +  @param  Offset       The offset of the operation register.
>>>>
>>>> +  @param  Bit          The bit of the register to wait for.
>>>>
>>>> +  @param  WaitToSet    Wait the bit to set or clear.
>>>>
>>>> +  @param  Timeout      The time to wait before abort (in millisecond, ms).
>>>>
>>>>
>>>>
>>>> -  @retval EFI_SUCCESS            The bit successfully changed by host
>>> controller.
>>>>
>>>> -  @retval EFI_TIMEOUT            The time out occurred.
>>>>
>>>> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
>> not
>>>> be allocated.
>>>>
>>>> +  @retval EFI_SUCCESS  The bit successfully changed by host controller.
>>>>
>>>> +  @retval EFI_TIMEOUT  The time out occurred.
>>>>
>>>>
>>>>
>>>>  **/
>>>>
>>>>  EFI_STATUS
>>>>
>>>> @@ -437,54 +437,35 @@ XhcWaitOpRegBit (
>>>>    IN UINT32             Timeout
>>>>
>>>>    )
>>>>
>>>>  {
>>>>
>>>> -  EFI_STATUS  Status;
>>>>
>>>> -  EFI_EVENT   TimeoutEvent;
>>>>
>>>> -
>>>>
>>>> -  TimeoutEvent = NULL;
>>>>
>>>> +  UINT64  TimeoutTime;
>>>>
>>>> +  UINT64  ElapsedTime;
>>>>
>>>> +  UINT64  TimeDelta;
>>>>
>>>> +  UINT64  CurrentTick;
>>>>
>>>>
>>>>
>>>>    if (Timeout == 0) {
>>>>
>>>>      return EFI_TIMEOUT;
>>>>
>>>>    }
>>>>
>>>>
>>>>
>>>> -  Status = gBS->CreateEvent (
>>>>
>>>> -                  EVT_TIMER,
>>>>
>>>> -                  TPL_CALLBACK,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  &TimeoutEvent
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>> -  }
>>>>
>>>> -
>>>>
>>>> -  Status = gBS->SetTimer (
>>>>
>>>> -                  TimeoutEvent,
>>>>
>>>> -                  TimerRelative,
>>>>
>>>> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>> -  }
>>>>
>>>> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
>>>> XHC_1_MILLISECOND);
>>>>
>>>> +  ElapsedTime = 0;
>>>>
>>>> +  CurrentTick = GetPerformanceCounter ();
>>>>
>>>>
>>>>
>>>>    do {
>>>>
>>>>      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
>>>>
>>>> -      Status = EFI_SUCCESS;
>>>>
>>>> -      goto DONE;
>>>>
>>>> +      return EFI_SUCCESS;
>>>>
>>>>      }
>>>>
>>>>
>>>>
>>>>      gBS->Stall (XHC_1_MICROSECOND);
>>>>
>>>> -  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
>>>>
>>>> -
>>>>
>>>> -  Status = EFI_TIMEOUT;
>>>>
>>>> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
>>>>
>>>> +    // Ensure that ElapsedTime is always incremented to avoid
>>>> + indefinite
>>>> hangs
>>>>
>>>> +    if (TimeDelta == 0) {
>>>>
>>>> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
>>>> (XHC_1_MICROSECOND);
>>>>
>>>> +    }
>>>>
>>>>
>>>>
>>>> -DONE:
>>>>
>>>> -  if (TimeoutEvent != NULL) {
>>>>
>>>> -    gBS->CloseEvent (TimeoutEvent);
>>>>
>>>> -  }
>>>>
>>>> +    ElapsedTime += TimeDelta;
>>>>
>>>> +  } while (ElapsedTime < TimeoutTime);
>>>>
>>>>
>>>>
>>>> -  return Status;
>>>>
>>>> +  return EFI_TIMEOUT;
>>>>
>>>>  }
>>>>
>>>>
>>>>
>>>>  /**
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> index 298fb88b81..5177886e52 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> @@ -2,6 +2,7 @@
>>>>
>>>>
>>>>    XHCI transfer scheduling routines.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2020, Intel Corporation. All rights
>>>> reserved.<BR>
>>>>
>>>>  Copyright (c) Microsoft Corporation.<BR>
>>>>
>>>>  Copyright (C) 2022 Advanced Micro Devices, Inc. All rights
>>>> reserved.<BR>
>>>>
>>>> @@ -1273,15 +1274,14 @@ EXIT:
>>>>  /**
>>>>
>>>>    Execute the transfer by polling the URB. This is a synchronous operation.
>>>>
>>>>
>>>>
>>>> -  @param  Xhc                    The XHCI Instance.
>>>>
>>>> -  @param  CmdTransfer            The executed URB is for cmd transfer or
>> not.
>>>>
>>>> -  @param  Urb                    The URB to execute.
>>>>
>>>> -  @param  Timeout                The time to wait before abort, in millisecond.
>>>>
>>>> +  @param  Xhc               The XHCI Instance.
>>>>
>>>> +  @param  CmdTransfer       The executed URB is for cmd transfer or not.
>>>>
>>>> +  @param  Urb               The URB to execute.
>>>>
>>>> +  @param  Timeout           The time to wait before abort, in millisecond.
>>>>
>>>>
>>>>
>>>> -  @return EFI_DEVICE_ERROR       The transfer failed due to transfer
>> error.
>>>>
>>>> -  @return EFI_TIMEOUT            The transfer failed due to time out.
>>>>
>>>> -  @return EFI_SUCCESS            The transfer finished OK.
>>>>
>>>> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
>> not
>>>> be allocated.
>>>>
>>>> +  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
>>>>
>>>> +  @return EFI_TIMEOUT       The transfer failed due to time out.
>>>>
>>>> +  @return EFI_SUCCESS       The transfer finished OK.
>>>>
>>>>
>>>>
>>>>  **/
>>>>
>>>>  EFI_STATUS
>>>>
>>>> @@ -1296,12 +1296,14 @@ XhcExecTransfer (
>>>>    UINT8       SlotId;
>>>>
>>>>    UINT8       Dci;
>>>>
>>>>    BOOLEAN     Finished;
>>>>
>>>> -  EFI_EVENT   TimeoutEvent;
>>>>
>>>> +  UINT64      TimeoutTime;
>>>>
>>>> +  UINT64      ElapsedTime;
>>>>
>>>> +  UINT64      TimeDelta;
>>>>
>>>> +  UINT64      CurrentTick;
>>>>
>>>>    BOOLEAN     IndefiniteTimeout;
>>>>
>>>>
>>>>
>>>>    Status            = EFI_SUCCESS;
>>>>
>>>>    Finished          = FALSE;
>>>>
>>>> -  TimeoutEvent      = NULL;
>>>>
>>>>    IndefiniteTimeout = FALSE;
>>>>
>>>>
>>>>
>>>>    if (CmdTransfer) {
>>>>
>>>> @@ -1319,34 +1321,14 @@ XhcExecTransfer (
>>>>
>>>>
>>>>    if (Timeout == 0) {
>>>>
>>>>      IndefiniteTimeout = TRUE;
>>>>
>>>> -    goto RINGDOORBELL;
>>>>
>>>> -  }
>>>>
>>>> -
>>>>
>>>> -  Status = gBS->CreateEvent (
>>>>
>>>> -                  EVT_TIMER,
>>>>
>>>> -                  TPL_CALLBACK,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  &TimeoutEvent
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>>    }
>>>>
>>>>
>>>>
>>>> -  Status = gBS->SetTimer (
>>>>
>>>> -                  TimeoutEvent,
>>>>
>>>> -                  TimerRelative,
>>>>
>>>> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>> -  }
>>>>
>>>> -
>>>>
>>>> -RINGDOORBELL:
>>>>
>>>>    XhcRingDoorBell (Xhc, SlotId, Dci);
>>>>
>>>>
>>>>
>>>> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
>>>> XHC_1_MILLISECOND);
>>>>
>>>> +  ElapsedTime = 0;
>>>>
>>>> +  CurrentTick = GetPerformanceCounter ();
>>>>
>>>> +
>>>>
>>>>    do {
>>>>
>>>>      Finished = XhcCheckUrbResult (Xhc, Urb);
>>>>
>>>>      if (Finished) {
>>>>
>>>> @@ -1354,22 +1336,22 @@ RINGDOORBELL:
>>>>      }
>>>>
>>>>
>>>>
>>>>      gBS->Stall (XHC_1_MICROSECOND);
>>>>
>>>> -  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent
>>>> (TimeoutEvent)));
>>>>
>>>> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
>>>>
>>>> +    // Ensure that ElapsedTime is always incremented to avoid
>>>> + indefinite
>>>> hangs
>>>>
>>>> +    if (TimeDelta == 0) {
>>>>
>>>> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
>>>> (XHC_1_MICROSECOND);
>>>>
>>>> +    }
>>>>
>>>>
>>>>
>>>> -DONE:
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
>>>>
>>>> -  } else if (!Finished) {
>>>>
>>>> +    ElapsedTime += TimeDelta;
>>>>
>>>> +  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
>>>>
>>>> +
>>>>
>>>> +  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.34.1
>>>>
>>>>
>>>>
>>>> -=-=-=-=-=-=
>>>> Groups.io Links: You receive all messages sent to this group.
>>>> View/Reply Online (#106668):
>>>> INVALID URI REMOVED
>>>> 06
>>>> 668__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
>>> lctg5_B3K1Lcta4
>>>> 52Gx-1twRt8At3cuWxQsO18$ Mute This Topic:
>>>> https://groups.io/mt/99972791/1768737
>>>> NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
>>> lctg5_B3K1Lcta452Gx-1tw
>>>> Rt8At3cubh2HDyk$ Group Owner: devel+owner@edk2.groups.io
>>>> Unsubscribe:
>>>> https://edk2.groups.io/g/devel/unsub
>>>> !N
>>>> pxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
>>> lctg5_B3K1Lcta452Gx-1twR
>>>> t8At3cuWYMMXVU$  [hao.a.wu@intel.com] -=-=-=-=-=-=
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110404): https://edk2.groups.io/g/devel/message/110404
Mute This Topic: https://groups.io/mt/99972791/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Henz, Patrick]

That’s a great catch, thank you for bringing this to my attention Laszlo. Work is in progress.

Patrick Henz

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Tuesday, October 31, 2023 7:16 AM
To: devel@edk2.groups.io; hao.a.wu@intel.com; Henz, Patrick <patrick.henz@hpe.com>; Michael Brown <mcb30@ipxe.org>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

On 10/31/23 02:41, Wu, Hao A wrote:
> (Add MdePkg maintainers for their feedbacks)
> 
> Sorry that I do not have strong opinion on this one.
> 
> Some of my thoughts are:
> * If you find the to-be-added APIs can be used in serveral places to reduce
>   repetative codes, then it will be worthwhile to add new library APIs.
> 
> * TimerLib has many instance implementations, my take is that many post-mem
>   instances use library level global variable to store information like timer
>   frequency to avoid calculating it every time.
>   For pre-mem instances, such caching is not feasible. I think it will be the
>   API consumer's responsibility to limit the usage of these APIs for performance
>   consideration.
> 
> Best Regards,
> Hao Wu

Patrick, while at it: I've recently filed the bug

  potentially invalid elapsed tick count calculation from commit 43dcf453fc15 

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

and assigned it to you (given that that commit is yours). I proposed the solution in the bugzilla too; please check it out if you can find the time.

Thanks
Laszlo

> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz, 
>> Patrick
>> Sent: Tuesday, October 31, 2023 4:36 AM
>> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io; Michael 
>> Brown <mcb30@ipxe.org>
>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use 
>> Performance Timer for XHCI Timeouts
>>
>> My changes have been in for a little over a month now, I've been 
>> looking into updating the TimerLib API but I'm questioning if it 
>> still makes sense to go down that path.  The functions I had 
>> implemented, XhcGetElapsedTicks () and XhcConvertTimeToTicks (), now 
>> rely heavily on the use of global variables for caching return values 
>> from GetPerformanceCounterProperties. As-is these functions won't 
>> work for all instances of TimerLib, specifically if the instance is 
>> used before memory (RAM) is available. I could implement the 
>> functions as they originally were, but I wouldn't be able to replace 
>> the Xhc functions without adding additional parameters to said 
>> TimerLib functions (e.g. adding Frequency, StartValue, StopValue to 
>> ConvertTimeToTicks), which feels clunky to me. Would a new library 
>> make sense here? Something that sits between the driver and the TimerLib library? Or do we just leave things as they currently are?
>>
>> Thanks,
>> Patrick Henz
>>
>> -----Original Message-----
>> From: Wu, Hao A <hao.a.wu@intel.com>
>> Sent: Thursday, August 10, 2023 8:43 PM
>> To: Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io; 
>> Michael Brown <mcb30@ipxe.org>
>> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use 
>> Performance Timer for XHCI Timeouts
>>
>> My personal preference is to do this as two steps.
>> Address the issue in XhciDxe first. Then add new TimerLib API to 
>> replace all driver/library internal implementations.
>>
>> Best Regards,
>> Hao Wu
>>
>>> -----Original Message-----
>>> From: Henz, Patrick <patrick.henz@hpe.com>
>>> Sent: Friday, August 11, 2023 6:45 AM
>>> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Michael 
>>> Brown <mcb30@ipxe.org>
>>> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use 
>>> Performance Timer for XHCI Timeouts
>>>
>>> I can certainly make that change.
>>>
>>> For what it's worth I have been working on adding the new function, 
>>> GetElapsedTicks, to the various implementations of TimerLib. I've 
>>> finished up testing, I would just need to clean up the commits for a 
>>> patch. Should I move forward with that, or would we rather I just 
>>> add XhcGetElapsedTime to XhciDxe for the time being?
>>>
>>> Thanks,
>>> Patrick Henz
>>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
>> Hao
>>> A
>>> Sent: Sunday, July 30, 2023 9:57 PM
>>> To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>; 
>>> Michael Brown <mcb30@ipxe.org>
>>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use 
>>> Performance Timer for XHCI Timeouts
>>>
>>> For the 2 occurrences of in XhcWaitOpRegBit & XhcExecTransfer:
>>>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout * 
>>> XHC_1_MILLISECOND); How about changing them to:
>>>   TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND ((UINT64)
>> Timeout
>>> * XHC_1_MILLISECOND); to address possible overflow during "Timeout * 
>>> XHC_1_MILLISECOND"?
>>>
>>> For extending XhcGetElapsedTime as a TimerLib API, I am fine to put 
>>> it in XhciDxe at this moment.
>>> If package maintainers suggest to make it as a public library API, 
>>> my take is that this should be done in a separate commit.
>>>
>>> Best Regards,
>>> Hao Wu
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
>>>> Henz, Patrick
>>>> Sent: Thursday, July 6, 2023 4:16 AM
>>>> To: devel@edk2.groups.io
>>>> Cc: Henz, Patrick <patrick.henz@hpe.com>
>>>> Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance 
>>>> Timer for XHCI Timeouts
>>>>
>>>> REF:INVALID URI REMOVED
>>>> bu
>>>> g.cgi?id=2948__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-
>>> NKbMjOV-lctg5
>>>> _B3K1Lcta452Gx-1twRt8At3cueAYDq_n$
>>>>
>>>> XhciDxe uses the timer functionality provided by the boot services 
>>>> table to detect timeout conditions. This breaks the driver's 
>>>> ExitBootServices call back, as CoreExitBootServices halts the timer 
>>>> before signaling the ExitBootServices event. If the host controller 
>>>> fails to halt in the call back, the timeout condition will never 
>>>> occur and the boot gets stuck in an indefinite spin loop. Use the 
>>>> free running timer provided by TimerLib to calculate timeouts, 
>>>> avoiding the potential hang.
>>>>
>>>> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
>>>> Reviewed-by:
>>>> ---
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
>>>>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68
>>>> +++++++++---------------
>>>>  5 files changed, 129 insertions(+), 86 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> index 62682dd27c..1dcbe20512 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>>>> @@ -1,6 +1,7 @@
>>>>  /** @file
>>>>
>>>>    The XHCI controller driver.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2022, Intel Corporation. All rights 
>>>> reserved.<BR>
>>>>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>>
>>>>
>>>> @@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
>>>>
>>>>
>>>>    return EFI_SUCCESS;
>>>>
>>>>  }
>>>>
>>>> +
>>>>
>>>> +/**
>>>>
>>>> +  Computes and returns the elapsed time in nanoseconds since
>>>>
>>>> +  PreviousTick. The value of PreviousTick is overwritten with the
>>>>
>>>> +  current performance counter value.
>>>>
>>>> +
>>>>
>>>> +  @param  PreviousTick    Pointer to PreviousTick count.
>>>>
>>>> +  @return The elapsed time in nanoseconds since PreviousCount.
>>>>
>>>> +          PreviousCount is overwritten with the current 
>>>> + performance
>>>>
>>>> +          counter value.
>>>>
>>>> +**/
>>>>
>>>> +UINT64
>>>>
>>>> +XhcGetElapsedTime (
>>>>
>>>> +  IN OUT UINT64  *PreviousTick
>>>>
>>>> +  )
>>>>
>>>> +{
>>>>
>>>> +  UINT64  StartValue;
>>>>
>>>> +  UINT64  EndValue;
>>>>
>>>> +  UINT64  CurrentTick;
>>>>
>>>> +  UINT64  Delta;
>>>>
>>>> +
>>>>
>>>> +  GetPerformanceCounterProperties (&StartValue, &EndValue);
>>>>
>>>> +
>>>>
>>>> +  CurrentTick = GetPerformanceCounter ();
>>>>
>>>> +
>>>>
>>>> +  //
>>>>
>>>> +  // Determine if the counter is counting up or down
>>>>
>>>> +  //
>>>>
>>>> +  if (StartValue < EndValue) {
>>>>
>>>> +    //
>>>>
>>>> +    // Counter counts upwards, check for an overflow condition
>>>>
>>>> +    //
>>>>
>>>> +    if (*PreviousTick > CurrentTick) {
>>>>
>>>> +      Delta = (EndValue - *PreviousTick) + CurrentTick;
>>>>
>>>> +    } else {
>>>>
>>>> +      Delta = CurrentTick - *PreviousTick;
>>>>
>>>> +    }
>>>>
>>>> +  } else {
>>>>
>>>> +    //
>>>>
>>>> +    // Counter counts downwards, check for an underflow condition
>>>>
>>>> +    //
>>>>
>>>> +    if (*PreviousTick < CurrentTick) {
>>>>
>>>> +      Delta = (StartValue - CurrentTick) + *PreviousTick;
>>>>
>>>> +    } else {
>>>>
>>>> +      Delta = *PreviousTick - CurrentTick;
>>>>
>>>> +    }
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  //
>>>>
>>>> +  // Set PreviousTick to CurrentTick
>>>>
>>>> +  //
>>>>
>>>> +  *PreviousTick = CurrentTick;
>>>>
>>>> +
>>>>
>>>> +  return GetTimeInNanoSecond (Delta);
>>>>
>>>> +}
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> index ca223bd20c..77feb66647 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
>>>> @@ -2,6 +2,7 @@
>>>>
>>>>
>>>>    Provides some data structure definitions used by the XHCI host 
>>>> controller driver.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2017, Intel Corporation. All rights 
>>>> reserved.<BR>
>>>>
>>>>  Copyright (c) Microsoft Corporation.<BR>
>>>>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>> @@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent 
>>>> #include <Library/UefiLib.h>
>>>>
>>>>  #include <Library/DebugLib.h>
>>>>
>>>>  #include <Library/ReportStatusCodeLib.h>
>>>>
>>>> +#include <Library/TimerLib.h>
>>>>
>>>>
>>>>
>>>>  #include <IndustryStandard/Pci.h>
>>>>
>>>>
>>>>
>>>> @@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT
>>> USB_DEV_CONTEXT;
>>>> #include "ComponentName.h"
>>>>
>>>>  #include "UsbHcMem.h"
>>>>
>>>>
>>>>
>>>> +//
>>>>
>>>> +// Converts a count from microseconds to nanoseconds
>>>>
>>>> +//
>>>>
>>>> +#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time)
>> *
>>>> 1000)
>>>>
>>>>  //
>>>>
>>>>  // The unit is microsecond, setting it as 1us.
>>>>
>>>>  //
>>>>
>>>> @@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
>>>>    IN     VOID                                *Context
>>>>
>>>>    );
>>>>
>>>>
>>>>
>>>> +/**
>>>>
>>>> +  Computes and returns the elapsed time in nanoseconds since
>>>>
>>>> +  PreviousTick. The value of PreviousTick is overwritten with the
>>>>
>>>> +  current performance counter value.
>>>>
>>>> +
>>>>
>>>> +  @param  PreviousTick    Pointer to PreviousTick count.
>>>>
>>>> +                          before calling this function.
>>>>
>>>> +  @return The elapsed time in nanoseconds since PreviousCount.
>>>>
>>>> +          PreviousCount is overwritten with the current 
>>>> + performance
>>>>
>>>> +          counter value.
>>>>
>>>> +**/
>>>>
>>>> +UINT64
>>>>
>>>> +XhcGetElapsedTime (
>>>>
>>>> +  IN OUT UINT64  *PreviousTick
>>>>
>>>> +  );
>>>>
>>>> +
>>>>
>>>>  #endif
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> index 5865d86822..18ef87916a 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
>>>> @@ -3,6 +3,7 @@
>>>>  #  It implements the interfaces of monitoring the status of all 
>>>> ports and transferring
>>>>
>>>>  #  Control, Bulk, Interrupt and Isochronous requests to those 
>>>> attached usb LS/FS/HS/SS devices.
>>>>
>>>>  #
>>>>
>>>> +#  (C) Copyright 2023 Hewlett Packard Enterprise Development 
>>>> +LP<BR>
>>>>
>>>>  #  Copyright (c) 2011 - 2018, Intel Corporation. All rights 
>>>> reserved.<BR>
>>>>
>>>>  #
>>>>
>>>>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>> @@ -54,6 +55,7 @@
>>>>    BaseMemoryLib
>>>>
>>>>    DebugLib
>>>>
>>>>    ReportStatusCodeLib
>>>>
>>>> +  TimerLib
>>>>
>>>>
>>>>
>>>>  [Guids]
>>>>
>>>>    gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
>>>> Event
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> index 5700fc5fb8..2499698715 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
>>>> @@ -2,6 +2,7 @@
>>>>
>>>>
>>>>    The XHCI register operation routines.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2017, Intel Corporation. All rights 
>>>> reserved.<BR>
>>>>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>>
>>>>
>>>> @@ -417,15 +418,14 @@ XhcClearOpRegBit (
>>>>    Wait the operation register's bit as specified by Bit
>>>>
>>>>    to become set (or clear).
>>>>
>>>>
>>>>
>>>> -  @param  Xhc                    The XHCI Instance.
>>>>
>>>> -  @param  Offset                 The offset of the operation register.
>>>>
>>>> -  @param  Bit                    The bit of the register to wait for.
>>>>
>>>> -  @param  WaitToSet              Wait the bit to set or clear.
>>>>
>>>> -  @param  Timeout                The time to wait before abort (in millisecond,
>>>> ms).
>>>>
>>>> +  @param  Xhc          The XHCI Instance.
>>>>
>>>> +  @param  Offset       The offset of the operation register.
>>>>
>>>> +  @param  Bit          The bit of the register to wait for.
>>>>
>>>> +  @param  WaitToSet    Wait the bit to set or clear.
>>>>
>>>> +  @param  Timeout      The time to wait before abort (in millisecond, ms).
>>>>
>>>>
>>>>
>>>> -  @retval EFI_SUCCESS            The bit successfully changed by host
>>> controller.
>>>>
>>>> -  @retval EFI_TIMEOUT            The time out occurred.
>>>>
>>>> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
>> not
>>>> be allocated.
>>>>
>>>> +  @retval EFI_SUCCESS  The bit successfully changed by host controller.
>>>>
>>>> +  @retval EFI_TIMEOUT  The time out occurred.
>>>>
>>>>
>>>>
>>>>  **/
>>>>
>>>>  EFI_STATUS
>>>>
>>>> @@ -437,54 +437,35 @@ XhcWaitOpRegBit (
>>>>    IN UINT32             Timeout
>>>>
>>>>    )
>>>>
>>>>  {
>>>>
>>>> -  EFI_STATUS  Status;
>>>>
>>>> -  EFI_EVENT   TimeoutEvent;
>>>>
>>>> -
>>>>
>>>> -  TimeoutEvent = NULL;
>>>>
>>>> +  UINT64  TimeoutTime;
>>>>
>>>> +  UINT64  ElapsedTime;
>>>>
>>>> +  UINT64  TimeDelta;
>>>>
>>>> +  UINT64  CurrentTick;
>>>>
>>>>
>>>>
>>>>    if (Timeout == 0) {
>>>>
>>>>      return EFI_TIMEOUT;
>>>>
>>>>    }
>>>>
>>>>
>>>>
>>>> -  Status = gBS->CreateEvent (
>>>>
>>>> -                  EVT_TIMER,
>>>>
>>>> -                  TPL_CALLBACK,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  &TimeoutEvent
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>> -  }
>>>>
>>>> -
>>>>
>>>> -  Status = gBS->SetTimer (
>>>>
>>>> -                  TimeoutEvent,
>>>>
>>>> -                  TimerRelative,
>>>>
>>>> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>> -  }
>>>>
>>>> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
>>>> XHC_1_MILLISECOND);
>>>>
>>>> +  ElapsedTime = 0;
>>>>
>>>> +  CurrentTick = GetPerformanceCounter ();
>>>>
>>>>
>>>>
>>>>    do {
>>>>
>>>>      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
>>>>
>>>> -      Status = EFI_SUCCESS;
>>>>
>>>> -      goto DONE;
>>>>
>>>> +      return EFI_SUCCESS;
>>>>
>>>>      }
>>>>
>>>>
>>>>
>>>>      gBS->Stall (XHC_1_MICROSECOND);
>>>>
>>>> -  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
>>>>
>>>> -
>>>>
>>>> -  Status = EFI_TIMEOUT;
>>>>
>>>> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
>>>>
>>>> +    // Ensure that ElapsedTime is always incremented to avoid 
>>>> + indefinite
>>>> hangs
>>>>
>>>> +    if (TimeDelta == 0) {
>>>>
>>>> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
>>>> (XHC_1_MICROSECOND);
>>>>
>>>> +    }
>>>>
>>>>
>>>>
>>>> -DONE:
>>>>
>>>> -  if (TimeoutEvent != NULL) {
>>>>
>>>> -    gBS->CloseEvent (TimeoutEvent);
>>>>
>>>> -  }
>>>>
>>>> +    ElapsedTime += TimeDelta;
>>>>
>>>> +  } while (ElapsedTime < TimeoutTime);
>>>>
>>>>
>>>>
>>>> -  return Status;
>>>>
>>>> +  return EFI_TIMEOUT;
>>>>
>>>>  }
>>>>
>>>>
>>>>
>>>>  /**
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> index 298fb88b81..5177886e52 100644
>>>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>>>> @@ -2,6 +2,7 @@
>>>>
>>>>
>>>>    XHCI transfer scheduling routines.
>>>>
>>>>
>>>>
>>>> +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
>>>>
>>>>  Copyright (c) 2011 - 2020, Intel Corporation. All rights 
>>>> reserved.<BR>
>>>>
>>>>  Copyright (c) Microsoft Corporation.<BR>
>>>>
>>>>  Copyright (C) 2022 Advanced Micro Devices, Inc. All rights 
>>>> reserved.<BR>
>>>>
>>>> @@ -1273,15 +1274,14 @@ EXIT:
>>>>  /**
>>>>
>>>>    Execute the transfer by polling the URB. This is a synchronous operation.
>>>>
>>>>
>>>>
>>>> -  @param  Xhc                    The XHCI Instance.
>>>>
>>>> -  @param  CmdTransfer            The executed URB is for cmd transfer or
>> not.
>>>>
>>>> -  @param  Urb                    The URB to execute.
>>>>
>>>> -  @param  Timeout                The time to wait before abort, in millisecond.
>>>>
>>>> +  @param  Xhc               The XHCI Instance.
>>>>
>>>> +  @param  CmdTransfer       The executed URB is for cmd transfer or not.
>>>>
>>>> +  @param  Urb               The URB to execute.
>>>>
>>>> +  @param  Timeout           The time to wait before abort, in millisecond.
>>>>
>>>>
>>>>
>>>> -  @return EFI_DEVICE_ERROR       The transfer failed due to transfer
>> error.
>>>>
>>>> -  @return EFI_TIMEOUT            The transfer failed due to time out.
>>>>
>>>> -  @return EFI_SUCCESS            The transfer finished OK.
>>>>
>>>> -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
>> not
>>>> be allocated.
>>>>
>>>> +  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
>>>>
>>>> +  @return EFI_TIMEOUT       The transfer failed due to time out.
>>>>
>>>> +  @return EFI_SUCCESS       The transfer finished OK.
>>>>
>>>>
>>>>
>>>>  **/
>>>>
>>>>  EFI_STATUS
>>>>
>>>> @@ -1296,12 +1296,14 @@ XhcExecTransfer (
>>>>    UINT8       SlotId;
>>>>
>>>>    UINT8       Dci;
>>>>
>>>>    BOOLEAN     Finished;
>>>>
>>>> -  EFI_EVENT   TimeoutEvent;
>>>>
>>>> +  UINT64      TimeoutTime;
>>>>
>>>> +  UINT64      ElapsedTime;
>>>>
>>>> +  UINT64      TimeDelta;
>>>>
>>>> +  UINT64      CurrentTick;
>>>>
>>>>    BOOLEAN     IndefiniteTimeout;
>>>>
>>>>
>>>>
>>>>    Status            = EFI_SUCCESS;
>>>>
>>>>    Finished          = FALSE;
>>>>
>>>> -  TimeoutEvent      = NULL;
>>>>
>>>>    IndefiniteTimeout = FALSE;
>>>>
>>>>
>>>>
>>>>    if (CmdTransfer) {
>>>>
>>>> @@ -1319,34 +1321,14 @@ XhcExecTransfer (
>>>>
>>>>
>>>>    if (Timeout == 0) {
>>>>
>>>>      IndefiniteTimeout = TRUE;
>>>>
>>>> -    goto RINGDOORBELL;
>>>>
>>>> -  }
>>>>
>>>> -
>>>>
>>>> -  Status = gBS->CreateEvent (
>>>>
>>>> -                  EVT_TIMER,
>>>>
>>>> -                  TPL_CALLBACK,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  NULL,
>>>>
>>>> -                  &TimeoutEvent
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>>    }
>>>>
>>>>
>>>>
>>>> -  Status = gBS->SetTimer (
>>>>
>>>> -                  TimeoutEvent,
>>>>
>>>> -                  TimerRelative,
>>>>
>>>> -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
>>>>
>>>> -                  );
>>>>
>>>> -
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    goto DONE;
>>>>
>>>> -  }
>>>>
>>>> -
>>>>
>>>> -RINGDOORBELL:
>>>>
>>>>    XhcRingDoorBell (Xhc, SlotId, Dci);
>>>>
>>>>
>>>>
>>>> +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout *
>>>> XHC_1_MILLISECOND);
>>>>
>>>> +  ElapsedTime = 0;
>>>>
>>>> +  CurrentTick = GetPerformanceCounter ();
>>>>
>>>> +
>>>>
>>>>    do {
>>>>
>>>>      Finished = XhcCheckUrbResult (Xhc, Urb);
>>>>
>>>>      if (Finished) {
>>>>
>>>> @@ -1354,22 +1336,22 @@ RINGDOORBELL:
>>>>      }
>>>>
>>>>
>>>>
>>>>      gBS->Stall (XHC_1_MICROSECOND);
>>>>
>>>> -  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent 
>>>> (TimeoutEvent)));
>>>>
>>>> +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
>>>>
>>>> +    // Ensure that ElapsedTime is always incremented to avoid 
>>>> + indefinite
>>>> hangs
>>>>
>>>> +    if (TimeDelta == 0) {
>>>>
>>>> +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
>>>> (XHC_1_MICROSECOND);
>>>>
>>>> +    }
>>>>
>>>>
>>>>
>>>> -DONE:
>>>>
>>>> -  if (EFI_ERROR (Status)) {
>>>>
>>>> -    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
>>>>
>>>> -  } else if (!Finished) {
>>>>
>>>> +    ElapsedTime += TimeDelta;
>>>>
>>>> +  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
>>>>
>>>> +
>>>>
>>>> +  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.34.1
>>>>
>>>>
>>>>
>>>> -=-=-=-=-=-=
>>>> Groups.io Links: You receive all messages sent to this group.
>>>> View/Reply Online (#106668):
>>>> INVALID URI REMOVED
>>>> 06
>>>> 668__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
>>> lctg5_B3K1Lcta4
>>>> 52Gx-1twRt8At3cuWxQsO18$ Mute This Topic:
>>>> https://groups.io/mt/99972791/1768737
>>>> NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
>>> lctg5_B3K1Lcta452Gx-1tw
>>>> Rt8At3cubh2HDyk$ Group Owner: devel+owner@edk2.groups.io
>>>> Unsubscribe:
>>>> https://edk2.groups.io/g/devel/unsub
>>>> !N
>>>> pxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-
>>> lctg5_B3K1Lcta452Gx-1twR
>>>> t8At3cuWYMMXVU$  [hao.a.wu@intel.com] -=-=-=-=-=-=
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110411): https://edk2.groups.io/g/devel/message/110411
Mute This Topic: https://groups.io/mt/99972791/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-