linux-kernel - Re: [PATCHv2] musb_host: fix lockup on rxcsr_h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160428143714.GA7222@uda0271908>
Date:	Thu, 28 Apr 2016 09:37:14 -0500
From:	Bin Liu <b-liu@...com>
To:	Maxim Uvarov <muvarov@...il.com>
CC:	<linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	<sergei.shtylyov@...entembedded.com>
Subject: Re: [PATCHv2] musb_host: fix lockup on rxcsr_h_error

Hi,

On Thu, Apr 28, 2016 at 09:51:37AM +0300, Maxim Uvarov wrote:

[snip]

> Hello Bin,
> 
> yes, it also works with that reset and go to finish:
> 
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index c3d5fc9..8cd98e7 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -1599,6 +1599,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
>                 status = -EPROTO;
>                 musb_writeb(epio, MUSB_RXINTERVAL, 0);
> 
> +               rx_csr &= ~MUSB_RXCSR_H_ERROR;
> +               musb_writew(epio, MUSB_RXCSR, rx_csr);
> +
> +               goto finish;
>         } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
> 
>                 if (USB_ENDPOINT_XFER_ISOC != qh->type) {
> 

Thanks for testing it.

> 
> That I think a key thing, which is done in other error. If that change
> is good for you than I'm also happy with it.

We need to understand why the controller keeps generating the same
interrupt to come out a proper fix.

I will take a look. But I can only use my spare time on this, so be
patient.

> 
> I also not sure if musb_writeb(epio, MUSB_RXINTERVAL, 0); is needed.
> In my case it's the same result with it and without it.
> In other scenarios might be reasonable...

It disables NAK timeout.

> 
> 
> > First of all, I don't like the idea of merging the two branches, it
> > makes the code ugly.
> 
> Yes, I don't like that function at all, it's too long and difficult to
> read if you first look on it first time. It will be good to split it
> on 3 small functions for each big if.

This particular function is not that hard to understand, but the driver
in general is messy. But I am not sure if anyone in the community can
refactory this driver. The community had some effort in the past to
clean up this driver, but it always broke usecases on different
platforms.

Regards,
-Bin.