[PATCH 0/7] usb: dwc3: gadget: Fix No Resource after End Transfer

[PATCH 0/7] usb: dwc3: gadget: Fix No Resource after End Transfer

[Thinh Nguyen]

The dwc3 driver can issue a Start Transfer command while a previous End
Transfer command on the same endpoint has not completed. The controller then
rejects it with "No Resource", and the endpoint stays unusable.

This happens in two cases:
 * On ep disable, the End Transfer command is issued without interrupt and its
   completion isn't tracked. A quick re-enable or queue to the endpoint can
   race with it.
 * A timed out End Transfer is treated as done, and the endpoint resource index
   is lost.

This series tracks the End Transfer completion on ep disable and on timeout. It
also avoids an unneeded no-op Start Transfer on non-stream endpoints, and
rejects requests queued to an endpoint that is being disabled. If the End
Transfer never completes, the driver recovers through the existing error
recovery, and stale endpoint flags are reset on controller start.

Thanks to Selvarasu for reporting and debugging the issue.

Link: https://lore.kernel.org/linux-usb/20260227121236.963-1-selvarasu.g@samsung.com/



Thinh Nguyen (7):
  usb: dwc3: gadget: Reject requests to a disabled endpoint
  usb: dwc3: gadget: Track a timed out End Transfer
  usb: dwc3: gadget: Clear delayed stop on End Transfer completion
  usb: dwc3: gadget: Issue no-op Start Transfer only for stream endpoints
  usb: dwc3: gadget: Wait for End Transfer completion on ep disable
  usb: dwc3: gadget: Reset endpoint flags on controller start
  usb: dwc3: gadget: Recover from End Transfer timeout

 drivers/usb/dwc3/core.h   |   4 +-
 drivers/usb/dwc3/ep0.c    |   2 +-
 drivers/usb/dwc3/gadget.c | 160 +++++++++++++++++++++++++-------------
 3 files changed, 109 insertions(+), 57 deletions(-)


base-commit: bdf12cde30830d46a8ef1ba93986356f1164b4f1
-- 
2.53.0

[PATCH 1/7] usb: dwc3: gadget: Reject requests to a disabled endpoint

[Thinh Nguyen]

__dwc3_gadget_ep_disable() releases the lock while giving back requests,
and DWC3_EP_ENABLED is only cleared afterward. A function driver can
queue a new request in this window, and the driver would start a
transfer on an endpoint that is already disabled in hardware.

Clear DWC3_EP_ENABLED before giving back the requests, and reject
requests queued to an endpoint that is not enabled.

Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ee837235630a..eb6666b7bb98 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1084,6 +1084,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
 	reg &= ~DWC3_DALEPENA_EP(dep->number);
 	dwc3_writel(dwc, DWC3_DALEPENA, reg);
 
+	dep->flags &= ~DWC3_EP_ENABLED;
+
 	dwc3_remove_requests(dwc, dep, -ESHUTDOWN);
 
 	dep->stream_capable = false;
