Hi, Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> writes: > From: Lars-Peter Clausen <lars@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> > --- > > * Added Michael Olbrich's Tested-by tag > https://lore.kernel.org/linux-usb/20191112144108.GA1859@xxxxxxxxxxxxxx/ > > 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:
signature.asc
Description: PGP signature
