Re: [PATCH 2/3] genirq: Export NMI APIs

[Doug Anderson]

Hi,

On Thu, Jul 30, 2026 at 3:55 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 7/30/26 14:49, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jul 30, 2026 at 2:33 PM Mayank Rungta via B4 Relay
> > <devnull+mrungta.google.com@xxxxxxxxxx> wrote:
> >>
> >> From: Mayank Rungta <mrungta@xxxxxxxxxx>
> >>
> >> Currently, request_nmi(), free_nmi(), enable_nmi() and disable_nmi_nosync()
> >> are restricted to built-in kernel code because they are not exported to
> >> loadable modules.
> >>
> >> Export these APIs to allow loadable modules to register and manage NMIs.
> >> This allows watchdog drivers configured as loadable modules to register
> >> their bark interrupt as an NMI.
> >>
> >> Signed-off-by: Mayank Rungta <mrungta@xxxxxxxxxx>
> >> ---
> >>   kernel/irq/manage.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> >> index 2fbff2618a1e..fb0b8da32f4c 100644
> >> --- a/kernel/irq/manage.c
> >> +++ b/kernel/irq/manage.c
> >> @@ -766,6 +766,7 @@ void disable_nmi_nosync(unsigned int irq)
> >>   {
> >>          disable_irq_nosync(irq);
> >>   }
> >> +EXPORT_SYMBOL_GPL(disable_nmi_nosync);
> >>
> >>   void __enable_irq(struct irq_desc *desc)
> >>   {
> >> @@ -833,6 +834,7 @@ void enable_nmi(unsigned int irq)
> >>   {
> >>          enable_irq(irq);
> >>   }
> >> +EXPORT_SYMBOL_GPL(enable_nmi);
> >>
> >>   static int set_irq_wake_real(unsigned int irq, unsigned int on)
> >>   {
> >> @@ -2080,6 +2082,7 @@ const void *free_nmi(unsigned int irq, void *dev_id)
> >>
> >>          return __cleanup_nmi(irq, desc);
> >>   }
> >> +EXPORT_SYMBOL_GPL(free_nmi);
> >>
> >>   /**
> >>    * request_threaded_irq - allocate an interrupt line
> >> @@ -2342,6 +2345,7 @@ int request_nmi(unsigned int irq, irq_handler_t handler,
> >>
> >>          return retval;
> >>   }
> >> +EXPORT_SYMBOL_GPL(request_nmi);
> >
> > This seems reasonable to me. One thought I had was that we could
> > possibly get by with fewer exported symbols by changing
> > disable_nmi_nosync() and enable_nmi() to "static inline" functions in
> > the header file. That being said, what Mayank has here feels slightly
> > better to me.
> >
>
> I don't claim to understand the NMI APIs used, but Sashiko's feedback
> seems real to me. Someone who knows that code will need to confirm that
> there are no unexpected pitfalls.

Since Sashiko doesn't reply-to all, here's a link to its feedback:

https://lore.kernel.org/all/20260730214758.DBD5E1F000E9@xxxxxxxxxxxxxxx/

Indeed, it does look like legitimate feedback. While the problems are
pre-existing, it is true that exporting as a module could widen the
exposure because the code backing the NMI handler could now be
unloaded.

I did a little bit of AI analysis of the problem myself. As far as I
can tell, it would be relatively safe to implement a synchronous
"disable_nmi" by just calling "disable_irq", much like is done for
other NMI functions. The caveat here is that it would only be safe on
IRQ controllers that provided irq_get_irqchip_state(). This is because
on NMI we don't set IRQD_IRQ_INPROGRESS and thus
__synchronize_hardirq() will fall back to calling
__irq_get_irqchip_state(). If any controllers supported NMI but
_didn't_ support irq_get_irqchip_state(), we'd silently skip waiting
for NMI completion. Maybe we could simply make it illegal for a
controller to support NMI without irq_get_irqchip_state()?

I guess maybe we need Thomas Gleixner to weigh in on this? Thomas:
should this issue block Mayank's patch? Would you expect him to
propose fixes as part of his series, or is this something you'd prefer
to post patches for yourself?



-Doug