@@ -1990,7 +1992,8 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 {
 	struct dwc3		*dwc = dep->dwc;
 
-	if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
+	if (!(dep->flags & DWC3_EP_ENABLED) || !dep->endpoint.desc ||
+	    !dwc->pullups_connected || !dwc->connected) {
 		dev_dbg(dwc->dev, "%s: can't queue to disabled endpoint\n",
 				dep->name);
 		return -ESHUTDOWN;
-- 
2.53.0

Re: [PATCH 1/7] usb: dwc3: gadget: Reject requests to a disabled endpoint

[sashiko-bot]

> __dwc3_gadget_ep_disable() releases the lock while giving back requests,
> and DWC3_EP_ENABLED is only cleared afterward. A function driver can
> queue a new request in this window, and the driver would start a
> transfer on an endpoint that is already disabled in hardware.
> 
> Clear DWC3_EP_ENABLED before giving back the requests, and reject
> requests queued to an endpoint that is not enabled.
> 
> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792542945.git.Thinh.Nguyen@synopsys.com?part=1

[PATCH 2/7] usb: dwc3: gadget: Track a timed out End Transfer

[Thinh Nguyen]

If an End Transfer command with CMDIOC times out, the driver clears the
resource index but keeps DWC3_EP_TRANSFER_STARTED without setting
DWC3_EP_END_TRANSFER_PENDING. The command is still outstanding, but a
later stop issues another End Transfer with resource index 0, and the
next Start Transfer fails with "No Resource".

Mark the timed out End Transfer as pending so that no other command is
issued to the endpoint until it completes. For any other failure, there
is no outstanding command, so clear DWC3_EP_TRANSFER_STARTED.

Fixes: e192cc7b5239 ("usb: dwc3: gadget: move cmd_endtransfer to extra function")
Cc: stable@vger.kernel.org
Reported-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
Closes: https://lore.kernel.org/linux-usb/3fdda532-8181-48e9-b58d-cad7289408e7@samsung.com/
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 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 eb6666b7bb98..df5407e75788 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1798,9 +1798,9 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
 
 	dep->resource_index = 0;
 
-	if (!interrupt)
+	if (!interrupt || (ret && ret != -ETIMEDOUT))
 		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
-	else if (!ret)
+	else
 		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
 
 	dep->flags &= ~DWC3_EP_DELAY_STOP;
-- 
2.53.0

Re: [PATCH 2/7] usb: dwc3: gadget: Track a timed out End Transfer

[sashiko-bot]

> If an End Transfer command with CMDIOC times out, the driver clears the
> resource index but keeps DWC3_EP_TRANSFER_STARTED without setting
> DWC3_EP_END_TRANSFER_PENDING. The command is still outstanding, but a
> later stop issues another End Transfer with resource index 0, and the
> next Start Transfer fails with "No Resource".
> 
> Mark the timed out End Transfer as pending so that no other command is
> issued to the endpoint until it completes. For any other failure, there
> is no outstanding command, so clear DWC3_EP_TRANSFER_STARTED.
> 
> Fixes: e192cc7b5239 ("usb: dwc3: gadget: move cmd_endtransfer to extra function")
> Cc: stable@vger.kernel.org
> Reported-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
> Closes: https://lore.kernel.org/linux-usb/3fdda532-8181-48e9-b58d-cad7289408e7@samsung.com/
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792542945.git.Thinh.Nguyen@synopsys.com?part=2

[PATCH 3/7] usb: dwc3: gadget: Clear delayed stop on End Transfer completion

[Thinh Nguyen]

When an End Transfer command times out outside of the SETUP phase, the
driver sets DWC3_EP_DELAY_STOP to retry it on the next SETUP phase. If
the command completes in the meantime, the flag is left set. Requests
queued to the endpoint are then deferred, and the retry may end a
transfer that is started later.

Clear DWC3_EP_DELAY_STOP when the End Transfer command completes.

Fixes: 4db0fbb60136 ("usb: dwc3: gadget: Don't delay End Transfer on delayed_status")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index df5407e75788..4de3b81cb678 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3875,6 +3875,7 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
 	if (dep->stream_capable)
 		dep->flags |= DWC3_EP_IGNORE_NEXT_NOSTREAM;
 
+	dep->flags &= ~DWC3_EP_DELAY_STOP;
 	dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 	dwc3_gadget_ep_cleanup_cancelled_requests(dep);
-- 
2.53.0

Re: [PATCH 3/7] usb: dwc3: gadget: Clear delayed stop on End Transfer completion

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: dwc3: gadget: skipped request cleanup on DWC3_EP_DELAY_STOP clear
--

commit f4ade355ebc7b629210edc8108aa9f5fb3f0bcc2
Author: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

usb: dwc3: gadget: Clear delayed stop on End Transfer completion

This commit clears the DWC3_EP_DELAY_STOP flag in
dwc3_gadget_endpoint_command_complete() when an End Transfer command
completes, preventing a later retry on the next SETUP phase from stopping a
newly started transfer.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index df5407e757884..4de3b81cb6781 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3875,6 +3875,7 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
>  	if (dep->stream_capable)
>  		dep->flags |= DWC3_EP_IGNORE_NEXT_NOSTREAM;
>  
> +	dep->flags &= ~DWC3_EP_DELAY_STOP;
>  	dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
>  	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>  	dwc3_gadget_ep_cleanup_cancelled_requests(dep);

[Severity: High]
Can clearing DWC3_EP_DELAY_STOP unconditionally here in
dwc3_gadget_endpoint_command_complete() in drivers/usb/dwc3/gadget.c skip
deferred request cleanup and leak usb_request objects (and their mapped DMA
buffers) if the endpoint was disabled or the controller disconnected while
DWC3_EP_DELAY_STOP was set?

In __dwc3_stop_active_transfer(), if dwc3_send_gadget_ep_cmd() with
DWC3_DEPCMD_CMDIOC returns -ETIMEDOUT while dep->dwc->ep0state !=
EP0_SETUP_PHASE (for example, while dwc->delayed_status is true in
dwc3_gadget_ep_dequeue(), __dwc3_gadget_ep_set_halt(),
__dwc3_gadget_kick_transfer(), or __dwc3_gadget_ep_disable() after commit
"usb: dwc3: gadget: Wait for End Transfer completion on ep disable", or via
__dwc3_gadget_start_isoc()), the driver sets DWC3_EP_DELAY_STOP and returns
0 without clearing DWC3_EP_TRANSFER_STARTED (and before scheduling error
recovery in commit "usb: dwc3: gadget: Recover from End Transfer timeout"),
while the End Transfer command remains in flight in hardware.

If __dwc3_gadget_ep_disable() or dwc3_gadget_soft_disconnect() then runs
before EP0 reaches EP0_SETUP_PHASE, dwc3_remove_requests() returns early
without giving back dep->started_list, dep->pending_list, or
dep->cancelled_list:

drivers/usb/dwc3/gadget.c:dwc3_remove_requests() {
	...
	dwc3_stop_active_transfer(dep, false, false);

	/* If endxfer is delayed, avoid unmapping requests */
	if (dep->flags & DWC3_EP_DELAY_STOP)
		return;
	...
}

And in __dwc3_gadget_ep_disable(), dep->endpoint.desc is cleared while
DWC3_EP_DELAY_STOP | DWC3_EP_TRANSFER_STARTED remain set:

drivers/usb/dwc3/gadget.c:__dwc3_gadget_ep_disable() {
	...
	if (dep->flags & DWC3_EP_DELAY_STOP)
		mask |= (DWC3_EP_DELAY_STOP | DWC3_EP_TRANSFER_STARTED);
	dep->flags &= mask;

	/* Clear out the ep descriptors for non-ep0 */
	if (dep->number > 1) {
		dep->endpoint.comp_desc = NULL;
		dep->endpoint.desc = NULL;
	}
	...
}

If the in-flight End Transfer command completes before dwc3_ep0_out_start()
runs, dwc3_gadget_endpoint_command_complete() now clears DWC3_EP_DELAY_STOP
and only calls dwc3_gadget_ep_cleanup_cancelled_requests(dep), which does
not clean up dep->started_list or dep->pending_list, and stops after the
first request in dep->cancelled_list when the endpoint is disabled:

drivers/usb/dwc3/gadget.c:dwc3_gadget_ep_cleanup_cancelled_requests() {
	...
		/*
		 * The endpoint is disabled, let the dwc3_remove_requests()
		 * handle the cleanup.
		 */
		if (!dep->endpoint.desc)
			break;
	}
}

Because DWC3_EP_DELAY_STOP has already been cleared, when
dwc3_ep0_out_start() later runs on entering EP0_SETUP_PHASE, it will skip
the endpoint instead of finishing the deferred dwc3_remove_requests()
cleanup:

drivers/usb/dwc3/ep0.c:dwc3_ep0_out_start() {
	...
		if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
			continue;

		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
		if (dwc->connected)
			dwc3_stop_active_transfer(dwc3_ep, false, true);
		else
			dwc3_remove_requests(dwc, dwc3_ep, -ESHUTDOWN);
	...
}

Additionally, in the dwc3_gadget_soft_disconnect() path where
!dwc->connected (while ep0state is still EP0_STATUS_PHASE after
dwc3_ep0_send_delayed_status() clears dwc->delayed_status),
dep->endpoint.desc is still non-NULL and DWC3_EP_ENABLED is still set. If
DWC3_EP_DELAY_START was set prior to disconnect, could
dwc3_gadget_endpoint_command_complete() also call
__dwc3_gadget_kick_transfer(dep) and start a new hardware transfer from
dep->pending_list while disconnecting?

Should dwc3_gadget_endpoint_command_complete() check whether the endpoint
was disabled (!dep->endpoint.desc) or disconnected (!dwc->connected) when
DWC3_EP_DELAY_STOP was set and invoke dwc3_remove_requests(dwc, dep,
-ESHUTDOWN) instead of kicking delayed transfers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792542945.git.Thinh.Nguyen@synopsys.com?part=3

[PATCH 4/7] usb: dwc3: gadget: Issue no-op Start Transfer only for stream endpoints

[Thinh Nguyen]

The function driver may re-enable an endpoint before the End Transfer
command issued on ep disable completes. The no-op Start Transfer issued
on ep enable then fails with "No Resource".

Only stream endpoints need the no-op Start Transfer to initiate a
stream. Other bulk and interrupt endpoints start their transfer when a
request is queued. Skip it for those endpoints, and move it to a helper
to be reused.

Fixes: a97ea994605e ("usb: dwc3: gadget: offset Start Transfer latency for bulk EPs")
Cc: stable@vger.kernel.org
Reported-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
Closes: https://lore.kernel.org/linux-usb/20260227121236.963-1-selvarasu.g@samsung.com/
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 97 ++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 47 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 4de3b81cb678..e68584f20fbd 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -906,6 +906,54 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)
 	return 0;
 }
 
