Re: usb: host: Reduce xhci_handshake timeout in xhci_reset

[Pavan Kondeti]

Hi Greg,

On Fri, Feb 11, 2022 at 01:13:31PM +0530, Pavan Kondeti wrote:
> Sorry for the spam. I have added an incorrect email address in my previous
> email.
> 
> On Fri, Feb 11, 2022 at 12:16:30PM +0530, Pavan Kondeti wrote:
> > On Mon, Jun 28, 2021 at 10:49:00AM +0300, Mathias Nyman wrote:
> > > On 28.6.2021 9.55, Jung Daehwan wrote:
> > > > On Mon, Jun 28, 2021 at 08:53:02AM +0200, Greg Kroah-Hartman wrote:
> > > >> On Mon, Jun 28, 2021 at 11:25:48AM +0900, Jung Daehwan wrote:
> > > >>> On Tue, Jun 22, 2021 at 09:56:20PM +0200, Greg Kroah-Hartman wrote:
> > > >>>> On Tue, Jun 22, 2021 at 08:24:56PM +0900, Daehwan Jung wrote:
> > > >>>>> It seems 10 secs timeout is too long in general case. A core would wait for
> > > >>>>> 10 secs without doing other task and it can be happended on every device.
> > > >>>>
> > > >>>> Only if the handshake does not come back sooner, right?
> > > >>>
> > > >>> Yes, right.
> > > >>>
> > > >>>> What is causing your device to timeout here?
> > > >>>
> > > >>> Host Controller doesn't respond handshake. I don't know why and I ask HW team
> > > >>> to debug it.
> > > >>
> > > >> Please work to fix your hardware, that feels like the root of the
> > > >> problem here.  If you require the timeout for xhci_reset() to happen,
> > > >> then how do you know that the hardware really did reset properly in the
> > > >> reduced amount of time you just provided?
> > > >>
> > > > 
> > > > I continue fixing this issue with hardware engineer, but currently just
> > > > host controller can crash whole system and that's why I want to fix it.
> > > > How about adding some error logs in this situation for recognizing this issue?
> > > > We can add error log in xhci_stop as xhci_reset can returns error like below.
> > > > 
> > > > static void xhci_stop(struct usb_hcd *hcd)
> > > > {
> > > >         u32 temp;
> > > >         struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > > > +       int ret;
> > > > 
> > > >         mutex_lock(&xhci->mutex);
> > > > 
> > > > @@ -733,6 +734,9 @@ static void xhci_stop(struct usb_hcd *hcd)
> > > >         xhci->cmd_ring_state = CMD_RING_STATE_STOPPED;
> > > >         xhci_halt(xhci);
> > > >         xhci_reset(xhci);
> > > > +       if (ret)
> > > > +               xhci_err(xhci, "%s: Error while reset xhci Host controller - ret = %d\n"
> > > > +                       , __func__, ret);
> > > >         spin_unlock_irq(&xhci->lock);
> > > > 
> > > 
> > > We can check the xhci_reset() return value here and print a message, makes sense.
> > > 
> > > The original reason for the 10 second timeout was because a host actually took 9 seconds:
> > > 
> > > commit 22ceac191211cf6688b1bf6ecd93c8b6bf80ed9b
> > > 
> > >     xhci: Increase reset timeout for Renesas 720201 host.
> > >     
> > >     The NEC/Renesas 720201 xHCI host controller does not complete its reset
> > >     within 250 milliseconds.  In fact, it takes about 9 seconds to reset the
> > >     host controller, and 1 second for the host to be ready for doorbell
> > >     rings.  Extend the reset and CNR polling timeout to 10 seconds each.
> > > 
> > Agreed.
> > 
> > We also run into the similar issue (very very rarely reproduced) on
> > our platforms like SM8450. The issue happens when host mode is de-activated
> > (type-c cable disconnected). Since xhci_reset() is called with interrupts
> > disabled, a timeout of 10 seconds is fatal to the system.

Can you please consider including this change? Let us know if you want this
patch to be resent again with error message and Fixes tag included.

Thanks,
Pavan