Re: OHCI root_port_reset() deadly loop...

[David Brownell <david-b@pacbell.net>]

> Regardless, here is a patch that hardens the OHCI reset handling
> loops so that they break out instead of hanging the entire system
> should this condition occur.  It's at least better than what the
> code does to a user right now which is hang the box completely:
>
> [USB] ohci: Do not hang the system if port reset does not complete.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> diff --git a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
> index bb9cc59..77ae5b4 100644
> --- a/drivers/usb/host/ohci-hub.c
> +++ b/drivers/usb/host/ohci-hub.c
> @@ -563,14 +563,19 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
>  	u32	temp;
>  	u16	now = ohci_readl(ohci, &ohci->regs->fmnumber);
>  	u16	reset_done = now + PORT_RESET_MSEC;
> +	int	limit_1;
>  
>  	/* build a "continuous enough" reset signal, with up to
>  	 * 3msec gap between pulses.  scheduler HZ==100 must work;
>  	 * this might need to be deadline-scheduled.
>  	 */
> -	do {
> +	limit_1 = 100;
> +	while (--limit_1 >= 0) {

Don't need this "limit_1" timeout; "reset_done" handles all
the timeout needed there.  The regs->fmnumber is essentially
a millisecond counter.


> +		int limit_2;
> +
>  		/* spin until any current reset finishes */
> -		for (;;) {
> +		limit_2 = PORT_RESET_MSEC * 2;

This is the loop that didn't terminate for you, right?
PORT_RESET_HW_MSEC is the ceiling you should use here,
not PORT_RESET_MSEC.


> +		while (--limit_2 >= 0) {
>  			temp = ohci_readl (ohci, portstat);
>  			/* handle e.g. CardBus eject */
>  			if (temp == ~(u32)0)
> @@ -579,6 +584,10 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
>  				break;
>  			udelay (500);
>  		}
> +		if (limit_2 < 0) {
> +			ohci_warn(ohci, "Root port inner-loop reset timeout, "
> +				  "portstat[%08x]\n", temp);
> +		}

What values do you see for "portstat"?

I suspect there will be some flag set which would allow a more
immediate exit from that loop.  RH_PS_CCS might clear, for example.

And in any case, if that fails I don't see any reason not to just
break, and return immediately.

>  
>  		if (!(temp & RH_PS_CCS))
>  			break;
> @@ -589,7 +598,14 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
>  		ohci_writel (ohci, RH_PS_PRS, portstat);
>  		msleep(PORT_RESET_HW_MSEC);
>  		now = ohci_readl(ohci, &ohci->regs->fmnumber);
> -	} while (tick_before(now, reset_done));
> +		if (!tick_before(now, reset_done))
> +			break;
> +	}
> +	if (limit_1 < 0) {
> +		ohci_warn(ohci, "Root port outer-loop reset timeout, "
> +			  "now[%04x] reset_done[%04x]\n",
> +			  now, reset_done);
> +	}
>  	/* caller synchronizes using PRSC */
>  
>  	return 0;
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/