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

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

[Elvira Khabirova]

Since commit 3fc137386c46 ("usb: musb: Fix hardware lockup on first Rx
endpoint request"), musb_ep_restart() flushes the RX FIFO instead of
calling rxstate(), with the premise that in case of Ethernet, dropping
a couple of packets during gadget initialization is not an issue,
because it will only happen when the software is not yet ready.

But when using other functions with musb, for example, f_mass_storage
or f_acm, this code path will be used not only when the software is not
yet ready, but also mid-communication, and it will cause data loss.
Data loss happens on gadgets that don't always have requests queued.

The timeline of a data loss:

1. A function that does not have a lot of requests pre-queued
   completes an OUT request. The queue is empty.
2. The next packet comes. MUSB auto-acks it, places it into the FIFO,
   sets RXPKTRDY, raises an interrupt.
3. musb_g_rx() is called, sees the request queue is empty,
   early returns.
4. The function queues the next request from the main fsg thread,
   musb_ep_restart does FLUSHFIFO, packet is lost.
5. The packet was auto-acked by MUSB, so it is not resent by the host.

Next data chunk arrives with an IRQ like so:

musb_g_rx returns, queue empty
        (drivers/usb/musb/musb_gadget.c:810)
musb_g_rx from musb_interrupt
        (drivers/usb/musb/musb_core.c:1786)
musb_interrupt from dsps_interrupt
        (drivers/usb/musb/musb_dsps.c:389)

and then it is dropped from the FIFO when usb_ep_queue() is called:

MUSB_RXCSR_FLUSHFIFO from musb_ep_restart
        (drivers/usb/musb/musb_gadget.c:1174)
musb_ep_restart from musb_ep_restart_resume_work
        (drivers/usb/musb/musb_gadget.c:1184)
musb_ep_restart_resume_work from musb_queue_resume_work
        (drivers/usb/musb/musb_core.c:2277)
musb_queue_resume_work from musb_gadget_queue
        (drivers/usb/musb/musb_gadget.c:1249)
musb_gadget_queue from usb_ep_queue
        (drivers/usb/gadget/udc/core.c:306)

In case of f_mass_storage, it keeps waiting for the lost data;
the host then times out and resets the device. Since this is a race
condition, this can be seen as data transfers taking too long,
or as drives getting mounted but disappearing when data
is read/written, and so on.

  usb 1-8.4: reset high-speed USB device number 31 using xhci_hcd
  I/O error, dev sdb, sector 240 op 0x1:(WRITE) flags 0x804800 phys_seg 30 prio class 2
  Buffer I/O error on dev sdb, logical block 30, lost async page write

When testing f_acm, this is seen as bytes loss on the tty.
Similar situation with f_hid, f_printer, f_fs.

Testing with commit 3fc137386c46 ("usb: musb: Fix hardware lockup on
first Rx endpoint request") reverted fixed these symptoms.

Move the data processing to system_bh_wq. Purpose of doing so is to run
in an interrupt context, honoring the usb_ep_queue docstring and avoiding
the lockup Hubert described in his commit message.
Alternative way of looking at this is it "re-triggers" the interrupt,
running the musb_g_rx that had to be skipped.

This issue was reproduced and the patch was tested on a BeagleBone Black
with f_mass_storage, f_acm, f_hid, f_printer, and f_fs.

Fixes: 3fc137386c46 ("usb: musb: Fix hardware lockup on first Rx endpoint request")
Signed-off-by: Elvira Khabirova <lineprinter0@gmail.com>
---
 drivers/usb/musb/musb_gadget.c | 54 ++++++++++++++++++++++++++++++++--
 drivers/usb/musb/musb_gadget.h |  3 ++
 2 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 016d3f3fc1e0..ace1a99999ab 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;
@@ -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;
 	}
+
+	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);
+out:
+	spin_unlock_irqrestore(&musb->lock, flags);
+
+	if (!requeued)
+		pm_runtime_put_autosuspend(musb->controller);
 }
 
 static int musb_ep_restart_resume_work(struct musb *musb, void *data)
@@ -1701,6 +1735,7 @@ init_peripheral_ep(struct musb *musb, struct musb_ep *ep, u8 epnum, int is_in)
 	ep->is_in = is_in;
 
 	INIT_LIST_HEAD(&ep->req_list);
+	INIT_WORK(&ep->rx_kick, musb_g_rx_kick);
 
 	sprintf(ep->name, "ep%d%s", epnum,
 			(!epnum || hw_ep->is_shared_fifo) ? "" : (
@@ -1805,11 +1840,24 @@ int musb_gadget_setup(struct musb *musb)
 
 void musb_gadget_cleanup(struct musb *musb)
 {
+	int i;
+
 	if (musb->port_mode == MUSB_HOST)
 		return;
 
 	cancel_delayed_work_sync(&musb->gadget_work);
 	usb_del_gadget_udc(&musb->g);
+
+	for (i = 0; i < musb->nr_endpoints; i++) {
+		struct musb_hw_ep *hw_ep = &musb->endpoints[i];
+
+		if (hw_ep->ep_in.rx_kick.func &&
+		    cancel_work_sync(&hw_ep->ep_in.rx_kick))
+			pm_runtime_put_noidle(musb->controller);
+		if (hw_ep->ep_out.rx_kick.func &&
+		    cancel_work_sync(&hw_ep->ep_out.rx_kick))
+			pm_runtime_put_noidle(musb->controller);
+	}
 }
 
 /*
diff --git a/drivers/usb/musb/musb_gadget.h b/drivers/usb/musb/musb_gadget.h
index f49f25b3bf56..334e6f9220ac 100644
--- a/drivers/usb/musb/musb_gadget.h
+++ b/drivers/usb/musb/musb_gadget.h
@@ -11,6 +11,7 @@
 #define __MUSB_GADGET_H
 
 #include <linux/list.h>
+#include <linux/workqueue.h>
 
 #if IS_ENABLED(CONFIG_USB_MUSB_GADGET) || IS_ENABLED(CONFIG_USB_MUSB_DUAL_ROLE)
 extern irqreturn_t musb_g_ep0_irq(struct musb *);
@@ -93,6 +94,8 @@ struct musb_ep {
 	/* true if lock must be dropped but req_list may not be advanced */
 	u8				busy;
 
+	struct work_struct		rx_kick;
+
 	u8				hb_mult;
 };
 

base-commit: 40288c9206c17eb66a603262e06a58d300d0f279
-- 
2.55.0

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

[sashiko-bot]

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