+static int dwc3_gadget_ep_start_noop_transfer(struct dwc3_ep *dep)
+{
+	struct dwc3_gadget_ep_cmd_params params;
+	struct dwc3		*dwc = dep->dwc;
+	struct dwc3_trb		*trb;
+	dma_addr_t		trb_dma;
+	u32			cmd;
+	int			ret;
+
+	memset(&params, 0, sizeof(params));
+	trb = &dep->trb_pool[0];
+	trb_dma = dwc3_trb_dma_offset(dep, trb);
+
+	params.param0 = upper_32_bits(trb_dma);
+	params.param1 = lower_32_bits(trb_dma);
+
+	cmd = DWC3_DEPCMD_STARTTRANSFER;
+
+	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * For streams, at start, there maybe a race where the host primes the
+	 * endpoint before the function driver queues a request to initiate a
+	 * stream. In that case, the controller will not see the prime to
+	 * generate the ERDY and start stream. To workaround this, issue a
+	 * no-op TRB as normal, but end it immediately. As a result, when the
+	 * function driver queues the request, the next START_TRANSFER command
+	 * will cause the controller to generate an ERDY to initiate the
+	 * stream.
+	 */
+	dwc3_stop_active_transfer(dep, false, true);
+
+	/*
+	 * All stream eps will reinitiate stream on NoStream rejection.
+	 *
+	 * However, if the controller is capable of TXF_FLUSH_BYPASS, then IN
+	 * direction endpoints will automatically restart the stream without
+	 * the driver initiation.
+	 */
+	if (!dep->direction ||
+	    !(dwc->hwparams.hwparams9 & DWC3_GHWPARAMS9_DEV_TXF_FLUSH_BYPASS))
+		dep->flags |= DWC3_EP_FORCE_RESTART_STREAM;
+
+	return 0;
+}
+
 /**
  * __dwc3_gadget_ep_enable - initializes a hw endpoint
  * @dep: endpoint to be initialized
@@ -971,55 +1019,10 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 	 * Issue StartTransfer here with no-op TRB so we can always rely on No
 	 * Response Update Transfer command.
 	 */
-	if (usb_endpoint_xfer_bulk(desc) ||
-			usb_endpoint_xfer_int(desc)) {
-		struct dwc3_gadget_ep_cmd_params params;
-		struct dwc3_trb	*trb;
-		dma_addr_t trb_dma;
-		u32 cmd;
-
-		memset(&params, 0, sizeof(params));
-		trb = &dep->trb_pool[0];
-		trb_dma = dwc3_trb_dma_offset(dep, trb);
-
-		params.param0 = upper_32_bits(trb_dma);
-		params.param1 = lower_32_bits(trb_dma);
-
-		cmd = DWC3_DEPCMD_STARTTRANSFER;
-
-		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
+	if (dep->stream_capable) {
+		ret = dwc3_gadget_ep_start_noop_transfer(dep);
 		if (ret < 0)
 			return ret;
-
-		if (dep->stream_capable) {
-			/*
-			 * For streams, at start, there maybe a race where the
-			 * host primes the endpoint before the function driver
-			 * queues a request to initiate a stream. In that case,
-			 * the controller will not see the prime to generate the
-			 * ERDY and start stream. To workaround this, issue a
-			 * no-op TRB as normal, but end it immediately. As a
-			 * result, when the function driver queues the request,
-			 * the next START_TRANSFER command will cause the
-			 * controller to generate an ERDY to initiate the
-			 * stream.
-			 */
-			dwc3_stop_active_transfer(dep, false, true);
-
-			/*
-			 * All stream eps will reinitiate stream on NoStream
-			 * rejection.
-			 *
-			 * However, if the controller is capable of
-			 * TXF_FLUSH_BYPASS, then IN direction endpoints will
-			 * automatically restart the stream without the driver
-			 * initiation.
-			 */
-			if (!dep->direction ||
-			    !(dwc->hwparams.hwparams9 &
-			      DWC3_GHWPARAMS9_DEV_TXF_FLUSH_BYPASS))
-				dep->flags |= DWC3_EP_FORCE_RESTART_STREAM;
-		}
 	}
 
 out:
-- 
2.53.0

Re: [PATCH 4/7] usb: dwc3: gadget: Issue no-op Start Transfer only for stream endpoints

[sashiko-bot]

> The function driver may re-enable an endpoint before the End Transfer
> command issued on ep disable completes. The no-op Start Transfer issued
> on ep enable then fails with "No Resource".
> 
> Only stream endpoints need the no-op Start Transfer to initiate a
> stream. Other bulk and interrupt endpoints start their transfer when a
> request is queued. Skip it for those endpoints, and move it to a helper
> to be reused.
> 
> Fixes: a97ea994605e ("usb: dwc3: gadget: offset Start Transfer latency for bulk EPs")
> Cc: stable@vger.kernel.org
> Reported-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
> Closes: https://lore.kernel.org/linux-usb/20260227121236.963-1-selvarasu.g@samsung.com/
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792542945.git.Thinh.Nguyen@synopsys.com?part=4

[PATCH 5/7] usb: dwc3: gadget: Wait for End Transfer completion on ep disable

[Thinh Nguyen]

The End Transfer command on ep disable is issued without interrupt, and
the endpoint flags are cleared right after a fixed delay. The command
may not have completed by then. If the function driver re-enables the
endpoint and queues a request, the next Start Transfer fails with "No
Resource".

Issue the End Transfer command with interrupt on ep disable, and keep
DWC3_EP_END_TRANSFER_PENDING and DWC3_EP_TRANSFER_STARTED until the
command completes. Defer the no-op Start Transfer of a stream endpoint
re-enabled in the meantime to the command completion.

Fixes: c5353b225df9 ("usb: dwc3: gadget: don't enable interrupt when disabling endpoint")
Cc: stable@vger.kernel.org
Reported-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
Closes: https://lore.kernel.org/linux-usb/20260227121236.963-1-selvarasu.g@samsung.com/
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/core.h   |  4 +++-
 drivers/usb/dwc3/ep0.c    |  2 +-
 drivers/usb/dwc3/gadget.c | 42 ++++++++++++++++++++++++++++++++-------
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index f3dabb37f806..267c922ca11b 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -771,6 +771,7 @@ struct dwc3_ep {
 #define DWC3_EP_TXFIFO_RESIZED		BIT(12)
 #define DWC3_EP_DELAY_STOP             BIT(13)
 #define DWC3_EP_RESOURCE_ALLOCATED	BIT(14)
+#define DWC3_EP_PENDING_NOOP_START	BIT(15)
 
 	/* This last one is specific to EP0 */
 #define DWC3_EP0_DIR_IN			BIT(31)
@@ -1695,7 +1696,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
 int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
 		u32 param);
 void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc);
