-----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