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

[sashiko-bot@kernel.org]

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")

> 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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107221904.82487-1-lineprinter0@gmail.com?part=1