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

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

[Jeff Brasen]

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

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

[Wu, Hao A]

Hello Heng,

It looks to me that the patch is addressing a similar issue you met on the UpXtreme board mentioned in:
https://bugzilla.tianocore.org/show_bug.cgi?id=3007

Could you help to check if such retry during the slot initialization helps for the USB device on your board?
Thanks in advance.

Best Regards,
Hao Wu

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

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

[Heng Luo]

Hi Hao,
I applying this patch, it cannot fix BZ https://bugzilla.tianocore.org/show_bug.cgi?id=3007.

Thanks,
Heng

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, October 22, 2020 8:54 AM
> To: devel@edk2.groups.io; jbrasen@nvidia.com; Luo, Heng
> <heng.luo@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Jon Hunter <jonathanh@nvidia.com>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Retry device slot
> init on failure
> 
> Hello Heng,
> 
> It looks to me that the patch is addressing a similar issue you met on the
> UpXtreme board mentioned in:
> https://bugzilla.tianocore.org/show_bug.cgi?id=3007
> 
> Could you help to check if such retry during the slot initialization helps for
> the USB device on your board?
> Thanks in advance.
> 
> Best Regards,
> Hao Wu
> 
> > -----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.
> >
> > 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;
> >
> >    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
> >
> >
> >
> > 
> >

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

[Jon Hunter]

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.

Cheers
Jon

-- 
nvpublic

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

[Jon Hunter]

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

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

[Heng Luo]

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

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

[Wu, Hao A]

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

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

[Heng Luo]

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

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

[Wu, Hao A]

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

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

[Jon Hunter]

On 22/10/2020 07:40, Wu, Hao A wrote:

...

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

Hi Hao Wu,

Thanks for the response. I have tried the above patch but it does not
work for us. The problem is with the above patch is that it only
disables the slot and does not retry the device initialisation again
afterwards. Therefore, if we fail enumeration, we don't retry.

The scenario that we are testing is booting an Linux OS distro from an
USB mass storage device. The problem we see with some superspeed mass
storage devices is ...

1. On the very first boot, USB mass storage is enumerated and we can
   boot the OS.
2. We then reboot the OS and on the next boot UEFI fails to enumerate
   the USB device and we can no longer boot the OS.

With the proposed patch, I see that after the USB mass storage fails to
enumerate, on the 2nd attempt we can enumerate it and boot successfully.

> Also, please refer below for 1 inline comment:

Yes we can take care of that.

Thanks
Jon

-- 
nvpublic