Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Retry device slot init on failure

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

Patch was merged via:
Pull request - https://github.com/tianocore/edk2/pull/1063
Commit - https://github.com/tianocore/edk2/commit/2363c6926098ee5c75c8780d07f88f5c21010683

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Thursday, October 29, 2020 9:03 AM
> To: Jon Hunter <jonathanh@nvidia.com>; Jeff Brasen <jbrasen@nvidia.com>;
> devel@edk2.groups.io
> Cc: Luo, Heng <heng.luo@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Retry
> device slot init on failure
> 
> > -----Original Message-----
> > From: Jeff Brasen <jbrasen@nvidia.com>
> > Sent: Thursday, October 29, 2020 1:20 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Luo, Heng <heng.luo@intel.com>;
> > Ni, Ray <ray.ni@intel.com>; Jon Hunter <jonathanh@nvidia.com>; Jeff
> > Brasen <jbrasen@nvidia.com>
> > Subject: [PATCH v3 1/1] MdeModulePkg/XhciDxe: Retry device slot init
> > on failure
> >
> > From: Jon Hunter <jonathanh@nvidia.com>
> >
> > With some super-speed USB mass storage devices it has been observed
> > that a USB transaction error may occur when attempting the set the
> > device address during enumeration.
> >
> > According the the xHCI specification (section 4.6.5) ...
> >
> > "A USB Transaction ErrorCompletion Code for an Address Device Command
> > may be due to a Stall response from a device. Software should issue a
> > Disable Slot Commandfor the Device Slot then an Enable Slot Command
> > to recover from this error."
> >
> > To fix this, retry the device slot initialization if it fails due to a device error.
> >
> > Change was verified using a superspeed mass storage device that was
> > occasionally failing to enumerate in UEFI. With this change this
> > failure to enumerate was resolved. This failure was also only seen in
> > UEFI and not in the OS.
> >
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |  1 +
> > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 34 +++++++++++++++++--
> --
> > ---
> >  2 files changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > index 2f1899502151..3f9cdb1c3609 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > @@ -11,6 +11,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > #define _EFI_XHCI_SCHED_H_
> >
> >  #define XHC_URB_SIG      SIGNATURE_32 ('U', 'S', 'B', 'R')
> > +#define XHC_INIT_DEVICE_SLOT_RETRIES 1
> >
> >  //
> >  // Transfer types, used in URB to identify the transfer type diff
> > --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > index 00e9cc63d63e..dc36945962a0 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > @@ -1717,9 +1717,11 @@ XhcPollPortStatusChange (
> >    EFI_STATUS        Status;
> >    UINT8             Speed;
> >    UINT8             SlotId;
> > +  UINT8             Retries;
> >    USB_DEV_ROUTE     RouteChart;
> >
> >    Status = EFI_SUCCESS;
> > +  Retries = XHC_INIT_DEVICE_SLOT_RETRIES;
> >
> >    if ((PortState->PortChangeStatus & (USB_PORT_STAT_C_CONNECTION |
> > USB_PORT_STAT_C_ENABLE | USB_PORT_STAT_C_OVERCURRENT |
> > USB_PORT_STAT_C_RESET)) == 0) {
> >      return EFI_SUCCESS;
> > @@ -1761,17 +1763,29 @@ XhcPollPortStatusChange (
> >      } else if ((PortState->PortStatus & USB_PORT_STAT_SUPER_SPEED) != 0)
> {
> >        Speed = EFI_USB_SPEED_SUPER;
> >      }
> > -    //
> > -    // Execute Enable_Slot cmd for attached device, initialize device context
> > and assign device address.
> > -    //
> > -    SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
> > -    if ((SlotId == 0) && ((PortState->PortChangeStatus &
> > USB_PORT_STAT_C_RESET) != 0)) {
> > -      if (Xhc->HcCParams.Data.Csz == 0) {
> > -        Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart, Port,
> > RouteChart, Speed);
> > -      } else {
> > -        Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart, Port,
> > RouteChart, Speed);
> > +
> > +    do {
> > +      //
> > +      // Execute Enable_Slot cmd for attached device, initialize
> > + device context
> > and assign device address.
> > +      //
> > +      SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
> > +      if ((SlotId == 0) && ((PortState->PortChangeStatus &
> > USB_PORT_STAT_C_RESET) != 0)) {
> > +        if (Xhc->HcCParams.Data.Csz == 0) {
> > +          Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart,
> > + Port,
> > RouteChart, Speed);
> > +        } else {
> > +          Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart,
> > + Port,
> > RouteChart, Speed);
> > +        }
> >        }
> > -    }
> > +
> > +      //
> > +      // According to the xHCI specification (section 4.6.5), "a USB
> Transaction
> > +      // Error Completion Code for an Address Device Command may be
> > + due
> > to a Stall
> > +      // response from a device. Software should issue a Disable Slot
> > Command for
> > +      // the Device Slot then an Enable Slot Command to recover from
> > + this
> > error."
> > +      // Therefore, retry the device slot initialization if it fails due to a
> > +      // device error.
> > +      //
> > +    } while ((Status == EFI_DEVICE_ERROR) && (Retries-- != 0));
> 
> 
> Thanks Jon and Jeff,
> 
> Patch looks good to me:
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> 
> I will wait a couple of days to see if there is any comment from others before
> merging the patch.
> 
> Best Regards,
> Hao Wu
> 
> 
> >    }
> >
> >    return Status;
> > --
> > 2.25.1
> 
> 
> 
> 
>