Re: [PATCH] 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 4 potential issue(s) to consider:
- [High] usb: musb: gadget: softirq busy-loop and livelock in musb_g_rx_kick()
- [High] usb: musb: gadget: race and ordering bug in musb_gadget_disable() for rx_kick
- [High] usb: musb: gadget: RX endpoint lockup after giveback in musb_g_rx_kick()
- [High] usb: musb: gadget: unushed rx_kick work in musb_suspend() accesses disabled MMIO
--

commit 15173995ac2630cef424095c4ec09d83b2d74409
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 per-endpoint
work item queued on system_bh_wq to call musb_g_rx() when MUSB_RXCSR_RXPKTRDY
is set. This avoids dropping waiting RX packets when requests are queued onto
an empty endpoint queue.

> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index 016d3f3fc1e05..ace1a99999abd 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -1111,6 +1111,8 @@ 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);
>  
>  	musb_ep->desc = NULL;
>  	musb_ep->end_point.desc = NULL;

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

> @@ -1171,10 +1173,42 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
>  		txstate(musb, req);
>  	} else {
>  		csr = musb_readw(epio, MUSB_RXCSR);
> -		csr |= MUSB_RXCSR_FLUSHFIFO | MUSB_RXCSR_P_WZC_BITS;
> -		musb_writew(epio, MUSB_RXCSR, csr);
> -		musb_writew(epio, MUSB_RXCSR, csr);
> +		if (csr & MUSB_RXCSR_RXPKTRDY) {
> +			pm_runtime_get_noresume(musb->controller);
> +			if (!queue_work(system_bh_wq, &req->ep->rx_kick))
> +				pm_runtime_put_noidle(musb->controller);
> +		}
> +	}
> +}
> +
> +static void musb_g_rx_kick(struct work_struct *work)
> +{
> +	struct musb_ep *musb_ep = container_of(work, struct musb_ep, rx_kick);
> +	struct musb *musb = musb_ep->musb;
> +	unsigned long flags;
> +	u8 epnum = musb_ep->current_epnum;
> +	bool requeued = false;
> +	u16 csr;
> +
> +	spin_lock_irqsave(&musb->lock, flags);
> +
> +	if (!musb_ep->desc)
> +		goto out;
> +
> +	if (musb_ep->busy) {
> +		requeued = queue_work(system_bh_wq, &musb_ep->rx_kick);
> +		goto out;
>  	}

[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().

> +
> +	musb_ep_select(musb->mregs, epnum);
> +	csr = musb_readw(musb->endpoints[epnum].regs, MUSB_RXCSR);
> +	if (csr & MUSB_RXCSR_RXPKTRDY)
> +		musb_g_rx(musb, epnum);

[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/20261027202744.79556-1-lineprinter0@gmail.com?part=1