[PATCH] usb: dwc3: Increase no-IOC ENDTRANSFER delay

[PATCH] usb: dwc3: Increase no-IOC ENDTRANSFER delay

[Elson Serrao]

When ENDTRANSFER is issued without CMDIOC, software does not wait for
command completion and instead relies on a fixed delay before continuing
request teardown. The DWC3 programming guide recommends using a timeout
greater than 1 ms for command completion. Increase the post-ENDTRANSFER
delay to 2 ms to provide additional margin before request teardown. This
prevents SMMU faults that can occur when the controller accesses unmapped
requests before ENDTRANSFER processing has completed.

Fixes: d8a2bb4eb758 ("usb: dwc3: gadget: Add 1ms delay after end transfer command without IOC")
Cc: stable@vger.kernel.org
Signed-off-by: Elson Serrao <elson.serrao@oss.qualcomm.com>
---
The DWC3 programming guide recommends a delay greater than 1 ms for
ENDTRANSFER completion without CMDIOC, but does not specify a worst-case
completion time. The delay is increased to 2 ms to provide additional
margin based on observed failures.

If 2 ms is considered excessive, guidance on an appropriate worst-case
timeout would be appreciated.
---
 drivers/usb/dwc3/gadget.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a76f669a796c..a907fdfac8d7 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -445,7 +445,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
 
 	if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
 	    !(cmd & DWC3_DEPCMD_CMDIOC))
-		mdelay(1);
+		mdelay(2);
 
 	if (saved_config) {
 		reg = dwc3_readl(dwc, DWC3_GUSB2PHYCFG(0));
@@ -4158,7 +4158,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * returning from this function.
 	 *
 	 * This mode is NOT available on the DWC_usb31 IP.  In this
-	 * case, if the IOC bit is not set, then delay by 1ms
+	 * case, if the IOC bit is not set, then delay by 2ms
 	 * after issuing the EndTransfer command.  This allows for the
 	 * controller to handle the command completely before DWC3
 	 * remove requests attempts to unmap USB request buffers.
-- 
2.34.1

Re: [PATCH] usb: dwc3: Increase no-IOC ENDTRANSFER delay

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: dwc3: unupdated 1 ms ENDTRANSFER delay in dwc3_remove_requests()
--

commit fee87ffa001e1c9181771b93176db403ea8ec8d4
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

usb: dwc3: Increase no-IOC ENDTRANSFER delay

This commit increases the fixed delay after issuing an ENDTRANSFER command
without CMDIOC from 1 ms to 2 ms in dwc3_send_gadget_ep_cmd() to prevent
SMMU faults when unmapping requests during teardown.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a76f669a796c2..a907fdfac8d71 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -445,7 +445,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
>  
>  	if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
>  	    !(cmd & DWC3_DEPCMD_CMDIOC))
> -		mdelay(1);
> +		mdelay(2);

[Severity: High]
Should the companion mdelay(1) in dwc3_remove_requests() also be increased
to 2 ms?

When __dwc3_gadget_ep_disable() tears down requests, commit 1338410c21a5
("usb: dwc3: gadget: Wait for End Transfer completion on ep disable")
changed it to pass interrupt = true:

drivers/usb/dwc3/gadget.c:__dwc3_gadget_ep_disable()
	dwc3_remove_requests(dwc, dep, -ESHUTDOWN, true);

With interrupt = true, __dwc3_stop_active_transfer() issues ENDTRANSFER
with DWC3_DEPCMD_CMDIOC set and sets DWC3_EP_END_TRANSFER_PENDING, so
dwc3_send_gadget_ep_cmd() skips the !(cmd & DWC3_DEPCMD_CMDIOC) delay
above. Similarly, if dwc3_remove_requests() is called while an earlier
CMDIOC ENDTRANSFER (such as from dwc3_gadget_ep_dequeue()) is already
pending, dwc3_stop_active_transfer() returns early when it sees
DWC3_EP_END_TRANSFER_PENDING.

In both paths, on controllers without DWC3_GUCTL2_RST_ACTBITLATER (such as
DWC_usb31), dwc3_remove_requests() relies on its own delay before
synchronously unmapping request buffers via dwc3_gadget_giveback():

drivers/usb/dwc3/gadget.c:dwc3_remove_requests() {
	dwc3_stop_active_transfer(dep, false, interrupt);
	...
	/* Give the controller time to stop accessing the request buffers */
	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
		mdelay(1);

	/* - giveback all requests to gadget driver */
	while (!list_empty(&dep->started_list)) {
		req = next_request(&dep->started_list);

		dwc3_gadget_giveback(dep, req, status);
	}
	...
}

Leaving this delay at 1 ms means those teardown paths can still unmap DMA
buffers in started_list and cancelled_list before the controller finishes
processing ENDTRANSFER.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103182953.3437202-1-elson.serrao@oss.qualcomm.com?part=1