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
