* [PATCH v3 1/1] MdeModulePkg/XhciDxe: Retry device slot init on failure
@ 2020-10-28 17:20 Jeff Brasen
2020-10-29 1:03 ` Wu, Hao A
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Brasen @ 2020-10-28 17:20 UTC (permalink / raw)
To: devel; +Cc: hao.a.wu, heng.luo, ray.ni, Jon Hunter, 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.
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));
}
return Status;
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3 1/1] MdeModulePkg/XhciDxe: Retry device slot init on failure
2020-10-28 17:20 [PATCH v3 1/1] MdeModulePkg/XhciDxe: Retry device slot init on failure Jeff Brasen
@ 2020-10-29 1:03 ` Wu, Hao A
2020-11-02 1:32 ` [edk2-devel] " Wu, Hao A
0 siblings, 1 reply; 3+ messages in thread
From: Wu, Hao A @ 2020-10-29 1:03 UTC (permalink / raw)
To: Jon Hunter, Jeff Brasen, devel@edk2.groups.io; +Cc: Luo, Heng, Ni, Ray
> -----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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Retry device slot init on failure
2020-10-29 1:03 ` Wu, Hao A
@ 2020-11-02 1:32 ` Wu, Hao A
0 siblings, 0 replies; 3+ messages in thread
From: Wu, Hao A @ 2020-11-02 1:32 UTC (permalink / raw)
To: devel@edk2.groups.io, Wu, Hao A, Jon Hunter, Jeff Brasen
Cc: Luo, Heng, Ni, Ray
Patch was merged via:
Pull request - https://github.com/tianocore/edk2/pull/1063
Commit - https://github.com/tianocore/edk2/commit/2363c6926098ee5c75c8780d07f88f5c21010683
Best Regards,
Hao Wu
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Thursday, October 29, 2020 9:03 AM
> To: Jon Hunter <jonathanh@nvidia.com>; Jeff Brasen <jbrasen@nvidia.com>;
> devel@edk2.groups.io
> Cc: Luo, Heng <heng.luo@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/XhciDxe: Retry
> device slot init on failure
>
> > -----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
>
>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-02 1:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-28 17:20 [PATCH v3 1/1] MdeModulePkg/XhciDxe: Retry device slot init on failure Jeff Brasen
2020-10-29 1:03 ` Wu, Hao A
2020-11-02 1:32 ` [edk2-devel] " Wu, Hao A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox