Re: [RFT PATCH] xhci: make xhci_handshake timeout for xhci_reset() adjustable

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 1.3.2022 6.03, Pavan Kondeti wrote:
> On Mon, Feb 28, 2022 at 05:49:49PM -0800, Jack Pham wrote:
>> Hi Mathias,
>>
>> On Thu, Feb 17, 2022 at 03:56:43PM +0200, Mathias Nyman wrote:
>>> xhci_reset() timeout was increased from 250ms to 10 seconds in order to
>>> give Renesas 720201 xHC enough time to get ready in probe.
>>
>> This suggests that the only place we really need the long timeout is
>> in xhci_gen_setup().
>>
>>> @@ -1163,7 +1161,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
>>>  		xhci_dbg(xhci, "Stop HCD\n");
>>>  		xhci_halt(xhci);
>>>  		xhci_zero_64b_regs(xhci);
>>> -		retval = xhci_reset(xhci);
>>> +		retval = xhci_reset(xhci, XHCI_RESET_LONG_USEC);
>>
>> Since preemption is disabled here (spin_lock_irq() is called near the
>> start of this function), shouldn't we also limit this to 250ms here in
>> xhci_resume() as well?
>>> The rationale of decreasing the timeout to 250 in certain places is based
> on the criticality of the operation but not on the preemption/irq state.
> Since xHC reset is critical in startup and resume, the 10 seconds timeout
> is enforced so that we don't break Renesas 720201 xHC.
> 
> Since all of our internl test reports indicate that the timeout is happening
> from stop hcd, this patch is helping.
> 

This was pretty much my reasoning as well.
I could add a comment about this to the commit message

In addition we want a targeted fix for a real world issue that we can send to
stable without changing too much, risking regressions.

I also think we should focus more on fixing the locking (preemption/irq state)
around xhci_reset() in xhci_resume() than tuning the timeout, but this needs more
thought and should be a separate patch for later.

Additionally I guess xhci_reset() is more likely to fail in xhci_stop() and
xhci_shutdown() as power or clocks for xHC may be disabled, or entire xHC removed from the
bus by then.   

Thanks
-Mathias