From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"jbrasen@nvidia.com" <jbrasen@nvidia.com>
Cc: "Ni, Ray" <ray.ni@intel.com>, Jon Hunter <jonathanh@nvidia.com>,
"Luo, Heng" <heng.luo@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Retry device slot init on failure
Date: Thu, 22 Oct 2020 06:40:17 +0000 [thread overview]
Message-ID: <BN8PR11MB36667B7C4D208033E2E265F6CA1D0@BN8PR11MB3666.namprd11.prod.outlook.com> (raw)
In-Reply-To: <bd22b2674a6ccaca7824b32dbe92d279647ab26b.1603294965.git.jbrasen@nvidia.com>
> -----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
>
>
>
>
>
next prev parent reply other threads:[~2020-10-22 6:40 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-21 15:44 [PATCH] MdeModulePkg/XhciDxe: Retry device slot init on failure Jeff Brasen
2020-10-22 0:54 ` [edk2-devel] " Wu, Hao A
2020-10-22 3:01 ` Heng Luo
2020-10-22 8:44 ` Jon Hunter
2020-10-22 13:57 ` Jon Hunter
2020-10-23 0:53 ` Heng Luo
2020-10-23 1:09 ` Wu, Hao A
2020-10-23 1:27 ` Heng Luo
2020-10-22 6:40 ` Wu, Hao A [this message]
2020-10-22 8:39 ` Jon Hunter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN8PR11MB36667B7C4D208033E2E265F6CA1D0@BN8PR11MB3666.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox