Re: [PATCH v2] usb: dwc3: gadget: Prevent EPs resource conflict during StartTransfer

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Wed, Dec 03, 2025, Selvarasu Ganesan wrote:
> 
> On 11/21/2025 8:38 AM, Alan Stern wrote:
> > On Fri, Nov 21, 2025 at 02:22:02AM +0000, Thinh Nguyen wrote:
> >> On Wed, Nov 19, 2025, Alan Stern wrote:
> >>> ->set_alt() is called by the composite core when a Set-Interface or
> >>> Set-Config control request arrives from the host.  It happens within the
> >>> composite_setup() handler, which is called by the UDC driver when a
> >>> control request arrives, which means it happens in the context of the
> >>> UDC driver's interrupt handler.  Therefore ->set_alt() callbacks must
> >>> not sleep.
> >> This should be changed. I don't think we can expect set_alt() to
> >> be in interrupt context only.
> > Agreed.
> >
> >>> To do this right, I can't think of any approach other than to make the
> >>> composite core use a work queue or other kernel thread for handling
> >>> Set-Interface and Set-Config calls.
> >> Sounds like it should've been like this initially.
> > I guess the nobody thought through the issues very carefully at the time
> > the composite framework was designed.  Maybe the UDCs that existed back
> > did not require a lot of time to flush endpoints; I can't remember.
> >
> >>> Without that ability, we will have to audit every function driver to
> >>> make sure the ->set_alt() callbacks do ensure that endpoints are flushed
> >>> before they are re-enabled.
> >>>
> >>> There does not seem to be any way to fix the problem just by changing
> >>> the gadget core.
> >>>
> >> We can have a workaround in dwc3 that can temporarily "work" with what
> >> we have. However, eventually, we will need to properly rework this and
> >> audit the gadget drivers.
> > Clearly, the first step is to change the composite core.  That can be
> > done without messing up anything else.  But yes, eventually the gadget
> > drivers will have to be audited.
> >
> > Alan Stern
> 
> 
> Hi Thinh,
> 
> Do you have any suggestions that might be helpful for us to try on our side?
> This EP resource‑conflict problem becomes easily observable when the 
> RNDIS network test executing ifconfig rndis0 down/up is run repeatedly 
> on the device side.
> 
> Thanks,
> Selva

At the moment, I can't think of a way to workaround for all cases. Let's
just leave bulk streams alone for now. Until we have proper fixes to the
gadget framework, let's just try the below.

Thanks,
Thinh

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3830aa2c10a9..974573304441 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -960,11 +960,18 @@ 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.
+	 * 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.
 	 */
-	if (usb_endpoint_xfer_bulk(desc) ||
-			usb_endpoint_xfer_int(desc)) {
+	if (dep->stream_capable) {
 		struct dwc3_gadget_ep_cmd_params params;
 		struct dwc3_trb	*trb;
 		dma_addr_t trb_dma;
@@ -983,35 +990,21 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 		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, true, true);
+		dwc3_stop_active_transfer(dep, true, 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;
-		}
+		/*
+		 * 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: