linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
To: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"balbi@kernel.org" <balbi@kernel.org>
Cc: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"lars@metafoo.de" <lars@metafoo.de>,
	"bigeasy@linutronix.de" <bigeasy@linutronix.de>,
	"m.olbrich@pengutronix.de" <m.olbrich@pengutronix.de>
Subject: Re: [PATCH][RESEND] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully
Date: Tue, 10 Mar 2020 13:22:52 +0000	[thread overview]
Message-ID: <5313c926109f39699e63342b4ff71102d3cfa495.camel@analog.com> (raw)
In-Reply-To: <87d0b1885e.fsf@kernel.org>

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
> 

  reply	other threads:[~2020-03-10 13:23 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-06 14:45 [PATCH] usb: dwc3: gadget: Handle dequeuing of non queued URB gracefully Alexandru Ardelean
2019-11-12 14:41 ` Michael Olbrich
2020-01-16 11:12   ` Ardelean, Alexandru
2020-01-16 13:05     ` Felipe Balbi
2020-01-16 13:24 ` [PATCH][RESEND] " Alexandru Ardelean
2020-01-30 12:02   ` Felipe Balbi
2020-03-10 13:22     ` Ardelean, Alexandru [this message]
2020-03-10 13:45       ` Lars-Peter Clausen
2020-03-10 14:07         ` Lars-Peter Clausen
2020-03-27  8:43           ` [PATCH] usb: dwc3: gadget: don't dequeue requests on already disabled endpoints Michael Grzeschik
2020-03-27  8:53             ` Sergei Shtylyov
2020-03-27 11:15               ` Andy Shevchenko
2020-03-27  9:14             ` Lars-Peter Clausen
2020-03-27 10:43               ` Michael Grzeschik
2020-03-27 10:55                 ` Lars-Peter Clausen
2020-03-28  8:27             ` Felipe Balbi
2020-03-29 19:02               ` Michael Grzeschik
2020-03-30  7:18                 ` Felipe Balbi
2020-03-30  8:25                   ` Michael Grzeschik
2020-03-30 10:06                     ` Felipe Balbi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5313c926109f39699e63342b4ff71102d3cfa495.camel@analog.com \
    --to=alexandru.ardelean@analog.com \
    --cc=balbi@kernel.org \
    --cc=bigeasy@linutronix.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=lars@metafoo.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=m.olbrich@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).