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

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

[Severity: High]
Can calling nuke() before cancelling musb_ep->rx_kick and clearing musb_ep->desc race with an pending or in-flight musb_g_rx_kick() execution?

Inside musb_gadget_disable() -> nuke(), ep->busy is set to 1 and musb_g_giveback() drops musb->lock around usb_gadget_giveback_request():

drivers/usb/musb/musb_gadget.c:nuke() {
	ep->busy = 1;
	...
	while (!list_empty(&ep->req_list)) {
		req = list_first_entry(&ep->req_list, struct musb_request, list);
		musb_g_giveback(ep, &req->request, status);
	}
}

While musb->lock is dropped in musb_g_giveback(), musb_ep->desc is still non-NULL and musb_ep->rx_kick has not yet been cancelled, so a pending musb_g_rx_kick() can run, see musb_ep->desc != NULL and musb_ep->busy == 1, and repeatedly re-queue itself onto system_bh_wq.

Additionally, cancel_work(&musb_ep->rx_kick) is non-synchronous and only clears WORK_STRUCT_PENDING_BIT. If bh_worker() on another CPU has already dequeued musb_ep->rx_kick before cancel_work() is called:

1. If musb_g_rx_kick() already entered musb_g_rx() -> musb_g_giveback() and dropped musb->lock, cancel_work() returns false and musb_gadget_disable() returns to the caller while musb_g_rx_kick() is still unmapping DMA and running req->complete().

2. If musb_g_rx_kick() is waiting on spin_lock_irqsave(&musb->lock, flags) at entry to musb_g_rx_kick() and the gadget driver immediately re-enables the endpoint via usb_ep_enable() -> musb_gadget_enable() (such as during set_alt or reset), musb_gadget_enable() sets musb_ep->desc = desc and musb_ep->busy = 0. When the waiting musb_g_rx_kick() then acquires musb->lock, it sees the newly enabled musb_ep->desc and runs musb_g_rx() from the stale kick.

drivers/usb/musb/musb_gadget.c void musb_ep_restart(struct musb *musb, struct musb_request *req) · line 1201

[Severity: High]
Won't re-queueing musb_ep->rx_kick onto system_bh_wq when musb_ep->busy is set cause a tight softirq busy-loop or single-CPU livelock?

In musb_g_giveback(), ep->busy is set to 1 while musb->lock is dropped across usb_gadget_giveback_request():

drivers/usb/musb/musb_gadget.c:musb_g_giveback() {
	ep->busy = 1;
	spin_unlock(&musb->lock);

	if (!dma_mapping_error(&musb->g.dev, request->dma))
		unmap_dma_buffer(req, musb);

	trace_musb_req_gb(req);
	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
	spin_lock(&musb->lock);
	ep->busy = busy;
}

Because system_bh_wq is a WQ_BH | WQ_PERCPU workqueue serviced by bh_worker() in softirq context, calling queue_work(system_bh_wq, &musb_ep->rx_kick) from inside musb_g_rx_kick() puts rx_kick directly back onto the active per-CPU BH pool worklist and raises TASKLET_SOFTIRQ.

On SMP, if musb_g_giveback() is executing on CPU A while musb_g_rx_kick() runs on CPU B, bh_worker() on CPU B will spin in softirq context repeatedly locking musb->lock, seeing musb_ep->busy == 1, and re-queueing rx_kick for the entire duration of usb_gadget_giveback_request().

On a single-CPU system (such as AM335x / BeagleBone Black) or when pinned to the same CPU, if musb_g_giveback() is entered from task context (for example via musb_gadget_dequeue() or musb_gadget_disable() -> nuke()) and the completion callback runs local_bh_enable() or spin_unlock_bh(), softirq runs musb_g_rx_kick() on top of the stack where musb_g_giveback() is blocked. Since musb_g_giveback() cannot resume to clear ep->busy until softirq finishes, musb_g_rx_kick() spins until both bh_worker() and __do_softirq() exhaust their restart budgets, or livelocks on CONFIG_PREEMPT_RT if the BH worker preempts the task inside musb_g_giveback().

drivers/usb/musb/musb_gadget.c void musb_ep_restart(struct musb *musb, struct musb_request *req) · line 1206

[Severity: High]
Can calling musb_g_rx() only once in musb_g_rx_kick() leave the RX endpoint stalled if another packet arrives or is queued while musb_g_giveback() drops musb->lock?

When musb_g_rx_kick() calls musb_g_rx() -> rxstate() in PIO mode, rxstate() reads the packet from the FIFO, clears MUSB_RXCSR_RXPKTRDY, and calls musb_g_giveback():

drivers/usb/musb/musb_gadget.c:rxstate() {
			/* ack the read! */
			csr |= MUSB_RXCSR_P_WZC_BITS;
			csr &= ~MUSB_RXCSR_RXPKTRDY;
			musb_writew(epio, MUSB_RXCSR, csr);
		}
	}

	/* reach the end or short packet detected */
	if (request->actual == request->length ||
	    fifo_count < musb_ep->packet_sz)
		musb_g_giveback(musb_ep, request, 0);
}

Inside musb_g_giveback(), the completed request is unlinked, musb_ep->busy is set to 1, and musb->lock is dropped while running req->complete().

Because musb_g_rx_kick() runs in a BH workqueue rather than the hardware IRQ handler, the controller IRQ is not masked on other CPUs. If the next OUT packet arrives and its hardware interrupt fires while musb_ep->req_list is temporarily empty during musb_g_giveback() (or if a second packet was already waiting in a double-buffered FIFO with its IRQ already consumed), musb_interrupt() -> musb_g_rx() sees next_request(musb_ep) == NULL and returns early with MUSB_RXCSR_RXPKTRDY set.

Meanwhile, when req->complete() (or another thread before rx_kick ran) queues the next request via musb_gadget_queue():

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);
}

musb_ep->busy is 1, so musb_ep_restart() is not called and rx_kick is not re-queued. When musb_g_giveback() returns and restores musb_ep->busy = 0, neither rxstate() nor musb_g_rx_kick() re-selects the endpoint and loops while next_request(musb_ep) != NULL and MUSB_RXCSR_RXPKTRDY is set, leaving the new request in musb_ep->req_list and MUSB_RXCSR_RXPKTRDY set in hardware with no further interrupt to advance it.

[Severity: High]
What prevents musb_g_rx_kick() from accessing MUSB MMIO registers after musb_suspend() has disabled the controller during system sleep?

In musb_ep_restart(), pm_runtime_get_noresume(musb->controller) prevents runtime suspend, but it does not block system suspend (musb_suspend()), and system_bh_wq is not a freezable workqueue. In addition, musb_suspend() calls pm_runtime_get_sync(dev), which runs musb_runtime_resume() -> musb_run_resume_work() -> musb_ep_restart_resume_work() -> musb_ep_restart(), and can queue musb_ep->rx_kick directly inside musb_suspend():

drivers/usb/musb/musb_core.c:musb_suspend() {
	musb_platform_disable(musb);
	musb_disable_interrupts(musb);

	musb->flush_irq_work = true;
	while (flush_delayed_work(&musb->irq_work))
		;
	musb->flush_irq_work = false;
}

musb_suspend() flushes musb->irq_work and saves context, but does not flush or cancel the per-endpoint rx_kick work items and does not clear musb_ep->desc. If musb_g_rx_kick() runs after musb_platform_disable() or after the parent glue driver gates the controller clocks, could musb_ep_select(), musb_readw(), and musb_g_rx() access unclocked or powered-down MMIO registers?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1