linux-kernel - Re: [PATCH v3] Revert "usb: dwc3: core: Enable AutoRetry feature in the controller"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230714213838.zil4c4wt3spbedvi@synopsys.com>
Date:   Fri, 14 Jul 2023 21:38:41 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Jakub Vanek <linuxtardis@...il.com>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Mauro Ribeiro <mauro.ribeiro@...dkernel.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH v3] Revert "usb: dwc3: core: Enable AutoRetry feature in
 the controller"

On Fri, Jul 14, 2023, Jakub Vanek wrote:
> This reverts commit b138e23d3dff90c0494925b4c1874227b81bddf7.
> 
> AutoRetry has been found to sometimes cause controller freezes when
> communicating with buggy USB devices.
> 
> This controller feature allows the controller in host mode to send
> non-terminating/burst retry ACKs instead of terminating retry ACKs
> to devices when a transaction error (CRC error or overflow) occurs.
> 
> Unfortunately, if the USB device continues to respond with a CRC error,
> the controller will not complete endpoint-related commands while it
> keeps trying to auto-retry. [3] The xHCI driver will notice this once
> it tries to abort the transfer using a Stop Endpoint command and
> does not receive a completion in time. [1]
> This situation is reported to 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]
> 
> Revert the enablement commit. This will keep the AutoRetry bit in
> the default state configured during SoC design [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!cQZmUozSCVry9wEn5TTVGljEo6B_nKIel0kWXsUQbaleJZwkOyrvc4FzWTlbfcUJ7hGz9V50rn0A2WMEceULIS9y9A$  [1]
> Link: https://urldefense.com/v3/__https://lore.kernel.org/r/20230711214834.kyr6ulync32d4ktk@synopsys.com/__;!!A4F2R9G_pg!cQZmUozSCVry9wEn5TTVGljEo6B_nKIel0kWXsUQbaleJZwkOyrvc4FzWTlbfcUJ7hGz9V50rn0A2WMEceXrbrG9iA$  [2]
> Link: https://urldefense.com/v3/__https://lore.kernel.org/r/20230712225518.2smu7wse6djc7l5o@synopsys.com/__;!!A4F2R9G_pg!cQZmUozSCVry9wEn5TTVGljEo6B_nKIel0kWXsUQbaleJZwkOyrvc4FzWTlbfcUJ7hGz9V50rn0A2WMEceWVJz8nuQ$  [3]
> Cc: stable@...r.kernel.org
> Cc: Mauro Ribeiro <mauro.ribeiro@...dkernel.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Suggested-by: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
> Signed-off-by: Jakub Vanek <linuxtardis@...il.com>
> ---
> V2 -> V3: Include more findings in changelog
> 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@...opsys.com>

Thanks,
Thinh