Messages in this thread | | | Date | Mon, 29 May 2017 08:56:00 +0200 (CEST) | From | Thomas Gleixner <> | 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@linutronix.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
| |