-void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status);
+void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status,
+			  bool interrupt);
 #else
 static inline int dwc3_gadget_init(struct dwc3 *dwc)
 { return 0; }
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 46a34c4a046b..cf5a47b71efc 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -315,7 +315,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
 		if (dwc->connected)
 			dwc3_stop_active_transfer(dwc3_ep, false, true);
 		else
-			dwc3_remove_requests(dwc, dwc3_ep, -ESHUTDOWN);
+			dwc3_remove_requests(dwc, dwc3_ep, -ESHUTDOWN, false);
 	}
 }
 
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e68584f20fbd..e531adc02a40 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1020,9 +1020,14 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 	 * Response Update Transfer command.
 	 */
 	if (dep->stream_capable) {
-		ret = dwc3_gadget_ep_start_noop_transfer(dep);
-		if (ret < 0)
-			return ret;
+		if (dep->flags & (DWC3_EP_END_TRANSFER_PENDING |
+				  DWC3_EP_DELAY_STOP)) {
+			dep->flags |= DWC3_EP_PENDING_NOOP_START;
+		} else {
+			ret = dwc3_gadget_ep_start_noop_transfer(dep);
+			if (ret < 0)
+				return ret;
+		}
 	}
 
 out:
@@ -1031,16 +1036,21 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 	return 0;
 }
 
-void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status)
+void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status,
+			  bool interrupt)
 {
 	struct dwc3_request		*req;
 
-	dwc3_stop_active_transfer(dep, false, false);
+	dwc3_stop_active_transfer(dep, false, interrupt);
 
 	/* If endxfer is delayed, avoid unmapping requests */
 	if (dep->flags & DWC3_EP_DELAY_STOP)
 		return;
 
+	/* 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);
@@ -1089,7 +1099,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
 
 	dep->flags &= ~DWC3_EP_ENABLED;
 
-	dwc3_remove_requests(dwc, dep, -ESHUTDOWN);
+	dwc3_remove_requests(dwc, dep, -ESHUTDOWN, true);
 
 	dep->stream_capable = false;
 	dep->type = 0;
@@ -1101,6 +1111,15 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
 	 */
 	if (dep->flags & DWC3_EP_DELAY_STOP)
 		mask |= (DWC3_EP_DELAY_STOP | DWC3_EP_TRANSFER_STARTED);
+
+	/*
+	 * The End Transfer command is still in progress.  Do not clear the
+	 * flags, so that the ep is only rearmed once the command completes.
+	 */
+	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
+		mask |= (DWC3_EP_END_TRANSFER_PENDING |
+			 DWC3_EP_TRANSFER_STARTED);
+
 	dep->flags &= mask;
 
 	/* Clear out the ep descriptors for non-ep0 */
@@ -2553,7 +2572,7 @@ static void dwc3_stop_active_transfers(struct dwc3 *dwc)
 		if (!dep)
 			continue;
 
-		dwc3_remove_requests(dwc, dep, -ESHUTDOWN);
+		dwc3_remove_requests(dwc, dep, -ESHUTDOWN, false);
 	}
 }
 
@@ -3901,7 +3920,16 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
 			dwc3_ep0_send_delayed_status(dwc);
 	}
 
+	if (dep->flags & DWC3_EP_PENDING_NOOP_START) {
+		dep->flags &= ~DWC3_EP_PENDING_NOOP_START;
+
+		if (!(dep->flags & DWC3_EP_DELAY_START) &&
+		    (dep->flags & DWC3_EP_ENABLED))
+			dwc3_gadget_ep_start_noop_transfer(dep);
+	}
+
 	if ((dep->flags & DWC3_EP_DELAY_START) &&
+	    (dep->flags & DWC3_EP_ENABLED) &&
 	    !usb_endpoint_xfer_isoc(dep->endpoint.desc))
 		__dwc3_gadget_kick_transfer(dep);
 
-- 
2.53.0

Re: [PATCH 5/7] usb: dwc3: gadget: Wait for End Transfer completion on ep disable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc3: gadget: missing DWC3_EP_FORCE_RESTART_STREAM on deferred noop start
- [Medium] usb: dwc3: gadget: broken NoStream handling on deferred stream ep enable
--

commit bcb5a08575e576634a58cb654feac5c3da3a21ed
Author: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

usb: dwc3: gadget: Wait for End Transfer completion on ep disable

