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

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

[Jakub Vanek]

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
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://lore.kernel.org/r/a21f34c04632d250cd0a78c7c6f4a1c9c7a43142.camel@gmail.com/ [1]
Link: https://lore.kernel.org/r/20230711214834.kyr6ulync32d4ktk@synopsys.com/ [2]
Cc: stable@vger.kernel.org
Cc: Mauro Ribeiro <mauro.ribeiro@hardkernel.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Jakub Vanek <linuxtardis@gmail.com>
---
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

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

[Thinh Nguyen]

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/a21f34c04632d250cd0a78c7c6f4a1c9c7a43142.camel@gmail.com/__;!!A4F2R9G_pg!aCe0isNmX63t6ILE2Tv2cX4UnrpDFo6LXWb6oS3-OYFYfX88igrfqmW4Z8UdO7sWz0mdco6vbBrR_KMzYaccpLF7Kw$  [1]
> Link: https://urldefense.com/v3/__https://lore.kernel.org/r/20230711214834.kyr6ulync32d4ktk@synopsys.com/__;!!A4F2R9G_pg!aCe0isNmX63t6ILE2Tv2cX4UnrpDFo6LXWb6oS3-OYFYfX88igrfqmW4Z8UdO7sWz0mdco6vbBrR_KMzYae75T8GCw$  [2]
> Cc: stable@vger.kernel.org
> Cc: Mauro Ribeiro <mauro.ribeiro@hardkernel.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Signed-off-by: Jakub Vanek <linuxtardis@gmail.com>
> ---
> 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 <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

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

[Jakub Vaněk]

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/a21f34c04632d250cd0a78c7c6f4a1c9c7a43142.camel@gmail.com/__;!!A4F2R9G_pg!aCe0isNmX63t6ILE2Tv2cX4UnrpDFo6LXWb6oS3-OYFYfX88igrfqmW4Z8UdO7sWz0mdco6vbBrR_KMzYaccpLF7Kw$
> >   [1]
> > Link:
> > https://urldefense.com/v3/__https://lore.kernel.org/r/20230711214834.kyr6ulync32d4ktk@synopsys.com/__;!!A4F2R9G_pg!aCe0isNmX63t6ILE2Tv2cX4UnrpDFo6LXWb6oS3-OYFYfX88igrfqmW4Z8UdO7sWz0mdco6vbBrR_KMzYae75T8GCw$
> >   [2]
> > Cc: stable@vger.kernel.org
> > Cc: Mauro Ribeiro <mauro.ribeiro@hardkernel.com>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > Signed-off-by: Jakub Vanek <linuxtardis@gmail.com>
> > ---
> > 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 <Thinh.Nguyen@synopsys.com>

Thanks!


> Thanks,
> Thinh

Best regards,
Jakub

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

[Thinh Nguyen]

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

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

[Jakub Vaněk]

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/20230714122419.27741-1-linuxtardis@gmail.com/
with an updated changelog.

> 
> Thanks,
> Thinh

Thanks,
Jakub