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

[Thomas Gleixner]

On Sat, 27 May 2017, Jeffy Chen wrote:

> If a irq is already disabled, irq_shutdown may try to disable it again,
> for example:
> devm_request_irq->irq_startup->irq_enable
> disable_irq					<-- disabled
> devm_free_irq->irq_shutdown			<-- disable it again
> 
> This would confuse some chips which require balanced irq enable and
> disable, for example pinctrl-rockchip & pinctrl-nomadik.

"would confuse" is an interesting technical term. Can you please start to
explain things in coherent sentences so people who do not have detailed
knowledge of the problem at hand can understand it?

> Add a state check before calling irq_disable to prevent that.

So you analyzed in half an hour that all of this is correct and fixes all
possible imbalances, right?  Really impressive!

You just forgot to mention this in the changelog.

> v2: Rewrite commit message.
> v3: Rewrite commit message and not skip irq_shutdown.

This does not belong into the changelog and having that information twice
is more than pointless.

Before you send yet another version of this within 5 minutes, can you
please sit down and analyze all the possible imbalance scenarios?

I'm not going to apply hastiliy cobbled together workarounds which "fix"
just half of the problem space. This is not random driver code which breaks
ONE type of machine. This is core code which has the potential to break ALL
machines out there in one go.

Either you come up with a properly analyzed solution which addresses all
possible imbalanced invocations or you have to wait until I find some time
to look at it myself.

Thanks,

	tglx