Hi Dan, On Wed, May 31, 2023, Dan Scally wrote: > Hi Thinh > > On 31/05/2023 09:55, Daniel Scally wrote: > > The DWC3_EP_PENDING_REQUEST flag is set against an endpoint when > > there are no pending or started requests available. This flag is > > cleared on queuing to the endpoint for endpoint 0, but not for any > > other endpoints. This can exacerbate timing problems by allowing a > > queue to go ahead for an isochronous endpoint that should not be > > started, so clear the flag upon a successful dwc3_gadget_ep_queue(). > > > > Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx> > > --- > > > Just wanted to give some background on the timing issues that this is > helping to relieve; we spotted this issue as a result of a "No resource for > ep1in" error being thrown occasionally during repeated stream on/off tests > for the UVC gadget on a platform using the DWC3; following the error stream > won't restart unless you reboot. That error occurs when the gadget's > workqueue function runs usb_ep_queue() whilst usb_ep_disable() is running > during stream off. The DWC3 gadget code's locking plus the nulling of the > endpoint descriptor during __dwc3_gadget_ep_disable() [1] and the check for > that situation in __dwc3_gadget_ep_queue() [2] should make that harmless, > but what occasionally happens is the dwc3_gadget_ep_queue() call sometimes > manages to grab the lock when it's briefly unlocked during > dwc3_gadget_giveback() [3]. That happens after the Stop Transfer command has > been sent, so __dwc3_gadget_ep_queue() running through triggers a Start > Transfer command, the dwc3_gadget_ep_disable() then finishes and stream > shuts down, but when it's started back up again another Start Transfer > command is sent and triggers the error. This patch ameliorates the impact of > that race in my case, because clearing the flag prevents > __dwc3_gadget_ep_queue() from running either __dwc3_gadget_start_isoc() or > __dwc3_gadget_kick_transfer() for a non started isoc endpoint - but the race > is still there. I think the potential for races is probably unavoidable > given the unlock, but I thought it was worth explaining what lead to the > patch in case it raises some issue that I'm missing. > > > Thanks > > Dan > > > [1] https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/gadget.c*L1044__;Iw!!A4F2R9G_pg!aqEKDZwHxzb0OYos1htFTiXTnPWcQbnNg8qezhCNJ7bVQP-Ewp4NDw2N_mijL02MRmoyOFj4dlO7vEg1NowmS4A3kjU6$ > [2] https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/gadget.c*L1923__;Iw!!A4F2R9G_pg!aqEKDZwHxzb0OYos1htFTiXTnPWcQbnNg8qezhCNJ7bVQP-Ewp4NDw2N_mijL02MRmoyOFj4dlO7vEg1NowmS7iHqZdg$ > [3] https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/gadget.c*L216__;Iw!!A4F2R9G_pg!aqEKDZwHxzb0OYos1htFTiXTnPWcQbnNg8qezhCNJ7bVQP-Ewp4NDw2N_mijL02MRmoyOFj4dlO7vEg1NowmS4WOrh98$ > Thanks for the detail background description. This helps a lot. The DWC3_EP_PENDING_REQUEST flag is actually only meant for ep0 and isoc endpoints. Regarding isoc endpoint, the gadget driver should prepare and queue isoc requests prior to the host requesting for it. Should that's not the case, this flag is set so that dwc3 can schedule transfer later when the are isoc requests are queued. The "No resource for ep1in" is expected in your case. The usb_ep_disable() API is documented that no other task may be using the endpoint when it's called. The UVC gadget driver is breaking this usage model when it tries to queue more request after calling usb_ep_disable(). Your patch will not resolve the issue you're trying to solve. The fix should be in the UVC gadget driver. Thanks, Thinh
