Re: [PATCH v2 1/2] xhci: Add a quirk for full reset on removal

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Hi Roy, Michał,

On Fri, May 16, 2025, Roy Luo wrote:
> > There's no state 0. Checking against that is odd. Couldn't we just use
> > xhci_handshake() equivalent instead?
> 
> Ok, I will change it in the next version.
> 
> On Thu, May 15, 2025 at 11:33 PM Michał Pecio <michal.pecio@gmail.com> wrote:
> >
> > On Thu, 15 May 2025 23:42:50 +0000, Thinh Nguyen wrote:
> > > In any case, this is basically a revert of this change:
> > > 6ccb83d6c497 ("usb: xhci: Implement xhci_handshake_check_state()
> > > helper")
> > >
> > > Can't we just revert or fix the above patch that causes a regression?
> >
> > Also note that 6ccb83d6c497 claimed to fix actual problems, so
> > disabling it on selected hardware could bring the old bug back:
> >
> > > In some situations where xhci removal happens parallel to
> > > xhci_handshake, we encounter a scenario where the xhci_handshake
> > > can't succeed, and it polls until timeout.
> > >
> > > If xhci_handshake runs until timeout it can on some platforms result
> > > in a long wait which might lead to a watchdog timeout.
> 
> On top of this, xhci_handshake_check_state(XHCI_STATE_REMOVING)
> is also used elsewhere like xhci_abort_cmd_ring(), so a simple revert is
> off the table. Commit 6ccb83d6c497 did not specify which platform and
> in what circumstance would xhci handshake timeout, adding a quirk for
> DWC3 seems to be the better option here.
> 

Regarding the commit 6ccb83d6c497, I'm assuming Udipto made the change
for Qcom platforms. Hi @Udipto, if you're reading this, please confirm.

Many of the Qcom platforms are using dwc3 controller. The change you
made here are affecting all the dwc3 DRD controllers, which has a good
chance to also impact the Qcom platforms.

> >
> > But on the other hand, xhci_handshake() has long timeouts because
> > the handshakes themselves can take a surprisingly long time (and
> > sometimes still succeed), so any reliance on handshake completing
> > before timeout is frankly a bug in itself.
> 
> This patch simply honors the contract between the software and
> hardware, allowing the handshake to complete. It doesn't assume the
> handshake will finish on time. If it times out, then it times out and
> returns a failure.
> 

As Michał pointed out, disregarding the xhci handshake timeout is not
proper. The change 6ccb83d6c497 seems to workaround some different
watchdog warning timeout instead of resolving the actual issue. The
watchdog timeout should not be less than the handshake timeout here.

BR,
Thinh