* [PATCH v2 0/1] Fix XhciDxe Timeouts
@ 2020-09-02 17:04 patrick.henz
2020-09-02 17:04 ` [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts patrick.henz
0 siblings, 1 reply; 7+ messages in thread
From: patrick.henz @ 2020-09-02 17:04 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 | 35 ++++++++++++++++---
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 43 +++++++++++++++++++-----
2 files changed, 65 insertions(+), 13 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-02 17:04 [PATCH v2 0/1] Fix XhciDxe Timeouts patrick.henz
@ 2020-09-02 17:04 ` patrick.henz
2020-09-03 2:24 ` [edk2-devel] " Wu, Hao A
0 siblings, 1 reply; 7+ messages in thread
From: patrick.henz @ 2020-09-02 17:04 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 | 35 ++++++++++++++++---
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 43 +++++++++++++++++++-----
2 files changed, 65 insertions(+), 13 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
index 42b773ab31be..33ac13504669 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
@@ -442,17 +442,44 @@ 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) {
+ goto TIMEOUT;
+ }
+
+ Status = gBS->CreateEvent (
+ EVT_TIMER,
+ TPL_CALLBACK,
+ NULL,
+ NULL,
+ &TimeoutEvent
+ );
+
+ if (!EFI_ERROR (Status)) {
+ Status = gBS->SetTimer (TimeoutEvent,
+ TimerRelative,
+ EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
+ }
+
+ if (EFI_ERROR(Status)) {
+ goto TIMEOUT;
+ }
+
+ do {
if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
return EFI_SUCCESS;
}
gBS->Stall (XHC_1_MICROSECOND);
+ } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
+
+TIMEOUT:
+ if (TimeoutEvent != NULL) {
+ gBS->CloseEvent (TimeoutEvent);
}
return EFI_TIMEOUT;
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index ab8957c546ee..d6290b5fe33b 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -1273,11 +1273,19 @@ XhcExecTransfer (
)
{
EFI_STATUS Status;
- UINTN Index;
- UINT64 Loop;
UINT8 SlotId;
UINT8 Dci;
BOOLEAN Finished;
+ EFI_EVENT TimeoutEvent;
+ EFI_STATUS TimerStatus;
+
+ Status = EFI_SUCCESS;
+ Finished = FALSE;
+ TimeoutEvent = NULL;
+
+ if (Timeout == 0) {
+ goto DONE;
+ }
if (CmdTransfer) {
SlotId = 0;
@@ -1291,29 +1299,46 @@ XhcExecTransfer (
ASSERT (Dci < 32);
}
- Status = EFI_SUCCESS;
- Loop = Timeout * XHC_1_MILLISECOND;
- if (Timeout == 0) {
- Loop = 0xFFFFFFFF;
+ TimerStatus = gBS->CreateEvent (
+ EVT_TIMER,
+ TPL_CALLBACK,
+ NULL,
+ NULL,
+ &TimeoutEvent
+ );
+
+ if (!EFI_ERROR (TimerStatus)) {
+ TimerStatus = gBS->SetTimer (TimeoutEvent,
+ TimerRelative,
+ EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
+ }
+
+ if (EFI_ERROR (TimerStatus)) {
+ goto DONE;
}
XhcRingDoorBell (Xhc, SlotId, Dci);
- for (Index = 0; Index < Loop; Index++) {
+ do {
Finished = XhcCheckUrbResult (Xhc, Urb);
if (Finished) {
break;
}
gBS->Stall (XHC_1_MICROSECOND);
- }
+ } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
- if (Index == Loop) {
+DONE:
+ 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 v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-02 17:04 ` [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts patrick.henz
@ 2020-09-03 2:24 ` Wu, Hao A
2020-09-10 18:43 ` Henz, Patrick
0 siblings, 1 reply; 7+ messages in thread
From: Wu, Hao A @ 2020-09-03 2:24 UTC (permalink / raw)
To: devel@edk2.groups.io, patrick.henz@hpe.com, Ni, Ray; +Cc: Wang, Jian J
Hello Patrick, a couple of inline comments below.
Hello Ray, need your input on one thing below as well.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> patrick.henz@hpe.com
> Sent: Thursday, September 3, 2020 1:05 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 v2 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 | 35 ++++++++++++++++---
> MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 43
> +++++++++++++++++++-----
> 2 files changed, 65 insertions(+), 13 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 42b773ab31be..33ac13504669 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -442,17 +442,44 @@ 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) {
> + goto TIMEOUT;
> + }
> +
> + Status = gBS->CreateEvent (
> + EVT_TIMER,
> + TPL_CALLBACK,
> + NULL,
> + NULL,
> + &TimeoutEvent
> + );
> +
> + if (!EFI_ERROR (Status)) {
> + Status = gBS->SetTimer (TimeoutEvent,
> + TimerRelative,
> + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> + }
> +
> + if (EFI_ERROR(Status)) {
> + goto TIMEOUT;
> + }
Could you help to refine the return status for the case when CreateEvent or SetTimer calls fail?
I think it will return EFI_TIMEOUT at this moment, which might confuse the caller.
You may need to modify the function description comment section for the new return value also.
A similar case applies to XhcExecTransfer() as well.
> +
> + do {
> if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> return EFI_SUCCESS;
> }
>
> gBS->Stall (XHC_1_MICROSECOND);
> + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> +
> +TIMEOUT:
> + if (TimeoutEvent != NULL) {
> + gBS->CloseEvent (TimeoutEvent);
> }
>
> return EFI_TIMEOUT;
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index ab8957c546ee..d6290b5fe33b 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -1273,11 +1273,19 @@ XhcExecTransfer (
> )
> {
> EFI_STATUS Status;
> - UINTN Index;
> - UINT64 Loop;
> UINT8 SlotId;
> UINT8 Dci;
> BOOLEAN Finished;
> + EFI_EVENT TimeoutEvent;
> + EFI_STATUS TimerStatus;
> +
> + Status = EFI_SUCCESS;
> + Finished = FALSE;
> + TimeoutEvent = NULL;
> +
> + if (Timeout == 0) {
> + goto DONE;
> + }
>
> if (CmdTransfer) {
> SlotId = 0;
> @@ -1291,29 +1299,46 @@ XhcExecTransfer (
> ASSERT (Dci < 32);
> }
>
> - Status = EFI_SUCCESS;
> - Loop = Timeout * XHC_1_MILLISECOND;
> - if (Timeout == 0) {
> - Loop = 0xFFFFFFFF;
Ray and Patrick, the previous behavior when 'Timeout' is 0 for this function is that it will do a 'psudo-indefinite' loop
by setting the 'Loop' variable to the value of MAX_UINT32.
But after the patch, the behavior got changed and the function will directly return EFI_TIMEOUT when 'Timeout' is 0.
This behavior change might impact the callers when they expecting a 'psudo-indefinite' timeout.
I think it would be better to keep the origin behavior, what is your thought?
Best Regards,
Hao Wu
> + TimerStatus = gBS->CreateEvent (
> + EVT_TIMER,
> + TPL_CALLBACK,
> + NULL,
> + NULL,
> + &TimeoutEvent
> + );
> +
> + if (!EFI_ERROR (TimerStatus)) {
> + TimerStatus = gBS->SetTimer (TimeoutEvent,
> + TimerRelative,
> +
> + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> + }
> +
> + if (EFI_ERROR (TimerStatus)) {
> + goto DONE;
> }
>
> XhcRingDoorBell (Xhc, SlotId, Dci);
>
> - for (Index = 0; Index < Loop; Index++) {
> + do {
> Finished = XhcCheckUrbResult (Xhc, Urb);
> if (Finished) {
> break;
> }
> gBS->Stall (XHC_1_MICROSECOND);
> - }
> + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
>
> - if (Index == Loop) {
> +DONE:
> + 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 v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-03 2:24 ` [edk2-devel] " Wu, Hao A
@ 2020-09-10 18:43 ` Henz, Patrick
2020-09-15 1:27 ` Wu, Hao A
0 siblings, 1 reply; 7+ messages in thread
From: Henz, Patrick @ 2020-09-10 18:43 UTC (permalink / raw)
To: devel@edk2.groups.io, hao.a.wu@intel.com, Ni, Ray; +Cc: Wang, Jian J
Hi Hao and Ray,
In regards to the behavior when Timeout == 0, I did try to account for that in my initial patch with the following logic:
(0 == Timeout)?(EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):(EFI_TIMER_PERIOD_MILLISECONDS(Timeout))
This results in a timeout that happens sooner than what the current code would produce, but it falls in line with what the original code seemed to intend to do. Ray suggested that I enhance the code by not creating the timer event when Timeout is 0, which I assumed meant that XhcExecTransfer () would just return. I personally think it would be a good idea to keep this behavior in the code, but would like Ray's input on the matter. Appreciate the help!
Thanks,
Patrick Henz
-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Wu, Hao A
Sent: Wednesday, September 2, 2020 9:25 PM
To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>; Ni, Ray <ray.ni@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
Hello Patrick, a couple of inline comments below.
Hello Ray, need your input on one thing below as well.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> patrick.henz@hpe.com
> Sent: Thursday, September 3, 2020 1:05 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 v2 1/1] MdeModulePkg/XhciDxe: Fix Broken
> Timeouts
>
> From: Patrick Henz <patrick.henz@hpe.com>
>
> REF:INVALID URI REMOVED
> ocore.org_show-5Fbug.cgi-3Fid-3D2948&d=DwIFAg&c=C5b8zRQO1miGmBeVZ2LFWg
> &r=wx4n0HbqxSAP19Eklmv6gq7ivDQlqQ_ITOkZIBUNNKg&m=OKlWpRL8ZyDfhUEh6S4OU
> aMasig0MPoajX7Vz2sDSvY&s=LTDbPsspkRCcWFBfThqhR_FaljF2kQLagB_t-kbAm80&e
> =
>
> 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 | 35 ++++++++++++++++---
> MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 43
> +++++++++++++++++++-----
> 2 files changed, 65 insertions(+), 13 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 42b773ab31be..33ac13504669 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -442,17 +442,44 @@ 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) {
> + goto TIMEOUT;
> + }
> +
> + Status = gBS->CreateEvent (
> + EVT_TIMER,
> + TPL_CALLBACK,
> + NULL,
> + NULL,
> + &TimeoutEvent
> + );
> +
> + if (!EFI_ERROR (Status)) {
> + Status = gBS->SetTimer (TimeoutEvent,
> + TimerRelative,
> + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> + }
> +
> + if (EFI_ERROR(Status)) {
> + goto TIMEOUT;
> + }
Could you help to refine the return status for the case when CreateEvent or SetTimer calls fail?
I think it will return EFI_TIMEOUT at this moment, which might confuse the caller.
You may need to modify the function description comment section for the new return value also.
A similar case applies to XhcExecTransfer() as well.
> +
> + do {
> if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> return EFI_SUCCESS;
> }
>
> gBS->Stall (XHC_1_MICROSECOND);
> + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> +
> +TIMEOUT:
> + if (TimeoutEvent != NULL) {
> + gBS->CloseEvent (TimeoutEvent);
> }
>
> return EFI_TIMEOUT;
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index ab8957c546ee..d6290b5fe33b 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -1273,11 +1273,19 @@ XhcExecTransfer (
> )
> {
> EFI_STATUS Status;
> - UINTN Index;
> - UINT64 Loop;
> UINT8 SlotId;
> UINT8 Dci;
> BOOLEAN Finished;
> + EFI_EVENT TimeoutEvent;
> + EFI_STATUS TimerStatus;
> +
> + Status = EFI_SUCCESS;
> + Finished = FALSE;
> + TimeoutEvent = NULL;
> +
> + if (Timeout == 0) {
> + goto DONE;
> + }
>
> if (CmdTransfer) {
> SlotId = 0;
> @@ -1291,29 +1299,46 @@ XhcExecTransfer (
> ASSERT (Dci < 32);
> }
>
> - Status = EFI_SUCCESS;
> - Loop = Timeout * XHC_1_MILLISECOND;
> - if (Timeout == 0) {
> - Loop = 0xFFFFFFFF;
Ray and Patrick, the previous behavior when 'Timeout' is 0 for this function is that it will do a 'psudo-indefinite' loop by setting the 'Loop' variable to the value of MAX_UINT32.
But after the patch, the behavior got changed and the function will directly return EFI_TIMEOUT when 'Timeout' is 0.
This behavior change might impact the callers when they expecting a 'psudo-indefinite' timeout.
I think it would be better to keep the origin behavior, what is your thought?
Best Regards,
Hao Wu
> + TimerStatus = gBS->CreateEvent (
> + EVT_TIMER,
> + TPL_CALLBACK,
> + NULL,
> + NULL,
> + &TimeoutEvent
> + );
> +
> + if (!EFI_ERROR (TimerStatus)) {
> + TimerStatus = gBS->SetTimer (TimeoutEvent,
> + TimerRelative,
> +
> + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> + }
> +
> + if (EFI_ERROR (TimerStatus)) {
> + goto DONE;
> }
>
> XhcRingDoorBell (Xhc, SlotId, Dci);
>
> - for (Index = 0; Index < Loop; Index++) {
> + do {
> Finished = XhcCheckUrbResult (Xhc, Urb);
> if (Finished) {
> break;
> }
> gBS->Stall (XHC_1_MICROSECOND);
> - }
> + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
>
> - if (Index == Loop) {
> +DONE:
> + 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 v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-10 18:43 ` Henz, Patrick
@ 2020-09-15 1:27 ` Wu, Hao A
2020-09-23 3:40 ` Ni, Ray
0 siblings, 1 reply; 7+ messages in thread
From: Wu, Hao A @ 2020-09-15 1:27 UTC (permalink / raw)
To: devel@edk2.groups.io, patrick.henz@hpe.com, Ni, Ray; +Cc: Wang, Jian J
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> Patrick
> Sent: Friday, September 11, 2020 2:43 AM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix
> Broken Timeouts
>
> Hi Hao and Ray,
>
> In regards to the behavior when Timeout == 0, I did try to account for that in
> my initial patch with the following logic:
>
> (0 ==
> Timeout)?(EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):(EFI_TIMER_
> PERIOD_MILLISECONDS(Timeout))
>
> This results in a timeout that happens sooner than what the current code
> would produce, but it falls in line with what the original code seemed to
> intend to do. Ray suggested that I enhance the code by not creating the
> timer event when Timeout is 0, which I assumed meant that XhcExecTransfer
> () would just return. I personally think it would be a good idea to keep this
> behavior in the code, but would like Ray's input on the matter. Appreciate
I prefer to keep the origin behavior.
Ray, what is your comment on this?
Best Regards,
Hao Wu
> the help!
>
> Thanks,
> Patrick Henz
>
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Wu, Hao A
> Sent: Wednesday, September 2, 2020 9:25 PM
> To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>; Ni, Ray
> <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix
> Broken Timeouts
>
> Hello Patrick, a couple of inline comments below.
> Hello Ray, need your input on one thing below as well.
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > patrick.henz@hpe.com
> > Sent: Thursday, September 3, 2020 1:05 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 v2 1/1] MdeModulePkg/XhciDxe: Fix Broken
> > Timeouts
> >
> > From: Patrick Henz <patrick.henz@hpe.com>
> >
> > REF:INVALID URI REMOVED
> > ocore.org_show-5Fbug.cgi-3Fid-
> 3D2948&d=DwIFAg&c=C5b8zRQO1miGmBeVZ2LFWg
> >
> &r=wx4n0HbqxSAP19Eklmv6gq7ivDQlqQ_ITOkZIBUNNKg&m=OKlWpRL8ZyDf
> hUEh6S4OU
> > aMasig0MPoajX7Vz2sDSvY&s=LTDbPsspkRCcWFBfThqhR_FaljF2kQLagB_t-
> kbAm80&e
> > =
> >
> > 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 | 35 ++++++++++++++++---
> > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 43
> > +++++++++++++++++++-----
> > 2 files changed, 65 insertions(+), 13 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > index 42b773ab31be..33ac13504669 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > @@ -442,17 +442,44 @@ 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) {
> > + goto TIMEOUT;
> > + }
> > +
> > + Status = gBS->CreateEvent (
> > + EVT_TIMER,
> > + TPL_CALLBACK,
> > + NULL,
> > + NULL,
> > + &TimeoutEvent
> > + );
> > +
> > + if (!EFI_ERROR (Status)) {
> > + Status = gBS->SetTimer (TimeoutEvent,
> > + TimerRelative,
> > + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> > + }
> > +
> > + if (EFI_ERROR(Status)) {
> > + goto TIMEOUT;
> > + }
>
>
> Could you help to refine the return status for the case when CreateEvent or
> SetTimer calls fail?
> I think it will return EFI_TIMEOUT at this moment, which might confuse the
> caller.
> You may need to modify the function description comment section for the
> new return value also.
>
> A similar case applies to XhcExecTransfer() as well.
>
>
> > +
> > + do {
> > if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> > return EFI_SUCCESS;
> > }
> >
> > gBS->Stall (XHC_1_MICROSECOND);
> > + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> > +
> > +TIMEOUT:
> > + if (TimeoutEvent != NULL) {
> > + gBS->CloseEvent (TimeoutEvent);
> > }
> >
> > return EFI_TIMEOUT;
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > index ab8957c546ee..d6290b5fe33b 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > @@ -1273,11 +1273,19 @@ XhcExecTransfer (
> > )
> > {
> > EFI_STATUS Status;
> > - UINTN Index;
> > - UINT64 Loop;
> > UINT8 SlotId;
> > UINT8 Dci;
> > BOOLEAN Finished;
> > + EFI_EVENT TimeoutEvent;
> > + EFI_STATUS TimerStatus;
> > +
> > + Status = EFI_SUCCESS;
> > + Finished = FALSE;
> > + TimeoutEvent = NULL;
> > +
> > + if (Timeout == 0) {
> > + goto DONE;
> > + }
> >
> > if (CmdTransfer) {
> > SlotId = 0;
> > @@ -1291,29 +1299,46 @@ XhcExecTransfer (
> > ASSERT (Dci < 32);
> > }
> >
> > - Status = EFI_SUCCESS;
> > - Loop = Timeout * XHC_1_MILLISECOND;
> > - if (Timeout == 0) {
> > - Loop = 0xFFFFFFFF;
>
>
> Ray and Patrick, the previous behavior when 'Timeout' is 0 for this function is
> that it will do a 'psudo-indefinite' loop by setting the 'Loop' variable to the
> value of MAX_UINT32.
>
> But after the patch, the behavior got changed and the function will directly
> return EFI_TIMEOUT when 'Timeout' is 0.
> This behavior change might impact the callers when they expecting a 'psudo-
> indefinite' timeout.
> I think it would be better to keep the origin behavior, what is your thought?
>
> Best Regards,
> Hao Wu
>
>
> > + TimerStatus = gBS->CreateEvent (
> > + EVT_TIMER,
> > + TPL_CALLBACK,
> > + NULL,
> > + NULL,
> > + &TimeoutEvent
> > + );
> > +
> > + if (!EFI_ERROR (TimerStatus)) {
> > + TimerStatus = gBS->SetTimer (TimeoutEvent,
> > + TimerRelative,
> > +
> > + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> > + }
> > +
> > + if (EFI_ERROR (TimerStatus)) {
> > + goto DONE;
> > }
> >
> > XhcRingDoorBell (Xhc, SlotId, Dci);
> >
> > - for (Index = 0; Index < Loop; Index++) {
> > + do {
> > Finished = XhcCheckUrbResult (Xhc, Urb);
> > if (Finished) {
> > break;
> > }
> > gBS->Stall (XHC_1_MICROSECOND);
> > - }
> > + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> >
> > - if (Index == Loop) {
> > +DONE:
> > + 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 v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-15 1:27 ` Wu, Hao A
@ 2020-09-23 3:40 ` Ni, Ray
2020-09-23 5:22 ` Wu, Hao A
0 siblings, 1 reply; 7+ messages in thread
From: Ni, Ray @ 2020-09-23 3:40 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io, patrick.henz@hpe.com; +Cc: Wang, Jian J
Hao,
Yes the behavior should not be changed.
Can we keep the original behavior while still not creating the timer when infinite-loop is needed?
Thanks,
Ray
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Tuesday, September 15, 2020 9:28 AM
> To: devel@edk2.groups.io; patrick.henz@hpe.com; Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken
> Timeouts
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> > Patrick
> > Sent: Friday, September 11, 2020 2:43 AM
> > To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> > <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix
> > Broken Timeouts
> >
> > Hi Hao and Ray,
> >
> > In regards to the behavior when Timeout == 0, I did try to account for that in
> > my initial patch with the following logic:
> >
> > (0 ==
> > Timeout)?(EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):(EFI_TIMER_
> > PERIOD_MILLISECONDS(Timeout))
> >
> > This results in a timeout that happens sooner than what the current code
> > would produce, but it falls in line with what the original code seemed to
> > intend to do. Ray suggested that I enhance the code by not creating the
> > timer event when Timeout is 0, which I assumed meant that XhcExecTransfer
> > () would just return. I personally think it would be a good idea to keep this
> > behavior in the code, but would like Ray's input on the matter. Appreciate
>
>
> I prefer to keep the origin behavior.
> Ray, what is your comment on this?
>
> Best Regards,
> Hao Wu
>
>
> > the help!
> >
> > Thanks,
> > Patrick Henz
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Wu, Hao A
> > Sent: Wednesday, September 2, 2020 9:25 PM
> > To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>; Ni, Ray
> > <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix
> > Broken Timeouts
> >
> > Hello Patrick, a couple of inline comments below.
> > Hello Ray, need your input on one thing below as well.
> >
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > patrick.henz@hpe.com
> > > Sent: Thursday, September 3, 2020 1:05 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 v2 1/1] MdeModulePkg/XhciDxe: Fix Broken
> > > Timeouts
> > >
> > > From: Patrick Henz <patrick.henz@hpe.com>
> > >
> > > REF:INVALID URI REMOVED
> > > ocore.org_show-5Fbug.cgi-3Fid-
> > 3D2948&d=DwIFAg&c=C5b8zRQO1miGmBeVZ2LFWg
> > >
> > &r=wx4n0HbqxSAP19Eklmv6gq7ivDQlqQ_ITOkZIBUNNKg&m=OKlWpRL8ZyDf
> > hUEh6S4OU
> > > aMasig0MPoajX7Vz2sDSvY&s=LTDbPsspkRCcWFBfThqhR_FaljF2kQLagB_t-
> > kbAm80&e
> > > =
> > >
> > > 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 | 35 ++++++++++++++++---
> > > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 43
> > > +++++++++++++++++++-----
> > > 2 files changed, 65 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > index 42b773ab31be..33ac13504669 100644
> > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > @@ -442,17 +442,44 @@ 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) {
> > > + goto TIMEOUT;
> > > + }
> > > +
> > > + Status = gBS->CreateEvent (
> > > + EVT_TIMER,
> > > + TPL_CALLBACK,
> > > + NULL,
> > > + NULL,
> > > + &TimeoutEvent
> > > + );
> > > +
> > > + if (!EFI_ERROR (Status)) {
> > > + Status = gBS->SetTimer (TimeoutEvent,
> > > + TimerRelative,
> > > + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> > > + }
> > > +
> > > + if (EFI_ERROR(Status)) {
> > > + goto TIMEOUT;
> > > + }
> >
> >
> > Could you help to refine the return status for the case when CreateEvent or
> > SetTimer calls fail?
> > I think it will return EFI_TIMEOUT at this moment, which might confuse the
> > caller.
> > You may need to modify the function description comment section for the
> > new return value also.
> >
> > A similar case applies to XhcExecTransfer() as well.
> >
> >
> > > +
> > > + do {
> > > if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> > > return EFI_SUCCESS;
> > > }
> > >
> > > gBS->Stall (XHC_1_MICROSECOND);
> > > + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> > > +
> > > +TIMEOUT:
> > > + if (TimeoutEvent != NULL) {
> > > + gBS->CloseEvent (TimeoutEvent);
> > > }
> > >
> > > return EFI_TIMEOUT;
> > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > index ab8957c546ee..d6290b5fe33b 100644
> > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > @@ -1273,11 +1273,19 @@ XhcExecTransfer (
> > > )
> > > {
> > > EFI_STATUS Status;
> > > - UINTN Index;
> > > - UINT64 Loop;
> > > UINT8 SlotId;
> > > UINT8 Dci;
> > > BOOLEAN Finished;
> > > + EFI_EVENT TimeoutEvent;
> > > + EFI_STATUS TimerStatus;
> > > +
> > > + Status = EFI_SUCCESS;
> > > + Finished = FALSE;
> > > + TimeoutEvent = NULL;
> > > +
> > > + if (Timeout == 0) {
> > > + goto DONE;
> > > + }
> > >
> > > if (CmdTransfer) {
> > > SlotId = 0;
> > > @@ -1291,29 +1299,46 @@ XhcExecTransfer (
> > > ASSERT (Dci < 32);
> > > }
> > >
> > > - Status = EFI_SUCCESS;
> > > - Loop = Timeout * XHC_1_MILLISECOND;
> > > - if (Timeout == 0) {
> > > - Loop = 0xFFFFFFFF;
> >
> >
> > Ray and Patrick, the previous behavior when 'Timeout' is 0 for this function is
> > that it will do a 'psudo-indefinite' loop by setting the 'Loop' variable to the
> > value of MAX_UINT32.
> >
> > But after the patch, the behavior got changed and the function will directly
> > return EFI_TIMEOUT when 'Timeout' is 0.
> > This behavior change might impact the callers when they expecting a 'psudo-
> > indefinite' timeout.
> > I think it would be better to keep the origin behavior, what is your thought?
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > > + TimerStatus = gBS->CreateEvent (
> > > + EVT_TIMER,
> > > + TPL_CALLBACK,
> > > + NULL,
> > > + NULL,
> > > + &TimeoutEvent
> > > + );
> > > +
> > > + if (!EFI_ERROR (TimerStatus)) {
> > > + TimerStatus = gBS->SetTimer (TimeoutEvent,
> > > + TimerRelative,
> > > +
> > > + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> > > + }
> > > +
> > > + if (EFI_ERROR (TimerStatus)) {
> > > + goto DONE;
> > > }
> > >
> > > XhcRingDoorBell (Xhc, SlotId, Dci);
> > >
> > > - for (Index = 0; Index < Loop; Index++) {
> > > + do {
> > > Finished = XhcCheckUrbResult (Xhc, Urb);
> > > if (Finished) {
> > > break;
> > > }
> > > gBS->Stall (XHC_1_MICROSECOND);
> > > - }
> > > + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> > >
> > > - if (Index == Loop) {
> > > +DONE:
> > > + 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 v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
2020-09-23 3:40 ` Ni, Ray
@ 2020-09-23 5:22 ` Wu, Hao A
0 siblings, 0 replies; 7+ messages in thread
From: Wu, Hao A @ 2020-09-23 5:22 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, patrick.henz@hpe.com; +Cc: Wang, Jian J
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, September 23, 2020 11:41 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io;
> patrick.henz@hpe.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix
> Broken Timeouts
>
> Hao,
> Yes the behavior should not be changed.
> Can we keep the original behavior while still not creating the timer when
> infinite-loop is needed?
Hello Patrick and Ray,
For the origin (current) behavior of XhcExecTransfer(), when 'Timeout' equals 0,
the function will attempt to set the timeout of value:
0xFFFFFFFF milliseconds = 4,294,967,295 milliseconds = ~1,193 hours
Instead of creating a timer event for this case, I think we can just replace it
with an indefinite loop (maybe introducing a 'IndefiniteLoop' flag for the
do-while statement).
Best Regards,
Hao Wu
>
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Tuesday, September 15, 2020 9:28 AM
> > To: devel@edk2.groups.io; patrick.henz@hpe.com; Ni, Ray
> > <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix
> > Broken Timeouts
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Henz,
> > > Patrick
> > > Sent: Friday, September 11, 2020 2:43 AM
> > > To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> > > <ray.ni@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix
> > > Broken Timeouts
> > >
> > > Hi Hao and Ray,
> > >
> > > In regards to the behavior when Timeout == 0, I did try to account
> > > for that in my initial patch with the following logic:
> > >
> > > (0 ==
> > >
> Timeout)?(EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):(EFI_TIMER_
> > > PERIOD_MILLISECONDS(Timeout))
> > >
> > > This results in a timeout that happens sooner than what the current
> > > code would produce, but it falls in line with what the original code
> > > seemed to intend to do. Ray suggested that I enhance the code by not
> > > creating the timer event when Timeout is 0, which I assumed meant
> > > that XhcExecTransfer
> > > () would just return. I personally think it would be a good idea to
> > > keep this behavior in the code, but would like Ray's input on the
> > > matter. Appreciate
> >
> >
> > I prefer to keep the origin behavior.
> > Ray, what is your comment on this?
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > > the help!
> > >
> > > Thanks,
> > > Patrick Henz
> > >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> > > Of Wu, Hao A
> > > Sent: Wednesday, September 2, 2020 9:25 PM
> > > To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>; Ni,
> > > Ray <ray.ni@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix
> > > Broken Timeouts
> > >
> > > Hello Patrick, a couple of inline comments below.
> > > Hello Ray, need your input on one thing below as well.
> > >
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > patrick.henz@hpe.com
> > > > Sent: Thursday, September 3, 2020 1:05 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 v2 1/1] MdeModulePkg/XhciDxe: Fix
> > > > Broken Timeouts
> > > >
> > > > From: Patrick Henz <patrick.henz@hpe.com>
> > > >
> > > > REF:INVALID URI REMOVED
> > > > ocore.org_show-5Fbug.cgi-3Fid-
> > > 3D2948&d=DwIFAg&c=C5b8zRQO1miGmBeVZ2LFWg
> > > >
> > >
> &r=wx4n0HbqxSAP19Eklmv6gq7ivDQlqQ_ITOkZIBUNNKg&m=OKlWpRL8ZyDf
> > > hUEh6S4OU
> > > >
> aMasig0MPoajX7Vz2sDSvY&s=LTDbPsspkRCcWFBfThqhR_FaljF2kQLagB_t-
> > > kbAm80&e
> > > > =
> > > >
> > > > 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 | 35
> ++++++++++++++++---
> > > > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 43
> > > > +++++++++++++++++++-----
> > > > 2 files changed, 65 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > index 42b773ab31be..33ac13504669 100644
> > > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > @@ -442,17 +442,44 @@ 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) {
> > > > + goto TIMEOUT;
> > > > + }
> > > > +
> > > > + Status = gBS->CreateEvent (
> > > > + EVT_TIMER,
> > > > + TPL_CALLBACK,
> > > > + NULL,
> > > > + NULL,
> > > > + &TimeoutEvent
> > > > + );
> > > > +
> > > > + if (!EFI_ERROR (Status)) {
> > > > + Status = gBS->SetTimer (TimeoutEvent,
> > > > + TimerRelative,
> > > > +
> > > > + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> > > > + }
> > > > +
> > > > + if (EFI_ERROR(Status)) {
> > > > + goto TIMEOUT;
> > > > + }
> > >
> > >
> > > Could you help to refine the return status for the case when
> > > CreateEvent or SetTimer calls fail?
> > > I think it will return EFI_TIMEOUT at this moment, which might
> > > confuse the caller.
> > > You may need to modify the function description comment section for
> > > the new return value also.
> > >
> > > A similar case applies to XhcExecTransfer() as well.
> > >
> > >
> > > > +
> > > > + do {
> > > > if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> > > > return EFI_SUCCESS;
> > > > }
> > > >
> > > > gBS->Stall (XHC_1_MICROSECOND);
> > > > + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> > > > +
> > > > +TIMEOUT:
> > > > + if (TimeoutEvent != NULL) {
> > > > + gBS->CloseEvent (TimeoutEvent);
> > > > }
> > > >
> > > > return EFI_TIMEOUT;
> > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > > index ab8957c546ee..d6290b5fe33b 100644
> > > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > > @@ -1273,11 +1273,19 @@ XhcExecTransfer (
> > > > )
> > > > {
> > > > EFI_STATUS Status;
> > > > - UINTN Index;
> > > > - UINT64 Loop;
> > > > UINT8 SlotId;
> > > > UINT8 Dci;
> > > > BOOLEAN Finished;
> > > > + EFI_EVENT TimeoutEvent;
> > > > + EFI_STATUS TimerStatus;
> > > > +
> > > > + Status = EFI_SUCCESS;
> > > > + Finished = FALSE;
> > > > + TimeoutEvent = NULL;
> > > > +
> > > > + if (Timeout == 0) {
> > > > + goto DONE;
> > > > + }
> > > >
> > > > if (CmdTransfer) {
> > > > SlotId = 0;
> > > > @@ -1291,29 +1299,46 @@ XhcExecTransfer (
> > > > ASSERT (Dci < 32);
> > > > }
> > > >
> > > > - Status = EFI_SUCCESS;
> > > > - Loop = Timeout * XHC_1_MILLISECOND;
> > > > - if (Timeout == 0) {
> > > > - Loop = 0xFFFFFFFF;
> > >
> > >
> > > Ray and Patrick, the previous behavior when 'Timeout' is 0 for this
> > > function is that it will do a 'psudo-indefinite' loop by setting the
> > > 'Loop' variable to the value of MAX_UINT32.
> > >
> > > But after the patch, the behavior got changed and the function will
> > > directly return EFI_TIMEOUT when 'Timeout' is 0.
> > > This behavior change might impact the callers when they expecting a
> > > 'psudo- indefinite' timeout.
> > > I think it would be better to keep the origin behavior, what is your
> thought?
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > >
> > > > + TimerStatus = gBS->CreateEvent (
> > > > + EVT_TIMER,
> > > > + TPL_CALLBACK,
> > > > + NULL,
> > > > + NULL,
> > > > + &TimeoutEvent
> > > > + );
> > > > +
> > > > + if (!EFI_ERROR (TimerStatus)) {
> > > > + TimerStatus = gBS->SetTimer (TimeoutEvent,
> > > > + TimerRelative,
> > > > +
> > > > + EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> > > > + }
> > > > +
> > > > + if (EFI_ERROR (TimerStatus)) {
> > > > + goto DONE;
> > > > }
> > > >
> > > > XhcRingDoorBell (Xhc, SlotId, Dci);
> > > >
> > > > - for (Index = 0; Index < Loop; Index++) {
> > > > + do {
> > > > Finished = XhcCheckUrbResult (Xhc, Urb);
> > > > if (Finished) {
> > > > break;
> > > > }
> > > > gBS->Stall (XHC_1_MICROSECOND);
> > > > - }
> > > > + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> > > >
> > > > - if (Index == Loop) {
> > > > +DONE:
> > > > + 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
end of thread, other threads:[~2020-09-23 5:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-02 17:04 [PATCH v2 0/1] Fix XhciDxe Timeouts patrick.henz
2020-09-02 17:04 ` [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts patrick.henz
2020-09-03 2:24 ` [edk2-devel] " Wu, Hao A
2020-09-10 18:43 ` Henz, Patrick
2020-09-15 1:27 ` Wu, Hao A
2020-09-23 3:40 ` Ni, Ray
2020-09-23 5:22 ` Wu, Hao A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox