Hi, Michael Grzeschik <mgr@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> >> >> > --- >> >> > @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:
signature.asc
Description: PGP signature
