Date:   Mon, 29 May 2017 08:56:00 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Tomasz Figa <tfiga@...omium.org>
cc:     Jeffy Chen <jeffy.chen@...k-chips.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Brian Norris <briannorris@...omium.org>,
        Douglas Anderson <dianders@...omium.org>
Subject: Re: [PATCH v3] genirq: Check irq disabled & masked states in
 irq_shutdown

Tomasz,

On Sun, 28 May 2017, Tomasz Figa wrote:
> On Sat, May 27, 2017 at 8:12 PM, Thomas Gleixner <tglx@...utronix.de> wrote:
> I think we might simply have a language barrier here unfortunately. I
> agree, though, that we need a better description of the problem. Next
> time we will help Jeffy with polishing the commit message. Please
> forgive him this time, as he is still learning "the art" of mainline
> patch submission.

No problem.

> The IRQ functionality provided by the pinctrl-rockchip has a power
> saving mechanism that attempts to gate clocks whenever there are no
> enabled interrupts. Currently the driver calls clk_enable() in
> .irq_enable() and clk_disable() in .irq_disable() callbacks of its IRQ
> chip. However there is no mention about ordering or reference counting
> of those in code documentation (which is likely to mean that there are
> no ordering guarantees and/or unbalanced calls may happen, please
> correct me if I'm wrong).

Correct. We try to avoid unbalanced calls, but it's not complete.

> We noticed that following scenario causes unbalanced clk_disable() calls:
>  1) request_irq(),
>  2) disable_irq(),
>  3) free_irq().
> 
> After checking what's going on, we found that free_irq() ends up
> calling irq_shutdown(), which defaults to chip's .irq_disable() if it
> doesn't have .irq_shutdown() specified. This means that regardless of
> whether disable_irq() was or wasn't called before, there is one more
> call to .irq_disable() after free_irq(), which is the reason for our
> unbalanced clk_disable() call from pinctrl-rockchip IRQ chip.
> 
> Now, we can simply hack the driver to not rely on any ordering of
> .enable/disable_irq() calls, but we thought it would make more sense
> to actually try to figure out whether it's the expected behavior from
> the IRQ chip core code.

Yes. But there are several ways this can happen not only via the above
scenario.

> > 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.
> 
> As I explained above, we might have introduced unnecessary confusion
> here. Please forgive us. On the other hand, I'd like to ask for a bit
> more understanding, as we have people involved in this, who are still
> learning the tough art of upstream submission, potentially also behind
> a language barrier and I'm pretty much sure they are more than happy
> to address all your concerns, but would be much more motivated to do
> the right thing if guided in a bit more humane manner. Thanks in
> advance.

Sorry, if I replied more harsh than necessary. I have neither a problem
with language barriers nor with patches which are not perfect.

The reason why I got a bit grumpy was the fact that I pointed out on my
reply to V2:

 ... irq_shutdown() is only one place where this can happen. This needs
 more thought ...

as a reaction I get yet another variant of the same patch fiddling in
exactly one function, i.e. irq_shutdown.

I didn't want to offend Jerry, but may I please ask that my review comments
are taken seriously and properly addressed. If there are questions then
better ask than ignore.

Thanks,

	tglx

Powered by blists - more mailing lists