[PATCH] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[PATCH] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

Trying to dequeue and URB that is currently not queued should be a no-op
and be handled gracefully.

Use the list field of the URB to indicate whether it is queued or not by
setting it to the empty list when it is not queued.

Handling this gracefully allows for race condition free synchronization
between the complete callback being called to to a completed transfer and
trying to call usb_ep_dequeue() at the same time.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/usb/dwc3/gadget.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a9aba716bf80..b500ec6b0aa8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -174,7 +174,7 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
 {
 	struct dwc3			*dwc = dep->dwc;
 
-	list_del(&req->list);
+	list_del_init(&req->list);
 	req->remaining = 0;
 	req->needs_extra_trb = false;
 
@@ -844,6 +844,7 @@ static struct usb_request *dwc3_gadget_ep_alloc_request(struct usb_ep *ep,
 	req->epnum	= dep->number;
 	req->dep	= dep;
 	req->status	= DWC3_REQUEST_STATUS_UNKNOWN;
+	INIT_LIST_HEAD(&req->list);
 
 	trace_dwc3_alloc_request(req);
 
@@ -1540,6 +1541,10 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
+	/* Not queued, nothing to do */
+	if (list_empty(&req->list))
+		goto out0;
+
 	list_for_each_entry(r, &dep->pending_list, list) {
 		if (r == req)
 			break;
-- 
2.20.1

Re: [PATCH] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[Michael Olbrich]

On Wed, Nov 06, 2019 at 04:45:53PM +0200, Alexandru Ardelean wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Trying to dequeue and URB that is currently not queued should be a no-op
> and be handled gracefully.
> 
> Use the list field of the URB to indicate whether it is queued or not by
> setting it to the empty list when it is not queued.
> 
> Handling this gracefully allows for race condition free synchronization
> between the complete callback being called to to a completed transfer and
> trying to call usb_ep_dequeue() at the same time.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Thanks, no more "dwc3 fe200000.usb: request 00000000cdd42e4a was not queued
to ep2in" messages with this patch applied.

Tested-by: Michael Olbrich <m.olbrich@pengutronix.de>

> ---
>  drivers/usb/dwc3/gadget.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a9aba716bf80..b500ec6b0aa8 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -174,7 +174,7 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
>  {
>  	struct dwc3			*dwc = dep->dwc;
>  
> -	list_del(&req->list);
> +	list_del_init(&req->list);
>  	req->remaining = 0;
>  	req->needs_extra_trb = false;
>  
> @@ -844,6 +844,7 @@ static struct usb_request *dwc3_gadget_ep_alloc_request(struct usb_ep *ep,
>  	req->epnum	= dep->number;
>  	req->dep	= dep;
>  	req->status	= DWC3_REQUEST_STATUS_UNKNOWN;
> +	INIT_LIST_HEAD(&req->list);
>  
>  	trace_dwc3_alloc_request(req);
>  
> @@ -1540,6 +1541,10 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  
> +	/* Not queued, nothing to do */
> +	if (list_empty(&req->list))
> +		goto out0;
> +
>  	list_for_each_entry(r, &dep->pending_list, list) {
>  		if (r == req)
>  			break;

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[Ardelean, Alexandru]

On Tue, 2019-11-12 at 15:41 +0100, Michael Olbrich wrote:
> [External]
> 
> On Wed, Nov 06, 2019 at 04:45:53PM +0200, Alexandru Ardelean wrote:
> > From: Lars-Peter Clausen <lars@metafoo.de>
> > 
> > Trying to dequeue and URB that is currently not queued should be a no-
> > op
> > and be handled gracefully.
> > 
> > Use the list field of the URB to indicate whether it is queued or not
> > by
> > setting it to the empty list when it is not queued.
> > 
> > Handling this gracefully allows for race condition free synchronization
> > between the complete callback being called to to a completed transfer
> > and
> > trying to call usb_ep_dequeue() at the same time.
> > 
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> Thanks, no more "dwc3 fe200000.usb: request 00000000cdd42e4a was not
> queued
> to ep2in" messages with this patch applied.
> 
> Tested-by: Michael Olbrich <m.olbrich@pengutronix.de>
> 

I thought I replied here, but I guess I never hit the Send button.
Many thanks @Michael for testing this.

I'd also use the opportunity to make this a patch-ping message.

Thanks
Alex

> > ---
> >  drivers/usb/dwc3/gadget.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index a9aba716bf80..b500ec6b0aa8 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -174,7 +174,7 @@ static void
> > dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
> >  {
> >  	struct dwc3			*dwc = dep->dwc;
> >  
> > -	list_del(&req->list);
> > +	list_del_init(&req->list);
> >  	req->remaining = 0;
> >  	req->needs_extra_trb = false;
> >  
> > @@ -844,6 +844,7 @@ static struct usb_request
> > *dwc3_gadget_ep_alloc_request(struct usb_ep *ep,
> >  	req->epnum	= dep->number;
> >  	req->dep	= dep;
> >  	req->status	= DWC3_REQUEST_STATUS_UNKNOWN;
> > +	INIT_LIST_HEAD(&req->list);
> >  
> >  	trace_dwc3_alloc_request(req);
> >  
> > @@ -1540,6 +1541,10 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep
> > *ep,
> >  
> >  	spin_lock_irqsave(&dwc->lock, flags);
> >  
> > +	/* Not queued, nothing to do */
> > +	if (list_empty(&req->list))
> > +		goto out0;
> > +
> >  	list_for_each_entry(r, &dep->pending_list, list) {
> >  		if (r == req)
> >  			break;

Re: [PATCH] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]


Hi,

"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> writes:
> On Tue, 2019-11-12 at 15:41 +0100, Michael Olbrich wrote:
>> [External]
>> 
>> On Wed, Nov 06, 2019 at 04:45:53PM +0200, Alexandru Ardelean wrote:
>> > From: Lars-Peter Clausen <lars@metafoo.de>
>> > 
>> > Trying to dequeue and URB that is currently not queued should be a no-
>> > op
>> > and be handled gracefully.
>> > 
>> > Use the list field of the URB to indicate whether it is queued or not
>> > by
>> > setting it to the empty list when it is not queued.
>> > 
>> > Handling this gracefully allows for race condition free synchronization
>> > between the complete callback being called to to a completed transfer
>> > and
>> > trying to call usb_ep_dequeue() at the same time.
>> > 
>> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>> 
>> Thanks, no more "dwc3 fe200000.usb: request 00000000cdd42e4a was not
>> queued
>> to ep2in" messages with this patch applied.
>> 
>> Tested-by: Michael Olbrich <m.olbrich@pengutronix.de>
>> 
>
> I thought I replied here, but I guess I never hit the Send button.
> Many thanks @Michael for testing this.
>
> I'd also use the opportunity to make this a patch-ping message.

https://lore.kernel.org/linux-usb/875zhd6pw0.fsf@kernel.org/T/#u

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH][RESEND] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

Trying to dequeue and URB that is currently not queued should be a no-op
and be handled gracefully.

Use the list field of the URB to indicate whether it is queued or not by
setting it to the empty list when it is not queued.

Handling this gracefully allows for race condition free synchronization
between the complete callback being called to to a completed transfer and
trying to call usb_ep_dequeue() at the same time.

Tested-by: Michael Olbrich <m.olbrich@pengutronix.de>
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

* Added Michael Olbrich's Tested-by tag
  https://lore.kernel.org/linux-usb/20191112144108.GA1859@pengutronix.de/

 drivers/usb/dwc3/gadget.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1b8014ab0b25..22a78eb41a5b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -177,7 +177,7 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
 {
 	struct dwc3			*dwc = dep->dwc;
 
-	list_del(&req->list);
+	list_del_init(&req->list);
 	req->remaining = 0;
 	req->needs_extra_trb = false;
 
@@ -847,6 +847,7 @@ static struct usb_request *dwc3_gadget_ep_alloc_request(struct usb_ep *ep,
 	req->epnum	= dep->number;
 	req->dep	= dep;
 	req->status	= DWC3_REQUEST_STATUS_UNKNOWN;
+	INIT_LIST_HEAD(&req->list);
 
 	trace_dwc3_alloc_request(req);
 
@@ -1549,6 +1550,10 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
+	/* Not queued, nothing to do */
+	if (list_empty(&req->list))
+		goto out0;
+
 	list_for_each_entry(r, &dep->pending_list, list) {
 		if (r == req)
 			break;
-- 
2.20.1

Re: [PATCH][RESEND] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]


Hi,

Alexandru Ardelean <alexandru.ardelean@analog.com> writes:

> From: Lars-Peter Clausen <lars@metafoo.de>
>
> Trying to dequeue and URB that is currently not queued should be a no-op
> and be handled gracefully.
>
> Use the list field of the URB to indicate whether it is queued or not by
> setting it to the empty list when it is not queued.
>
> Handling this gracefully allows for race condition free synchronization
> between the complete callback being called to to a completed transfer and
> trying to call usb_ep_dequeue() at the same time.

We need a little more information here. Can you further explain what
happens and how you caught this?

> Tested-by: Michael Olbrich <m.olbrich@pengutronix.de>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>
> * Added Michael Olbrich's Tested-by tag
>   https://lore.kernel.org/linux-usb/20191112144108.GA1859@pengutronix.de/
>
>  drivers/usb/dwc3/gadget.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 1b8014ab0b25..22a78eb41a5b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -177,7 +177,7 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
>  {
>  	struct dwc3			*dwc = dep->dwc;
>  
> -	list_del(&req->list);
> +	list_del_init(&req->list);

this should *not* be necessary. Neither the INIT_LIST_HEAD() below.

>  	req->remaining = 0;
>  	req->needs_extra_trb = false;
>  
> @@ -847,6 +847,7 @@ static struct usb_request *dwc3_gadget_ep_alloc_request(struct usb_ep *ep,
>  	req->epnum	= dep->number;
>  	req->dep	= dep;
>  	req->status	= DWC3_REQUEST_STATUS_UNKNOWN;
> +	INIT_LIST_HEAD(&req->list);
>  
>  	trace_dwc3_alloc_request(req);
>  
> @@ -1549,6 +1550,10 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  
> +	/* Not queued, nothing to do */
> +	if (list_empty(&req->list))
> +		goto out0;

The loop below is actually looking for the request in our lists. You
just made the entire loop below unnecessary, but you didn't change it
accordingly. Moreover, I think that a user dequeueing a request that
wasn't queued for the current endpoint indicates a possible bug in the
gadget driver which needs to be fixed.

If you really disagree, suffice to change "ret = -EINVAL;" to "ret =
0;" and you would get what you want, without any of the extra cruft.

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH][RESEND] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[Ardelean, Alexandru]

On Thu, 2020-01-30 at 14:02 +0200, Felipe Balbi wrote:
> [External]
> 
> 
> Hi,
> 
> Alexandru Ardelean <alexandru.ardelean@analog.com> writes:
> 
> > From: Lars-Peter Clausen <lars@metafoo.de>
> > 
> > Trying to dequeue and URB that is currently not queued should be a no-op
> > and be handled gracefully.
> > 
> > Use the list field of the URB to indicate whether it is queued or not by
> > setting it to the empty list when it is not queued.
> > 
> > Handling this gracefully allows for race condition free synchronization
> > between the complete callback being called to to a completed transfer and
> > trying to call usb_ep_dequeue() at the same time.
> 
> We need a little more information here. Can you further explain what
> happens and how you caught this?

Apologies for the delay [of this reply].
It's been a while since this patch was created, and it was on a 4.14 kernel.
Lars was trying to fix various crashes with USB DWC3 OTG + some Xilinx patches.
I did not track the status of the OTG stuff upstream. I think it's a lot of
patches in the Xilinx tree.

The context has changed from 4.14 [obviously], and there were many things that
could have influenced things.
I've been trying to RFC some of these patches now.
[ yeah I know: maybe I should have [probably] also added an RFC tag :) ]
Some of the patches [including this one] seemed to make sense, even outside of
the context of the crashes that were happening on 4.14.
Atm, we're at 4.19 and we don't see issues, but we still have this patch.
We may drop it and see what happens.
¯\_(ツ)_/¯

But in any case, it does require a bit more re-investigation.
Apologies for the noise that this patch created :)

> 
> > Tested-by: Michael Olbrich <m.olbrich@pengutronix.de>
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > 
> > * Added Michael Olbrich's Tested-by tag
> >   https://lore.kernel.org/linux-usb/20191112144108.GA1859@pengutronix.de/
> > 
> >  drivers/usb/dwc3/gadget.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 1b8014ab0b25..22a78eb41a5b 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -177,7 +177,7 @@ static void dwc3_gadget_del_and_unmap_request(struct
> > dwc3_ep *dep,
> >  {
> >  	struct dwc3			*dwc = dep->dwc;
> >  
> > -	list_del(&req->list);
> > +	list_del_init(&req->list);
> 
> this should *not* be necessary. Neither the INIT_LIST_HEAD() below.
> 
> >  	req->remaining = 0;
> >  	req->needs_extra_trb = false;
> >  
> > @@ -847,6 +847,7 @@ static struct usb_request
> > *dwc3_gadget_ep_alloc_request(struct usb_ep *ep,
> >  	req->epnum	= dep->number;
> >  	req->dep	= dep;
> >  	req->status	= DWC3_REQUEST_STATUS_UNKNOWN;
> > +	INIT_LIST_HEAD(&req->list);
> >  
> >  	trace_dwc3_alloc_request(req);
> >  
> > @@ -1549,6 +1550,10 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
> >  
> >  	spin_lock_irqsave(&dwc->lock, flags);
> >  
> > +	/* Not queued, nothing to do */
> > +	if (list_empty(&req->list))
> > +		goto out0;
> 
> The loop below is actually looking for the request in our lists. You
> just made the entire loop below unnecessary, but you didn't change it
> accordingly. Moreover, I think that a user dequeueing a request that
> wasn't queued for the current endpoint indicates a possible bug in the
> gadget driver which needs to be fixed.
> 

Yeah, that could be.
Will see about reverting the patch on our end, and trying to track this again.

Thanks
Alex

> If you really disagree, suffice to change "ret = -EINVAL;" to "ret =
> 0;" and you would get what you want, without any of the extra cruft.
> 
> cheers
> 

Re: [PATCH][RESEND] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[Lars-Peter Clausen]

On 3/10/20 2:22 PM, Ardelean, Alexandru wrote:
> On Thu, 2020-01-30 at 14:02 +0200, Felipe Balbi wrote:
>> [External]
>>
>>
>> Hi,
>>
>> Alexandru Ardelean <alexandru.ardelean@analog.com> writes:
>>
>>> From: Lars-Peter Clausen <lars@metafoo.de>
>>>
>>> Trying to dequeue and URB that is currently not queued should be a no-op
>>> and be handled gracefully.
>>>
>>> Use the list field of the URB to indicate whether it is queued or not by
>>> setting it to the empty list when it is not queued.
>>>
>>> Handling this gracefully allows for race condition free synchronization
>>> between the complete callback being called to to a completed transfer and
>>> trying to call usb_ep_dequeue() at the same time.
>> We need a little more information here. Can you further explain what
>> happens and how you caught this?
> Apologies for the delay [of this reply].
> It's been a while since this patch was created, and it was on a 4.14 kernel.
> Lars was trying to fix various crashes with USB DWC3 OTG + some Xilinx patches.
> I did not track the status of the OTG stuff upstream. I think it's a lot of
> patches in the Xilinx tree.
>
> The context has changed from 4.14 [obviously], and there were many things that
> could have influenced things.
> I've been trying to RFC some of these patches now.
> [ yeah I know: maybe I should have [probably] also added an RFC tag :) ]
> Some of the patches [including this one] seemed to make sense, even outside of
> the context of the crashes that were happening on 4.14.
> Atm, we're at 4.19 and we don't see issues, but we still have this patch.
> We may drop it and see what happens.
> ¯\_(ツ)_/¯
>
> But in any case, it does require a bit more re-investigation.
> Apologies for the noise that this patch created :)

The race condition is between a gadget calling usb_ep_dequeue() and the 
driver completing the URB.

Lets say in a thread you have a reference to a in-flight URB and you 
want to abort the request, e.g. because the application that sent the 
request has been closed. But concurrently to that the URB is completed 
by the hardware and the interrupt fires and marks the URB as complete. 
Your thread is suspended while the interrupt is running, once the 
interrupt has finished the thread wakes up, still has the reference to 
the URB, but now it has been completed. The thread still calls 
usb_ep_dequeue() though and then undefined behavior occurs.

The context in which we observed the issue is when using function fs to 
create a userspace gadget and using aio_cancel() to abort a pending URB. 
But really any gadget that aborts a transfer before it is completed or 
before the timeout occurred can run into this issue.

- Lars

Re: [PATCH][RESEND] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

[Lars-Peter Clausen]

On 3/10/20 2:45 PM, Lars-Peter Clausen wrote:
> On 3/10/20 2:22 PM, Ardelean, Alexandru wrote:
>> On Thu, 2020-01-30 at 14:02 +0200, Felipe Balbi wrote:
>>> [External]
>>>
>>>
>>> Hi,
>>>
>>> Alexandru Ardelean <alexandru.ardelean@analog.com> writes:
>>>
>>>> From: Lars-Peter Clausen <lars@metafoo.de>
>>>>
>>>> Trying to dequeue and URB that is currently not queued should be a 
>>>> no-op
>>>> and be handled gracefully.
>>>>
>>>> Use the list field of the URB to indicate whether it is queued or 
>>>> not by
>>>> setting it to the empty list when it is not queued.
>>>>
>>>> Handling this gracefully allows for race condition free synchronization
>>>> between the complete callback being called to to a completed 
>>>> transfer and
>>>> trying to call usb_ep_dequeue() at the same time.
>>> We need a little more information here. Can you further explain what
>>> happens and how you caught this?
>> Apologies for the delay [of this reply].
>> It's been a while since this patch was created, and it was on a 4.14 
>> kernel.
>> Lars was trying to fix various crashes with USB DWC3 OTG + some Xilinx 
>> patches.
>> I did not track the status of the OTG stuff upstream. I think it's a 
>> lot of
>> patches in the Xilinx tree.
>>
>> The context has changed from 4.14 [obviously], and there were many 
>> things that
>> could have influenced things.
>> I've been trying to RFC some of these patches now.
>> [ yeah I know: maybe I should have [probably] also added an RFC tag :) ]
>> Some of the patches [including this one] seemed to make sense, even 
>> outside of
>> the context of the crashes that were happening on 4.14.
>> Atm, we're at 4.19 and we don't see issues, but we still have this patch.
>> We may drop it and see what happens.
>> ¯\_(ツ)_/¯
>>
>> But in any case, it does require a bit more re-investigation.
>> Apologies for the noise that this patch created :)
> 
> The race condition is between a gadget calling usb_ep_dequeue() and the 
> driver completing the URB.
> 
> Lets say in a thread you have a reference to a in-flight URB and you 
> want to abort the request, e.g. because the application that sent the 
> request has been closed. But concurrently to that the URB is completed 
> by the hardware and the interrupt fires and marks the URB as complete. 
> Your thread is suspended while the interrupt is running, once the 
> interrupt has finished the thread wakes up, still has the reference to 
> the URB, but now it has been completed. The thread still calls 
> usb_ep_dequeue() though and then undefined behavior occurs.
> 

Sorry, one quick correction. I believe the issue actually occurs when 
you have more than one CPU and the thread is not suspended, while the 
interrupt is running. In this case it is possible that the IRQ fires the 
driver marks the URB as complete, then unlocks, the driver lock and 
calls the complete callback, but before the complete callback runs the 
other thread calls usb_ep_dequeue(). There is no way to protect against 
this condition at the gadget level and it needs to be handled in the driver.

Basically

  CPU 1                         | CPU 2
--------------------------------------------------------------
URB IRQ fires                  |                             |
spin_lock(&dwc->lock);         |                             |
                                |                             |
Driver handles completed URB   |                             |
frees resources, etc           | spin_lock(&gadget->lock)    |
                                | usb_ep_dequeue()            |
spin_unlock(&dwc->lock);       |                             |
usb_gadget_giveback_request()  | spin_lock(&dwc->lock)       |
                                |                             |
Call compelte callback         | Try to free URB resources   |
                                | again => Undefined behavior |
                                |                             |
                                | spin_lock(&dwc->lock)       |
                                | spin_unlock(&gadget->lock)  |
spin_lock(&gadget->lock)       |                             |
                                |                             |
Mark URB as compelte in gadget |                             |
spin_unlock(&gadget->lock)     |                             |
spin_unlock(&dwc->lock)        |                             |
spin_lock(&dwc->lock)          |                             |
finish IRQ                     |                             |


> The context in which we observed the issue is when using function fs to 
> create a userspace gadget and using aio_cancel() to abort a pending URB. 
> But really any gadget that aborts a transfer before it is completed or 
> before the timeout occurred can run into this issue.
> 
> - Lars
> 

[PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Michael Grzeschik]

dwc3_gadget_ep_disable gets called before the last request gets
dequeued.

In __dwc3_gadget_ep_disable all started, pending and cancelled
lists for this endpoint will call dwc3_gadget_giveback in
dwc3_remove_requests.

After that no list containing the afterwards dequed request,
therefor it is not necessary to run the dequeue routine.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
@Lars-Peter Clausen:

This patch addresses the case that not queued requests get dequeued.
The only case that this happens seems on disabling the gadget.

 drivers/usb/dwc3/gadget.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9a6f741d1db0dc..5d4fa8d6c93e49 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1609,6 +1609,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 	trace_dwc3_ep_dequeue(req);
 
+	if (!(dep->flags & DWC3_EP_ENABLED))
+		return 0;
+
 	spin_lock_irqsave(&dwc->lock, flags);
 
 	list_for_each_entry(r, &dep->pending_list, list) {
-- 
2.26.0.rc2

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Sergei Shtylyov]

Hello!

On 27.03.2020 11:43, Michael Grzeschik wrote:

> dwc3_gadget_ep_disable gets called before the last request gets
> dequeued.
> 
> In __dwc3_gadget_ep_disable all started, pending and cancelled
> lists for this endpoint will call dwc3_gadget_giveback in
> dwc3_remove_requests.
> 
> After that no list containing the afterwards dequed request,

    Dequeued.

> therefor it is not necessary to run the dequeue routine.

    Therefore?

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
[...]

MBR, Sergei

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Andy Shevchenko]

On Fri, Mar 27, 2020 at 10:54 AM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 27.03.2020 11:43, Michael Grzeschik wrote:

...

> > therefor it is not necessary to run the dequeue routine.
>
>     Therefore?

Original as good on its own.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Lars-Peter Clausen]

On 3/27/20 9:43 AM, Michael Grzeschik wrote:
> dwc3_gadget_ep_disable gets called before the last request gets
> dequeued.
>
> In __dwc3_gadget_ep_disable all started, pending and cancelled
> lists for this endpoint will call dwc3_gadget_giveback in
> dwc3_remove_requests.
>
> After that no list containing the afterwards dequed request,
> therefor it is not necessary to run the dequeue routine.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> @Lars-Peter Clausen:
>
> This patch addresses the case that not queued requests get dequeued.
> The only case that this happens seems on disabling the gadget.


I don't believe it does. Calling usb_ep_dequeue() is not limited to be 
called after the endpoint has been disabled. It is part of the API and 
can be called at any time. E.g. with function_fs you can abort a queued 
transfer from userspace at any time.

- Lars

>
>   drivers/usb/dwc3/gadget.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 9a6f741d1db0dc..5d4fa8d6c93e49 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1609,6 +1609,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>   
>   	trace_dwc3_ep_dequeue(req);
>   
> +	if (!(dep->flags & DWC3_EP_ENABLED))
> +		return 0;
> +
>   	spin_lock_irqsave(&dwc->lock, flags);
>   
>   	list_for_each_entry(r, &dep->pending_list, list) {

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

On Fri, Mar 27, 2020 at 10:14:10AM +0100, Lars-Peter Clausen wrote:
> On 3/27/20 9:43 AM, Michael Grzeschik wrote:
> > dwc3_gadget_ep_disable gets called before the last request gets
> > dequeued.
> > 
> > In __dwc3_gadget_ep_disable all started, pending and cancelled
> > lists for this endpoint will call dwc3_gadget_giveback in
> > dwc3_remove_requests.
> > 
> > After that no list containing the afterwards dequed request,
> > therefor it is not necessary to run the dequeue routine.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> > @Lars-Peter Clausen:
> > 
> > This patch addresses the case that not queued requests get dequeued.
> > The only case that this happens seems on disabling the gadget.
> 
> 
> I don't believe it does. Calling usb_ep_dequeue() is not limited to be
> called after the endpoint has been disabled. It is part of the API and can
> be called at any time. E.g. with function_fs you can abort a queued transfer
> from userspace at any time.

OK, can you give me an Userspace example how to simply trigger the
issue. I tried to figure your mentioned function stack but it would
be much easier if it could be reproduced.

The patch on the other hand can stand on itself, as it then
fixes another code path that is much more common.

Regads,
Michael

> > 
> >   drivers/usb/dwc3/gadget.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 9a6f741d1db0dc..5d4fa8d6c93e49 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1609,6 +1609,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
> >   	trace_dwc3_ep_dequeue(req);
> > +	if (!(dep->flags & DWC3_EP_ENABLED))
> > +		return 0;
> > +
> >   	spin_lock_irqsave(&dwc->lock, flags);
> >   	list_for_each_entry(r, &dep->pending_list, list) {
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Lars-Peter Clausen]

On 3/27/20 11:43 AM, Michael Grzeschik wrote:
> On Fri, Mar 27, 2020 at 10:14:10AM +0100, Lars-Peter Clausen wrote:
>> On 3/27/20 9:43 AM, Michael Grzeschik wrote:
>>> dwc3_gadget_ep_disable gets called before the last request gets
>>> dequeued.
>>>
>>> In __dwc3_gadget_ep_disable all started, pending and cancelled
>>> lists for this endpoint will call dwc3_gadget_giveback in
>>> dwc3_remove_requests.
>>>
>>> After that no list containing the afterwards dequed request,
>>> therefor it is not necessary to run the dequeue routine.
>>>
>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>> ---
>>> @Lars-Peter Clausen:
>>>
>>> This patch addresses the case that not queued requests get dequeued.
>>> The only case that this happens seems on disabling the gadget.
>>
>> I don't believe it does. Calling usb_ep_dequeue() is not limited to be
>> called after the endpoint has been disabled. It is part of the API and can
>> be called at any time. E.g. with function_fs you can abort a queued transfer
>> from userspace at any time.
> OK, can you give me an Userspace example how to simply trigger the
> issue. I tried to figure your mentioned function stack but it would
> be much easier if it could be reproduced.
>
> The patch on the other hand can stand on itself, as it then
> fixes another code path that is much more common.

Hi,

I don't have a standalone example. But the issue occurs if you submit a 
request when using function_fs through the AIO API and then call 
io_cancel() to abort it. At the same time there must be traffic on the 
USB bus so that the URB has a chance to complete.

This is a race condition between the IRQ handler running on one CPU and 
the usb_ep_dequeue() running on another CPU. As such it might take a 
while of stress testing before it is triggered. The more CPUs your 
system has the higher the chance of trigger the issue.

You can find the code which triggers the issue below.

Submission of the request:

https://github.com/analogdevicesinc/libiio/blob/master/iiod/ops.c#L139-L156

Canceling it:

https://github.com/analogdevicesinc/libiio/blob/master/iiod/ops.c#L193

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]


Hi,

Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> dwc3_gadget_ep_disable gets called before the last request gets
> dequeued.
>
> In __dwc3_gadget_ep_disable all started, pending and cancelled
> lists for this endpoint will call dwc3_gadget_giveback in
> dwc3_remove_requests.
>
> After that no list containing the afterwards dequed request,
> therefor it is not necessary to run the dequeue routine.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> @Lars-Peter Clausen:
>
> This patch addresses the case that not queued requests get dequeued.
> The only case that this happens seems on disabling the gadget.
>
>  drivers/usb/dwc3/gadget.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 9a6f741d1db0dc..5d4fa8d6c93e49 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1609,6 +1609,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  
>  	trace_dwc3_ep_dequeue(req);
>  
> +	if (!(dep->flags & DWC3_EP_ENABLED))
> +		return 0;

which driver is trying to call dequeue after the endpoint is disabled?
Got some tracepoints of the problem happening?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 1979 bytes --]

On Sat, Mar 28, 2020 at 10:27:49AM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> > dwc3_gadget_ep_disable gets called before the last request gets
> > dequeued.
> >
> > In __dwc3_gadget_ep_disable all started, pending and cancelled
> > lists for this endpoint will call dwc3_gadget_giveback in
> > dwc3_remove_requests.
> >
> > After that no list containing the afterwards dequed request,
> > therefor it is not necessary to run the dequeue routine.
> >
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> > @Lars-Peter Clausen:
> >
> > This patch addresses the case that not queued requests get dequeued.
> > The only case that this happens seems on disabling the gadget.
> >
> >  drivers/usb/dwc3/gadget.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 9a6f741d1db0dc..5d4fa8d6c93e49 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1609,6 +1609,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
> >  
> >  	trace_dwc3_ep_dequeue(req);
> >  
> > +	if (!(dep->flags & DWC3_EP_ENABLED))
> > +		return 0;
> 
> which driver is trying to call dequeue after the endpoint is disabled?
> Got some tracepoints of the problem happening?

I see the case when using uvc-gadget.

Look into uvc_v4l2_release in uvc_v4l2.c:

uvc_function_disconnect
   composite_disconnect
      reset_config
         uvc_function_disable->usb_ep_disable

uvcg_video_enable
   usb_ep_dequeue
      dwc3_gadget_ep_dequeue

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]


Hi,

Michael Grzeschik <mgr@pengutronix.de> writes:
>> > dwc3_gadget_ep_disable gets called before the last request gets
>> > dequeued.
>> >
>> > In __dwc3_gadget_ep_disable all started, pending and cancelled
>> > lists for this endpoint will call dwc3_gadget_giveback in
>> > dwc3_remove_requests.
>> >
>> > After that no list containing the afterwards dequed request,
>> > therefor it is not necessary to run the dequeue routine.
>> >
>> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> > ---
>> > @Lars-Peter Clausen:
>> >
>> > This patch addresses the case that not queued requests get dequeued.
>> > The only case that this happens seems on disabling the gadget.
>> >
>> >  drivers/usb/dwc3/gadget.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> > index 9a6f741d1db0dc..5d4fa8d6c93e49 100644
>> > --- a/drivers/usb/dwc3/gadget.c
>> > +++ b/drivers/usb/dwc3/gadget.c
>> > @@ -1609,6 +1609,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>> >  
>> >  	trace_dwc3_ep_dequeue(req);
>> >  
>> > +	if (!(dep->flags & DWC3_EP_ENABLED))
>> > +		return 0;
>> 
>> which driver is trying to call dequeue after the endpoint is disabled?
>> Got some tracepoints of the problem happening?
>
> I see the case when using uvc-gadget.
>
> Look into uvc_v4l2_release in uvc_v4l2.c:
>
> uvc_function_disconnect
>    composite_disconnect
>       reset_config
>          uvc_function_disable->usb_ep_disable
>
> uvcg_video_enable
>    usb_ep_dequeue
>       dwc3_gadget_ep_dequeue

Oh, I see what you mean. We get a disconnect, which disables the
endpoints, which forces all requests to be dequeued. Now I remember why
this exists: we giveback the requests from disconnect because not all
gadget drivers will call usb_ep_dequeue() if simply told about the
disconnect. Then UDC drivers have to be a little more careful and make
sure that all requests are givenback.

In any case, why is it a problem to call usb_ep_dequeue()? Is it only
because of that dev_err()? We could just remove that message,
really. Eventually, I want to move more of this logic into UDC core so
udc drivers can be simplified. For that work, though, first we would
have to add a "generic" struct usb_ep_hw implementation and manage list
of requests as part of UDC core as well.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 3502 bytes --]

On Mon, Mar 30, 2020 at 10:18:57AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Michael Grzeschik <mgr@pengutronix.de> writes:
> >> > dwc3_gadget_ep_disable gets called before the last request gets
> >> > dequeued.
> >> >
> >> > In __dwc3_gadget_ep_disable all started, pending and cancelled
> >> > lists for this endpoint will call dwc3_gadget_giveback in
> >> > dwc3_remove_requests.
> >> >
> >> > After that no list containing the afterwards dequed request,
> >> > therefor it is not necessary to run the dequeue routine.
> >> >
> >> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> >> > ---
> >> > @Lars-Peter Clausen:
> >> >
> >> > This patch addresses the case that not queued requests get dequeued.
> >> > The only case that this happens seems on disabling the gadget.
> >> >
> >> >  drivers/usb/dwc3/gadget.c | 3 +++
> >> >  1 file changed, 3 insertions(+)
> >> >
> >> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >> > index 9a6f741d1db0dc..5d4fa8d6c93e49 100644
> >> > --- a/drivers/usb/dwc3/gadget.c
> >> > +++ b/drivers/usb/dwc3/gadget.c
> >> > @@ -1609,6 +1609,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
> >> >  
> >> >  	trace_dwc3_ep_dequeue(req);
> >> >  
> >> > +	if (!(dep->flags & DWC3_EP_ENABLED))
> >> > +		return 0;
> >> 
> >> which driver is trying to call dequeue after the endpoint is disabled?
> >> Got some tracepoints of the problem happening?
> >
> > I see the case when using uvc-gadget.
> >
> > Look into uvc_v4l2_release in uvc_v4l2.c:
> >
> > uvc_function_disconnect
> >    composite_disconnect
> >       reset_config
> >          uvc_function_disable->usb_ep_disable
> >
> > uvcg_video_enable
> >    usb_ep_dequeue
> >       dwc3_gadget_ep_dequeue
> 
> Oh, I see what you mean. We get a disconnect, which disables the
> endpoints, which forces all requests to be dequeued. Now I remember why
> this exists: we giveback the requests from disconnect because not all
> gadget drivers will call usb_ep_dequeue() if simply told about the
> disconnect. Then UDC drivers have to be a little more careful and make
> sure that all requests are givenback.
> 
> In any case, why is it a problem to call usb_ep_dequeue()? Is it only
> because of that dev_err()? We could just remove that message,
> really.

In my case, it is not a problem removing the dev_err. The ep_dequeue
will only be called once for each request at the stream end. I don't
know about the case Lars has mentioned.

If we have to search all lists for the request every n times while in
traffic, only to find out that it was not enqueued, I think it would be
worth it to keep the dev_err and let these cases trigger so we have an
option to find and avoid/fix these.

> Eventually, I want to move more of this logic into UDC core so
> udc drivers can be simplified. For that work, though, first we would
> have to add a "generic" struct usb_ep_hw implementation and manage list
> of requests as part of UDC core as well.

I don't know about the cases you plan to abstract but it sounds
like a good idea to get some gadget logic out of the drivers.

Thanks,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3899 bytes --]


Hi,

Michael Grzeschik <mgr@pengutronix.de> writes:
>> >> > dwc3_gadget_ep_disable gets called before the last request gets
>> >> > dequeued.
>> >> >
>> >> > In __dwc3_gadget_ep_disable all started, pending and cancelled
>> >> > lists for this endpoint will call dwc3_gadget_giveback in
>> >> > dwc3_remove_requests.
>> >> >
>> >> > After that no list containing the afterwards dequed request,
>> >> > therefor it is not necessary to run the dequeue routine.
>> >> >
>> >> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> >> > ---
>> >> > @Lars-Peter Clausen:
>> >> >
>> >> > This patch addresses the case that not queued requests get dequeued.
>> >> > The only case that this happens seems on disabling the gadget.
>> >> >
>> >> >  drivers/usb/dwc3/gadget.c | 3 +++
>> >> >  1 file changed, 3 insertions(+)
>> >> >
>> >> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> >> > index 9a6f741d1db0dc..5d4fa8d6c93e49 100644
>> >> > --- a/drivers/usb/dwc3/gadget.c
>> >> > +++ b/drivers/usb/dwc3/gadget.c
>> >> > @@ -1609,6 +1609,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>> >> >  
>> >> >  	trace_dwc3_ep_dequeue(req);
>> >> >  
>> >> > +	if (!(dep->flags & DWC3_EP_ENABLED))
>> >> > +		return 0;
>> >> 
>> >> which driver is trying to call dequeue after the endpoint is disabled?
>> >> Got some tracepoints of the problem happening?
>> >
>> > I see the case when using uvc-gadget.
>> >
>> > Look into uvc_v4l2_release in uvc_v4l2.c:
>> >
>> > uvc_function_disconnect
>> >    composite_disconnect
>> >       reset_config
>> >          uvc_function_disable->usb_ep_disable
>> >
>> > uvcg_video_enable
>> >    usb_ep_dequeue
>> >       dwc3_gadget_ep_dequeue
>> 
>> Oh, I see what you mean. We get a disconnect, which disables the
>> endpoints, which forces all requests to be dequeued. Now I remember why
>> this exists: we giveback the requests from disconnect because not all
>> gadget drivers will call usb_ep_dequeue() if simply told about the
>> disconnect. Then UDC drivers have to be a little more careful and make
>> sure that all requests are givenback.
>> 
>> In any case, why is it a problem to call usb_ep_dequeue()? Is it only
>> because of that dev_err()? We could just remove that message,
>> really.
>
> In my case, it is not a problem removing the dev_err. The ep_dequeue
> will only be called once for each request at the stream end. I don't
> know about the case Lars has mentioned.

Okay

> If we have to search all lists for the request every n times while in
> traffic, only to find out that it was not enqueued, I think it would be
> worth it to keep the dev_err and let these cases trigger so we have an
> option to find and avoid/fix these.

Yeah, I agree. That's why the dev_err() was placed there to start
with. In fact, I found a few gadget drivers which were trying to reuse a
request a allocated for EPxIN and queueing it to EPxOUT, clearly a
violation of request lifetime rules.

As for the search on three separate lists, I never considered this to be
a problem since it happens so infrequently. One thing we can do to make
it maybe faster, is convert those list_for_each_entry() to
list_for_each_entry_reverse(). I'm betting that there's higher
likelihood that the oldest request will be dequeued first, then again,
this needs to be profiled.

>> Eventually, I want to move more of this logic into UDC core so
>> udc drivers can be simplified. For that work, though, first we would
>> have to add a "generic" struct usb_ep_hw implementation and manage list
>> of requests as part of UDC core as well.
>
> I don't know about the cases you plan to abstract but it sounds
> like a good idea to get some gadget logic out of the drivers.

Yeah, this will take a lot of time, though. Hopefully it'll happen :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]