Re: [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts


Wu, Hao A
 

Merged via
Pull request: https://github.com/tianocore/edk2/pull/968
Commit: 71dd80f14f2724ccc99dd7d349b7392adf114019

Best Regards,
Hao Wu

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
A
Sent: Thursday, September 24, 2020 9:22 AM
To: devel@edk2.groups.io; patrick.henz@...
Cc: Wang, Jian J <jian.j.wang@...>; Ni, Ray <ray.ni@...>
Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix
Broken Timeouts

Reviewed-by: Hao A Wu <hao.a.wu@...>

Best Regards,
Hao Wu

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
Patrick
Sent: Thursday, September 24, 2020 3:36 AM
To: devel@edk2.groups.io
Cc: Patrick Henz <patrick.henz@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Ni, Ray
<ray.ni@...>
Subject: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken
Timeouts

From: Patrick Henz <patrick.henz@...>

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

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

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

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
index 42b773ab31be..2bab09415b28 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
@@ -423,14 +423,15 @@ 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_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.

**/
EFI_STATUS
@@ -442,20 +443,52 @@ XhcWaitOpRegBit (
IN UINT32 Timeout
)
{
- UINT32 Index;
- UINT64 Loop;
+ EFI_STATUS Status;
+ EFI_EVENT TimeoutEvent;

- Loop = Timeout * XHC_1_MILLISECOND;
+ TimeoutEvent = NULL;

- for (Index = 0; Index < Loop; Index++) {
+ 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;
+ }
+
+ do {
if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
- return EFI_SUCCESS;
+ Status = EFI_SUCCESS;
+ goto DONE;
}

gBS->Stall (XHC_1_MICROSECOND);
+ } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
+
+ Status = EFI_TIMEOUT;
+
+DONE:
+ if (TimeoutEvent != NULL) {
+ gBS->CloseEvent (TimeoutEvent);
}

- return EFI_TIMEOUT;
+ return Status;
}

/**
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index ab8957c546ee..9cb115363c8b 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -1254,14 +1254,15 @@ XhcCheckUrbResult (
/**
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.
+ @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.

**/
EFI_STATUS
@@ -1273,11 +1274,16 @@ XhcExecTransfer (
)
{
EFI_STATUS Status;
- UINTN Index;
- UINT64 Loop;
UINT8 SlotId;
UINT8 Dci;
BOOLEAN Finished;
+ EFI_EVENT TimeoutEvent;
+ BOOLEAN IndefiniteTimeout;
+
+ Status = EFI_SUCCESS;
+ Finished = FALSE;
+ TimeoutEvent = NULL;
+ IndefiniteTimeout = FALSE;

if (CmdTransfer) {
SlotId = 0;
@@ -1291,29 +1297,56 @@ XhcExecTransfer (
ASSERT (Dci < 32);
}

- Status = EFI_SUCCESS;
- Loop = Timeout * XHC_1_MILLISECOND;
if (Timeout == 0) {
- Loop = 0xFFFFFFFF;
+ 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);

- for (Index = 0; Index < Loop; Index++) {
+ do {
Finished = XhcCheckUrbResult (Xhc, Urb);
if (Finished) {
break;
}
gBS->Stall (XHC_1_MICROSECOND);
- }
+ } while (IndefiniteTimeout || EFI_ERROR(gBS->CheckEvent
+ (TimeoutEvent)));

- if (Index == Loop) {
+DONE:
+ if (EFI_ERROR(Status)) {
+ Urb->Result = EFI_USB_ERR_NOTEXECUTE;
+ } else 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.28.0







Join devel@edk2.groups.io to automatically receive all group messages.