Re: [PATCH v2] xhci: reduce xhci_handshake timeout in xhci_reset

[Pavan Kondeti]

On Mon, Feb 14, 2022 at 01:29:12PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Feb 14, 2022 at 05:50:16PM +0530, Pavankumar Kondeti wrote:
> > From: Daehwan Jung <dh10.jung@xxxxxxxxxxx>
> > 
> > xhci_reset() is called with interrupts disabled. Waiting 10 seconds for
> > controller reset and controller ready operations can be fatal to the
> > system when controller is timed out. Reduce the timeout to 1 second
> > and print a error message when the time out happens.
> > 
> > Fixes: 22ceac191211 ("xhci: Increase reset timeout for Renesas 720201 host.")
> > Signed-off-by: Daehwan Jung <dh10.jung@xxxxxxxxxxx>
> > Signed-off-by: Pavankumar Kondeti <quic_pkondeti@xxxxxxxxxxx>
> > ---
> > 
> > v2:
> > - Add error print statements in the code that change log refers to
> > 
> >  drivers/usb/host/xhci.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index dc357ca..bb9ea3f 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -196,9 +196,11 @@ int xhci_reset(struct xhci_hcd *xhci)
> >  		udelay(1000);
> >  
> >  	ret = xhci_handshake(&xhci->op_regs->command,
> > -			CMD_RESET, 0, 10 * 1000 * 1000);
> > -	if (ret)
> > +			CMD_RESET, 0, 1 * 1000 * 1000);
> > +	if (ret) {
> > +		xhci_err(xhci, "Host controller reset timed out\n");
> 
> A timeout is not the only error that could have happened here.  So why
> claim that all errors are timeout errors?

Thanks for pointing this out. xhci_handshake() can return an error code
other than -ETIMEDOUT. ret == -ETIMEDOUT check needs to be added or
just print the ret error in the print message.
> 
> How did you test this?

This is a hard to reproduce issue. So I have hacked the code to use 1 usec
instead of 1 sec as timeout and see running into the timeout issue. I know
this is not a perfect test. The test patch is included below.

Thanks,
Pavan


diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e95a5bc..6147544 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -37,6 +37,9 @@ static unsigned long long quirks;
 module_param(quirks, ullong, S_IRUGO);
 MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default");
 
+static int reset_error;
+module_param(reset_error, int, S_IRUGO | S_IWUSR);
+
 static bool td_on_ring(struct xhci_td *td, struct xhci_ring *ring)
 {
 	struct xhci_segment *seg = ring->first_seg;
@@ -195,8 +198,15 @@ int xhci_reset(struct xhci_hcd *xhci)
 	if (xhci->quirks & XHCI_INTEL_HOST)
 		udelay(1000);
 
-	ret = xhci_handshake(&xhci->op_regs->command,
-			CMD_RESET, 0, 1 * 1000 * 1000);
+	if (reset_error) {
+		xhci_err(xhci, "forcing timeout\n");
+		ret = xhci_handshake(&xhci->op_regs->command,
+				CMD_RESET, 0, 1); /* 1 usec */
+	} else {
+		ret = xhci_handshake(&xhci->op_regs->command,
+				CMD_RESET, 0, 1 * 1000 * 1000);
+	}
+
 	if (ret) {
 		xhci_err(xhci, "Host controller reset timed out\n");
 		return ret;