Re: [PATCH v3] genirq: Check irq disabled & masked states in irq_shutdown

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, 29 May 2017, jeffy.chen wrote:
> i think if we want to make all irq enable/disable balance, maybe we can:
> 
> 1/ only call irq_enable/disable from enable/disable_irq(change other
> irq_enable/disable to enable/disable_irq), so they would be protected by the
> refcnt(deph)

You cannot call enable/disable_irq() from places which call
irq_enable/disable() due to locking reasons.

__disable_irq()/__enable_irq() can/must be called with desc->lock held, but
__enable_irq() does more than just calling irq_enable().

So no, it's not that simple.

> 2/ disable lazy stuff in irq_shoutdown(we already did this in free_irq)

No, irq_shutdown() is called from other places as well. 

> 3/ in irq_shutdown, set depth to 0 if it's not disabled and masked(for lazy stuff)
> before calling disable_irq, 

Uurgh, no. That's a unholy hack.

So what should be done to fix this is to make consequent use of the state bits.

     irq_disable()
        if (irqd_irq_disabled())
	   return;
	irqd_set_irq_disabled();
	....

This should be done for both mask/unmask disable/enable. You get the idea.

We probably want a third state bit for STARTED_UP and do the same dance in
startup/shutdown as well. Which brings me to a different issue, which is
outside the scope of your problem, but looking at the code made me find it.

If a interrupt is marked IRQ_NOAUTOEN then request_irq() will not invoke
irq_startup(). The interrupt is just completely set up, but stays disabled.
It is enabled later via enable_irq(). That works so far with no complaints,
but there is an interesting twist:

In that NOAUTOEN case nothing ever calls irq_startup() on that irq, which
means that in case the irq_chip has a irq_startup() callback nothing will
invoke it and also irq_domain_activate_irq() will never be invoked on such
an irq.

Looks like all implementations which use IRQ_NOAUTOEN are not sensitive to
that. It's been broken forever.

Fixing this needs the extra state bit IRQD_ STARTED_UP as we cannot reuse
the IRQD_ACTIVATED bit because some of the interrupts are actually
activated before they are requested.

Too tired to fix that now, but I'll have a look tomorrow. Once this is
fixed, you can add the extra bits to prevent this disable/enable calls
which cause the imbalance deeper down.

Thanks,

	tglx