Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4246badbbe4c106f965cc8758849390effc48c36.camel@gmail.com>
Subject: Re: [PATCH] usb: dwc3: Disable AutoRetry controller feature for
 dwc_usb3 v2.00a
From:   Jakub =?iso-8859-2?Q?Van=ECk?= <linuxtardis@gmail.com>
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        Mauro Ribeiro <mauro.ribeiro@hardkernel.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Date:   Thu, 13 Jul 2023 00:51:41 +0200
In-Reply-To: <20230711214834.kyr6ulync32d4ktk@synopsys.com>
References: <20230630223638.2250-1-linuxtardis@gmail.com>
         <20230705224754.zmffebz2geg3bclh@synopsys.com>
         <d4576e776d49498dcc1f82ddf2b81b415e0c6e4b.camel@gmail.com>
         <20230711214834.kyr6ulync32d4ktk@synopsys.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu1 
MIME-Version: 1.0
Precedence: bulk

Hi Thinh,

On Tue, 2023-07-11 at 21:48 +0000, Thinh Nguyen wrote:
> Hi Jakub,
>=20
> On Thu, Jul 06, 2023, Jakub Van=C4=9Bk wrote:
> > On Wed, 2023-07-05 at 22:47 +0000, Thinh Nguyen wrote:
> > > Hi,
> > >=20
> > > On Sat, Jul 01, 2023, Jakub Vanek wrote:
> > > > AutoRetry has been found to cause issues in this controller
> > > > revision.
> > > > This feature allows the controller to send non-
> > > > terminating/burst retry
> > > > ACKs (Retry=3D1 and Nump!=3D0) as opposed to terminating retry ACKs
> > > > (Retry=3D1 and Nump=3D0) to devices when a transaction error
> > > > occurs.
> > > >=20
> > > > Unfortunately, some USB devices do not retry transfers when
> > > > the controller sends them a non-terminating retry ACK. After
> > > > the transfer
> > > > times out, the xHCI driver tries to resume normal operation of
> > > > the
> > > > controller by sending a Stop Endpoint command to it. However,
> > > > this
> > > > revision of the controller fails to respond to that command in
> > > > this
> > > > state and the xHCI driver therefore gives up. This is reported
> > > > via dmesg:
> > > >=20
> > > > [sda] tag#29 uas_eh_abort_handler 0 uas-tag 1 inflight: CMD IN
> > > > [sda] tag#29 CDB: opcode=3D0x28 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
> > > >=20
> > > > This problem has been observed on Odroid HC2. This board has
> > > > an integrated JMS578 USB3-to-SATA bridge. The above problem
> > > > could 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.
> > > >=20
> > > > Reverting b138e23d3dff ("usb: dwc3: core: Enable AutoRetry
> > > > feature in
> > > > the controller") stopped the issue from occurring on Odroid
> > > > HC2.
> > > > As this problem hasn't been reported on other devices, disable
> > > > AutoRetry just for the dwc_usb3 revision v2.00a that the board
> > > > SoC
> > > > (Exynos 5422) uses.
> > > >=20
> > > > Fixes: b138e23d3dff ("usb: dwc3: core: Enable AutoRetry feature
> > > > in the controller")
> > > > Link:
> > > > https://urldefense.com/v3/__https://lore.kernel.org/r/a21f34c04632d=
250cd0a78c7c6f4a1c9c7a43142.camel@gmail.com/__;!!A4F2R9G_pg!YWgF6oqfuVY6xst=
ZmroukjlrwAFEYEQE8uj_iUu4fd10mnJxEPG345k75dMLLdNFP8rT1leok-aPNkz_FuAJ1zxnmw=
$
> > > > =C2=A0
> > > > Link:
> > > > https://urldefense.com/v3/__https://forum.odroid.com/viewtopic.php?=
t=3D42630__;!!A4F2R9G_pg!YWgF6oqfuVY6xstZmroukjlrwAFEYEQE8uj_iUu4fd10mnJxEP=
G345k75dMLLdNFP8rT1leok-aPNkz_FuCzOGIVPA$
> > > > =C2=A0
> > > > 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>
> > > > ---
> > > > =C2=A0drivers/usb/dwc3/core.c | 10 ++++++++--
> > > > =C2=A01 file changed, 8 insertions(+), 2 deletions(-)
> > > >=20
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index d68958e151a7..d742fc882d7e 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -1218,9 +1218,15 @@ static int dwc3_core_init(struct dwc3
> > > > *dwc)
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Host mode on seeing transaction errors(CRC
> > > > errors or internal
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * overrun scenerios) on IN transfers to repl=
y
> > > > to the device
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * with a non-terminating retry ACK (i.e, an
> > > > ACK transcation
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 * packet with Retry=3D1 & Nump !=3D 0)
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 * packet with Retry=3D1 & Nump !=3D 0).
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 *
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 * Do not enable this for DWC_usb3 v2.00a. This
> > > > controller
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 * revision stops responding to Stop Endpoint
> > > > commands if
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 * a USB device does not retry after a non-
> > > > terminating retry
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 * ACK is sent to it.
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0reg |=3D DWC3_GUCTL_HSTINAUTORETRY;
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0if (!DWC3_VER_IS(DWC3, 200A))
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0reg=
 |=3D DWC3_GUCTL_HSTINAUTORETRY;
> > > > =C2=A0
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > > > --=20
> > > > 2.25.1
> > > >=20
> > >=20
> > > I brought up this inquiry internally. Please wait as I need to
> > > review
> > > this further. The handling for this may be different depending on
> > > the
> > > team's feedback.
> > >=20
> >=20
> > OK; feel free to contact me if I could be of any help.
> >=20
>=20
> Sorry for the delay. After discussion, this behavior is the same
> across
> different controller versions. The consensus is to disable this
> feature
> altoghether. There will not be any noticeable performance impact.
>=20
> Would you mind revise the patch to disable this feature for all?

Not at all, I have sent a new revision of the patch:
https://lore.kernel.org/linux-usb/20230712224037.24948-1-linuxtardis@gmail.=
com/

Reverting the commit altogether seemed preferable to me to updating the
revision-specific patch. However, if the other way would be better, I
can send an updated revision.

> Many thanks,
> Thinh

Thank you,
Jakub