* [PATCH v3 0/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
@ 2020-09-23 19:36 Henz, Patrick
2020-09-23 19:36 ` [PATCH v3 1/1] " Henz, Patrick
0 siblings, 1 reply; 7+ messages in thread
From: Henz, Patrick @ 2020-09-23 19:36 UTC (permalink / raw)
To: devel; +Cc: Patrick Henz, Jian J Wang, Hao A Wu, Ray Ni
From: Patrick Henz <patrick.henz@hpe.com>
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. This patch was tested
using forced timeouts and print statements with QEmu as
well as phycial hardware. The forced timeouts were
implemented in code via static variables that guaranteed
a timeout the first time the function with the broken
timeout was called.
Example:
XhcExecTransfer (
.
.
)
{
.
.
static int do_once = 1; // test line
.
.
do {
Finished = XhcCheckUrbResult (Xhc, Urb);
if (do_once) Finished = 0; // test line
if (Finished) {
break;
}
gBS->Stall (XHC_1_MICROSECOND);
} while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
do_once = 0; // test line
Using this forced timeout approach the correct timeouts
were observed on both hardware and in QEmu.
Similar broken timeout loops have been found in the Uhci
and Ehci drivers. This patch does not fix those issues.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
Patrick Henz (1):
MdeModulePkg/XhciDxe: Fix Broken Timeouts
MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 59 +++++++++++++++++-----
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 63 ++++++++++++++++++------
2 files changed, 94 insertions(+), 28 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-23 19:36 [PATCH v3 0/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts Henz, Patrick
@ 2020-09-23 19:36 ` Henz, Patrick
2020-09-24 1:22 ` [edk2-devel] " Wu, Hao A
2023-06-07 21:37 ` Jessica Clarke
0 siblings, 2 replies; 7+ messages in thread
From: Henz, Patrick @ 2020-09-23 19:36 UTC (permalink / raw)
To: devel; +Cc: Patrick Henz, Jian J Wang, Hao A Wu, Ray Ni
From: Patrick Henz <patrick.henz@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@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Patrick Henz <patrick.henz@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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-23 19:36 ` [PATCH v3 1/1] " Henz, Patrick
@ 2020-09-24 1:22 ` Wu, Hao A
2020-09-29 1:31 ` Wu, Hao A
2023-06-07 21:37 ` Jessica Clarke
1 sibling, 1 reply; 7+ messages in thread
From: Wu, Hao A @ 2020-09-24 1:22 UTC (permalink / raw)
To: devel@edk2.groups.io, patrick.henz@hpe.com; +Cc: Wang, Jian J, Ni, Ray
Reviewed-by: Hao A Wu <hao.a.wu@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.henz@hpe.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken
> Timeouts
>
> From: Patrick Henz <patrick.henz@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@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@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
>
>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-24 1:22 ` [edk2-devel] " Wu, Hao A
@ 2020-09-29 1:31 ` Wu, Hao A
0 siblings, 0 replies; 7+ messages in thread
From: Wu, Hao A @ 2020-09-29 1:31 UTC (permalink / raw)
To: devel@edk2.groups.io, Wu, Hao A, patrick.henz@hpe.com
Cc: Wang, Jian J, Ni, Ray
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@hpe.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix
> Broken Timeouts
>
> Reviewed-by: Hao A Wu <hao.a.wu@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.henz@hpe.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> > <ray.ni@intel.com>
> > Subject: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken
> > Timeouts
> >
> > From: Patrick Henz <patrick.henz@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@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Patrick Henz <patrick.henz@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
> >
> >
> >
> >
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-23 19:36 ` [PATCH v3 1/1] " Henz, Patrick
2020-09-24 1:22 ` [edk2-devel] " Wu, Hao A
@ 2023-06-07 21:37 ` Jessica Clarke
2023-06-08 10:32 ` Leif Lindholm
2023-06-08 16:51 ` Henz, Patrick
1 sibling, 2 replies; 7+ messages in thread
From: Jessica Clarke @ 2023-06-07 21:37 UTC (permalink / raw)
To: Henz, Patrick, devel
On Wed, Sep 23, 2020 at 08:36 PM, Henz, Patrick wrote:
>
> From: Patrick Henz <patrick.henz@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.
This strategy doesn't work during ExitBootServices, as called from
XhcExitBootService via XhcHaltHC. The net result is that, if the XHCI
controller fails to halt upon clearing R/S, EDK2 hangs in an infinite
loop (something I just had the joy of debugging).
Jess
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2023-06-07 21:37 ` Jessica Clarke
@ 2023-06-08 10:32 ` Leif Lindholm
2023-06-08 16:51 ` Henz, Patrick
1 sibling, 0 replies; 7+ messages in thread
From: Leif Lindholm @ 2023-06-08 10:32 UTC (permalink / raw)
To: devel, jrtc27, Henz, Patrick
Cc: Wang, Jian J, Gao, Liming, Wu, Hao A, Ni, Ray
+maintainers
On 2023-06-07 22:37, Jessica Clarke wrote:
> On Wed, Sep 23, 2020 at 08:36 PM, Henz, Patrick wrote:
>
>>
>> From: Patrick Henz <patrick.henz@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.
>
> This strategy doesn't work during ExitBootServices, as called from
> XhcExitBootService via XhcHaltHC. The net result is that, if the XHCI
> controller fails to halt upon clearing R/S, EDK2 hangs in an infinite
> loop (something I just had the joy of debugging).
>
> Jess
>
>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2023-06-07 21:37 ` Jessica Clarke
2023-06-08 10:32 ` Leif Lindholm
@ 2023-06-08 16:51 ` Henz, Patrick
1 sibling, 0 replies; 7+ messages in thread
From: Henz, Patrick @ 2023-06-08 16:51 UTC (permalink / raw)
To: Jessica Clarke, Henz@mx0a-002e3701.pphosted.com,
devel@edk2.groups.io
Hi Jess,
Thank you for reporting this, it's certainly a bug. I'm planning to implement a fix.
Thanks,
Patrick Henz
-----Original Message-----
From: Jessica Clarke <jrtc27@jrtc27.com>
Sent: Wednesday, June 7, 2023 4:38 PM
To: Henz@mx0a-002e3701.pphosted.com; Henz, Patrick <patrick.henz@hpe.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
On Wed, Sep 23, 2020 at 08:36 PM, Henz, Patrick wrote:
>
> From: Patrick Henz <patrick.henz@hpe.com>
>
> REF:INVALID URI REMOVED
> g.cgi?id=2948__;!!NpxR!iZG1WFNYYsLpsDRVpII4yD3VcV3qTNWYYBKIkjOpmdsJq4e
> cXWzlnbRiJf9vTEZUUu73BuQmhJJwUHocWQ$
>
> 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.
This strategy doesn't work during ExitBootServices, as called from XhcExitBootService via XhcHaltHC. The net result is that, if the XHCI controller fails to halt upon clearing R/S, EDK2 hangs in an infinite loop (something I just had the joy of debugging).
Jess
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-08 16:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-23 19:36 [PATCH v3 0/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts Henz, Patrick
2020-09-23 19:36 ` [PATCH v3 1/1] " Henz, Patrick
2020-09-24 1:22 ` [edk2-devel] " Wu, Hao A
2020-09-29 1:31 ` Wu, Hao A
2023-06-07 21:37 ` Jessica Clarke
2023-06-08 10:32 ` Leif Lindholm
2023-06-08 16:51 ` Henz, Patrick
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox