[edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts
Wu, Hao A
hao.a.wu at intel.com
Thu Sep 7 01:27:44 UTC 2023
Sorry for the late response.
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 7, 2023 4:37 AM
> To: devel at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Henz,
> Patrick
> Sent: Monday, August 14, 2023 10:52 AM
> To: devel at edk2.groups.io
> Cc: Henz, Patrick <patrick.henz at 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 at 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/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