Re: [PATCH v2] usb: musb: gadget: don't flush a waiting RX packet when queueing a request

["Elvira Khabirova" <lineprinter0@gmail.com>]

On Sat Nov 7, 2026 at 12:30 PM CET, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [High] usb: musb: gadget: overlapping musb_g_giveback() calls wedge
>   ep->busy
> - [High] usb: musb: gadget: race and ordering bug in
>   musb_gadget_disable() for rx_kick
> - [Medium] usb: musb: gadget: lost rx_kick across system suspend and
>   resume
> --
>
> commit cef9b9e2278bf856fd53af421c899c0ae67866b7
> Author: Elvira Khabirova <lineprinter0@gmail.com>
>
> usb: musb: gadget: don't flush a waiting RX packet when queueing a request
>
> This commit replaces the RX FIFO flush in musb_ep_restart() with a
> system_bh_wq work item that invokes musb_g_rx(), and defers RX processing
> via ep->rx_kick_deferred while ep->busy is set.
>
>> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
>> index 016d3f3fc1e05..c71c60efe5106 100644
>> --- a/drivers/usb/musb/musb_gadget.c
>> +++ b/drivers/usb/musb/musb_gadget.c
>> @@ -148,6 +148,21 @@ __acquires(ep->musb->lock)
>>  	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
>>  	spin_lock(&musb->lock);
>>  	ep->busy = busy;
>> +
>> +	if (!ep->busy && ep->rx_kick_deferred) {
>> +		ep->rx_kick_deferred = false;
>> +		if (!queue_work(system_bh_wq, &ep->rx_kick))
>> +			pm_runtime_put_noidle(musb->controller);
>> +	}
>> +}
>
> [Severity: High]
> Can overlapping musb_g_giveback() calls finish out of LIFO order and leave
> ep->busy permanently stuck at 1?
>
> musb_g_giveback() saves ep->busy in a stack variable, sets ep->busy = 1,
> and drops musb->lock around usb_gadget_giveback_request().  With RX
> givebacks running in system_bh_wq, a second giveback on the same endpoint
> (for example via musb_gadget_dequeue(), which enters musb_g_giveback()
> when musb_ep->busy is set) can overlap on another CPU:
>
> CPU0                              CPU1
> musb_g_rx_kick()
>   musb_g_rx()
>     musb_g_giveback()
>       busy = ep->busy; // 0
>       ep->busy = 1;
>       spin_unlock(&musb->lock)
>       usb_gadget_giveback_request()
>                                   musb_gadget_dequeue()
>                                     spin_lock_irqsave(&musb->lock, ...)
>                                     musb_g_giveback()
>                                       busy = ep->busy; // 1
>                                       ep->busy = 1;
>                                       spin_unlock(&musb->lock)
>                                       usb_gadget_giveback_request()
>       spin_lock(&musb->lock)
>       ep->busy = 0;
>       queue_work(..., &ep->rx_kick)
>       spin_unlock(&musb->lock)
>                                       spin_lock(&musb->lock)
>                                       ep->busy = 1;  <-- stuck at 1
>
> First, CPU0 clears ep->busy to 0 and schedules ep->rx_kick while CPU1 is
> still in usb_gadget_giveback_request().  Second, when CPU1 finishes, it
> writes its saved busy = 1 back to ep->busy.  After that, every subsequent
> musb_g_rx(), musb_g_rx_kick(), and musb_gadget_queue() call sees
> ep->busy == 1 and defers work forever, wedging the endpoint and leaking
> the runtime PM reference taken by musb_g_defer_rx_kick().
>
> Making ep->busy an active giveback counter (incremented before dropping
> musb->lock and decremented after re-acquiring it) would ensure ep->busy
> only reaches 0 when the last giveback completes.

Yes, that is possible, but this is a pre-existing bug. I even have
a separate patch prepared to make `busy` a counter instead of a flag.
This patch does not aggravate this problem.
The same race is possible between a dequeue and a HW IRQ.

