[PATCH RESEND] usb: mtu3: don't run gadget completion callbacks with interrupts disabled

[PATCH RESEND] usb: mtu3: don't run gadget completion callbacks with interrupts disabled

[Ryan Brue]

mtu3_irq() is a threaded handler, so it runs in process context, but it
takes mtu->lock with spin_lock_irqsave(). mtu3_req_complete() then drops
the lock with a plain spin_unlock() around
usb_gadget_giveback_request(), which leaves hardware interrupts disabled
across the gadget function's completion callback.

That breaks any callback that reaches netif_rx(). Since the threaded
handler is in neither hardirq nor softirq context, netif_rx() takes its
local_bh_disable()/local_bh_enable() path, and enabling softirqs with
interrupts disabled is not allowed:

  WARNING: kernel/softirq.c:430 at __local_bh_enable_ip+0x17c/0x188
    __local_bh_enable_ip
    netif_rx
    rx_complete [u_ether]
    usb_gadget_giveback_request
    mtu3_req_complete
    mtu3_qmu_isr
    mtu3_irq
    irq_thread_fn

With CONFIG_TRACE_IRQFLAGS, __local_bh_enable_ip() ends with an
unconditional local_irq_enable(), so the plain spin_lock() re-taking
mtu->lock afterwards runs with interrupts on and is recorded
SOFTIRQ-ON-W, while mtu3_gadget_queue() takes the same lock from a
softirq (ncm's tx hrtimer), and lockdep follows up with an inconsistent
lock state report naming mtu->lock.

The locking was right when mtu3_irq() was a hardirq handler. It stopped
being right when the handler became threaded without a primary handler;
dwc3 hit the same thing the same way and fixed it in the controller
driver, in commit 84918a89d6ef ("usb: dwc3: gadget: Let the interrupt
handler disable bottom halves.").

mtu->lock is never taken in hardirq context: the only handler is
threaded and has no primary handler, so disabling softirqs is enough.
Take it with spin_lock_bh() in mtu3_irq(). The completion callback then
runs with interrupts enabled and softirqs disabled, which is the context
netif_rx() expects. The other sites keep spin_lock_irqsave(), which is
strictly stronger and still correct; several of them really can be
reached from softirq context.

Both reports need lockdep to be seen, but the context violation is real:
netif_rx() wants either hardirq/softirq context or interrupts enabled,
and mtu3 gives it neither. The fallout on a PROVE_LOCKING kernel is not
cosmetic either - copying a 21 MB file over USB ethernet hung and reset
this board repeatedly before this change. Once interrupts have been
left enabled while the thread holds mtu->lock, the tx hrtimer softirq on
the same CPU spins on it forever, which is the "CPU0: lock(&mtu->lock);
<Interrupt> lock(&mtu->lock);" scenario lockdep names. The same copy is
reliable after it.

Fixes: 13118959cb1a ("usb: mtu3: register mtu3_irq by threaded irq")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 drivers/usb/mtu3/mtu3_core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3_core.c b/drivers/usb/mtu3/mtu3_core.c
index a40bf5bad2d5..ed0e426645fd 100644
--- a/drivers/usb/mtu3/mtu3_core.c
+++ b/drivers/usb/mtu3/mtu3_core.c
@@ -790,10 +790,9 @@ static irqreturn_t mtu3_u2_common_isr(struct mtu3 *mtu)
 static irqreturn_t mtu3_irq(int irq, void *data)
 {
 	struct mtu3 *mtu = (struct mtu3 *)data;
-	unsigned long flags;
 	u32 level1;
 
-	spin_lock_irqsave(&mtu->lock, flags);
+	spin_lock_bh(&mtu->lock);
 
 	/* U3D_LV1ISR is RU */
 	level1 = mtu3_readl(mtu->mac_base, U3D_LV1ISR);
@@ -814,7 +813,7 @@ static irqreturn_t mtu3_irq(int irq, void *data)
 	if (level1 & QMU_INTR)
 		mtu3_qmu_isr(mtu);
 
-	spin_unlock_irqrestore(&mtu->lock, flags);
+	spin_unlock_bh(&mtu->lock);
 
 	return IRQ_HANDLED;
 }

---
base-commit: df2908090cda368b01ff43709f51890076c56157
change-id: 20260913-usb-mtu3-gadget-completion-callback-fix-d334f7e99a17

Best regards,
--
Ryan Brue <ryanbrue.dev@gmail.com>
-- 
Ryan Brue <ryanbrue.dev@gmail.com>

Re: [PATCH RESEND] usb: mtu3: don't run gadget completion callbacks with interrupts disabled

[Ryan Brue]

Hi Greg,

Gentle ping on this one too. Similarly has had no comments since the 
resend on the 29th of September.

It fixes the softirq WARN in the commit message, which any gadget 
function that reaches netif_rx() (u_ether in this case) hits on mtu3.

Thanks,
Ryan Brue

[PATCH RESEND] usb: mtu3: don't run gadget completion callbacks with interrupts disabled

[Ryan Brue]

mtu3_irq() is a threaded handler, so it runs in process context, but it
takes mtu->lock with spin_lock_irqsave(). mtu3_req_complete() then drops
the lock with a plain spin_unlock() around
usb_gadget_giveback_request(), which leaves hardware interrupts disabled
across the gadget function's completion callback.

That breaks any callback that reaches netif_rx(). Since the threaded
handler is in neither hardirq nor softirq context, netif_rx() takes its
local_bh_disable()/local_bh_enable() path, and enabling softirqs with
interrupts disabled is not allowed:

  WARNING: kernel/softirq.c:430 at __local_bh_enable_ip+0x17c/0x188
    __local_bh_enable_ip
    netif_rx
    rx_complete [u_ether]
    usb_gadget_giveback_request
    mtu3_req_complete
    mtu3_qmu_isr
    mtu3_irq
    irq_thread_fn

With CONFIG_TRACE_IRQFLAGS, __local_bh_enable_ip() ends with an
unconditional local_irq_enable(), so the plain spin_lock() re-taking
mtu->lock afterwards runs with interrupts on and is recorded
SOFTIRQ-ON-W, while mtu3_gadget_queue() takes the same lock from a
softirq (ncm's tx hrtimer), and lockdep follows up with an inconsistent
lock state report naming mtu->lock.

The locking was right when mtu3_irq() was a hardirq handler. It stopped
being right when the handler became threaded without a primary handler;
dwc3 hit the same thing the same way and fixed it in the controller
driver, in commit 84918a89d6ef ("usb: dwc3: gadget: Let the interrupt
handler disable bottom halves.").

mtu->lock is never taken in hardirq context: the only handler is
threaded and has no primary handler, so disabling softirqs is enough.
Take it with spin_lock_bh() in mtu3_irq(). The completion callback then
runs with interrupts enabled and softirqs disabled, which is the context
netif_rx() expects. The other sites keep spin_lock_irqsave(), which is
strictly stronger and still correct; several of them really can be
reached from softirq context.

Both reports need lockdep to be seen, but the context violation is real:
netif_rx() wants either hardirq/softirq context or interrupts enabled,
and mtu3 gives it neither. The fallout on a PROVE_LOCKING kernel is not
cosmetic either - copying a 21 MB file over USB ethernet hung and reset
this board repeatedly before this change. Once interrupts have been
left enabled while the thread holds mtu->lock, the tx hrtimer softirq on
the same CPU spins on it forever, which is the "CPU0: lock(&mtu->lock);
<Interrupt> lock(&mtu->lock);" scenario lockdep names. The same copy is
reliable after it.

Fixes: 13118959cb1a ("usb: mtu3: register mtu3_irq by threaded irq")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 drivers/usb/mtu3/mtu3_core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3_core.c b/drivers/usb/mtu3/mtu3_core.c
index a40bf5bad2d5..ed0e426645fd 100644
--- a/drivers/usb/mtu3/mtu3_core.c
+++ b/drivers/usb/mtu3/mtu3_core.c
@@ -790,10 +790,9 @@ static irqreturn_t mtu3_u2_common_isr(struct mtu3 *mtu)
 static irqreturn_t mtu3_irq(int irq, void *data)
 {
 	struct mtu3 *mtu = (struct mtu3 *)data;
-	unsigned long flags;
 	u32 level1;
 
-	spin_lock_irqsave(&mtu->lock, flags);
+	spin_lock_bh(&mtu->lock);
 
 	/* U3D_LV1ISR is RU */
 	level1 = mtu3_readl(mtu->mac_base, U3D_LV1ISR);
@@ -814,7 +813,7 @@ static irqreturn_t mtu3_irq(int irq, void *data)
 	if (level1 & QMU_INTR)
 		mtu3_qmu_isr(mtu);
 
-	spin_unlock_irqrestore(&mtu->lock, flags);
+	spin_unlock_bh(&mtu->lock);
 
 	return IRQ_HANDLED;
 }

---
base-commit: df2908090cda368b01ff43709f51890076c56157
change-id: 20260913-usb-mtu3-gadget-completion-callback-fix-d334f7e99a17

Best regards,
--
Ryan Brue <ryanbrue.dev@gmail.com>
-- 
Ryan Brue <ryanbrue.dev@gmail.com>

Re: [PATCH RESEND] usb: mtu3: don't run gadget completion callbacks with interrupts disabled

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: mtu3: spin_lock_bh() in mtu3_irq() leaves hardirqs enabled in callbacks
--

commit 153a59750e6c0f99e7e0b96dbcf13012e52f3fc8
Author: Ryan Brue <ryanbrue.dev@gmail.com>

usb: mtu3: don't run gadget completion callbacks with interrupts disabled

This commit changes mtu3_irq() in drivers/usb/mtu3/mtu3_core.c to acquire
and release mtu->lock using spin_lock_bh() and spin_unlock_bh() instead of
spin_lock_irqsave() and spin_unlock_irqrestore() so that softirqs are
disabled when gadget completion callbacks invoke netif_rx().

> diff --git a/drivers/usb/mtu3/mtu3_core.c b/drivers/usb/mtu3/mtu3_core.c
> index a40bf5bad2d5d..ed0e426645fd6 100644
> --- a/drivers/usb/mtu3/mtu3_core.c
> +++ b/drivers/usb/mtu3/mtu3_core.c
> @@ -790,10 +790,9 @@ static irqreturn_t mtu3_u2_common_isr(struct mtu3 *mtu)
>  static irqreturn_t mtu3_irq(int irq, void *data)
>  {
>  	struct mtu3 *mtu = (struct mtu3 *)data;
> -	unsigned long flags;
>  	u32 level1;
>  
> -	spin_lock_irqsave(&mtu->lock, flags);
> +	spin_lock_bh(&mtu->lock);
>  
>  	/* U3D_LV1ISR is RU */
>  	level1 = mtu3_readl(mtu->mac_base, U3D_LV1ISR);
> @@ -814,7 +813,7 @@ static irqreturn_t mtu3_irq(int irq, void *data)
>  	if (level1 & QMU_INTR)
>  		mtu3_qmu_isr(mtu);
>  
> -	spin_unlock_irqrestore(&mtu->lock, flags);
> +	spin_unlock_bh(&mtu->lock);

[Severity: High]
Does replacing spin_lock_irqsave() with spin_lock_bh() in mtu3_irq() break
the USB gadget completion contract and risk a hardirq deadlock?

When mtu3_irq() invokes mtu3_qmu_isr() or mtu3_ep0_isr() ->
mtu3_req_complete() (as well as forward_to_driver(),
mtu3_gadget_disconnect(), mtu3_gadget_suspend(), or mtu3_gadget_resume() in
drivers/usb/mtu3/mtu3_gadget.c), those helpers release mtu->lock with a
plain spin_unlock():

drivers/usb/mtu3/mtu3_gadget.c:mtu3_req_complete() {
	...
	spin_unlock(&mtu->lock);
	usb_gadget_giveback_request(&mep->ep, req);
	spin_lock(&mtu->lock);
	...
}

Because mtu3_irq() now only holds spin_lock_bh(&mtu->lock), dropping
mtu->lock with spin_unlock() leaves hardware interrupts enabled across
usb_gadget_giveback_request() and the gadget driver callbacks.

However, the struct usb_request kerneldoc in include/linux/usb/gadget.h
requires completion callbacks to be called with hardware interrupts
disabled:

include/linux/usb/gadget.h:
 * @complete: Function called when request completes, so this request and
 *	its buffer may be re-used.  The function will always be called with
 *	interrupts disabled, and it must not sleep.

Gadget function drivers rely on this guarantee and take hardirq-unsafe
locks with plain spin_lock() inside their callbacks. For example, in
drivers/usb/gadget/function/u_serial.c, gs_console_complete_out(),
gs_console_connect(), and gs_console_disconnect() acquire cons->lock with a
plain spin_lock():

drivers/usb/gadget/function/u_serial.c:gs_console_complete_out() {
	...
	default:
		spin_lock(&cons->lock);
		req->length = 0;
		schedule_work(&cons->work);
		spin_unlock(&cons->lock);
		break;
	}
}

Meanwhile, gs_console_write() is a console write callback that can be
invoked from any hardirq via printk() and acquires cons->lock with
spin_lock_irqsave(). Running completion or setup callbacks with hardware
interrupts enabled causes a lockdep inconsistent lock state splat and
deadlocks if a hardware interrupt on the same CPU calls printk() while
cons->lock is held. Similarly, u_audio_iso_complete() in
drivers/usb/gadget/function/u_audio.c calls snd_pcm_stream_lock() assuming
hardware interrupts are disabled.

In addition, mtu->lock itself can be acquired from hardirq context via
usb_ep_queue() -> mtu3_gadget_queue() when f_midi2 is driven by the ALSA
sequencer hrtimer backend (snd_hrtimer_callback() -> snd_timer_interrupt()
-> snd_seq_timer_interrupt() -> snd_seq_check_queue() ->
seq_ump_process_event() -> snd_rawmidi_kernel_write() ->
f_midi2_ump_trigger() -> process_ump_transmit() -> usb_ep_queue()), and
other callers of mtu3_req_complete() such as mtu3_gadget_ep_disable(),
mtu3_gadget_dequeue(), and mtu3_gadget_stop() still hold
spin_lock_irqsave(&mtu->lock, flags).

The commit message notes that dwc3 fixed the same issue in commit
84918a89d6ef ("usb: dwc3: gadget: Let the interrupt handler disable bottom
halves."), but dwc3 kept spin_lock_irqsave() and wrapped it with
local_bh_disable() and local_bh_enable():

drivers/usb/dwc3/gadget.c:dwc3_thread_interrupt() {
	unsigned long flags;
	irqreturn_t ret = IRQ_NONE;
	int i;

	local_bh_disable();
	spin_lock_irqsave(&dwc->lock, flags);
	ret = dwc3_process_event_buf(evt);
	spin_unlock_irqrestore(&dwc->lock, flags);
	local_bh_enable();

	return ret;
}

In net/core/dev.c, netif_rx() checks:

	need_bh_off = !(hardirq_count() | softirq_count());

When local_bh_disable() is held around spin_lock_irqsave() and
spin_unlock_irqrestore() (as done in dwc3, cdns3, and cdnsp),
softirq_count() is non-zero so netif_rx() does not call
local_bh_disable()/local_bh_enable() while hardware interrupts remain
disabled across usb_gadget_giveback_request(), and local_bh_enable() runs
pending softirqs after spin_unlock_irqrestore() re-enables hardware
interrupts.

Should mtu3_irq() wrap spin_lock_irqsave() and spin_unlock_irqrestore()
with local_bh_disable() and local_bh_enable() instead of using
spin_lock_bh()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-usb-mtu3-gadget-completion-callback-fix-v1-1-93c3a6dda31f@gmail.com?part=1