Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts

["Wu, Hao A" <hao.a.wu@intel.com>]

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