MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 71 ++++++++++++++---------- 1 file changed, 42 insertions(+), 29 deletions(-)
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66496): https://edk2.groups.io/g/devel/message/66496
Mute This Topic: https://groups.io/mt/77707715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66530): https://edk2.groups.io/g/devel/message/66530
Mute This Topic: https://groups.io/mt/77707715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66533): https://edk2.groups.io/g/devel/message/66533 Mute This Topic: https://groups.io/mt/77707715/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66513): https://edk2.groups.io/g/devel/message/66513
Mute This Topic: https://groups.io/mt/77707715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66526): https://edk2.groups.io/g/devel/message/66526
Mute This Topic: https://groups.io/mt/77707715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66534): https://edk2.groups.io/g/devel/message/66534 Mute This Topic: https://groups.io/mt/77707715/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66538): https://edk2.groups.io/g/devel/message/66538
Mute This Topic: https://groups.io/mt/77707715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66546): https://edk2.groups.io/g/devel/message/66546
Mute This Topic: https://groups.io/mt/77707715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66547): https://edk2.groups.io/g/devel/message/66547
Mute This Topic: https://groups.io/mt/77707715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66549): https://edk2.groups.io/g/devel/message/66549
Mute This Topic: https://groups.io/mt/77707715/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.