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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] usb: musb: gadget: missing Fixes trailer on ep->busy race fix
--

commit b6b3d8fc1d6d15564bfaa947266d072504b05814
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 bug fix include a Fixes tag for the commit that introduced
the ep->busy save-and-restore logic?

  Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")

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