[PATCH 0/2] usb: musb: fix dropped packets

[PATCH 0/2] usb: musb: fix dropped packets

[Andrew Goodbody]

The musb driver can drop rx packets when heavily loaded. These two
patches address two issues that can cause this. Both issues arose
when an endpoint was reprogrammed. The first patch is a logic bug
that resulted in a shared_fifo in rx mode not having its state
cleared out. The second patch fixes a race condition caused by
not stopping the dedicated endpoint for bulk packets before
rotating its queue which allowed a packet to be recieved and then
thrown away.

Andrew Goodbody (2):
  usb: musb: Ensure rx reinit occurs for shared_fifo endpoints
  usb: musb: Stop bulk endpoint while queue is rotated

 drivers/usb/musb/musb_host.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH 1/2] usb: musb: Ensure rx reinit occurs for shared_fifo endpoints

[Andrew Goodbody]

shared_fifo endpoints would only get a previous tx state cleared
out, the rx state was only cleared for non shared_fifo endpoints
Change this so that the rx state is cleared for all endpoints.
This addresses an issue that resulted in rx packets being dropped
silently.

Signed-off-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>
Cc: stable@vger.kernel.org
---
 drivers/usb/musb/musb_host.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 2f8ad7f..30e0d65 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -594,14 +594,15 @@ musb_rx_reinit(struct musb *musb, struct musb_qh *qh, u8 epnum)
 		musb_writew(ep->regs, MUSB_TXCSR, 0);
 
 	/* scrub all previous state, clearing toggle */
-	} else {
-		csr = musb_readw(ep->regs, MUSB_RXCSR);
-		if (csr & MUSB_RXCSR_RXPKTRDY)
-			WARNING("rx%d, packet/%d ready?\n", ep->epnum,
-				musb_readw(ep->regs, MUSB_RXCOUNT));
-
-		musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
 	}
