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

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

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