usb: musb: gadget: don't flush a waiting RX packet when queueing a request

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 <[email protected]>

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.

Quoted 5 lines

drivers/usb/musb/musb_gadget.c +0 -0

index 016d3f3fc1e05..c71c60efe5106 100644

drivers/usb/musb/musb_gadget.c __acquires(ep->musb->lock) · line 157

Quoted 10 lines

drivers/usb/musb/musb_gadget.c +7 -0

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

drivers/usb/musb/musb_gadget.c static int musb_gadget_disable(struct usb_ep *ep) · line 1148

Quoted 13 lines

drivers/usb/musb/musb_gadget.c +6 -0

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

Quoted 25 lines

drivers/usb/musb/musb_gadget.c +21 -0

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

drivers/usb/musb/musb_gadget.c void musb_gadget_cleanup(struct musb *musb) · line 1914

Quoted 10 lines

drivers/usb/musb/musb_gadget.c +9 -0

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