From 71dd80f14f2724ccc99dd7d349b7392adf114019 Mon Sep 17 00:00:00 2001 From: Patrick Henz Date: Thu, 24 Sep 2020 03:36:03 +0800 Subject: [PATCH] MdeModulePkg/XhciDxe: Fix Broken Timeouts 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 Cc: Hao A Wu Cc: Ray Ni Signed-off-by: Patrick Henz Reviewed-by: Hao A Wu --- 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 42b773ab31..2bab09415b 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; + + TimeoutEvent = NULL; + + if (Timeout == 0) { + return EFI_TIMEOUT; + } + + Status = gBS->CreateEvent ( + EVT_TIMER, + TPL_CALLBACK, + NULL, + NULL, + &TimeoutEvent + ); + + if (EFI_ERROR(Status)) { + goto DONE; + } - Loop = Timeout * XHC_1_MILLISECOND; + Status = gBS->SetTimer (TimeoutEvent, + TimerRelative, + EFI_TIMER_PERIOD_MILLISECONDS(Timeout)); - for (Index = 0; Index < Loop; Index++) { + 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 ab8957c546..9cb115363c 100644 --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c @@ -1254,14 +1254,15 @@ 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. + @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.39.2