* [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
@ 2023-08-14 15:51 Henz, Patrick
0 siblings, 0 replies; 11+ messages in thread
From: Henz, Patrick @ 2023-08-14 15:51 UTC (permalink / raw)
To: devel; +Cc: Patrick Henz
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..07e57772b6 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 ((UINT64)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 53421e64a8..7d4b7a769d 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>
@@ -1276,15 +1277,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
@@ -1299,12 +1299,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) {
@@ -1322,34 +1324,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 ((UINT64)Timeout * XHC_1_MILLISECOND);
+ ElapsedTime = 0;
+ CurrentTick = GetPerformanceCounter ();
+
do {
Finished = XhcCheckUrbResult (Xhc, Urb);
if (Finished) {
@@ -1357,22 +1339,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 (#107733): https://edk2.groups.io/g/devel/message/107733
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
[not found] <177B4AD1F8C3A6A2.8497@groups.io>
@ 2023-09-06 20:36 ` Henz, Patrick
2023-09-06 20:49 ` Pedro Falcato
2023-09-07 1:27 ` Wu, Hao A
0 siblings, 2 replies; 11+ messages in thread
From: Henz, Patrick @ 2023-09-06 20:36 UTC (permalink / raw)
To: devel@edk2.groups.io
I sent this patch out a few weeks ago now but haven't seen a reply, just checking in to make sure it didn't get missed.
Thanks,
Patrick Henz
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz, Patrick
Sent: Monday, August 14, 2023 10:52 AM
To: devel@edk2.groups.io
Cc: Henz, Patrick <patrick.henz@hpe.com>
Subject: [edk2-devel] [PATCH v2] 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..07e57772b6 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 ((UINT64)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 53421e64a8..7d4b7a769d 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> @@ -1276,15 +1277,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
@@ -1299,12 +1299,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) {
@@ -1322,34 +1324,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 ((UINT64)Timeout *
+ XHC_1_MILLISECOND); ElapsedTime = 0; CurrentTick =
+ GetPerformanceCounter ();
+
do {
Finished = XhcCheckUrbResult (Xhc, Urb);
if (Finished) {
@@ -1357,22 +1339,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 (#108340): https://edk2.groups.io/g/devel/message/108340
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
2023-09-06 20:36 ` [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts Henz, Patrick
@ 2023-09-06 20:49 ` Pedro Falcato
2023-09-07 1:27 ` Wu, Hao A
1 sibling, 0 replies; 11+ messages in thread
From: Pedro Falcato @ 2023-09-06 20:49 UTC (permalink / raw)
To: devel, patrick.henz
On Wed, Sep 6, 2023 at 9:37 PM Henz, Patrick <patrick.henz@hpe.com> wrote:
>
> I sent this patch out a few weeks ago now but haven't seen a reply, just checking in to make sure it didn't get missed.
Hi,
Seems like you're missing the proper CC's (for the reviewers and
maintainers) on the patch's email.
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108341): https://edk2.groups.io/g/devel/message/108341
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
2023-09-06 20:36 ` [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts Henz, Patrick
2023-09-06 20:49 ` Pedro Falcato
@ 2023-09-07 1:27 ` Wu, Hao A
2023-09-07 2:59 ` Wu, Hao A
2023-09-07 7:36 ` Mike Maslenkin
1 sibling, 2 replies; 11+ messages in thread
From: Wu, Hao A @ 2023-09-07 1:27 UTC (permalink / raw)
To: devel@edk2.groups.io, Henz, Patrick
Sorry for the late response.
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> Patrick
> Sent: Thursday, September 7, 2023 4:37 AM
> To: devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
>
> I sent this patch out a few weeks ago now but haven't seen a reply, just
> checking in to make sure it didn't get missed.
>
> Thanks,
> Patrick Henz
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> Patrick
> Sent: Monday, August 14, 2023 10:52 AM
> To: devel@edk2.groups.io
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: [edk2-devel] [PATCH v2] 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..07e57772b6 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 ((UINT64)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 53421e64a8..7d4b7a769d 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> @@ -1276,15 +1277,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
> @@ -1299,12 +1299,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) {
> @@ -1322,34 +1324,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 ((UINT64)Timeout
> *
> + XHC_1_MILLISECOND); ElapsedTime = 0; CurrentTick =
> + GetPerformanceCounter ();
> +
> do {
> Finished = XhcCheckUrbResult (Xhc, Urb);
> if (Finished) {
> @@ -1357,22 +1339,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 (#108348): https://edk2.groups.io/g/devel/message/108348
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
2023-09-07 1:27 ` Wu, Hao A
@ 2023-09-07 2:59 ` Wu, Hao A
2023-09-07 7:36 ` Mike Maslenkin
1 sibling, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2023-09-07 2:59 UTC (permalink / raw)
To: devel@edk2.groups.io, Henz, Patrick
Sorry, I found that there is a build failure for the proposed change:
PR - https://github.com/tianocore/edk2/pull/4796
Details:
https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=101097&view=logs&j=2dc6585a-9807-582d-5eb8-65e26db2b1d0&t=fe91d259-3a7c-5e73-4892-c865ef48cb16
ERROR - Linker #2001 from XhciDxe.lib(XhciSched.obj) : unresolved external symbol __allmul
ERROR - Linker #2001 from XhciDxe.lib(XhciReg.obj) : unresolved external symbol __allmul
ERROR - Linker #1120 from d:\a\1\s\Build\OvmfIa32\NOOPT_VS2019\IA32\MdeModulePkg\Bus\Pci\XhciDxe\XhciDxe\DEBUG\XhciDxe.dll : fatal 1 unresolved externals
I think we need to update "(UINT64)Timeout * XHC_1_MILLISECOND" by using BaseLib API MultU64x32().
Could you help to update the patch and check if it can pass the CI builds, thanks.
Best Regards,
Hao Wu
> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, September 7, 2023 9:28 AM
> To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>
> Subject: RE: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
>
> Sorry for the late response.
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
>
> Best Regards,
> Hao Wu
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > Patrick
> > Sent: Thursday, September 7, 2023 4:37 AM
> > To: devel@edk2.groups.io
> > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > Performance Timer for XHCI Timeouts
> >
> > I sent this patch out a few weeks ago now but haven't seen a reply,
> > just checking in to make sure it didn't get missed.
> >
> > Thanks,
> > Patrick Henz
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > Patrick
> > Sent: Monday, August 14, 2023 10:52 AM
> > To: devel@edk2.groups.io
> > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > Subject: [edk2-devel] [PATCH v2] 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..07e57772b6 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
> ((UINT64)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 53421e64a8..7d4b7a769d 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> @@ -1276,15
> +1277,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
> > @@ -1299,12 +1299,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) {
> > @@ -1322,34 +1324,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
> ((UINT64)Timeout
> > *
> > + XHC_1_MILLISECOND); ElapsedTime = 0; CurrentTick =
> > + GetPerformanceCounter ();
> > +
> > do {
> > Finished = XhcCheckUrbResult (Xhc, Urb);
> > if (Finished) {
> > @@ -1357,22 +1339,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 (#108356): https://edk2.groups.io/g/devel/message/108356
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
2023-09-07 1:27 ` Wu, Hao A
2023-09-07 2:59 ` Wu, Hao A
@ 2023-09-07 7:36 ` Mike Maslenkin
2023-09-07 7:48 ` Wu, Hao A
1 sibling, 1 reply; 11+ messages in thread
From: Mike Maslenkin @ 2023-09-07 7:36 UTC (permalink / raw)
To: devel, hao.a.wu; +Cc: Henz, Patrick
Hello Hao Wu,
Isn't GetPerformanceCounterProperties (&StartValue, &EndValue) call
too "heavy" for XHCI paths?
May be it's better to cache timer direction once?
I'm asking in a context of PcAtChipsetPkg/Library/AcpiTimerLib
instance used by a number of Intel's platform in edk-platforms.
For this library GetPerformanceCounterProperties finally calls
InternalCalculateTscFrequency, that isn't simple.
Regards,
Mike
On Thu, Sep 7, 2023 at 4:27 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> Sorry for the late response.
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
>
> Best Regards,
> Hao Wu
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > Patrick
> > Sent: Thursday, September 7, 2023 4:37 AM
> > To: devel@edk2.groups.io
> > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > Performance Timer for XHCI Timeouts
> >
> > I sent this patch out a few weeks ago now but haven't seen a reply, just
> > checking in to make sure it didn't get missed.
> >
> > Thanks,
> > Patrick Henz
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > Patrick
> > Sent: Monday, August 14, 2023 10:52 AM
> > To: devel@edk2.groups.io
> > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > Subject: [edk2-devel] [PATCH v2] 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..07e57772b6 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 ((UINT64)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 53421e64a8..7d4b7a769d 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> @@ -1276,15 +1277,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
> > @@ -1299,12 +1299,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) {
> > @@ -1322,34 +1324,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 ((UINT64)Timeout
> > *
> > + XHC_1_MILLISECOND); ElapsedTime = 0; CurrentTick =
> > + GetPerformanceCounter ();
> > +
> > do {
> > Finished = XhcCheckUrbResult (Xhc, Urb);
> > if (Finished) {
> > @@ -1357,22 +1339,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 (#108362): https://edk2.groups.io/g/devel/message/108362
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
2023-09-07 7:36 ` Mike Maslenkin
@ 2023-09-07 7:48 ` Wu, Hao A
2023-09-07 7:57 ` Wu, Hao A
2023-09-07 18:38 ` Henz, Patrick
0 siblings, 2 replies; 11+ messages in thread
From: Wu, Hao A @ 2023-09-07 7:48 UTC (permalink / raw)
To: Mike Maslenkin, devel@edk2.groups.io; +Cc: Henz, Patrick
I do not have strong opinion on this considering it is an IO driver.
The call of GetTimeInNanoSecond() is also likely to invoke GetPerformanceCounterProperties() call.
I will let the patch owner to decide on the open raised below.
Best Regards,
Hao Wu
> -----Original Message-----
> From: Mike Maslenkin <mike.maslenkin@gmail.com>
> Sent: Thursday, September 7, 2023 3:36 PM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
>
> Hello Hao Wu,
>
> Isn't GetPerformanceCounterProperties (&StartValue, &EndValue) call too
> "heavy" for XHCI paths?
> May be it's better to cache timer direction once?
> I'm asking in a context of PcAtChipsetPkg/Library/AcpiTimerLib
> instance used by a number of Intel's platform in edk-platforms.
> For this library GetPerformanceCounterProperties finally calls
> InternalCalculateTscFrequency, that isn't simple.
>
> Regards,
> Mike
>
> On Thu, Sep 7, 2023 at 4:27 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
> >
> > Sorry for the late response.
> > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> >
> > Best Regards,
> > Hao Wu
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > > Patrick
> > > Sent: Thursday, September 7, 2023 4:37 AM
> > > To: devel@edk2.groups.io
> > > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > Performance Timer for XHCI Timeouts
> > >
> > > I sent this patch out a few weeks ago now but haven't seen a reply,
> > > just checking in to make sure it didn't get missed.
> > >
> > > Thanks,
> > > Patrick Henz
> > >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > > Patrick
> > > Sent: Monday, August 14, 2023 10:52 AM
> > > To: devel@edk2.groups.io
> > > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > > Subject: [edk2-devel] [PATCH v2] 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..07e57772b6 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
> ((UINT64)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 53421e64a8..7d4b7a769d 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> @@ -
> 1276,15 +1277,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
> > > @@ -1299,12 +1299,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) {
> > > @@ -1322,34 +1324,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
> ((UINT64)Timeout
> > > *
> > > + XHC_1_MILLISECOND); ElapsedTime = 0; CurrentTick =
> > > + GetPerformanceCounter ();
> > > +
> > > do {
> > > Finished = XhcCheckUrbResult (Xhc, Urb);
> > > if (Finished) {
> > > @@ -1357,22 +1339,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 (#108363): https://edk2.groups.io/g/devel/message/108363
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
2023-09-07 7:48 ` Wu, Hao A
@ 2023-09-07 7:57 ` Wu, Hao A
2023-09-07 18:38 ` Henz, Patrick
1 sibling, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2023-09-07 7:57 UTC (permalink / raw)
To: devel@edk2.groups.io, Wu, Hao A, Mike Maslenkin; +Cc: Henz, Patrick
Speaking of PcAtChipsetPkg/Library/AcpiTimerLib, the implementation of below instances:
* PeiAcpiTimerLib.inf
* DxeAcpiTimerLib.inf
* StandaloneMmAcpiTimerLib.inf
will not calculate the TSC frequency upon every call of GetPerformanceCounterProperties().
Best Regards,
Hao Wu
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Thursday, September 7, 2023 3:48 PM
> To: Mike Maslenkin <mike.maslenkin@gmail.com>; devel@edk2.groups.io
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
>
> I do not have strong opinion on this considering it is an IO driver.
> The call of GetTimeInNanoSecond() is also likely to invoke
> GetPerformanceCounterProperties() call.
>
> I will let the patch owner to decide on the open raised below.
>
> Best Regards,
> Hao Wu
>
> > -----Original Message-----
> > From: Mike Maslenkin <mike.maslenkin@gmail.com>
> > Sent: Thursday, September 7, 2023 3:36 PM
> > To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>
> > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > Performance Timer for XHCI Timeouts
> >
> > Hello Hao Wu,
> >
> > Isn't GetPerformanceCounterProperties (&StartValue, &EndValue) call
> > too "heavy" for XHCI paths?
> > May be it's better to cache timer direction once?
> > I'm asking in a context of PcAtChipsetPkg/Library/AcpiTimerLib
> > instance used by a number of Intel's platform in edk-platforms.
> > For this library GetPerformanceCounterProperties finally calls
> > InternalCalculateTscFrequency, that isn't simple.
> >
> > Regards,
> > Mike
> >
> > On Thu, Sep 7, 2023 at 4:27 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
> > >
> > > Sorry for the late response.
> > > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Henz, Patrick
> > > > Sent: Thursday, September 7, 2023 4:37 AM
> > > > To: devel@edk2.groups.io
> > > > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > > Performance Timer for XHCI Timeouts
> > > >
> > > > I sent this patch out a few weeks ago now but haven't seen a
> > > > reply, just checking in to make sure it didn't get missed.
> > > >
> > > > Thanks,
> > > > Patrick Henz
> > > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Henz, Patrick
> > > > Sent: Monday, August 14, 2023 10:52 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > > > Subject: [edk2-devel] [PATCH v2] 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..07e57772b6 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
> > ((UINT64)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 53421e64a8..7d4b7a769d 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> @@
> > > > -
> > 1276,15 +1277,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
> > > > @@ -1299,12 +1299,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) {
> > > > @@ -1322,34 +1324,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
> > ((UINT64)Timeout
> > > > *
> > > > + XHC_1_MILLISECOND); ElapsedTime = 0; CurrentTick =
> > > > + GetPerformanceCounter ();
> > > > +
> > > > do {
> > > > Finished = XhcCheckUrbResult (Xhc, Urb);
> > > > if (Finished) {
> > > > @@ -1357,22 +1339,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 (#108364): https://edk2.groups.io/g/devel/message/108364
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
2023-09-07 7:48 ` Wu, Hao A
2023-09-07 7:57 ` Wu, Hao A
@ 2023-09-07 18:38 ` Henz, Patrick
2023-09-08 2:20 ` Wu, Hao A
1 sibling, 1 reply; 11+ messages in thread
From: Henz, Patrick @ 2023-09-07 18:38 UTC (permalink / raw)
To: devel@edk2.groups.io, hao.a.wu@intel.com, Mike Maslenkin
I don’t have strong opinions either. If we would prefer to cache the values returned by GetPerformanceCounterProperties I would be more than happy to do that. I could also modify XhcGetElapsedTime to return the elapsed ticks instead of the elapsed time in nano seconds, the functions that call into XhcGetElapedTime/Ticks could convert the millisecond timeout value into a tick count and compare the elapsed ticks against that, this would remove the reliance on GetTimeInNanoSecond. Does that sound more appropriate?
Thanks,
Patrick Henz
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
Sent: Thursday, September 7, 2023 2:48 AM
To: Mike Maslenkin <mike.maslenkin@gmail.com>; devel@edk2.groups.io
Cc: Henz, Patrick <patrick.henz@hpe.com>
Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
I do not have strong opinion on this considering it is an IO driver.
The call of GetTimeInNanoSecond() is also likely to invoke GetPerformanceCounterProperties() call.
I will let the patch owner to decide on the open raised below.
Best Regards,
Hao Wu
> -----Original Message-----
> From: Mike Maslenkin <mike.maslenkin@gmail.com>
> Sent: Thursday, September 7, 2023 3:36 PM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
>
> Hello Hao Wu,
>
> Isn't GetPerformanceCounterProperties (&StartValue, &EndValue) call
> too "heavy" for XHCI paths?
> May be it's better to cache timer direction once?
> I'm asking in a context of PcAtChipsetPkg/Library/AcpiTimerLib
> instance used by a number of Intel's platform in edk-platforms.
> For this library GetPerformanceCounterProperties finally calls
> InternalCalculateTscFrequency, that isn't simple.
>
> Regards,
> Mike
>
> On Thu, Sep 7, 2023 at 4:27 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
> >
> > Sorry for the late response.
> > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> >
> > Best Regards,
> > Hao Wu
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Henz, Patrick
> > > Sent: Thursday, September 7, 2023 4:37 AM
> > > To: devel@edk2.groups.io
> > > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > Performance Timer for XHCI Timeouts
> > >
> > > I sent this patch out a few weeks ago now but haven't seen a
> > > reply, just checking in to make sure it didn't get missed.
> > >
> > > Thanks,
> > > Patrick Henz
> > >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Henz, Patrick
> > > Sent: Monday, August 14, 2023 10:52 AM
> > > To: devel@edk2.groups.io
> > > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > > Subject: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > Performance Timer for XHCI Timeouts
> > >
> > > REF:INVALID URI REMOVED
> > > w_bug.cgi?id=2948__;!!NpxR!mZRuMBKPdeR-UvY7sr8tWNOfIQDe0W_TW3q-K8M
> > > tNN1cynyRZ1_bls8osaEqFWupFmjR29X_zvw0Zx1Y$
> > >
> > > 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..07e57772b6 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
> ((UINT64)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 53421e64a8..7d4b7a769d 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> @@
> > > -
> 1276,15 +1277,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
> > > @@ -1299,12 +1299,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) {
> > > @@ -1322,34 +1324,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
> ((UINT64)Timeout
> > > *
> > > + XHC_1_MILLISECOND); ElapsedTime = 0; CurrentTick =
> > > + GetPerformanceCounter ();
> > > +
> > > do {
> > > Finished = XhcCheckUrbResult (Xhc, Urb);
> > > if (Finished) {
> > > @@ -1357,22 +1339,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 (#108409): https://edk2.groups.io/g/devel/message/108409
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
2023-09-07 18:38 ` Henz, Patrick
@ 2023-09-08 2:20 ` Wu, Hao A
2023-09-08 13:57 ` Henz, Patrick
0 siblings, 1 reply; 11+ messages in thread
From: Wu, Hao A @ 2023-09-08 2:20 UTC (permalink / raw)
To: Henz, Patrick, devel@edk2.groups.io, Mike Maslenkin
Thanks. I am fine with the proposal below.
Could you please help to follow the step 11 in page: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
and check the CI test results for the upcoming patch?
Best Regards,
Hao Wu
> -----Original Message-----
> From: Henz, Patrick <patrick.henz@hpe.com>
> Sent: Friday, September 8, 2023 2:39 AM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Mike
> Maslenkin <mike.maslenkin@gmail.com>
> Subject: RE: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
>
> I don’t have strong opinions either. If we would prefer to cache the values
> returned by GetPerformanceCounterProperties I would be more than happy
> to do that. I could also modify XhcGetElapsedTime to return the elapsed ticks
> instead of the elapsed time in nano seconds, the functions that call into
> XhcGetElapedTime/Ticks could convert the millisecond timeout value into a
> tick count and compare the elapsed ticks against that, this would remove the
> reliance on GetTimeInNanoSecond. Does that sound more appropriate?
>
> Thanks,
> Patrick Henz
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Thursday, September 7, 2023 2:48 AM
> To: Mike Maslenkin <mike.maslenkin@gmail.com>; devel@edk2.groups.io
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
>
> I do not have strong opinion on this considering it is an IO driver.
> The call of GetTimeInNanoSecond() is also likely to invoke
> GetPerformanceCounterProperties() call.
>
> I will let the patch owner to decide on the open raised below.
>
> Best Regards,
> Hao Wu
>
> > -----Original Message-----
> > From: Mike Maslenkin <mike.maslenkin@gmail.com>
> > Sent: Thursday, September 7, 2023 3:36 PM
> > To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>
> > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > Performance Timer for XHCI Timeouts
> >
> > Hello Hao Wu,
> >
> > Isn't GetPerformanceCounterProperties (&StartValue, &EndValue) call
> > too "heavy" for XHCI paths?
> > May be it's better to cache timer direction once?
> > I'm asking in a context of PcAtChipsetPkg/Library/AcpiTimerLib
> > instance used by a number of Intel's platform in edk-platforms.
> > For this library GetPerformanceCounterProperties finally calls
> > InternalCalculateTscFrequency, that isn't simple.
> >
> > Regards,
> > Mike
> >
> > On Thu, Sep 7, 2023 at 4:27 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
> > >
> > > Sorry for the late response.
> > > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Henz, Patrick
> > > > Sent: Thursday, September 7, 2023 4:37 AM
> > > > To: devel@edk2.groups.io
> > > > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > > Performance Timer for XHCI Timeouts
> > > >
> > > > I sent this patch out a few weeks ago now but haven't seen a
> > > > reply, just checking in to make sure it didn't get missed.
> > > >
> > > > Thanks,
> > > > Patrick Henz
> > > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Henz, Patrick
> > > > Sent: Monday, August 14, 2023 10:52 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > > > Subject: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > > Performance Timer for XHCI Timeouts
> > > >
> > > > REF:https://urldefense.com/v3/__https://bugzilla.tianocore.org/sho
> > > > w_bug.cgi?id=2948__;!!NpxR!mZRuMBKPdeR-
> UvY7sr8tWNOfIQDe0W_TW3q-K8M
> > > > tNN1cynyRZ1_bls8osaEqFWupFmjR29X_zvw0Zx1Y$
> > > >
> > > > 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..07e57772b6 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
> > ((UINT64)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 53421e64a8..7d4b7a769d 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> @@
> > > > -
> > 1276,15 +1277,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
> > > > @@ -1299,12 +1299,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) {
> > > > @@ -1322,34 +1324,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
> > ((UINT64)Timeout
> > > > *
> > > > + XHC_1_MILLISECOND); ElapsedTime = 0; CurrentTick =
> > > > + GetPerformanceCounter ();
> > > > +
> > > > do {
> > > > Finished = XhcCheckUrbResult (Xhc, Urb);
> > > > if (Finished) {
> > > > @@ -1357,22 +1339,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 (#108437): https://edk2.groups.io/g/devel/message/108437
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
2023-09-08 2:20 ` Wu, Hao A
@ 2023-09-08 13:57 ` Henz, Patrick
0 siblings, 0 replies; 11+ messages in thread
From: Henz, Patrick @ 2023-09-08 13:57 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io, Mike Maslenkin
I saw your earlier message in regard to the build failure too, I'll make sure to run the CI check.
Thanks,
Patrick Henz
-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Thursday, September 7, 2023 9:20 PM
To: Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io; Mike Maslenkin <mike.maslenkin@gmail.com>
Subject: RE: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
Thanks. I am fine with the proposal below.
Could you please help to follow the step 11 in page: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
and check the CI test results for the upcoming patch?
Best Regards,
Hao Wu
> -----Original Message-----
> From: Henz, Patrick <patrick.henz@hpe.com>
> Sent: Friday, September 8, 2023 2:39 AM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Mike
> Maslenkin <mike.maslenkin@gmail.com>
> Subject: RE: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
>
> I don’t have strong opinions either. If we would prefer to cache the
> values returned by GetPerformanceCounterProperties I would be more
> than happy to do that. I could also modify XhcGetElapsedTime to return
> the elapsed ticks instead of the elapsed time in nano seconds, the
> functions that call into XhcGetElapedTime/Ticks could convert the
> millisecond timeout value into a tick count and compare the elapsed
> ticks against that, this would remove the reliance on GetTimeInNanoSecond. Does that sound more appropriate?
>
> Thanks,
> Patrick Henz
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Thursday, September 7, 2023 2:48 AM
> To: Mike Maslenkin <mike.maslenkin@gmail.com>; devel@edk2.groups.io
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
>
> I do not have strong opinion on this considering it is an IO driver.
> The call of GetTimeInNanoSecond() is also likely to invoke
> GetPerformanceCounterProperties() call.
>
> I will let the patch owner to decide on the open raised below.
>
> Best Regards,
> Hao Wu
>
> > -----Original Message-----
> > From: Mike Maslenkin <mike.maslenkin@gmail.com>
> > Sent: Thursday, September 7, 2023 3:36 PM
> > To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>
> > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > Performance Timer for XHCI Timeouts
> >
> > Hello Hao Wu,
> >
> > Isn't GetPerformanceCounterProperties (&StartValue, &EndValue) call
> > too "heavy" for XHCI paths?
> > May be it's better to cache timer direction once?
> > I'm asking in a context of PcAtChipsetPkg/Library/AcpiTimerLib
> > instance used by a number of Intel's platform in edk-platforms.
> > For this library GetPerformanceCounterProperties finally calls
> > InternalCalculateTscFrequency, that isn't simple.
> >
> > Regards,
> > Mike
> >
> > On Thu, Sep 7, 2023 at 4:27 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
> > >
> > > Sorry for the late response.
> > > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Henz, Patrick
> > > > Sent: Thursday, September 7, 2023 4:37 AM
> > > > To: devel@edk2.groups.io
> > > > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > > Performance Timer for XHCI Timeouts
> > > >
> > > > I sent this patch out a few weeks ago now but haven't seen a
> > > > reply, just checking in to make sure it didn't get missed.
> > > >
> > > > Thanks,
> > > > Patrick Henz
> > > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Henz, Patrick
> > > > Sent: Monday, August 14, 2023 10:52 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > > > Subject: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > > Performance Timer for XHCI Timeouts
> > > >
> > > > REF:INVALID URI REMOVED
> > > > ho
> > > > w_bug.cgi?id=2948__;!!NpxR!mZRuMBKPdeR-
> UvY7sr8tWNOfIQDe0W_TW3q-K8M
> > > > tNN1cynyRZ1_bls8osaEqFWupFmjR29X_zvw0Zx1Y$
> > > >
> > > > 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..07e57772b6 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
> > ((UINT64)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 53421e64a8..7d4b7a769d 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>
> > > > @@
> > > > -
> > 1276,15 +1277,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
> > > > @@ -1299,12 +1299,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) {
> > > > @@ -1322,34 +1324,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
> > ((UINT64)Timeout
> > > > *
> > > > + XHC_1_MILLISECOND); ElapsedTime = 0; CurrentTick =
> > > > + GetPerformanceCounter ();
> > > > +
> > > > do {
> > > > Finished = XhcCheckUrbResult (Xhc, Urb);
> > > > if (Finished) {
> > > > @@ -1357,22 +1339,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 (#108447): https://edk2.groups.io/g/devel/message/108447
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-09-08 13:57 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <177B4AD1F8C3A6A2.8497@groups.io>
2023-09-06 20:36 ` [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts Henz, Patrick
2023-09-06 20:49 ` Pedro Falcato
2023-09-07 1:27 ` Wu, Hao A
2023-09-07 2:59 ` Wu, Hao A
2023-09-07 7:36 ` Mike Maslenkin
2023-09-07 7:48 ` Wu, Hao A
2023-09-07 7:57 ` Wu, Hao A
2023-09-07 18:38 ` Henz, Patrick
2023-09-08 2:20 ` Wu, Hao A
2023-09-08 13:57 ` Henz, Patrick
2023-08-14 15:51 Henz, Patrick
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox