Re: [PATCH v2 2/5] genirq: Implement synchronous disable_nmi()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Because NMIs do not set the IRQD_IRQ_INPROGRESS flag, __synchronize_hardirq() skips its cpu_relax() yield loop and tightly polls the hardware state, causing severe bus and lock contention.
--

commit 3611450f7345bc1611ae9b9599bb6e44ba2b6477
Author: Mayank Rungta <mrungta@google.com>

genirq: Implement synchronous disable_nmi()

This patch introduces a synchronous disable_nmi() interface. Drivers can use
this in process context to disable an NMI line and ensure running handlers
have completed on other CPUs by wrapping disable_irq().

> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 3bf969ad8fe07..f13b3ab3829af 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h

[ ... ]

> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 61384925b921a..40013aa94481e 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -767,6 +767,21 @@ void disable_nmi_nosync(unsigned int irq)
>  	disable_irq_nosync(irq);
>  }
>  
> +/**
> + * disable_nmi - disable an nmi and wait for any pending handlers
> + * @irq: Interrupt to disable
> + *
> + * Disable the selected interrupt line. Disables and enables are nested.
> + *
> + * The interrupt to disable must have been requested through request_nmi.
> + * This function ensures existing instances of the NMI handler have
> + * completed before returning.
> + */
> +void disable_nmi(unsigned int irq)
> +{
> +	disable_irq(irq);

[Severity: High]
This is a pre-existing issue, but does wrapping disable_irq() here create a
severe hardware polling loop when synchronizing NMIs?

Because disable_irq() delegates to __synchronize_hardirq(), it expects to wait
for the IRQD_IRQ_INPROGRESS flag using a cpu_relax() loop:

kernel/irq/manage.c:__synchronize_hardirq() {
    ...
		while (irqd_irq_inprogress(&desc->irq_data))
			cpu_relax();

		/* Ok, that indicated we're done: double-check carefully. */
		guard(raw_spinlock_irqsave)(&desc->lock);
		inprogress = irqd_irq_inprogress(&desc->irq_data);
    ...
}

However, NMI flow handlers (such as handle_fasteoi_nmi() in kernel/irq/chip.c)
execute locklessly and never set the IRQD_IRQ_INPROGRESS flag.

Since the flag is false, the cpu_relax() yield is skipped entirely. When an NMI
is executing concurrently, __synchronize_hardirq() will repeatedly acquire
desc->lock and query the hardware state via __irq_get_irqchip_state().

Will this cause a tight MMIO polling loop on the IRQ controller without
yielding, generating severe bus contention and potentially delaying the NMI
handler it is waiting for?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-qcom-wdt-nmi-series-v2-0-363979fe6b6b@google.com?part=2