Re: [PATCH v3 1/5] genirq: Synchronize in-flight handlers during NMI teardown

[Doug Anderson]

Hi,

On Fri, Sep 4, 2026 at 6:38 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Fri, 04 Sep 2026 10:17:42 +0100,
> Thomas Gleixner <tglx@xxxxxxxxxx> wrote:
> >
> > On Wed, Sep 02 2026 at 18:54, Mayank Rungta wrote:
> > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> > > index 2fbff2618a1e..a9973b61163a 100644
> > > --- a/kernel/irq/manage.c
> > > +++ b/kernel/irq/manage.c
> > > @@ -1379,6 +1379,14 @@ static bool irq_supports_nmi(struct irq_desc *desc)
> > >     if (d->chip->irq_bus_lock || d->chip->irq_bus_sync_unlock)
> > >             return false;
> > >
> > > +   /*
> > > +    * NMIs cannot set IRQD_IRQ_INPROGRESS because they cannot acquire
> > > +    * spinlocks. Synchronous disable and teardown require querying the
> > > +    * hardware state via ->irq_get_irqchip_state().
> > > +    */
> > > +   if (!d->chip->irq_get_irqchip_state)
> > > +           return false;
> > > +
> > >     return d->chip->flags & IRQCHIP_SUPPORTS_NMI;
> > >  }
> > >
> > > @@ -2034,11 +2042,20 @@ static const void *__cleanup_nmi(unsigned int irq, struct irq_desc *desc)
> > >     struct irqaction *action = NULL;
> > >     const char *devname = NULL;
> > >
> > > +   guard(mutex)(&desc->request_mutex);
> > > +
> > >     scoped_guard(raw_spinlock_irqsave, &desc->lock) {
> > > -           irq_nmi_teardown(desc);
> > > +           irq_settings_clr_disable_unlazy(desc);
> > > +           irq_shutdown(desc);
> > > +   }
> > >
> > > -           desc->istate &= ~IRQS_NMI;
> > > +   /*
> > > +    * Ensure all in-flight NMI handlers on other CPUs complete before
> > > +    * clearing desc->action or tearing down NMI state.
> > > +    */
> > > +   __synchronize_hardirq(desc, true);
> >
> > NMIs are strictly per CPU interrupts. So how is this supposed to work
> > correctly when looking at irqchip_state(ACTIVE) ?
> >
> > Marc?
>
> I have no idea what this is trying to achieve.
>
> This can only work for a global interrupt, not for a CPU-private
> interrupt, since in general you can't observe the state of the
> interrupt on another CPU.
>
> My guess is that the OP is trying to trigger an NMI using a global
> interrupt, which we never intended to be supported. That's not to say
> that it cannot be supported, but this patch seems to be breaking the
> core use case...

The overall goal is to take the watchdog bark interrupt (which is a
GIC Shared Peripheral Interrupt) and promote it to NMI. An example of
that interrupt in the device tree for one Qualcomm board:

interrupts = <GIC_SPI 0 IRQ_TYPE_EDGE_RISING>;

The patches in this series seem to accomplish that. With the current
arm64 pseudo-NMI implemnttion, requesting as NMI just bumps up the
priority of an interrupt to "NMI" level and that works fine on SPIs as
well. On arm64/GIC interrupts are routed to a single CPU anyway, so
requesting it with `IRQF_PERCPU | IRQF_NOBALANCING` didn't seem
absurd.

The biggest problem we had was properly cleaning up the NMI at module
"unload" time. To make that work, we needed a way to synchronize the
NMIs to ensure they were quiescent before the module was unloaded.
That's what this patch is attempting, and is mostly attempting to
respond to Sashiko feedback on earlier patches.

I don't think Mayank is tied to any particular implementation and the
overall goal here is to allow the watchdog bark interrupt to run at an
elevated level so it can produce good backtraces even if all the CPUs
in the system are locked up and the buddy lockup detector doesn't
fire. Is this something you think should be doable? If so, what's the
best way for Mayank to go about doing it?


-Doug