2023-07-12 23:03:35

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH v2] Revert "usb: dwc3: core: Enable AutoRetry feature in the controller"

On Thu, Jul 13, 2023, Jakub Vanek wrote:
> This reverts commit b138e23d3dff90c0494925b4c1874227b81bddf7.
>
> AutoRetry has been found to cause some issues. This feature allows
> the controller in host mode (further referred to as the xHC) to send
> non-terminating/burst retry ACKs (Retry=1 and Nump!=0) instead of
> terminating retry ACKs (Retry=1 and Nump=0) to devices when
> a transaction error occurs.
>
> Unfortunately, some USB devices fail to retry transactions when
> the xHC sends them a burst retry ACK. When this happens, the xHC

For some clarity: if the device continues to respond with CRC error,
the xHC will not complete endpoint related commands while it keeps
autoretry.

> enters a strange state. After the affected transfer times out,
> the xHCI driver tries to resume normal operation of the xHC
> by sending it a Stop Endpoint command. However, the xHC fails
> to respond to it, and the xHCI driver gives up. [1]
> This fact is reported via dmesg:
>
> [sda] tag#29 uas_eh_abort_handler 0 uas-tag 1 inflight: CMD IN
> [sda] tag#29 CDB: opcode=0x28 28 00 00 69 42 80 00 00 48 00
> xhci-hcd: xHCI host not responding to stop endpoint command
> xhci-hcd: xHCI host controller not responding, assume dead
> xhci-hcd: HC died; cleaning up
>
> Some users observed this problem on an Odroid HC2 with the JMS578
> USB3-to-SATA bridge. The issue can be triggered by starting
> a read-heavy workload on an attached SSD. After a while, the host
> controller would die and the SSD would disappear from the system. [1]
>
> Further analysis by Synopsys determined that controller revisions
> other than the one in Odroid HC2 are also affected by this.
> The recommended solution was to disable AutoRetry altogether.
> This change does not have a noticeable performance impact. [2]
>
> Fixes: b138e23d3dff ("usb: dwc3: core: Enable AutoRetry feature in the controller")
> Link: https://urldefense.com/v3/__https://lore.kernel.org/r/[email protected]/__;!!A4F2R9G_pg!aCe0isNmX63t6ILE2Tv2cX4UnrpDFo6LXWb6oS3-OYFYfX88igrfqmW4Z8UdO7sWz0mdco6vbBrR_KMzYaccpLF7Kw$ [1]
> Link: https://urldefense.com/v3/__https://lore.kernel.org/r/[email protected]/__;!!A4F2R9G_pg!aCe0isNmX63t6ILE2Tv2cX4UnrpDFo6LXWb6oS3-OYFYfX88igrfqmW4Z8UdO7sWz0mdco6vbBrR_KMzYae75T8GCw$ [2]
> Cc: [email protected]
> Cc: Mauro Ribeiro <[email protected]>
> Cc: Krzysztof Kozlowski <[email protected]>
> Suggested-by: Thinh Nguyen <[email protected]>
> Signed-off-by: Jakub Vanek <[email protected]>
> ---
> V1 -> V2: Updated to disable AutoRetry everywhere based on Synopsys feedback
> Reworded the changelog a bit to make it clearer
>
> drivers/usb/dwc3/core.c | 16 ----------------
> drivers/usb/dwc3/core.h | 3 ---
> 2 files changed, 19 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index f6689b731718..a4e079d37566 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1209,22 +1209,6 @@ static int dwc3_core_init(struct dwc3 *dwc)
> dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> }
>
> - if (dwc->dr_mode == USB_DR_MODE_HOST ||
> - dwc->dr_mode == USB_DR_MODE_OTG) {
> - reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
> -
> - /*
> - * Enable Auto retry Feature to make the controller operating in
> - * Host mode on seeing transaction errors(CRC errors or internal
> - * overrun scenerios) on IN transfers to reply to the device
> - * with a non-terminating retry ACK (i.e, an ACK transcation
> - * packet with Retry=1 & Nump != 0)
> - */
> - reg |= DWC3_GUCTL_HSTINAUTORETRY;
> -
> - dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> - }
> -
> /*
> * Must config both number of packets and max burst settings to enable
> * RX and/or TX threshold.
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8b1295e4dcdd..a69ac67d89fe 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -256,9 +256,6 @@
> #define DWC3_GCTL_GBLHIBERNATIONEN BIT(1)
> #define DWC3_GCTL_DSBLCLKGTNG BIT(0)
>
> -/* Global User Control Register */
> -#define DWC3_GUCTL_HSTINAUTORETRY BIT(14)
> -
> /* Global User Control 1 Register */
> #define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT BIT(31)
> #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS BIT(28)
> --
> 2.25.1
>

