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

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

Hello Heng and Jon,

For the patch provided by Heng:
It deals with the issue that when the initialization of a device slot fails for
an unworkable device (even after retry). The fix adds a call to
XhcDisableSlotCmd(64) to disable the slot and free the resources managed by the
driver so that the enumeration of the next port won't be affected.

For the patch provided by Jon:
It adds a retry scheme for the device slot initialization, which will involve
a call to XhcDisableSlotCmd(64) during the retry.

Since both of the patches will introduce a call XhcDisableSlotCmd(64) after the
slot initialization fails, just want to sync with you to ensure
XhcDisableSlotCmd(64) is only called once upon failure.

I agree with Jon's proposal to rebase Jon's patch on the base of Heng's patch.
So Heng, could you help to submit a new version of your patch to let the return
value of function XhcInitializeDeviceSlot(64) not depend on the result of
XhcDisableSlotCmd(64), but on the result of whether the device slot is
successfully initialized:

  } else {
    DEBUG ((DEBUG_INFO, "    Address %d assigned unsuccessfully\n"));
    XhcDisableSlotCmd(64) (Xhc, SlotId);
  }
  
I think doing so will not block the retry scheme introduced in Jon's patch.
Thanks in advance.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Luo, Heng <heng.luo@intel.com>
> Sent: Friday, October 23, 2020 8:54 AM
> To: Jon Hunter <jonathanh@nvidia.com>; Wu, Hao A <hao.a.wu@intel.com>;
> devel@edk2.groups.io; jbrasen@nvidia.com
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Retry device
> slot init on failure
> 
> Hi Jon,
> Thank  you for you testing.
> In my patch, it does not try again if slot init failure, the failed device will be
> ignored.
> Currently the first failure will affect next devices, next devices will not be
> initialized successfully, my patch is to fix this bug.
> I think this two patches are to resolve different problems, both changes are
> needed.
> 
> Thank,
> Heng
> 
> > -----Original Message-----
> > From: Jon Hunter <jonathanh@nvidia.com>
> > Sent: Thursday, October 22, 2020 9:57 PM
> > To: Luo, Heng <heng.luo@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > devel@edk2.groups.io; jbrasen@nvidia.com
> > Cc: Ni, Ray <ray.ni@intel.com>
> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Retry device
> > slot init on failure
> >
> > Hi Heng,
> >
> > On 22/10/2020 09:44, Jon Hunter wrote:
> > > Hi Heng,
> > >
> > > On 22/10/2020 04:01, Luo, Heng wrote:
> > >> Hi Hao,
> > >> I applying this patch, it cannot fix BZ
> > https://bugzilla.tianocore.org/show_bug.cgi?id=3007.
> > >
> > > Can you elaborate more on why that is? Unfortunately, your patch
> > > does not work for the scenario we are testing.
> > >
> > > In your case does the bad USB device never enumerate even on the 2nd
> > > attempt?
> > >
> > > If so, then maybe we can apply your patch to do the disable slot and
> > > in our patch we just try the device init again once, without calling
> > > the slot disable because that is handled by your patch.
> >
> > Applying the below change on top of your change, works for me.
> > One change in the below is not to return the status from the call to
> > XhcDisableSlotCmd64() which is necessary to retry the device initialisation.
> >
> > Jon
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > index 21bc9ce17556..82efd4a03f8e 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > @@ -1684,9 +1684,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;
> > @@ -1728,17 +1730,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--));
> >    }
> >    return Status;
> > @@ -2248,7 +2262,7 @@ XhcInitializeDeviceSlot (
> >      Xhc->UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;
> >    } else {
> >      DEBUG ((DEBUG_INFO, "    Address %d assigned unsuccessfully\n"));
> > -    Status = XhcDisableSlotCmd (Xhc, SlotId);
> > +    XhcDisableSlotCmd (Xhc, SlotId);
> >    }
> >    return Status;
> > @@ -2461,7 +2475,7 @@ XhcInitializeDeviceSlot64 (
> >      Xhc->UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;
> >    } else {
> >      DEBUG ((DEBUG_INFO, "    Address %d assigned unsuccessfully\n"));
> > -    Status = XhcDisableSlotCmd64 (Xhc, SlotId);
> > +    XhcDisableSlotCmd64 (Xhc, SlotId);
> >    }
> >    return Status;
> > 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
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > --
> > nvpublic