-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeff
Brasen
Sent: Wednesday, October 21, 2020 11:45 PM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Jon
Hunter <jonathanh@nvidia.com>; Jeff Brasen <jbrasen@nvidia.com>
Subject: [edk2-devel] [PATCH] 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.

Hello Jeff,

Thanks a lot for the patch.
I found that there is another patch current on the mailing list that also
addresses an issue during device slot initialization:
https://bugzilla.tianocore.org/show_bug.cgi?id=3007

For the issue mentioned in BZ-3007, it will handle the case that:
a) The device slot initialization fails;
b) A call to XhcDisableSlotCmd(64) is needed. Otherwise, the initialization of
the next port will fail due to the content in 'Xhc->UsbDevContext' is not
cleared.

The current resolution for BZ-3007 is to add XhcDisableSlotCmd(64) calls when
an error occurs in XhcInitializeDeviceSlot(64).

Could you help to see if the patch provided at:
https://edk2.groups.io/g/devel/message/66529
has any impact on your proposed patch?
I think the XhcDisableSlotCmd(64) calls can be moved out of the 'do-while'
loop in your proposed change. Thanks in advance.

Also, please refer below for 1 inline comment:

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 71 ++++++++++++++--------
--
1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 9cb115363c..1a16864205 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 = 1;

Could you help to introduce a macro in XhciSched.h:
#define XHC_INIT_DEVICE_SLOT_RETRIES 1
and use the macro here?

Best Regards,
Hao Wu

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;
@@ -1739,40 +1741,51 @@ XhcPollPortStatusChange (
RouteChart.Route.TierNum = ParentRouteChart.Route.TierNum + 1;
}

- SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
- if (SlotId != 0) {
- if (Xhc->HcCParams.Data.Csz == 0) {
- Status = XhcDisableSlotCmd (Xhc, SlotId);
- } else {
- Status = XhcDisableSlotCmd64 (Xhc, SlotId);
- }
- }
-
- if (((PortState->PortStatus & USB_PORT_STAT_ENABLE) != 0) &&
- ((PortState->PortStatus & USB_PORT_STAT_CONNECTION) != 0)) {
- //
- // Has a device attached, Identify device speed after port is enabled.
- //
- Speed = EFI_USB_SPEED_FULL;
- if ((PortState->PortStatus & USB_PORT_STAT_LOW_SPEED) != 0) {
- Speed = EFI_USB_SPEED_LOW;
- } else if ((PortState->PortStatus & USB_PORT_STAT_HIGH_SPEED) != 0) {
- Speed = EFI_USB_SPEED_HIGH;
- } 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.
- //
+ do {
SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
- if ((SlotId == 0) && ((PortState->PortChangeStatus &
USB_PORT_STAT_C_RESET) != 0)) {
+ if (SlotId != 0) {
if (Xhc->HcCParams.Data.Csz == 0) {
- Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart, Port,
RouteChart, Speed);
+ Status = XhcDisableSlotCmd (Xhc, SlotId);
} else {
- Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart, Port,
RouteChart, Speed);
+ Status = XhcDisableSlotCmd64 (Xhc, SlotId);
}
}
- }
+
+ if (((PortState->PortStatus & USB_PORT_STAT_ENABLE) != 0) &&
+ ((PortState->PortStatus & USB_PORT_STAT_CONNECTION) != 0)) {
+ //
+ // Has a device attached, Identify device speed after port is enabled.
+ //
+ Speed = EFI_USB_SPEED_FULL;
+ if ((PortState->PortStatus & USB_PORT_STAT_LOW_SPEED) != 0) {
+ Speed = EFI_USB_SPEED_LOW;
+ } else if ((PortState->PortStatus & USB_PORT_STAT_HIGH_SPEED) != 0) {
+ Speed = EFI_USB_SPEED_HIGH;
+ } 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);
+ }
+ }
+ }
+
+ //
+ // 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;
}
--
2.25.1