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

["Heng Luo" <heng.luo@intel.com>]

Hi Hao,
I applying this patch, it cannot fix BZ https://bugzilla.tianocore.org/show_bug.cgi?id=3007.

Thanks,
Heng

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, October 22, 2020 8:54 AM
> To: devel@edk2.groups.io; jbrasen@nvidia.com; Luo, Heng
> <heng.luo@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Jon Hunter <jonathanh@nvidia.com>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Retry device slot
> init on failure
> 
> Hello Heng,
> 
> It looks to me that the patch is addressing a similar issue you met on the
> UpXtreme board mentioned in:
> https://bugzilla.tianocore.org/show_bug.cgi?id=3007
> 
> Could you help to check if such retry during the slot initialization helps for
> the USB device on your board?
> Thanks in advance.
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeff
> > Brasen
> > Sent: Wednesday, October 21, 2020 11:45 PM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Jon
> > Hunter <jonathanh@nvidia.com>; Jeff Brasen <jbrasen@nvidia.com>
> > Subject: [edk2-devel] [PATCH] 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.
> >
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 71 ++++++++++++++--------
> > --
> >  1 file changed, 42 insertions(+), 29 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > index 9cb115363c..1a16864205 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 = 1;
> >
> >    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;
> > @@ -1739,40 +1741,51 @@ XhcPollPortStatusChange (
> >      RouteChart.Route.TierNum       = ParentRouteChart.Route.TierNum + 1;
> >    }
> >
> > -  SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
> > -  if (SlotId != 0) {
> > -    if (Xhc->HcCParams.Data.Csz == 0) {
> > -      Status = XhcDisableSlotCmd (Xhc, SlotId);
> > -    } else {
> > -      Status = XhcDisableSlotCmd64 (Xhc, SlotId);
> > -    }
> > -  }
> > -
> > -  if (((PortState->PortStatus & USB_PORT_STAT_ENABLE) != 0) &&
> > -      ((PortState->PortStatus & USB_PORT_STAT_CONNECTION) != 0)) {
> > -    //
> > -    // Has a device attached, Identify device speed after port is enabled.
> > -    //
> > -    Speed = EFI_USB_SPEED_FULL;
> > -    if ((PortState->PortStatus & USB_PORT_STAT_LOW_SPEED) != 0) {
> > -      Speed = EFI_USB_SPEED_LOW;
> > -    } else if ((PortState->PortStatus & USB_PORT_STAT_HIGH_SPEED) != 0) {
> > -      Speed = EFI_USB_SPEED_HIGH;
> > -    } 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.
> > -    //
> > +  do {
> >      SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
> > -    if ((SlotId == 0) && ((PortState->PortChangeStatus &
> > USB_PORT_STAT_C_RESET) != 0)) {
> > +    if (SlotId != 0) {
> >        if (Xhc->HcCParams.Data.Csz == 0) {
> > -        Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart, Port,
> > RouteChart, Speed);
> > +        Status = XhcDisableSlotCmd (Xhc, SlotId);
> >        } else {
> > -        Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart, Port,
> > RouteChart, Speed);
> > +        Status = XhcDisableSlotCmd64 (Xhc, SlotId);
> >        }
> >      }
> > -  }
> > +
> > +    if (((PortState->PortStatus & USB_PORT_STAT_ENABLE) != 0) &&
> > +        ((PortState->PortStatus & USB_PORT_STAT_CONNECTION) != 0)) {
> > +      //
> > +      // Has a device attached, Identify device speed after port is enabled.
> > +      //
> > +      Speed = EFI_USB_SPEED_FULL;
> > +      if ((PortState->PortStatus & USB_PORT_STAT_LOW_SPEED) != 0) {
> > +        Speed = EFI_USB_SPEED_LOW;
> > +      } else if ((PortState->PortStatus & USB_PORT_STAT_HIGH_SPEED) != 0)
> {
> > +        Speed = EFI_USB_SPEED_HIGH;
> > +      } 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);
> > +        }
> > +      }
> > +    }
> > +
> > +    //
> > +    // 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--));
> >
> >    return Status;
> >  }
> > --
> > 2.25.1
> >
> >
> >
> > 
> >