* [PATCH V2 0/2] usb: musb: fix dropped packets
@ 2016-05-23 12:00 Andrew Goodbody
2016-05-23 12:00 ` [PATCH V2 1/2] usb: musb: Ensure rx reinit occurs for shared_fifo endpoints Andrew Goodbody
2016-05-23 12:00 ` [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated Andrew Goodbody
0 siblings, 2 replies; 9+ messages in thread
From: Andrew Goodbody @ 2016-05-23 12:00 UTC (permalink / raw)
To: b-liu; +Cc: gregkh, linux-usb, linux-kernel, 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.
V2 added a comment and removed debugging code
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 | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 1/2] usb: musb: Ensure rx reinit occurs for shared_fifo endpoints
2016-05-23 12:00 [PATCH V2 0/2] usb: musb: fix dropped packets Andrew Goodbody
@ 2016-05-23 12:00 ` Andrew Goodbody
2016-05-23 12:00 ` [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated Andrew Goodbody
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Goodbody @ 2016-05-23 12:00 UTC (permalink / raw)
To: b-liu; +Cc: gregkh, linux-usb, linux-kernel, Andrew Goodbody, stable
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
---
V2 removed debugging call
drivers/usb/musb/musb_host.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 2f8ad7f..2966596 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -594,14 +594,13 @@ 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));
+
+ musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
/* target addr and (for multipoint) hub addr/port */
if (musb->is_multipoint) {
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated
2016-05-23 12:00 [PATCH V2 0/2] usb: musb: fix dropped packets Andrew Goodbody
2016-05-23 12:00 ` [PATCH V2 1/2] usb: musb: Ensure rx reinit occurs for shared_fifo endpoints Andrew Goodbody
@ 2016-05-23 12:00 ` Andrew Goodbody
2016-05-23 12:38 ` Sergei Shtylyov
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Goodbody @ 2016-05-23 12:00 UTC (permalink / raw)
To: b-liu; +Cc: gregkh, linux-usb, linux-kernel, Andrew Goodbody, stable
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
---
V2 added comment about clearing REQPKT before DATAERR_NAKTIMEOUT
drivers/usb/musb/musb_host.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 2966596..676cb98 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -997,6 +997,12 @@ 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;
+ /*
+ * Need to stop the transaction by clearing REQPKT before
+ * DATAERR_NAKTIMEOUT ref TRM section 16.3.8.2.2.1.2
+ */
+ 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated
2016-05-23 12:00 ` [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated Andrew Goodbody
@ 2016-05-23 12:38 ` Sergei Shtylyov
2016-05-23 13:26 ` Andrew Goodbody
0 siblings, 1 reply; 9+ messages in thread
From: Sergei Shtylyov @ 2016-05-23 12:38 UTC (permalink / raw)
To: Andrew Goodbody, b-liu; +Cc: gregkh, linux-usb, linux-kernel, stable
Hello.
On 5/23/2016 3:00 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
> ---
> V2 added comment about clearing REQPKT before DATAERR_NAKTIMEOUT
>
> drivers/usb/musb/musb_host.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index 2966596..676cb98 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -997,6 +997,12 @@ 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;
> + /*
> + * Need to stop the transaction by clearing REQPKT before
Transcation? Maybe transfer?
> + * DATAERR_NAKTIMEOUT ref TRM section 16.3.8.2.2.1.2
Which TRM? Do you understand that the MUSB core is used by multiple SoCs?
I'd recommend referring just to the "abstract" manual or the MUSB programmer's
guide (section 9.2.2 if you want an exact ref.).
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated
2016-05-23 12:38 ` Sergei Shtylyov
@ 2016-05-23 13:26 ` Andrew Goodbody
2016-05-23 13:40 ` Sergei Shtylyov
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Goodbody @ 2016-05-23 13:26 UTC (permalink / raw)
To: Sergei Shtylyov, b-liu; +Cc: gregkh, linux-usb, linux-kernel, stable
> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
>
> Hello.
>
> On 5/23/2016 3:00 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
> > ---
> > V2 added comment about clearing REQPKT before DATAERR_NAKTIMEOUT
> >
> > drivers/usb/musb/musb_host.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/usb/musb/musb_host.c
> > b/drivers/usb/musb/musb_host.c index 2966596..676cb98 100644
> > --- a/drivers/usb/musb/musb_host.c
> > +++ b/drivers/usb/musb/musb_host.c
> > @@ -997,6 +997,12 @@ 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;
> > + /*
> > + * Need to stop the transaction by clearing REQPKT before
>
> Transcation? Maybe transfer?
The quote from the TRM is "If the DATAERR_NAKTIMEOUT bit is set, the controller can be directed either to continue trying this transaction (until it times out again) by clearing the DATAERR_NAKTIMEOUT bit or to abort the transaction by clearing REQPKT bit before clearing the DATAERR_NAKTIMEOUT bit."
So 'transaction' is correct.
> > + * DATAERR_NAKTIMEOUT ref TRM section 16.3.8.2.2.1.2
>
> Which TRM? Do you understand that the MUSB core is used by multiple
> SoCs?
> I'd recommend referring just to the "abstract" manual or the MUSB
> programmer's guide (section 9.2.2 if you want an exact ref.).
That would be the AM335x Sitara Processors TRM. I don't have the MUSB programmer's guide, is it available online somewhere? I do not understand what you mean by the "abstract" manual. Would you be OK with "ref MUSB Programmer's Guide section 9.2.2"
Andrew
> [...]
>
> MBR, Sergei
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated
2016-05-23 13:26 ` Andrew Goodbody
@ 2016-05-23 13:40 ` Sergei Shtylyov
2016-05-23 14:37 ` Sergei Shtylyov
0 siblings, 1 reply; 9+ messages in thread
From: Sergei Shtylyov @ 2016-05-23 13:40 UTC (permalink / raw)
To: Andrew Goodbody, b-liu; +Cc: gregkh, linux-usb, linux-kernel, stable
On 5/23/2016 4:26 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
>>> ---
>>> V2 added comment about clearing REQPKT before DATAERR_NAKTIMEOUT
>>>
>>> drivers/usb/musb/musb_host.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/usb/musb/musb_host.c
>>> b/drivers/usb/musb/musb_host.c index 2966596..676cb98 100644
>>> --- a/drivers/usb/musb/musb_host.c
>>> +++ b/drivers/usb/musb/musb_host.c
>>> @@ -997,6 +997,12 @@ 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;
>>> + /*
>>> + * Need to stop the transaction by clearing REQPKT before
>>
>> Transcation? Maybe transfer?
>
> The quote from the TRM is "If the DATAERR_NAKTIMEOUT bit is set, the controller can be directed either to continue trying this transaction (until it times out again) by clearing the DATAERR_NAKTIMEOUT bit or to abort the transaction by clearing REQPKT bit before clearing the DATAERR_NAKTIMEOUT bit."
> So 'transaction' is correct.
OK.
>>> + * DATAERR_NAKTIMEOUT ref TRM section 16.3.8.2.2.1.2
>>
>> Which TRM? Do you understand that the MUSB core is used by multiple
>> SoCs?
>> I'd recommend referring just to the "abstract" manual or the MUSB
>> programmer's guide (section 9.2.2 if you want an exact ref.).
> That would be the AM335x Sitara Processors TRM. I don't have the MUSB programmer's guide, is it available online somewhere?
AFAIK, no.
> I do not understand what you mean by the "abstract" manual.
Just saying "the manual" or "the manuals".
> Would you be OK with "ref MUSB Programmer's Guide section 9.2.2"
Yes. However, that spec calls the bit just "NAK Timeout" in this context,
not DATAERR_NAKTIMEOUT. Overall, this spec. uses CamelCase for the register
bit names (I recall dbrownell complaining over that and uppercase all the bit
names in my patches :-).
> Andrew
>
>> [...]
MBR, Sergei
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated
2016-05-23 13:40 ` Sergei Shtylyov
@ 2016-05-23 14:37 ` Sergei Shtylyov
2016-05-23 15:19 ` Andrew Goodbody
0 siblings, 1 reply; 9+ messages in thread
From: Sergei Shtylyov @ 2016-05-23 14:37 UTC (permalink / raw)
To: Andrew Goodbody, b-liu; +Cc: gregkh, linux-usb, linux-kernel, stable
On 05/23/2016 04:40 PM, Sergei Shtylyov 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
>>>> ---
>>>> V2 added comment about clearing REQPKT before DATAERR_NAKTIMEOUT
>>>>
>>>> drivers/usb/musb/musb_host.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/musb/musb_host.c
>>>> b/drivers/usb/musb/musb_host.c index 2966596..676cb98 100644
>>>> --- a/drivers/usb/musb/musb_host.c
>>>> +++ b/drivers/usb/musb/musb_host.c
>>>> @@ -997,6 +997,12 @@ 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;
>>>> + /*
>>>> + * Need to stop the transaction by clearing REQPKT before
>>>
>>> Transcation? Maybe transfer?
>>
>> The quote from the TRM is "If the DATAERR_NAKTIMEOUT bit is set, the
>> controller can be directed either to continue trying this transaction (until
>> it times out again) by clearing the DATAERR_NAKTIMEOUT bit or to abort the
>> transaction by clearing REQPKT bit before clearing the DATAERR_NAKTIMEOUT bit."
>> So 'transaction' is correct.
>
> OK.
>
>>>> + * DATAERR_NAKTIMEOUT ref TRM section 16.3.8.2.2.1.2
>>>
>>> Which TRM? Do you understand that the MUSB core is used by multiple
>>> SoCs?
>>> I'd recommend referring just to the "abstract" manual or the MUSB
>>> programmer's guide (section 9.2.2 if you want an exact ref.).
>
>> That would be the AM335x Sitara Processors TRM. I don't have the MUSB
>> programmer's guide, is it available online somewhere?
>
> AFAIK, no.
>
>> I do not understand what you mean by the "abstract" manual.
>
> Just saying "the manual" or "the manuals".
>
>> Would you be OK with "ref MUSB Programmer's Guide section 9.2.2"
>
> Yes.
The full document name is "MUSBMHDRC USB 2.0 HIGH-SPEED DUAL-ROLE
CONTROLLER Programmer’s Guide".
>> Andrew
>>
>>> [...]
MBR, Sergei
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated
2016-05-23 14:37 ` Sergei Shtylyov
@ 2016-05-23 15:19 ` Andrew Goodbody
2016-05-23 16:46 ` Sergei Shtylyov
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Goodbody @ 2016-05-23 15:19 UTC (permalink / raw)
To: Sergei Shtylyov, b-liu; +Cc: gregkh, linux-usb, linux-kernel, stable
> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
> On 05/23/2016 04:40 PM, Sergei Shtylyov 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
> >>>> ---
> >>>> V2 added comment about clearing REQPKT before
> DATAERR_NAKTIMEOUT
> >>>>
> >>>> drivers/usb/musb/musb_host.c | 6 ++++++
> >>>> 1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/drivers/usb/musb/musb_host.c
> >>>> b/drivers/usb/musb/musb_host.c index 2966596..676cb98 100644
> >>>> --- a/drivers/usb/musb/musb_host.c
> >>>> +++ b/drivers/usb/musb/musb_host.c
> >>>> @@ -997,6 +997,12 @@ 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;
> >>>> + /*
> >>>> + * Need to stop the transaction by clearing REQPKT before
> >>>
> >>> Transcation? Maybe transfer?
> >>
> >> The quote from the TRM is "If the DATAERR_NAKTIMEOUT bit is set, the
> >> controller can be directed either to continue trying this transaction
> >> (until it times out again) by clearing the DATAERR_NAKTIMEOUT bit or
> >> to abort the transaction by clearing REQPKT bit before clearing the
> DATAERR_NAKTIMEOUT bit."
> >> So 'transaction' is correct.
> >
> > OK.
> >
> >>>> + * DATAERR_NAKTIMEOUT ref TRM section 16.3.8.2.2.1.2
> >>>
> >>> Which TRM? Do you understand that the MUSB core is used by
> >>> multiple SoCs?
> >>> I'd recommend referring just to the "abstract" manual or the MUSB
> >>> programmer's guide (section 9.2.2 if you want an exact ref.).
> >
> >> That would be the AM335x Sitara Processors TRM. I don't have the MUSB
> >> programmer's guide, is it available online somewhere?
> >
> > AFAIK, no.
> >
> >> I do not understand what you mean by the "abstract" manual.
> >
> > Just saying "the manual" or "the manuals".
> >
> >> Would you be OK with "ref MUSB Programmer's Guide section 9.2.2"
> >
> > Yes.
>
> The full document name is "MUSBMHDRC USB 2.0 HIGH-SPEED DUAL-ROLE
> CONTROLLER Programmer's Guide".
I moved the comment up to replace the one at the top so the patch now looks like this
if (is_in) {
dma = is_dma_capable() ? ep->rx_channel : NULL;
- /* clear nak timeout bit */
+ /*
+ * Need to stop the transaction by clearing REQPKT first
+ * then the NAK Timeout bit ref MUSBMHDRC USB 2.0 HIGH-SPEED
+ * DUAL-ROLE CONTROLLER Programmer's Guide, section 9.2.2
+ */
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);
Are you OK with that now?
Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated
2016-05-23 15:19 ` Andrew Goodbody
@ 2016-05-23 16:46 ` Sergei Shtylyov
0 siblings, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2016-05-23 16:46 UTC (permalink / raw)
To: Andrew Goodbody, b-liu; +Cc: gregkh, linux-usb, linux-kernel, stable
On 05/23/2016 06:19 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
>>>>>> ---
>>>>>> V2 added comment about clearing REQPKT before
>> DATAERR_NAKTIMEOUT
>>>>>>
>>>>>> drivers/usb/musb/musb_host.c | 6 ++++++
>>>>>> 1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/musb/musb_host.c
>>>>>> b/drivers/usb/musb/musb_host.c index 2966596..676cb98 100644
>>>>>> --- a/drivers/usb/musb/musb_host.c
>>>>>> +++ b/drivers/usb/musb/musb_host.c
>>>>>> @@ -997,6 +997,12 @@ 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;
>>>>>> + /*
>>>>>> + * Need to stop the transaction by clearing REQPKT before
>>>>>
>>>>> Transcation? Maybe transfer?
>>>>
>>>> The quote from the TRM is "If the DATAERR_NAKTIMEOUT bit is set, the
>>>> controller can be directed either to continue trying this transaction
>>>> (until it times out again) by clearing the DATAERR_NAKTIMEOUT bit or
>>>> to abort the transaction by clearing REQPKT bit before clearing the
>> DATAERR_NAKTIMEOUT bit."
>>>> So 'transaction' is correct.
>>>
>>> OK.
>>>
>>>>>> + * DATAERR_NAKTIMEOUT ref TRM section 16.3.8.2.2.1.2
>>>>>
>>>>> Which TRM? Do you understand that the MUSB core is used by
>>>>> multiple SoCs?
>>>>> I'd recommend referring just to the "abstract" manual or the MUSB
>>>>> programmer's guide (section 9.2.2 if you want an exact ref.).
>>>
>>>> That would be the AM335x Sitara Processors TRM. I don't have the MUSB
>>>> programmer's guide, is it available online somewhere?
>>>
>>> AFAIK, no.
>>>
>>>> I do not understand what you mean by the "abstract" manual.
>>>
>>> Just saying "the manual" or "the manuals".
>>>
>>>> Would you be OK with "ref MUSB Programmer's Guide section 9.2.2"
>>>
>>> Yes.
>>
>> The full document name is "MUSBMHDRC USB 2.0 HIGH-SPEED DUAL-ROLE
>> CONTROLLER Programmer's Guide".
>
> I moved the comment up to replace the one at the top so the patch now looks like this
>
> if (is_in) {
> dma = is_dma_capable() ? ep->rx_channel : NULL;
>
> - /* clear nak timeout bit */
> + /*
> + * Need to stop the transaction by clearing REQPKT first
> + * then the NAK Timeout bit ref MUSBMHDRC USB 2.0 HIGH-SPEED
> + * DUAL-ROLE CONTROLLER Programmer's Guide, section 9.2.2
> + */
> 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);
>
>
> Are you OK with that now?
> Andrew
Yes, thank you.
MBR, Sergei
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-05-23 16:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-23 12:00 [PATCH V2 0/2] usb: musb: fix dropped packets Andrew Goodbody
2016-05-23 12:00 ` [PATCH V2 1/2] usb: musb: Ensure rx reinit occurs for shared_fifo endpoints Andrew Goodbody
2016-05-23 12:00 ` [PATCH V2 2/2] usb: musb: Stop bulk endpoint while queue is rotated Andrew Goodbody
2016-05-23 12:38 ` Sergei Shtylyov
2016-05-23 13:26 ` Andrew Goodbody
2016-05-23 13:40 ` Sergei Shtylyov
2016-05-23 14:37 ` Sergei Shtylyov
2016-05-23 15:19 ` Andrew Goodbody
2016-05-23 16:46 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).