Re: [RFC PATCH 1/4] genirq: Provide basic NMI management for interrupt lines

[Julien Thierry]

Hi Thomas,

Thanks for the review.

On 17/07/18 10:22, Thomas Gleixner wrote:
> On Mon, 9 Jul 2018, Julien Thierry wrote:
> > +/**
> > + *	disable_nmi_nosync - disable an nmi without waiting
> > + *	@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.
> > + *	Unlike disable_nmi(), this function does not ensure existing
> > + *	instances of the IRQ handler have completed before returning.
> > + */
> > +void disable_nmi_nosync(unsigned int irq)
> > +{
> > +	disable_irq_nosync(irq);
> > +}
> > +
> > +/**
> > + *	disable_nmi - disable an nmi and wait for completion
> > + *	@irq: Interrupt to disable
> > + *
> > + *	Disable the selected interrupt line. Enables and disables are
> > + *	nested.
> > + *	The interrupt to disable must have been requested through request_nmi.
> > + *	This function waits for any pending IRQ handlers for this interrupt
> > + *	to complete before returning. If you use this function while
> > + *	holding a resource the IRQ handler may need you will deadlock.
>
> I don't see how that wait would work with NMIs.

I'm a bit unsure about this one. In practice NMI handlers shouldn't take 
forever nor block the system for too long. And if we are able to disable 
interrupt line used for the NMI I'd think that the sequence:

	disable_nmi_interrupt_line(irq);
	wait_for_remaining_handlers(irq);

should terminate if the NMI handler is not broken (unless I am missing 
something).

In any case, I don't mind getting rid of the waiting disable for NMIs, I 
doubt there is a strong need for it. So if the function is likely to be 
broken I'm happy to get rid of it.

> > +/* This function must be called with desc->lock held */
> > +static const void *__cleanup_nmi(unsigned int irq, struct irq_desc *desc)
> > +{
> > +	const char *devname = NULL;
> > +
> > +	desc->istate &= ~IRQS_NMI;
> > +
> > +#ifdef CONFIG_SMP
> > +	if (WARN_ON(desc->affinity_notify))
> > +		desc->affinity_notify = NULL;
> > +#endif
>
> We should not support that affinity notify horror for NMIs at all.

Makes sense, I'll add it to the constraints for NMIs and remove that part.

> > +	if (!WARN_ON(desc->action == NULL)) {
> > +		irq_pm_remove_action(desc, desc->action);
> > +		devname = desc->action->name;
> > +		unregister_handler_proc(irq, desc->action);
> > +
> > +		kfree(desc->action);
> > +		desc->action = NULL;
> > +	}
> > +
> > +	irq_settings_clr_disable_unlazy(desc);
> > +	irq_shutdown(desc);
> > +
> > +#ifdef CONFIG_SMP
> > +	/* make sure affinity_hint is cleaned up */
> > +	if (WARN_ON_ONCE(desc->affinity_hint))
> > +		desc->affinity_hint = NULL;
> > +#endif
>
> Especially not twice :)

Oops... I'll remove that as well.

Thanks,

--
Julien Thierry