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

[sashiko-bot@kernel.org]

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