Acked-by: Thinh Nguyen <[email protected]>

Thanks,
Thinh


2023-07-13 22:21:02

by Jakub Vaněk

[permalink] [raw]
Subject: Re: [PATCH v2] Revert "usb: dwc3: core: Enable AutoRetry feature in the controller"

Hi Thinh,

On Wed, 2023-07-12 at 22:55 +0000, Thinh Nguyen wrote:
> On Thu, Jul 13, 2023, Jakub Vanek wrote:
> > This reverts commit b138e23d3dff90c0494925b4c1874227b81bddf7.
> >
> > AutoRetry has been found to cause some issues. This feature allows
> > the controller in host mode (further referred to as the xHC) to
> > send
> > non-terminating/burst retry ACKs (Retry=1 and Nump!=0) instead of
> > terminating retry ACKs (Retry=1 and Nump=0) to devices when
> > a transaction error occurs.
> >
> > Unfortunately, some USB devices fail to retry transactions when
> > the xHC sends them a burst retry ACK. When this happens, the xHC
>
> For some clarity: if the device continues to respond with CRC error,
> the xHC will not complete endpoint related commands while it keeps
> autoretry.

Acknowledged. Do you think it it would be better to respin this patch
once more to include this in the changelog?

> > enters a strange state. After the affected transfer times out,
> > the xHCI driver tries to resume normal operation of the xHC
> > by sending it a Stop Endpoint command. However, the xHC fails
> > to respond to it, and the xHCI driver gives up. [1]
> > This fact is reported via dmesg:
> >
> > [sda] tag#29 uas_eh_abort_handler 0 uas-tag 1 inflight: CMD IN
> > [sda] tag#29 CDB: opcode=0x28 28 00 00 69 42 80 00 00 48 00
> > xhci-hcd: xHCI host not responding to stop endpoint command
> > xhci-hcd: xHCI host controller not responding, assume dead
> > xhci-hcd: HC died; cleaning up
> >
> > Some users observed this problem on an Odroid HC2 with the JMS578
> > USB3-to-SATA bridge. The issue can be triggered by starting
> > a read-heavy workload on an attached SSD. After a while, the host
> > controller would die and the SSD would disappear from the system.
> > [1]
> >
> > Further analysis by Synopsys determined that controller revisions
> > other than the one in Odroid HC2 are also affected by this.
> > The recommended solution was to disable AutoRetry altogether.
> > This change does not have a noticeable performance impact. [2]
> >
> > Fixes: b138e23d3dff ("usb: dwc3: core: Enable AutoRetry feature in
> > the controller")
> > Link:
> > https://urldefense.com/v3/__https://lore.kernel.org/r/[email protected]/__;!!A4F2R9G_pg!aCe0isNmX63t6ILE2Tv2cX4UnrpDFo6LXWb6oS3-OYFYfX88igrfqmW4Z8UdO7sWz0mdco6vbBrR_KMzYaccpLF7Kw$
> >   [1]
> > Link:
> > https://urldefense.com/v3/__https://lore.kernel.org/r/[email protected]/__;!!A4F2R9G_pg!aCe0isNmX63t6ILE2Tv2cX4UnrpDFo6LXWb6oS3-OYFYfX88igrfqmW4Z8UdO7sWz0mdco6vbBrR_KMzYae75T8GCw$
> >   [2]
> > Cc: [email protected]
> > Cc: Mauro Ribeiro <[email protected]>
> > Cc: Krzysztof Kozlowski <[email protected]>
> > Suggested-by: Thinh Nguyen <[email protected]>
> > Signed-off-by: Jakub Vanek <[email protected]>
> > ---
> > V1 -> V2: Updated to disable AutoRetry everywhere based on Synopsys
> > feedback
> >           Reworded the changelog a bit to make it clearer
> >
> >  drivers/usb/dwc3/core.c | 16 ----------------
> >  drivers/usb/dwc3/core.h |  3 ---
> >  2 files changed, 19 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index f6689b731718..a4e079d37566 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1209,22 +1209,6 @@ static int dwc3_core_init(struct dwc3 *dwc)
> >                 dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> >         }
> >  
> > -       if (dwc->dr_mode == USB_DR_MODE_HOST ||
> > -           dwc->dr_mode == USB_DR_MODE_OTG) {
> > -               reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
> > -
> > -               /*
> > -                * Enable Auto retry Feature to make the controller
> > operating in
> > -                * Host mode on seeing transaction errors(CRC
> > errors or internal
> > -                * overrun scenerios) on IN transfers to reply to
> > the device
> > -                * with a non-terminating retry ACK (i.e, an ACK
> > transcation
> > -                * packet with Retry=1 & Nump != 0)
> > -                */
> > -               reg |= DWC3_GUCTL_HSTINAUTORETRY;
> > -
> > -               dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> > -       }
> > -
> >         /*
> >          * Must config both number of packets and max burst
> > settings to enable
> >          * RX and/or TX threshold.
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 8b1295e4dcdd..a69ac67d89fe 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -256,9 +256,6 @@
> >  #define DWC3_GCTL_GBLHIBERNATIONEN     BIT(1)
> >  #define DWC3_GCTL_DSBLCLKGTNG          BIT(0)
> >  
> > -/* Global User Control Register */
> > -#define DWC3_GUCTL_HSTINAUTORETRY      BIT(14)
> > -
> >  /* Global User Control 1 Register */
> >  #define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT      BIT(31)
> >  #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS     BIT(28)
> > --
> > 2.25.1
> >
>
> Acked-by: Thinh Nguyen <[email protected]>