This commit issues the End Transfer command with an interrupt when disabling
an endpoint and keeps DWC3_EP_END_TRANSFER_PENDING and
DWC3_EP_TRANSFER_STARTED set until the command completes. It also defers the
no-op Start Transfer for a re-enabled stream endpoint until the End Transfer
command completion interrupt is processed.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e68584f20fbde..e531adc02a401 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1020,9 +1020,14 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
>  	 * Response Update Transfer command.
>  	 */
>  	if (dep->stream_capable) {
> -		ret = dwc3_gadget_ep_start_noop_transfer(dep);
> -		if (ret < 0)
> -			return ret;
> +		if (dep->flags & (DWC3_EP_END_TRANSFER_PENDING |
> +				  DWC3_EP_DELAY_STOP)) {
> +			dep->flags |= DWC3_EP_PENDING_NOOP_START;
> +		} else {
> +			ret = dwc3_gadget_ep_start_noop_transfer(dep);
> +			if (ret < 0)
> +				return ret;
> +		}
>  	}

[Severity: High]
Can deferring dwc3_gadget_ep_start_noop_transfer() here leave
DWC3_EP_FORCE_RESTART_STREAM unset when a request is queued before the End
Transfer command completes?

In drivers/usb/dwc3/gadget.c, __dwc3_gadget_ep_disable() clears
DWC3_EP_FORCE_RESTART_STREAM when masking dep->flags, while keeping
DWC3_EP_END_TRANSFER_PENDING (or DWC3_EP_DELAY_STOP) and
DWC3_EP_TRANSFER_STARTED set.

When the function driver re-enables a stream-capable endpoint in
__dwc3_gadget_ep_enable() before DWC3_DEPEVT_EPCMDCMPLT is processed,
DWC3_EP_PENDING_NOOP_START is set instead of calling
dwc3_gadget_ep_start_noop_transfer(). However,
dwc3_gadget_ep_start_noop_transfer() is the only place that sets
DWC3_EP_FORCE_RESTART_STREAM:

	if (!dep->direction ||
	    !(dwc->hwparams.hwparams9 & DWC3_GHWPARAMS9_DEV_TXF_FLUSH_BYPASS))
		dep->flags |= DWC3_EP_FORCE_RESTART_STREAM;

If the function driver then queues a request via __dwc3_gadget_ep_queue()
before the End Transfer command completes, __dwc3_gadget_ep_queue() sees
DWC3_EP_END_TRANSFER_PENDING or DWC3_EP_DELAY_STOP and sets
DWC3_EP_DELAY_START.

When dwc3_gadget_endpoint_command_complete() later runs, it skips
dwc3_gadget_ep_start_noop_transfer() because DWC3_EP_DELAY_START is set, and
calls __dwc3_gadget_kick_transfer() directly. That leaves
DWC3_EP_FORCE_RESTART_STREAM unset on the endpoint, so
dwc3_gadget_endpoint_stream_event() will not schedule dep->nostream_work on
subsequent DEPEVT_STREAM_NOSTREAM events:

	case DEPEVT_STREAM_NOSTREAM:
		dep->flags &= ~DWC3_EP_STREAM_PRIMED;
		if (dep->flags & DWC3_EP_FORCE_RESTART_STREAM)
			queue_delayed_work(system_percpu_wq, &dep->nostream_work,
					   msecs_to_jiffies(100));
		break;

In addition, if that queued request is dequeued from dep->pending_list via
dwc3_gadget_ep_dequeue() before the End Transfer completes,
DWC3_EP_DELAY_START remains set while dep->pending_list is empty, so both
dwc3_gadget_ep_start_noop_transfer() and DWC3_DEPCMD_STARTTRANSFER in
__dwc3_gadget_kick_transfer() will be skipped.

[ ... ]

> @@ -1089,7 +1099,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  
>  	dep->flags &= ~DWC3_EP_ENABLED;
>  
> -	dwc3_remove_requests(dwc, dep, -ESHUTDOWN);
> +	dwc3_remove_requests(dwc, dep, -ESHUTDOWN, true);
>  
>  	dep->stream_capable = false;
>  	dep->type = 0;
> @@ -1101,6 +1111,15 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  	 */
>  	if (dep->flags & DWC3_EP_DELAY_STOP)
>  		mask |= (DWC3_EP_DELAY_STOP | DWC3_EP_TRANSFER_STARTED);
> +
> +	/*
> +	 * The End Transfer command is still in progress.  Do not clear the
> +	 * flags, so that the ep is only rearmed once the command completes.
> +	 */
> +	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
> +		mask |= (DWC3_EP_END_TRANSFER_PENDING |
> +			 DWC3_EP_TRANSFER_STARTED);
> +
>  	dep->flags &= mask;
>  
>  	/* Clear out the ep descriptors for non-ep0 */

[ ... ]

> @@ -3901,7 +3920,16 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
>  			dwc3_ep0_send_delayed_status(dwc);
>  	}
>  
> +	if (dep->flags & DWC3_EP_PENDING_NOOP_START) {
> +		dep->flags &= ~DWC3_EP_PENDING_NOOP_START;
> +
> +		if (!(dep->flags & DWC3_EP_DELAY_START) &&
> +		    (dep->flags & DWC3_EP_ENABLED))
> +			dwc3_gadget_ep_start_noop_transfer(dep);
> +	}
> +
>  	if ((dep->flags & DWC3_EP_DELAY_START) &&
> +	    (dep->flags & DWC3_EP_ENABLED) &&
>  	    !usb_endpoint_xfer_isoc(dep->endpoint.desc))
>  		__dwc3_gadget_kick_transfer(dep);
>  

[Severity: Medium]
What happens to NoStream handling in dwc3_gadget_endpoint_command_complete()
when an endpoint is disabled, re-enabled as stream-capable, and has a request
queued before the End Transfer command from __dwc3_gadget_ep_disable()
completes?

In drivers/usb/dwc3/gadget.c, when __dwc3_gadget_ep_disable() calls
dwc3_remove_requests(dwc, dep, -ESHUTDOWN, true), the End Transfer command is
issued for the prior transfer while DWC3_EP_END_TRANSFER_PENDING remains set.
If __dwc3_gadget_ep_enable() re-enables the endpoint for streams before
DWC3_DEPEVT_EPCMDCMPLT is processed, dwc3_gadget_set_ep_config() sets
dep->stream_capable = true and __dwc3_gadget_ep_enable() sets
DWC3_EP_PENDING_NOOP_START.

