Re: [PATCH v4 1/2] usb: musb: gadget: count givebacks in busy

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

On Sat Nov 7, 2026 at 11:35 PM CET, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Medium] usb: musb: gadget: no TX restart when busy drops to 0 in
>   musb_g_giveback()
> - [Low] usb: musb: gadget: missing Fixes trailer for ep->busy race fix
> --
>
> commit 38e2611af12585f304377ba06a8095186ff11968
> Author: Elvira Khabirova <lineprinter0@gmail.com>
>
>> usb: musb: gadget: count givebacks in busy
>>
>> The ep->busy flag is used by musb_g_giveback() to signal that
>> req->complete() is running on an endpoint, the ep is busy and
>> starting a new request is not appropriate right now.
>>
>> It is only a flag and not a counter currently;
>> musb_g_giveback() saves the old ep->busy value, sets busy to 1,
>> releases musb->lock, runs req->complete(), restores the old ep->busy.
>> This works when there is no possibility of two musb_g_giveback()s
>> running in parallel.
>>
>> This does not work when musb_gadget_dequeue()->...->musb_g_giveback()
>> happens at the same time as musb_interrupt()->...->musb_g_giveback().
>> The above save-then-restore can get the busy flag stuck at 1.
>>
>>   CPU0 (interrupt)                CPU1 (usb_ep_dequeue())
>>   giveback: saved 0, busy = 1
>>   unlock, ->complete()
>>                                   giveback: saved 1, busy = 1
>>                                   unlock, ->complete()
>>   lock, busy = 0
>>                                   lock, busy = 1
>>
>> ...and this gets the entire endpoint stuck.
>>
>> This was reproduced and the patch was tested on a Pine A64+
>> (Allwinner A64, four cores, no DMA) with PREEMPT_DYNAMIC and lockdep,
>> using a specially constructed f_fs function that keeps two AIO requests
>> queued on a bulk endpoint and cancels the newer one with io_cancel(),
>> in a loop. Tested with both a bulk IN and a bulk OUT endpoint.
>>
>> If musb_g_giveback() were to be started from some other context,
>> the same issue would come up.
>>
>> "Promote" ep->busy from a flag to a counter.
>>
>> musb_gadget_enable() should not reset `busy` to 0 anymore because
>> musb_g_giveback() decrements busy, and if these two run in parallel,
>> it would be problematic (cause an underflow).
>>
>> Signed-off-by: Elvira Khabirova <lineprinter0@gmail.com>
>
> [Severity: Low]
> Should this concurrency fix include a Fixes tag?
>
>   Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")

Feels useless to mark this as "Fixes" for a commit from 2008
that brings MUSB support.

>> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
>> index 016d3f3fc1e05..b500e0d8b0bd3 100644
>> --- a/drivers/usb/musb/musb_gadget.c
>> +++ b/drivers/usb/musb/musb_gadget.c
>> @@ -129,7 +129,6 @@ __acquires(ep->musb->lock)
>>  {
>>  	struct musb_request	*req;
>>  	struct musb		*musb;
>> -	int			busy = ep->busy;
>>  
>>  	req = to_musb_request(request);
>>  
>> @@ -138,7 +137,7 @@ __acquires(ep->musb->lock)
>>  		req->request.status = status;
>>  	musb = req->musb;
>>  
>> -	ep->busy = 1;
>> +	ep->busy++;
>>  	spin_unlock(&musb->lock);
>>  
>>  	if (!dma_mapping_error(&musb->g.dev, request->dma))
>> @@ -147,7 +146,7 @@ __acquires(ep->musb->lock)
>>  	trace_musb_req_gb(req);
>>  	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
>>  	spin_lock(&musb->lock);
>> -	ep->busy = busy;
>> +	ep->busy--;
>>  }
>
> [Severity: Medium]
> Can an IN endpoint still stall when a new TX request is queued while a
> concurrent dequeue giveback holds ep->busy non-zero after musb_g_tx() has
> returned?
>
> With two TX requests req1 and req2 queued on an IN endpoint:
>
> CPU0                              CPU1
> musb_g_tx()
>   musb_g_giveback(..., req1, 0)
>     list_del(&req1->list)
>     ep->busy++ (0 -> 1)
>     spin_unlock(&musb->lock)
>     req1->complete()
>                                   musb_gadget_dequeue(..., req2)
>                                     musb_g_giveback(..., req2, -ECONNRESET)
>                                       list_del(&req2->list)
>                                       ep->busy++ (1 -> 2)
>                                       spin_unlock(&musb->lock)
>                                       req2->complete()
>     spin_lock(&musb->lock)
>     ep->busy-- (2 -> 1)
>   next_request(musb_ep) == NULL
>   return
>                                       musb_gadget_queue(..., req3)
>                                         list_add_tail(&req3->list, ...)
>                                         !musb_ep->busy is false (busy == 1)
>                                       spin_lock(&musb->lock)
>                                       ep->busy-- (1 -> 0)
>                                     return  <-- req3 is never started
>
> When musb_g_tx() finishes its giveback first, req_list is empty because
> musb_gadget_dequeue() already unlinked req2, so musb_g_tx() exits while
> ep->busy is still 1.  If req3 is queued before CPU1 finishes
> req2->complete(), musb_gadget_queue() skips starting I/O because
> musb_ep->busy is non-zero:
>
> drivers/usb/musb/musb_gadget.c:musb_gadget_queue() {
> 	...
> 	/* it this is the head of the queue, start i/o ... */
> 	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
> 		status = musb_queue_resume_work(musb,
> 						musb_ep_restart_resume_work,
> 						request);
> 	...
> }
>
> When CPU1 then decrements ep->busy to 0 and returns from
> musb_gadget_dequeue(), nothing calls txstate() for req3, so no further TX
> interrupt fires and the IN endpoint stays wedged with req3 at the head of
> req_list.  Commit "usb: musb: gadget: don't flush a waiting RX packet when
> queueing a request" later in this series defers and kicks RX when ep->busy
> drops to 0, but TX requests (!request->tx is false) are not restarted
> there either.

Yes, it can. This is pre-existing and not made worse by this patch.
This needs a separate fix.