Re: [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@...>
Sent: Friday, October 23, 2020 8:54 AM
To: Jon Hunter <jonathanh@...>; Wu, Hao A <hao.a.wu@...>;
devel@edk2.groups.io; jbrasen@...
Cc: Ni, Ray <ray.ni@...>
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@...>
Sent: Thursday, October 22, 2020 9:57 PM
To: Luo, Heng <heng.luo@...>; Wu, Hao A <hao.a.wu@...>;
devel@edk2.groups.io; jbrasen@...
Cc: Ni, Ray <ray.ni@...>
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

Join devel@edk2.groups.io to automatically receive all group messages.