When the End Transfer from __dwc3_gadget_ep_disable() completes in
dwc3_gadget_endpoint_command_complete(), dep->stream_capable is already true,
so it sets DWC3_EP_IGNORE_NEXT_NOSTREAM at the start of the handler:

	if (dep->stream_capable)
		dep->flags |= DWC3_EP_IGNORE_NEXT_NOSTREAM;

If a request was queued in the meantime (setting DWC3_EP_DELAY_START),
dwc3_gadget_endpoint_command_complete() skips
dwc3_gadget_ep_start_noop_transfer() (leaving DWC3_EP_FORCE_RESTART_STREAM
unset) and immediately starts the new stream transfer via
__dwc3_gadget_kick_transfer() with DWC3_EP_IGNORE_NEXT_NOSTREAM set.

Even if DWC3_EP_FORCE_RESTART_STREAM were set during
__dwc3_gadget_ep_enable(), wouldn't setting DWC3_EP_IGNORE_NEXT_NOSTREAM right
before __dwc3_gadget_kick_transfer() starts the new transfer (with no pending
synthetic NoStream event from a no-op End Transfer) cause dwc3_nostream_work()
to ignore the first real host NoStream rejection?

	if ((dep->flags & DWC3_EP_IGNORE_NEXT_NOSTREAM) ||
	    (!DWC3_MST_CAPABLE(&dwc->hwparams) &&
	     !(dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE)))
		goto out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792542945.git.Thinh.Nguyen@synopsys.com?part=5

[PATCH 6/7] usb: dwc3: gadget: Reset endpoint flags on controller start

[Thinh Nguyen]

The flags of a disabled endpoint may be preserved while its End
Transfer command is pending. If the command never completes, the stale
flags persist across a soft disconnect or an error recovery, and the
endpoint can't be used again.

Reset the flags of all non-control endpoints when starting the
controller.

Fixes: 76bff31c7fba ("usb: dwc3: gadget: Do not clear ep delayed stop flag during ep disable")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e531adc02a40..a8f12e583a0f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2972,6 +2972,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
 {
 	struct dwc3_ep		*dep;
 	int			ret = 0;
+	u32			epnum;
 	u32			reg;
 
 	/*
@@ -3029,6 +3030,15 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
 		return ret;
 	}
 
+	for (epnum = 2; epnum < dwc->num_eps; epnum++) {
+		dep = dwc->eps[epnum];
+		if (!dep)
+			continue;
+
+		dep->flags &= (DWC3_EP_TXFIFO_RESIZED |
+			       DWC3_EP_RESOURCE_ALLOCATED);
+	}
+
 	dep = dwc->eps[0];
 	dep->flags = 0;
 	ret = __dwc3_gadget_ep_enable(dep, DWC3_DEPCFG_ACTION_INIT);
-- 
2.53.0

Re: [PATCH 6/7] usb: dwc3: gadget: Reset endpoint flags on controller start

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: dwc3: gadget: __dwc3_gadget_start() clears DWC3_EP_ENABLED on active eps
--

commit 4093b4f01f6fe6cc6713a8ff97925d5c1b4491d8
Author: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

usb: dwc3: gadget: Reset endpoint flags on controller start

This commit resets the endpoint flags of all non-control endpoints in
__dwc3_gadget_start(), preserving only DWC3_EP_TXFIFO_RESIZED and
DWC3_EP_RESOURCE_ALLOCATED so stale flags from a pending End Transfer command
do not persist across a soft disconnect or error recovery.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e531adc02a401..a8f12e583a0f4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2972,6 +2972,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
>  {
>  	struct dwc3_ep		*dep;
>  	int			ret = 0;
> +	u32			epnum;
>  	u32			reg;
>  
>  	/*
> @@ -3029,6 +3030,15 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
>  		return ret;
>  	}
>  
> +	for (epnum = 2; epnum < dwc->num_eps; epnum++) {
> +		dep = dwc->eps[epnum];
> +		if (!dep)
> +			continue;
> +
> +		dep->flags &= (DWC3_EP_TXFIFO_RESIZED |
> +			       DWC3_EP_RESOURCE_ALLOCATED);
> +	}

[Severity: High]
What happens if __dwc3_gadget_start() is called while non-control endpoints
are still enabled by the gadget driver (dep->endpoint.enabled is true)?

For example, in dwc3_gadget_suspend(), if dwc3_gadget_soft_disconnect() fails,
dwc3_gadget_soft_connect() -> __dwc3_gadget_start() is called before
dwc3_disconnect_gadget_sleepable() runs:

	ret = dwc3_gadget_soft_disconnect(dwc);
	if (ret && dwc->softconnect) {
		dwc3_gadget_soft_connect(dwc);
		return -EAGAIN;
	}

	dwc3_disconnect_gadget_sleepable(dwc);

Similarly, in dwc3_err_recovery_work(), dwc3_gadget_suspend() /
dwc3_gadget_resume(), or dwc3_gadget_pullup(), gadget drivers such as
raw_gadget and gadgetfs defer usb_ep_disable() to userspace rather than
disabling endpoints synchronously during their disconnect callback, so
endpoints can still be enabled when __dwc3_gadget_start() executes.

Clearing DWC3_EP_ENABLED here while dep->endpoint.enabled remains true causes
a subsequent usb_ep_disable() -> dwc3_gadget_ep_disable() call to hit
dev_WARN_ONCE() and return early without calling __dwc3_gadget_ep_disable():

	if (dev_WARN_ONCE(dwc->dev, !(dep->flags & DWC3_EP_ENABLED),
					"%s is already disabled\n",
					dep->name))
		return 0;

	spin_lock_irqsave(&dwc->lock, flags);
	ret = __dwc3_gadget_ep_disable(dep);
	spin_unlock_irqrestore(&dwc->lock, flags);

Because __dwc3_gadget_ep_disable() is skipped, dwc3_remove_requests() is not
run and dep->stream_capable, dep->type, dep->endpoint.comp_desc, and
dep->endpoint.desc are not cleared:

	/* Clear out the ep descriptors for non-ep0 */
	if (dep->number > 1) {
		dep->endpoint.comp_desc = NULL;
		dep->endpoint.desc = NULL;
	}

In drivers/usb/gadget/legacy/raw_gadget.c, raw_ioctl_ep_disable() frees
ep->desc immediately after usb_ep_disable(), which leaves dep->endpoint.desc
as a dangling pointer:

	usb_ep_disable(dev->eps[i].ep);

	spin_lock_irqsave(&dev->lock, flags);
	usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req);
	kfree(dev->eps[i].ep->desc);

