On Tue, Aug 19, 2025 at 01:04:57PM +0200, Sebastian Andrzej Siewior wrote:
> On 2025-08-17 10:27:11 [-0400], Alan Stern wrote:
> > On Sat, Aug 16, 2025 at 10:16:34AM -0400, Alan Stern wrote:
> > > So it looks like we should be using a different function instead of
> > > local_irq_disable(). We need something which in a non-RT build will
> > > disable interrupts on the local CPU, but in an RT build will merely
> > > disable preemption. (In fact, every occurrence of local_irq_disable()
> > > in the USB subsystem probably should be changed in this way.)
> >
> > Or maybe what we need is something that in a non-RT build will disable
> > local interrupts and in an RT build will do nothing. (I suspect that RT
> > kernels won't like it if we call spin_lock() while preemption is
> > disabled.)
>
> This is the local_irq_disable() in vhci_urb_enqueue() before
> usb_hcd_giveback_urb() is invoked. It was added in 9e8586827a706
> ("usbip: vhci_hcd: fix calling usb_hcd_giveback_urb() with irqs
> enabled").
> The warning that fixed back then was
> | if (WARN_ON(in_task() && kcov_mode_enabled(mode))) {
> which was kernel/kcov.c:834 as of v5.9-rc8 (as of report the mentioned
> in the commit).
> local_irq_disable() does not change the preemption counter so I am a bit
> puzzled why this did shut the warning.
>
> > > Is there such a function?
>
> We could use some API that accidentally does what you ask for. There
> would be local_lock_t where local_lock_irq() does that.
> What about moving the completion callback to softirq by setting HCD_BH?
You're missing the point.
There are several places in the USB stack that disable local interrupts.
The idea was that -- on a non-RT system, which was all we had at the
time -- spin_lock_irqsave() is logically equivalent to a combination of
local_irq_save() and spin_lock(). Similarly, spin_lock_irq() is
logically equivalent to local_irq_disable() plus spin_lock().
So code was written which, for various reasons, used local_irq_save()
(or local_irq_disable()) and spin_lock() instead of spin_lock_irqsave()
(or spin_lock_irq()). But now we see that in RT builds, this
equivalency is not valid. Instead, spin_lock_irqsave(flags) is
logically equivalent to "flags = 0" plus spin_lock() (and
spin_lock_irq() is logically equivalent to a nop plus spin_lock()). At
least, that's how the material quoted earlier by Yunseong defines it.
Therefore, throughout the USB stack, we should replace calls to
local_irq_save() and local_irq_disable() with functions that behave like
the original in non-RT builds but do nothing in RT builds. We shouldn't
just worry about this one spot.
I would expect that RT already defines functions which do this, but I
don't know their names.
Alan Stern