Thanks!


> Thanks,
> Thinh

Best regards,
Jakub

2023-07-14 01:06:00

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH v2] Revert "usb: dwc3: core: Enable AutoRetry feature in the controller"

On Thu, Jul 13, 2023, Jakub Vaněk wrote:
> Hi Thinh,
>
> On Wed, 2023-07-12 at 22:55 +0000, Thinh Nguyen wrote:
> > On Thu, Jul 13, 2023, Jakub Vanek wrote:
> > > This reverts commit b138e23d3dff90c0494925b4c1874227b81bddf7.
> > >
> > > AutoRetry has been found to cause some issues. This feature allows
> > > the controller in host mode (further referred to as the xHC) to
> > > send
> > > non-terminating/burst retry ACKs (Retry=1 and Nump!=0) instead of
> > > terminating retry ACKs (Retry=1 and Nump=0) to devices when
> > > a transaction error occurs.
> > >
> > > Unfortunately, some USB devices fail to retry transactions when
> > > the xHC sends them a burst retry ACK. When this happens, the xHC
> >
> > For some clarity: if the device continues to respond with CRC error,
> > the xHC will not complete endpoint related commands while it keeps
> > autoretry.
>
> Acknowledged. Do you think it it would be better to respin this patch
> once more to include this in the changelog?
>

It'll be better to provide more detail. However, I'm fine whether you
want to send v3 with this or not.

Thanks,
Thinh

2023-07-14 13:26:13

by Jakub Vaněk

[permalink] [raw]
Subject: Re: [PATCH v2] Revert "usb: dwc3: core: Enable AutoRetry feature in the controller"

Hi Thinh,

On Fri, 2023-07-14 at 00:30 +0000, Thinh Nguyen wrote:
> On Thu, Jul 13, 2023, Jakub Vaněk wrote:
> > Hi Thinh,
> >
> > On Wed, 2023-07-12 at 22:55 +0000, Thinh Nguyen wrote:
> > > On Thu, Jul 13, 2023, Jakub Vanek wrote:
> > > > This reverts commit b138e23d3dff90c0494925b4c1874227b81bddf7.
> > > >
> > > > AutoRetry has been found to cause some issues. This feature
> > > > allows
> > > > the controller in host mode (further referred to as the xHC) to
> > > > send
> > > > non-terminating/burst retry ACKs (Retry=1 and Nump!=0) instead
> > > > of
> > > > terminating retry ACKs (Retry=1 and Nump=0) to devices when
> > > > a transaction error occurs.
> > > >
> > > > Unfortunately, some USB devices fail to retry transactions when
> > > > the xHC sends them a burst retry ACK. When this happens, the
> > > > xHC
> > >
> > > For some clarity: if the device continues to respond with CRC
> > > error,
> > > the xHC will not complete endpoint related commands while it
> > > keeps
> > > autoretry.
> >
> > Acknowledged. Do you think it it would be better to respin this
> > patch
> > once more to include this in the changelog?
> >
>
> It'll be better to provide more detail. However, I'm fine whether you
> want to send v3 with this or not.

The urge to scratch the itch was too strong :D I have sent a v3 in
https://lore.kernel.org/linux-usb/[email protected]/
with an updated changelog.

>
> Thanks,
> Thinh

Thanks,
Jakub