Conversely, if the gadget driver calls usb_ep_enable() while ep->enabled is
still true, usb_ep_enable() returns early without calling into dwc3, leaving
DWC3_EP_ENABLED cleared so subsequent usb_ep_queue() calls fail with
-ESHUTDOWN.

Could this regression be avoided if __dwc3_gadget_start() keeps
dep->flags synchronized with dep->endpoint.enabled, or properly cleans up any
still-enabled endpoints when resetting their flags?

> +
>  	dep = dwc->eps[0];
>  	dep->flags = 0;
>  	ret = __dwc3_gadget_ep_enable(dep, DWC3_DEPCFG_ACTION_INIT);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792542945.git.Thinh.Nguyen@synopsys.com?part=6

[PATCH 7/7] usb: dwc3: gadget: Recover from End Transfer timeout

[Thinh Nguyen]

If the End Transfer command times out, the endpoint resource is never
released and every subsequent Start Transfer to the endpoint fails with
"No Resource". Schedule an error recovery to reset the controller.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a8f12e583a0f..a76f669a796c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1775,6 +1775,8 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
 	return DWC3_DSTS_SOFFN(reg);
 }
 
+static void dwc3_schedule_err_recovery(struct dwc3 *dwc);
+
 /**
  * __dwc3_stop_active_transfer - stop the current active transfer
  * @dep: isoc endpoint
@@ -1818,6 +1820,9 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
 		dev_err_ratelimited(dep->dwc->dev,
 				"end transfer failed: %d\n", ret);
 
+	if (ret == -ETIMEDOUT && dep->dwc->err_state == DWC3_ERR_NONE)
+		dwc3_schedule_err_recovery(dep->dwc);
+
 	dep->resource_index = 0;
 
 	if (!interrupt || (ret && ret != -ETIMEDOUT))
-- 
2.53.0

Re: [PATCH 7/7] usb: dwc3: gadget: Recover from End Transfer timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] usb: dwc3: gadget: __dwc3_stop_active_transfer() re-arms work on exit
- [High] usb: dwc3: dwc3_err_recovery_work() skips reset on soft_disconnect() timeout
- [High] usb: dwc3: gadget: race between dwc3_err_recovery_work() and suspend
- [High] usb: dwc3: gadget: race between dwc3_err_recovery_work() and threaded IRQ
- [Medium] usb: dwc3: gadget: __dwc3_gadget_start() clears DWC3_EP_ENABLED on active eps
--

commit d30e9ae79405a29ae6e289723c7c0106f74fcf1a
Author: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

usb: dwc3: gadget: Recover from End Transfer timeout

This commit schedules controller error recovery via
dwc3_schedule_err_recovery() when an End Transfer command times out in
__dwc3_stop_active_transfer().

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a8f12e583a0f4..a76f669a796c2 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
[ ... ]
> @@ -1818,6 +1820,9 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>  		dev_err_ratelimited(dep->dwc->dev,
>  				"end transfer failed: %d\n", ret);
>  
> +	if (ret == -ETIMEDOUT && dep->dwc->err_state == DWC3_ERR_NONE)
> +		dwc3_schedule_err_recovery(dep->dwc);
> +
>  	dep->resource_index = 0;
>  
>  	if (!interrupt || (ret && ret != -ETIMEDOUT))

[Severity: Critical]
Can __dwc3_stop_active_transfer() re-schedule dwc->err_recovery_work during
teardown after cancel_work_sync() has already run in dwc3_gadget_exit()?

In drivers/usb/dwc3/gadget.c, dwc3_gadget_exit() cancels err_recovery_work
and destroys connect_mutex before tearing down the gadget and freeing
endpoints:

	cancel_work_sync(&dwc->err_recovery_work);
	mutex_destroy(&dwc->connect_mutex);
	dwc3_enable_susphy(dwc, true);
	usb_del_gadget(dwc->gadget);
	dwc3_gadget_free_endpoints(dwc);

Between cancel_work_sync() and the completion of usb_del_gadget() and
dwc3_gadget_free_endpoints(), __dwc3_stop_active_transfer() can still be
called with interrupt set to true and time out in dwc3_send_gadget_ep_cmd():

- usb_del_gadget() -> gadget_unbind_driver() invokes the gadget driver
  unbind callback, which calls usb_ep_disable() -> dwc3_gadget_ep_disable()
  -> __dwc3_gadget_ep_disable() -> dwc3_remove_requests() ->
  dwc3_stop_active_transfer() -> __dwc3_stop_active_transfer().

- Until usb_del_gadget() stops the gadget and frees dwc->irq_gadget,
  dwc3_thread_interrupt() can still invoke __dwc3_stop_active_transfer().

- Until dwc3_gadget_free_endpoints() cancels dep->nostream_work,
  dwc3_nostream_work() can still call dwc3_stop_active_transfer() ->
  __dwc3_stop_active_transfer().

If dwc3_send_gadget_ep_cmd() times out on any of those paths while in the
setup phase, wouldn't __dwc3_stop_active_transfer() re-queue
dwc->err_recovery_work after cancel_work_sync(), causing
dwc3_err_recovery_work() to lock the destroyed dwc->connect_mutex and access
freed dwc->eps[], dwc->gadget, and struct dwc3?

[Severity: High]
When an End Transfer command times out in __dwc3_stop_active_transfer() and
does not complete in hardware, will dwc3_err_recovery_work() actually reach
the soft reset in dwc3_gadget_soft_connect()?

In drivers/usb/dwc3/gadget.c, __dwc3_stop_active_transfer() schedules
dwc3_err_recovery_work() and sets the end transfer pending flag while leaving
the transfer started flag set:

	if (!interrupt || (ret && ret != -ETIMEDOUT))
		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
	else
		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;

When dwc3_err_recovery_work() runs, it calls dwc3_gadget_soft_disconnect()
first and aborts recovery if that fails:

	ret = dwc3_gadget_soft_disconnect(dwc);

	dwc3_disconnect_gadget_sleepable(dwc);

	if (ret)
		goto err_unrecoverable;

Inside dwc3_gadget_soft_disconnect() -> dwc3_stop_active_transfers() ->
dwc3_remove_requests() -> dwc3_stop_active_transfer(), the endpoint is
skipped because the end transfer pending flag is set:

	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
		return;

Then dwc3_gadget_soft_disconnect() calls dwc3_gadget_run_stop(dwc, false) to
clear run/stop and poll for the controller halt bit. Because the controller
cannot halt while an active transfer's End Transfer command remains
incomplete in hardware, dwc3_gadget_run_stop() times out as well.

Unlike dwc3_gadget_suspend(), which calls dwc3_gadget_soft_connect() ->
dwc3_core_soft_reset() when dwc3_gadget_soft_disconnect() fails:

	ret = dwc3_gadget_soft_disconnect(dwc);
	if (ret && dwc->softconnect) {
		dwc3_gadget_soft_connect(dwc);
		return -EAGAIN;
	}

doesn't dwc3_err_recovery_work() jump straight to err_unrecoverable without
ever resetting the controller?

[Severity: High]
Could scheduling dwc->err_recovery_work from __dwc3_stop_active_transfer()
race with system or runtime suspend and access registers after
dwc3_core_exit() has disabled clocks and asserted reset?

In drivers/usb/dwc3/core.c, dwc3_suspend_common() suspends the gadget and
calls dwc3_core_exit():

	case DWC3_GCTL_PRTCAP_DEVICE:
		if (pm_runtime_suspended(dwc->dev))
			break;
		ret = dwc3_gadget_suspend(dwc);
		if (ret)
			return ret;
		synchronize_irq(dwc->irq_gadget);
		dwc3_core_exit(dwc);
		break;

During dwc3_gadget_suspend() -> dwc3_gadget_soft_disconnect() ->
dwc3_stop_active_transfers() -> dwc3_remove_requests() ->
dwc3_stop_active_transfer() -> __dwc3_stop_active_transfer(), an End Transfer
timeout will now schedule dwc->err_recovery_work, while
dwc3_stop_active_transfers() ignores the error. Similarly, during
dwc3_gadget_disconnect_interrupt(), dwc->connected is set to false, endpoint
disable in dwc3_disconnect_gadget() can time out in
__dwc3_stop_active_transfer() and schedule dwc->err_recovery_work while
releasing per-request runtime power management references, and
pm_request_idle(dwc->dev) is called.

Since dwc3_suspend_common() does not cancel dwc->err_recovery_work before
dwc3_core_exit(), and dwc3_err_recovery_work() in drivers/usb/dwc3/gadget.c
does not acquire a runtime power management reference before calling
dwc3_gadget_soft_disconnect() and dwc3_gadget_soft_connect(), what prevents
dwc3_err_recovery_work() from accessing controller registers while clocks are
disabled and reset is asserted?

[Severity: High]
How is dwc3_err_recovery_work() synchronized against dwc3_thread_interrupt()
when __dwc3_stop_active_transfer() schedules recovery from inside the
threaded interrupt handler?

Previously, dwc3_schedule_err_recovery() was only called from the hardirq
handler dwc3_check_event_buf() when waking dwc3_thread_interrupt() was
skipped. With this change, __dwc3_stop_active_transfer() can call
dwc3_schedule_err_recovery() from inside dwc3_thread_interrupt() ->
dwc3_process_event_buf(), both directly from endpoint event handlers and
while dwc->lock is dropped for gadget callbacks such as dwc3_reset_gadget(),
dwc3_disconnect_gadget(), or dwc3_ep0_delegate_req() -> usb_ep_disable().

In drivers/usb/dwc3/gadget.c, dwc3_err_recovery_work() executes
dwc3_gadget_soft_disconnect() and dwc3_disconnect_gadget_sleepable() before
calling synchronize_irq(dwc->irq_gadget):

	ret = dwc3_gadget_soft_disconnect(dwc);

	dwc3_disconnect_gadget_sleepable(dwc);

	if (ret)
		goto err_unrecoverable;

	if (dwc->softconnect) {
		u32 count;
		synchronize_irq(dwc->irq_gadget);

When dwc3_thread_interrupt() drops dwc->lock (for example in
dwc3_gadget_giveback() or during an in-flight gadget driver callback), can't
dwc3_err_recovery_work() concurrently halt the controller in
dwc3_gadget_soft_disconnect(), invoke the gadget disconnect callback in
dwc3_disconnect_gadget_sleepable(), and then have dwc3_thread_interrupt()
re-acquire dwc->lock and overwrite controller state (such as
dwc3_gadget_reset_interrupt() setting dwc->connected = true after
dwc3_reset_gadget())?

[Severity: Medium]
When dwc3_err_recovery_work() or dwc3_gadget_suspend() calls
dwc3_gadget_soft_connect() -> __dwc3_gadget_start() while non-control
endpoints are still enabled in the gadget layer, doesn't clearing the endpoint
enabled flag in __dwc3_gadget_start() desynchronize dep->flags from
dep->endpoint.enabled?

In drivers/usb/dwc3/gadget.c, __dwc3_gadget_start() masks dep->flags on all
non-control endpoints:

	for (epnum = 2; epnum < dwc->num_eps; epnum++) {
		dep = dwc->eps[epnum];
		if (!dep)
			continue;

		dep->flags &= (DWC3_EP_TXFIFO_RESIZED |
			       DWC3_EP_RESOURCE_ALLOCATED);
	}

In dwc3_gadget_suspend(), dwc3_gadget_soft_connect() is called on
soft-disconnect failure without calling dwc3_disconnect_gadget_sleepable(),
and in dwc3_err_recovery_work(), dwc3_gadget_soft_connect() runs immediately
after dwc3_disconnect_gadget_sleepable() even if a function driver defers
usb_ep_disable() to a worker thread or userspace. When usb_ep_disable() is
subsequently called while dep->endpoint.enabled is still true, won't
dwc3_gadget_ep_disable() hit dev_WARN_ONCE() and return early without running
__dwc3_gadget_ep_disable():

	if (dev_WARN_ONCE(dwc->dev, !(dep->flags & DWC3_EP_ENABLED),
					"%s is already disabled\n",
					dep->name))
		return 0;

leaving endpoint state such as dep->endpoint.desc uncleaned?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792542945.git.Thinh.Nguyen@synopsys.com?part=7