[edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
Wu, Hao A
hao.a.wu at intel.com
Thu Sep 24 01:22:00 UTC 2020
Reviewed-by: Hao A Wu <hao.a.wu at intel.com>
Best Regards,
Hao Wu
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Henz,
> Patrick
> Sent: Thursday, September 24, 2020 3:36 AM
> To: devel at edk2.groups.io
> Cc: Patrick Henz <patrick.henz at hpe.com>; Wang, Jian J
> <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray
> <ray.ni at intel.com>
> Subject: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken
> Timeouts
>
> From: Patrick Henz <patrick.henz at 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.wang at intel.com>
> Cc: Hao A Wu <hao.a.wu at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Signed-off-by: Patrick Henz <patrick.henz at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list