Re: [PATCHv2] musb_host: fix lockup on rxcsr_h_error

From: Bin Liu <b-liu@ti.com>
To: Max Uvarov <muvarov@gmail.com>
Cc: <linux-kernel@vger.kernel.org>, <linux-usb@vger.kernel.org>,
	<gregkh@linuxfoundation.org>,
	<sergei.shtylyov@cogentembedded.com>
Subject: Re: [PATCHv2] musb_host: fix lockup on rxcsr_h_error
Date: Wed, 27 Apr 2016 10:46:20 -0500	[thread overview]
Message-ID: <20160427154620.GB5604@uda0271908> (raw)
In-Reply-To: <1461739918-9583-1-git-send-email-muvarov@gmail.com>

Hi,

On Wed, Apr 27, 2016 at 09:51:58AM +0300, Max Uvarov wrote:
> Fix soft lockup when resetting remote device attached
> to usb host. Configuration:
> pppd -> musb hub -> usb-serial -> gsm modem

I have heard a few reports similar to this symptom, but never been able
to reproduce it on my side.

> When gsm modem resets, musb rolls in incoming rx interrupts
> which does not give any time to other application as result
> it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR

Have you looked where exact place in the interrupt routine the execution
has stuck in?

> and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code
> for setting rx stall with MUSB_RXCSR_H_WZC_BITS.

MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures
MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in
musb_regs.h.

> 
> Signed-off-by: Max Uvarov <muvarov@gmail.com>
> ---
>  v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).
> 
>  drivers/usb/musb/musb_host.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index c3d5fc9..2d9aa78 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)

What kernel do you use? This line # is away off from upstream kernel.

>  
>  		/* stall; record URB status */
>  		status = -EPIPE;
> +	} else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
>  
> -	} else if (rx_csr & MUSB_RXCSR_H_ERROR) {
> -		dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
> -
> -		status = -EPROTO;
> -		musb_writeb(epio, MUSB_RXINTERVAL, 0);
> -
> -	} else if (rx_csr & MUSB_RXCSR_DATAERROR) {
> +		if (rx_csr & MUSB_RXCSR_H_ERROR) {
> +			status = -EPROTO;
> +			musb_writeb(epio, MUSB_RXINTERVAL, 0);
> +		}

Please help me to understand how this change fixes the issue. I see the
most effect of the change here is directly 'goto finish' so that 'done'
flag is not set, then musb_advance_schedule() is not called. Is this the
case or I missed other important pieces?

Thanks,
-Bin.

>  
>  		if (USB_ENDPOINT_XFER_ISOC != qh->type) {
>  			dev_dbg(musb->controller, "RX end %d NAK timeout\n", epnum);
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html