[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.20.1705290848030.1916@nanos>
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