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

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

Sure, I will send out patch V4.

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Friday, October 23, 2020 9:10 AM
> To: Luo, Heng <heng.luo@intel.com>; Jon Hunter <jonathanh@nvidia.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
> 
> 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