+	csr = musb_readw(ep->regs, MUSB_RXCSR);
+	if (csr & MUSB_RXCSR_RXPKTRDY) {
+		WARNING("rx%d, packet/%d ready?\n", ep->epnum,
+			musb_readw(ep->regs, MUSB_RXCOUNT));
+		urb_qh_dump(musb);
+	}
+
+	musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
 
 	/* target addr and (for multipoint) hub addr/port */
 	if (musb->is_multipoint) {
-- 
2.7.4

Re: [PATCH 1/2] usb: musb: Ensure rx reinit occurs for shared_fifo endpoints

[Sergei Shtylyov]

Hello.

On 05/20/2016 05:51 PM, Andrew Goodbody wrote:

> shared_fifo endpoints would only get a previous tx state cleared
> out, the rx state was only cleared for non shared_fifo endpoints
> Change this so that the rx state is cleared for all endpoints.
> This addresses an issue that resulted in rx packets being dropped
> silently.
>
> Signed-off-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/usb/musb/musb_host.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index 2f8ad7f..30e0d65 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -594,14 +594,15 @@ musb_rx_reinit(struct musb *musb, struct musb_qh *qh, u8 epnum)
>  		musb_writew(ep->regs, MUSB_TXCSR, 0);
>
>  	/* scrub all previous state, clearing toggle */
> -	} else {
> -		csr = musb_readw(ep->regs, MUSB_RXCSR);
> -		if (csr & MUSB_RXCSR_RXPKTRDY)
> -			WARNING("rx%d, packet/%d ready?\n", ep->epnum,
> -				musb_readw(ep->regs, MUSB_RXCOUNT));
> -
> -		musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
>  	}
> +	csr = musb_readw(ep->regs, MUSB_RXCSR);
> +	if (csr & MUSB_RXCSR_RXPKTRDY) {
> +		WARNING("rx%d, packet/%d ready?\n", ep->epnum,
> +			musb_readw(ep->regs, MUSB_RXCOUNT));
> +		urb_qh_dump(musb);

    I'm not seeing this function anywhere... debugging leftover?

> +	}
> +
> +	musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
>
>  	/* target addr and (for multipoint) hub addr/port */
>  	if (musb->is_multipoint) {

MBR, Sergei

RE: [PATCH 1/2] usb: musb: Ensure rx reinit occurs for shared_fifo endpoints

[Andrew Goodbody]

> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
> 
> Hello.
> 
> On 05/20/2016 05:51 PM, Andrew Goodbody wrote:
> 
> > shared_fifo endpoints would only get a previous tx state cleared out,
> > the rx state was only cleared for non shared_fifo endpoints Change
> > this so that the rx state is cleared for all endpoints.
> > This addresses an issue that resulted in rx packets being dropped
> > silently.
> >
> > Signed-off-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/usb/musb/musb_host.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/musb/musb_host.c
> > b/drivers/usb/musb/musb_host.c index 2f8ad7f..30e0d65 100644
> > --- a/drivers/usb/musb/musb_host.c
> > +++ b/drivers/usb/musb/musb_host.c
> > @@ -594,14 +594,15 @@ musb_rx_reinit(struct musb *musb, struct
> musb_qh *qh, u8 epnum)
> >  		musb_writew(ep->regs, MUSB_TXCSR, 0);
> >
> >  	/* scrub all previous state, clearing toggle */
> > -	} else {
> > -		csr = musb_readw(ep->regs, MUSB_RXCSR);
> > -		if (csr & MUSB_RXCSR_RXPKTRDY)
> > -			WARNING("rx%d, packet/%d ready?\n", ep-
> >epnum,
> > -				musb_readw(ep->regs, MUSB_RXCOUNT));
> > -
> > -		musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
> >  	}
> > +	csr = musb_readw(ep->regs, MUSB_RXCSR);
> > +	if (csr & MUSB_RXCSR_RXPKTRDY) {
> > +		WARNING("rx%d, packet/%d ready?\n", ep->epnum,
> > +			musb_readw(ep->regs, MUSB_RXCOUNT));
> > +		urb_qh_dump(musb);
> 
>     I'm not seeing this function anywhere... debugging leftover?

Sorry, yes, my bad. I'll remove it for v2.

Andrew

> > +	}
> > +
> > +	musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
> >
> >  	/* target addr and (for multipoint) hub addr/port */
> >  	if (musb->is_multipoint) {
> 
> MBR, Sergei

[PATCH 2/2] usb: musb: Stop bulk endpoint while queue is rotated

[Andrew Goodbody]

Ensure that the endpoint is stopped by clearing REQPKT before
clearing DATAERR_NAKTIMEOUT before rotating the queue on the
dedicated bulk endpoint.
This addresses an issue where a race could result in the endpoint
receiving data before it was reprogrammed resulting in a warning
about such data from musb_rx_reinit before it was thrown away.
The data thrown away was a valid packet that had been correctly
ACKed which meant the host and device got out of sync.

Signed-off-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>
Cc: stable@vger.kernel.org
---
 drivers/usb/musb/musb_host.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 30e0d65..777ff30 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -999,6 +999,8 @@ static void musb_bulk_nak_timeout(struct musb *musb, struct musb_hw_ep *ep,
 		/* clear nak timeout bit */
 		rx_csr = musb_readw(epio, MUSB_RXCSR);
 		rx_csr |= MUSB_RXCSR_H_WZC_BITS;
+		rx_csr &= ~MUSB_RXCSR_H_REQPKT;
+		musb_writew(epio, MUSB_RXCSR, rx_csr);
 		rx_csr &= ~MUSB_RXCSR_DATAERROR;
 		musb_writew(epio, MUSB_RXCSR, rx_csr);
 
-- 
2.7.4

Re: [PATCH 2/2] usb: musb: Stop bulk endpoint while queue is rotated

[Sergei Shtylyov]

On 05/20/2016 05:51 PM, Andrew Goodbody wrote:

> Ensure that the endpoint is stopped by clearing REQPKT before
> clearing DATAERR_NAKTIMEOUT before rotating the queue on the
> dedicated bulk endpoint.
> This addresses an issue where a race could result in the endpoint
> receiving data before it was reprogrammed resulting in a warning
> about such data from musb_rx_reinit before it was thrown away.
> The data thrown away was a valid packet that had been correctly
> ACKed which meant the host and device got out of sync.
>
> Signed-off-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/usb/musb/musb_host.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index 30e0d65..777ff30 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -999,6 +999,8 @@ static void musb_bulk_nak_timeout(struct musb *musb, struct musb_hw_ep *ep,
>  		/* clear nak timeout bit */
>  		rx_csr = musb_readw(epio, MUSB_RXCSR);
>  		rx_csr |= MUSB_RXCSR_H_WZC_BITS;
> +		rx_csr &= ~MUSB_RXCSR_H_REQPKT;
> +		musb_writew(epio, MUSB_RXCSR, rx_csr);
>  		rx_csr &= ~MUSB_RXCSR_DATAERROR;
>  		musb_writew(epio, MUSB_RXCSR, rx_csr);

    Can we not clear both in one write?

[...]

MBR, Sergei

RE: [PATCH 2/2] usb: musb: Stop bulk endpoint while queue is rotated

[Andrew Goodbody]

> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
> On 05/20/2016 05:51 PM, Andrew Goodbody wrote:
> 
> > Ensure that the endpoint is stopped by clearing REQPKT before clearing
> > DATAERR_NAKTIMEOUT before rotating the queue on the dedicated bulk
> > endpoint.
> > This addresses an issue where a race could result in the endpoint
> > receiving data before it was reprogrammed resulting in a warning about
> > such data from musb_rx_reinit before it was thrown away.
> > The data thrown away was a valid packet that had been correctly ACKed
> > which meant the host and device got out of sync.
> >
> > Signed-off-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/usb/musb/musb_host.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/usb/musb/musb_host.c
> > b/drivers/usb/musb/musb_host.c index 30e0d65..777ff30 100644
> > --- a/drivers/usb/musb/musb_host.c
> > +++ b/drivers/usb/musb/musb_host.c
> > @@ -999,6 +999,8 @@ static void musb_bulk_nak_timeout(struct musb
> *musb, struct musb_hw_ep *ep,
> >  		/* clear nak timeout bit */
> >  		rx_csr = musb_readw(epio, MUSB_RXCSR);
> >  		rx_csr |= MUSB_RXCSR_H_WZC_BITS;
> > +		rx_csr &= ~MUSB_RXCSR_H_REQPKT;
> > +		musb_writew(epio, MUSB_RXCSR, rx_csr);
> >  		rx_csr &= ~MUSB_RXCSR_DATAERROR;
> >  		musb_writew(epio, MUSB_RXCSR, rx_csr);
> 
>     Can we not clear both in one write?

Section 16.3.8.2.2.1.2 of the TRM says to clear REQPKT before DATAERR_NAKTIMEOUT.

Andrew

> [...]
> 
> MBR, Sergei

Re: [PATCH 2/2] usb: musb: Stop bulk endpoint while queue is rotated

[Sergei Shtylyov]

On 05/20/2016 08:06 PM, Andrew Goodbody wrote:

>>> Ensure that the endpoint is stopped by clearing REQPKT before clearing
>>> DATAERR_NAKTIMEOUT before rotating the queue on the dedicated bulk
>>> endpoint.
>>> This addresses an issue where a race could result in the endpoint
>>> receiving data before it was reprogrammed resulting in a warning about
>>> such data from musb_rx_reinit before it was thrown away.
>>> The data thrown away was a valid packet that had been correctly ACKed
>>> which meant the host and device got out of sync.
>>>
>>> Signed-off-by: Andrew Goodbody <andrew.goodbody@cambrionix.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  drivers/usb/musb/musb_host.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/usb/musb/musb_host.c
>>> b/drivers/usb/musb/musb_host.c index 30e0d65..777ff30 100644
>>> --- a/drivers/usb/musb/musb_host.c
>>> +++ b/drivers/usb/musb/musb_host.c
>>> @@ -999,6 +999,8 @@ static void musb_bulk_nak_timeout(struct musb
>> *musb, struct musb_hw_ep *ep,
>>>  		/* clear nak timeout bit */
>>>  		rx_csr = musb_readw(epio, MUSB_RXCSR);
>>>  		rx_csr |= MUSB_RXCSR_H_WZC_BITS;
>>> +		rx_csr &= ~MUSB_RXCSR_H_REQPKT;
>>> +		musb_writew(epio, MUSB_RXCSR, rx_csr);
>>>  		rx_csr &= ~MUSB_RXCSR_DATAERROR;
>>>  		musb_writew(epio, MUSB_RXCSR, rx_csr);
>>
>>     Can we not clear both in one write?
>
> Section 16.3.8.2.2.1.2 of the TRM says to clear REQPKT before DATAERR_NAKTIMEOUT.

    Right, the MUSB programmer's guide also says that. Then a comment wouldn't 
hurt here.

> Andrew

MBR, Sergei

RE: [PATCH 2/2] usb: musb: Stop bulk endpoint while queue is rotated

[Andrew Goodbody]

> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
> On 05/20/2016 08:06 PM, Andrew Goodbody wrote:
> 
> >>> Ensure that the endpoint is stopped by clearing REQPKT before
> >>> clearing DATAERR_NAKTIMEOUT before rotating the queue on the
> >>> dedicated bulk endpoint.
> >>> This addresses an issue where a race could result in the endpoint
> >>> receiving data before it was reprogrammed resulting in a warning
> >>> about such data from musb_rx_reinit before it was thrown away.
> >>> The data thrown away was a valid packet that had been correctly
> >>> ACKed which meant the host and device got out of sync.
> >>>
> >>> Signed-off-by: Andrew Goodbody
> <andrew.goodbody@cambrionix.com>
> >>> Cc: stable@vger.kernel.org
> >>> ---
> >>>  drivers/usb/musb/musb_host.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/usb/musb/musb_host.c
> >>> b/drivers/usb/musb/musb_host.c index 30e0d65..777ff30 100644
> >>> --- a/drivers/usb/musb/musb_host.c
> >>> +++ b/drivers/usb/musb/musb_host.c
> >>> @@ -999,6 +999,8 @@ static void musb_bulk_nak_timeout(struct musb
> >> *musb, struct musb_hw_ep *ep,
> >>>  		/* clear nak timeout bit */
> >>>  		rx_csr = musb_readw(epio, MUSB_RXCSR);
> >>>  		rx_csr |= MUSB_RXCSR_H_WZC_BITS;
> >>> +		rx_csr &= ~MUSB_RXCSR_H_REQPKT;
> >>> +		musb_writew(epio, MUSB_RXCSR, rx_csr);
> >>>  		rx_csr &= ~MUSB_RXCSR_DATAERROR;
> >>>  		musb_writew(epio, MUSB_RXCSR, rx_csr);
> >>
> >>     Can we not clear both in one write?
> >
> > Section 16.3.8.2.2.1.2 of the TRM says to clear REQPKT before
> DATAERR_NAKTIMEOUT.
> 
>     Right, the MUSB programmer's guide also says that. Then a comment
> wouldn't hurt here.

I'll add that for v2, thanks.

Andrew

> > Andrew
> 
> MBR, Sergei