diff mbox series

usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully

Message ID 20191106144553.16956-1-alexandru.ardelean@analog.com (mailing list archive)
State New, archived
Headers show
Series usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully | expand

Commit Message

Alexandru Ardelean Nov. 6, 2019, 2:45 p.m. UTC 
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(-)

Comments

Michael Olbrich Nov. 12, 2019, 2:41 p.m. UTC | #1 
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;
Alexandru Ardelean Jan. 16, 2020, 11:12 a.m. UTC | #2 
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;
Felipe Balbi Jan. 16, 2020, 1:05 p.m. UTC | #3 
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
diff mbox series

Patch

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;