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

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

On Thu, Dec 11, 2025, Selvarasu Ganesan wrote:
> 
> On 12/5/2025 6:48 AM, Thinh Nguyen wrote:
> >> I was hoping that the dwc3_gadget_ep_queue() won't come early to run
> >> into this scenario. What I've provided will only mitigate and will not
> >> resolve for all cases. It seems adding more checks in dwc3 will be
> >> more messy.
> 
> 
> Hi Thinh,
> 
> 
> Thank you for the insightful comments. I agree that adding more checks 
> directly in the dwc3 driver would be messy, and a comprehensive rework 
> of the dwc3 ep disable would ultimately be the cleaner solution.
> 
> In the meantime, Introducing additional checks for 
> DWC3_EP_TRANSFER_STARTED in dwc3 driver is the most practical way to 
> unblock the current issue while we work toward that longer‑term fix.
> We have applied the patches and performed additional testing, no 
> regressions or new issues were observed.
> 
> Could you please confirm whether below interim fix is acceptable along 
> with your proposed earlier patch for unblocking the current development 
> flow?
> 
> 
> Patch 2: usb: dwc3: protect dep->flags from concurrent modify in 
> dwc3_gadget_ep_disable
> =======================================================================================
> 
> Subject: [PATCH] usb: dwc3: protect dep->flags from concurrent modify in 
> dwc3_gadget_ep_disable
> The below warnings in `dwc3_gadget_ep_queue` observed during the RNDIS
> enable/disable test is caused by a race between `dwc3_gadget_ep_disable`
> and `dwc3_gadget_ep_queue`. Both functions manipulate `dep->flags`, and
> the lock released temporarily by `dwc3_gadget_giveback` (called from
> `dwc3_gadget_ep_disable`) can be acquired by `dwc3_gadget_ep_queue`
> before `dwc3_gadget_ep_disable` has finished. This leads to an
> inconsistent state of the `DWC3_EP_TRANSFER_STARTED` dep->flag.
> 
> To fix this issue by add a condition check when masking `dep->flags`
> in `dwc3_gadget_ep_disable` to ensure the `DWC3_EP_TRANSFER_STARTED`
> flag is not cleared when it is actually set. This prevents the spurious
> warning and eliminates the race.
> 
> Thread#1:
> dwc3_gadget_ep_disable
>    ->__dwc3_gadget_ep_disable
>     ->dwc3_remove_requests
>      ->dwc3_stop_active_transfer
>       ->__dwc3_stop_active_transfer
>        -> dwc3_send_gadget_ep_cmd (cmd =DWC3_DEPCMD_ENDTRANSFER)
>         ->if(!interrupt)dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>          ->dwc3_gadget_giveback
>           ->spin_unlock(&dwc->lock)
>             ...
>             While Thread#1 is still running, Thread#2 starts:
> 
> Thread#2:
> usb_ep_queue
>    ->dwc3_gadget_ep_queue
>     ->__dwc3_gadget_kick_transfer
>      -> starting = !(dep->flags & DWC3_EP_TRANSFER_STARTED);
>       ->if(starting)
>         ->dwc3_send_gadget_ep_cmd (cmd = DWC3_DEPCMD_STARTTRANSFER)
>          ->dep->flags |= DWC3_EP_TRANSFER_STARTED;
>            ...
>             ->__dwc3_gadget_ep_disable
>              ->mask = DWC3_EP_TXFIFO_RESIZED |DWC3_EP_RESOURCE_ALLOCATED;
>               ->dep->flags &= mask; --> // Possible of clears
>                   DWC3_EP_TRANSFER_STARTED flag as well without
>                   sending DWC3_DEPCMD_ENDTRANSFER
> 
>   ------------[ cut here ]------------
>    dwc3 13200000.dwc3: No resource for ep1in
>    WARNING: CPU: 7 PID: 1748 at drivers/usb/dwc3/gadget.c:398
> dwc3_send_gadget_ep_cmd+0x2f8/0x76c
>    pc : dwc3_send_gadget_ep_cmd+0x2f8/0x76c
>    lr : dwc3_send_gadget_ep_cmd+0x2f8/0x76c
>    Call trace:
>      dwc3_send_gadget_ep_cmd+0x2f8/0x76c
>      __dwc3_gadget_kick_transfer+0x2ec/0x3f4
>      dwc3_gadget_ep_queue+0x140/0x1f0
>      usb_ep_queue+0x60/0xec
>      mp_tx_task+0x100/0x134
>      mp_tx_timeout+0xd0/0x1e0
>      __hrtimer_run_queues+0x130/0x318
>      hrtimer_interrupt+0xe8/0x340
>      exynos_mct_comp_isr+0x58/0x80
>      __handle_irq_event_percpu+0xcc/0x25c
>      handle_irq_event+0x40/0x9c
>      handle_fasteoi_irq+0x154/0x2c8
>      generic_handle_domain_irq+0x58/0x80
>      gic_handle_irq+0x48/0x104
>      call_on_irq_stack+0x3c/0x50
>      do_interrupt_handler+0x4c/0x84
>      el1_interrupt+0x34/0x58
>      el1h_64_irq_handler+0x18/0x24
>      el1h_64_irq+0x68/0x6c
> 
> Change-Id: Ib6a77ce5130e25d0162f72d0e52c845dbb1d18f5
> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
> ---
>   drivers/usb/dwc3/gadget.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index b42d225b67408..1dc5798072120 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1051,6 +1051,22 @@ 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);
> +
> +    /*
> +     * When dwc3_gadget_ep_disable() calls dwc3_gadget_giveback(),
> +     * the  dwc->lock is temporarily released.  If dwc3_gadget_ep_queue()
> +     * runs in that window it may set the DWC3_EP_TRANSFER_STARTED flag as
> +     * part of dwc3_send_gadget_ep_cmd. The original code cleared the flag
> +     * unconditionally, which could overwrite the concurrent modification.
> +     *
> +     * The added check ensures the DWC3_EP_TRANSFER_STARTED flag is only
> +     * cleared if it is not set already, preserving the state updated 
> by the
> +     * concurrent ep_queue path and eliminating the EP resource conflict
> +     * warning.
> +     */

We need to explain the underlining problem here and in the commit
message. The function usb_ep_disable() is expected be used interrupt
context, and it's being used in interrupt context in the composite
framework. There's no wait for flushing of endpoint is handled before
usb_ep_disable completes.

We are adding a temporary workaround to handle the endpoint
reconfiguration and restart before the flushing completed.

> +    if (dep->flags & DWC3_EP_TRANSFER_STARTED)
> +        mask |= DWC3_EP_TRANSFER_STARTED;
> +
>       dep->flags &= mask;
> 
>       /* Clear out the ep descriptors for non-ep0 */
> -- 
> 
> 2.31.1
> 
> 

For your case, it may work because the endpoint is probably reconfigured
to be the same in usb_ep_enable(). If we reconfigure the endpoint before
the endpoint is stopped, the behavior is underfined.

You can create the patches and Cc stable. However, I would not add the
"Fixes" tag since they (IMHO) are not really fixes. May also need to
note that under the "---" in the commit explain why there's no Fixes tag
also.

Thanks,
Thinh