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

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

> -----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