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

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

Thanks for the patch, inline comments below:


> -----Original Message-----
> From: Jeff Brasen <jbrasen@nvidia.com>
> Sent: Wednesday, October 28, 2020 12:00 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 v2 ] 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: 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..77664940a791 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--));


Please help to refine the above line to 
   } while ((Status == EFI_DEVICE_ERROR) && (Retries-- != 0));
to align with the edk2 coding style. Really sorry for not catching this on V1 patch.

Also, could you help to add the information on what kind of tests are done for
the patch?

Best Regards,
Hao Wu


>    }
> 
>    return Status;
> --
> 2.25.1