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

[sashiko-bot@kernel.org]

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.

[ ... ]

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

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.

[ ... ]

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

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