>> @@ -1111,6 +1137,12 @@ static int musb_gadget_disable(struct usb_ep *ep)
>>  
>>  	/* abort all pending DMA and requests */
>>  	nuke(musb_ep, -ESHUTDOWN);
>> +	if (cancel_work(&musb_ep->rx_kick))
>> +		pm_runtime_put_autosuspend(musb->controller);
>> +	if (musb_ep->rx_kick_deferred) {
>> +		musb_ep->rx_kick_deferred = false;
>> +		pm_runtime_put_autosuspend(musb->controller);
>> +	}
>>  
>>  	musb_ep->desc = NULL;
>>  	musb_ep->end_point.desc = NULL;
>
> [Severity: High]
> Can musb_g_rx_kick() run while nuke() drops musb->lock or after the
> endpoint is re-enabled?
>
> Because musb_ep->desc is cleared after nuke(), musb->lock is dropped
> inside nuke() -> musb_g_giveback() while musb_ep->desc is still non-NULL.
> If an earlier giveback on another CPU finishes during that window, it
> restores ep->busy = 0 and queues ep->rx_kick, allowing musb_g_rx_kick() ->
> musb_g_rx() -> rxstate() to run after nuke() has already freed
> musb_ep->dma.

Good catch, and actually I think this can be resolved with making
ep->busy a counter instead.
I will send them together as a series for v3.

> In addition, cancel_work() is non-synchronous:
>
> CPU0                              CPU1
>                                   <softirq>
>                                   musb_g_rx_kick()
>                                     // waiting for musb->lock
> musb_gadget_disable()
>   spin_lock_irqsave(&musb->lock, ...)
>   cancel_work(&musb_ep->rx_kick) // false
>   musb_ep->desc = NULL;
>   spin_unlock_irqrestore(&musb->lock, ...)
> musb_gadget_enable()
>   spin_lock_irqsave(&musb->lock, ...)
>   musb_ep->desc = desc;
>   musb_ep->busy = 0;
>   spin_unlock_irqrestore(&musb->lock, ...)
>                                     spin_lock_irqsave(&musb->lock, ...)
>                                     if (!musb_ep->desc || ...) // false
>                                     musb_g_rx()  <-- stale kick runs
>
> Clearing musb_ep->desc and musb_ep->end_point.desc before calling nuke()
> prevents musb_g_rx_kick() and musb_gadget_queue() from acting on the
> endpoint while nuke() drops musb->lock, and prevented stale kicks will see
> the updated state if musb_gadget_enable() also cancels any pending
> rx_kick.

Is this any different from before the patch, with a HW IRQ?
I think a stale kick handles everything well, if there is no work,
musb_g_rx() just returns.

>> @@ -1810,6 +1881,36 @@ void musb_gadget_cleanup(struct musb *musb)
>>  
>>  	cancel_delayed_work_sync(&musb->gadget_work);
>>  	usb_del_gadget_udc(&musb->g);
>> +
>> +	musb_gadget_disable_rx_kicks(musb);
>> +}
>> +
>> +static void musb_g_disable_rx_kick(struct musb *musb, struct musb_ep *musb_ep)
>> +{
>> +	if (musb_ep->rx_kick.func && disable_work_sync(&musb_ep->rx_kick))
>> +		pm_runtime_put_noidle(musb->controller);
>> +}
>> +
>> +void musb_gadget_disable_rx_kicks(struct musb *musb)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < musb->nr_endpoints; i++) {
>> +		musb_g_disable_rx_kick(musb, &musb->endpoints[i].ep_in);
>> +		musb_g_disable_rx_kick(musb, &musb->endpoints[i].ep_out);
>> +	}
>> +}
>> +
>> +void musb_gadget_enable_rx_kicks(struct musb *musb)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < musb->nr_endpoints; i++) {
>> +		if (musb->endpoints[i].ep_in.rx_kick.func)
>> +			enable_work(&musb->endpoints[i].ep_in.rx_kick);
>> +		if (musb->endpoints[i].ep_out.rx_kick.func)
>> +			enable_work(&musb->endpoints[i].ep_out.rx_kick);
>> +	}
>>  }
>
> [Severity: Medium]
> Does musb_gadget_disable_rx_kicks() lose a pending or deferred rx_kick
> across system suspend and resume?
>
> If ep->rx_kick is pending when musb_suspend() calls disable_work_sync(),
> the work item is cancelled and its runtime PM reference is dropped, or if
> musb_g_rx_kick() is currently in musb_g_giveback() with
> ep->rx_kick_deferred set, disable_work_sync() marks ep->rx_kick disabled
> before waiting for completion so queue_work() in musb_g_giveback() fails.
> On resume, musb_gadget_enable_rx_kicks() only calls enable_work(), which
> does not re-queue the cancelled work.  When the USB session is preserved
> across suspend/resume (such as with MUSB_PRESERVE_SESSION or standby),
> MUSB_RXCSR_RXPKTRDY remains set in hardware with no interrupt to
> re-trigger it, stalling RX on the endpoint.

I see, I wish I had more hardware to test on.
This seems to be possible on da8xx.
I will find a way to re-queue the cancelled work items for v3.