Reviewed-by: Hao A Wu <hao.a...@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 24, 2020 3:36 AM > To: devel@edk2.groups.io > Cc: Patrick Henz <patrick.h...@hpe.com>; Wang, Jian J > <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; Ni, Ray > <ray...@intel.com> > Subject: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken > Timeouts > > From: Patrick Henz <patrick.h...@hpe.com> > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2948 > > Timeouts in the XhciDxe driver are taking longer than expected due to the > timeout loops not accounting for code execution time. As en example, 5 > second timeouts have been observed to take around 36 seconds to > complete. > Use SetTimer and Create/CheckEvent from Boot Services to determine when > timeout occurred. > > Cc: Jian J Wang <jian.j.w...@intel.com> > Cc: Hao A Wu <hao.a...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Signed-off-by: Patrick Henz <patrick.h...@hpe.com> > --- > 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 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#65550): https://edk2.groups.io/g/devel/message/65550 Mute This Topic: https://groups.io/mt/77042848/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
