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://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/gadget.c#L1044
[2] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/gadget.c#L1923
[3] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/gadget.c#L216
drivers/usb/dwc3/gadget.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 578804dc29ca..bc1d93c56d82 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1988,13 +1988,17 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
*/
if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) {
+ int ret = 0;
+
if ((dep->flags & DWC3_EP_PENDING_REQUEST))
- return __dwc3_gadget_start_isoc(dep);
+ ret = __dwc3_gadget_start_isoc(dep);
- return 0;
+ dep->flags &= ~DWC3_EP_PENDING_REQUEST;
+ return ret;
}
}
+ dep->flags &= ~DWC3_EP_PENDING_REQUEST;
__dwc3_gadget_kick_transfer(dep);
return 0;
