This addresses a bug in the irqchip core that was triggered by new code assuming a strict semantic order of the irqchip calls:
.irq_request_resources() .irq_enable() .irq_disable() .irq_release_resources()
Mostly this is working fine when handled inside manage.c, the calls are strictly happening in the above assumed order.
However on a Qualcomm platform I get the following in dmesg:
WARNING: CPU: 0 PID: 1 at ../drivers/gpio/gpiolib.c:3513 gpiochip_irq_enable+0x18/0x44 Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.20.0-rc4-00002-g1b8a75b25c6e-dirty #9 Hardware name: Generic DT based system [<c03124ac>] (unwind_backtrace) from [<c030d994>] (show_stack+0x10/0x14) [<c030d994>] (show_stack) from [<c0b48aec>] (dump_stack+0x78/0x8c) [<c0b48aec>] (dump_stack) from [<c03213e4>] (__warn+0xe0/0xf8) [<c03213e4>] (__warn) from [<c0321514>] (warn_slowpath_null+0x40/0x48) [<c0321514>] (warn_slowpath_null) from [<c06ad5d0>] (gpiochip_irq_enable+0x18/0x44) [<c06ad5d0>] (gpiochip_irq_enable) from [<c0371198>] (irq_enable+0x44/0x64) [<c0371198>] (irq_enable) from [<c0371258>] (__irq_startup+0xa0/0xa8) [<c0371258>] (__irq_startup) from [<c03712ac>] (irq_startup+0x4c/0x130) [<c03712ac>] (irq_startup) from [<c03716d0>] (irq_set_chained_handler_and_data+0x4c/0x78) [<c03716d0>] (irq_set_chained_handler_and_data) from [<c081c17c>] (pm8xxx_probe+0x160/0x22c) [<c081c17c>] (pm8xxx_probe) from [<c07f439c>] (platform_drv_probe+0x48/0x98)
What happens is that when the pm8xxx driver tries to register a chained IRQ irq_set_chained_handler_and_data() will eventually set the type and call irq_startup() and thus the .irq_enable() callback on the irqchip.
This irqchip is in drivers/pinctrl/qcom/pinctrl-msm.c and known as TLMM.
However, the irqchip core helpers in GPIO assumes that the .irq_request_resources() callback is always called before .irq_enable(), and the latter is where module refcount and also gpiochip_lock_as_irq() is normally called.
When .irq_enable() is called without .irq_request_resources() having been called first, it seems like we are enabling an IRQ on a GPIO line that has not first been locked to be used as IRQ and we get the above warning. This happens since as of commit 461c1a7d4733d ("gpiolib: override irq_enable/disable") this is strictly assumed for all GPIO-based IRQs.
As it seems reasonable to assume that .irq_request_resources() is always strictly called before .irq_enable(), we add the irq_[request|release]_resources() functions to the internal API and call them also when adding a chained irqchip to an IRQ.
I am a bit uncertain about the call site for irq_released_resources() inside chip.c, but it appears this path is for uninstalling a chained handler.
Cc: stable@vger.kernel.org Cc: Bjorn Andersson bjorn.andersson@linaro.org Cc: Hans Verkuil hverkuil@xs4all.nl Fixes: 461c1a7d4733d ("gpiolib: override irq_enable/disable") Signed-off-by: Linus Walleij linus.walleij@linaro.org --- kernel/irq/chip.c | 2 ++ kernel/irq/internals.h | 3 +++ kernel/irq/manage.c | 4 ++-- 3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index a2b3d9de999c..eef67a0b1889 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -947,6 +947,7 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle, if (desc->irq_data.chip != &no_irq_chip) mask_ack_irq(desc); irq_state_set_disabled(desc); + irq_release_resources(desc); if (is_chained) desc->action = NULL; desc->depth = 1; @@ -974,6 +975,7 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle, irq_settings_set_norequest(desc); irq_settings_set_nothread(desc); desc->action = &chained_action; + irq_request_resources(desc); irq_activate_and_startup(desc, IRQ_RESEND); } } diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h index ca6afa267070..f408a7544c6a 100644 --- a/kernel/irq/internals.h +++ b/kernel/irq/internals.h @@ -75,6 +75,9 @@ extern void __enable_irq(struct irq_desc *desc); #define IRQ_START_FORCE true #define IRQ_START_COND false
+extern int irq_request_resources(struct irq_desc *desc); +extern void irq_release_resources(struct irq_desc *desc); + extern int irq_activate(struct irq_desc *desc); extern int irq_activate_and_startup(struct irq_desc *desc, bool resend); extern int irq_startup(struct irq_desc *desc, bool resend, bool force); diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 9dbdccab3b6a..38bb0ec07180 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1108,7 +1108,7 @@ static int irq_setup_forced_threading(struct irqaction *new) return 0; }
-static int irq_request_resources(struct irq_desc *desc) +int irq_request_resources(struct irq_desc *desc) { struct irq_data *d = &desc->irq_data; struct irq_chip *c = d->chip; @@ -1116,7 +1116,7 @@ static int irq_request_resources(struct irq_desc *desc) return c->irq_request_resources ? c->irq_request_resources(d) : 0; }
-static void irq_release_resources(struct irq_desc *desc) +void irq_release_resources(struct irq_desc *desc) { struct irq_data *d = &desc->irq_data; struct irq_chip *c = d->chip;
On Thu, Nov 29, 2018 at 2:31 PM Linus Walleij linus.walleij@linaro.org wrote:
This addresses a bug in the irqchip core that was triggered by new code assuming a strict semantic order of the irqchip calls:
.irq_request_resources() .irq_enable() .irq_disable() .irq_release_resources()
Mostly this is working fine when handled inside manage.c, the calls are strictly happening in the above assumed order.
However on a Qualcomm platform I get the following in dmesg:
WARNING: CPU: 0 PID: 1 at ../drivers/gpio/gpiolib.c:3513 gpiochip_irq_enable+0x18/0x44 Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.20.0-rc4-00002-g1b8a75b25c6e-dirty #9 Hardware name: Generic DT based system [<c03124ac>] (unwind_backtrace) from [<c030d994>] (show_stack+0x10/0x14) [<c030d994>] (show_stack) from [<c0b48aec>] (dump_stack+0x78/0x8c) [<c0b48aec>] (dump_stack) from [<c03213e4>] (__warn+0xe0/0xf8) [<c03213e4>] (__warn) from [<c0321514>] (warn_slowpath_null+0x40/0x48) [<c0321514>] (warn_slowpath_null) from [<c06ad5d0>] (gpiochip_irq_enable+0x18/0x44) [<c06ad5d0>] (gpiochip_irq_enable) from [<c0371198>] (irq_enable+0x44/0x64) [<c0371198>] (irq_enable) from [<c0371258>] (__irq_startup+0xa0/0xa8) [<c0371258>] (__irq_startup) from [<c03712ac>] (irq_startup+0x4c/0x130) [<c03712ac>] (irq_startup) from [<c03716d0>] (irq_set_chained_handler_and_data+0x4c/0x78) [<c03716d0>] (irq_set_chained_handler_and_data) from [<c081c17c>] (pm8xxx_probe+0x160/0x22c) [<c081c17c>] (pm8xxx_probe) from [<c07f439c>] (platform_drv_probe+0x48/0x98)
What happens is that when the pm8xxx driver tries to register a chained IRQ irq_set_chained_handler_and_data() will eventually set the type and call irq_startup() and thus the .irq_enable() callback on the irqchip.
This irqchip is in drivers/pinctrl/qcom/pinctrl-msm.c and known as TLMM.
However, the irqchip core helpers in GPIO assumes that the .irq_request_resources() callback is always called before .irq_enable(), and the latter is where module refcount and also gpiochip_lock_as_irq() is normally called.
When .irq_enable() is called without .irq_request_resources() having been called first, it seems like we are enabling an IRQ on a GPIO line that has not first been locked to be used as IRQ and we get the above warning. This happens since as of commit 461c1a7d4733d ("gpiolib: override irq_enable/disable") this is strictly assumed for all GPIO-based IRQs.
As it seems reasonable to assume that .irq_request_resources() is always strictly called before .irq_enable(), we add the irq_[request|release]_resources() functions to the internal API and call them also when adding a chained irqchip to an IRQ.
I am a bit uncertain about the call site for irq_released_resources() inside chip.c, but it appears this path is for uninstalling a chained handler.
Cc: stable@vger.kernel.org Cc: Bjorn Andersson bjorn.andersson@linaro.org Cc: Hans Verkuil hverkuil@xs4all.nl Fixes: 461c1a7d4733d ("gpiolib: override irq_enable/disable") Signed-off-by: Linus Walleij linus.walleij@linaro.org
IRQ folks: did you have time to look at this?
It's a very real regression for me.
Yours, Linus Walleij
On Fri, 7 Dec 2018, Linus Walleij wrote:
Sorry for answering late.
On Thu, Nov 29, 2018 at 2:31 PM Linus Walleij linus.walleij@linaro.org wrote:
This irqchip is in drivers/pinctrl/qcom/pinctrl-msm.c and known as TLMM.
TLMM == Totally Lost Manufacturer Mess?
However, the irqchip core helpers in GPIO assumes that the .irq_request_resources() callback is always called before .irq_enable(), and the latter is where module refcount and also gpiochip_lock_as_irq() is normally called.
When .irq_enable() is called without .irq_request_resources() having been called first, it seems like we are enabling an IRQ on a GPIO line that has not first been locked to be used as IRQ and we get the above warning. This happens since as of commit 461c1a7d4733d ("gpiolib: override irq_enable/disable") this is strictly assumed for all GPIO-based IRQs.
As it seems reasonable to assume that .irq_request_resources() is always strictly called before .irq_enable(), we add the irq_[request|release]_resources() functions to the internal API and call them also when adding a chained irqchip to an IRQ.
I am a bit uncertain about the call site for irq_released_resources() inside chip.c, but it appears this path is for uninstalling a chained handler.
You cannot invoke those callbacks from __irq_do_set_handler() as that is holding the irq descriptor lock with interrupts disabled.
The calling convention for irq_released_resources() is that it's called with the bus lock held (if the chip provides the bus lock callbacks), but definitely not with desc->lock held.
Needs more thought. Btw, the uninstall path does not invoke irq_deactive() either.... I so hate that chained handler mess....
Thanks,
tglx
On Fri, Dec 7, 2018 at 12:52 PM Thomas Gleixner tglx@linutronix.de wrote:
This irqchip is in drivers/pinctrl/qcom/pinctrl-msm.c and known as TLMM.
TLMM == Totally Lost Manufacturer Mess?
It's pretty OK for the most part actually.
You cannot invoke those callbacks from __irq_do_set_handler() as that is holding the irq descriptor lock with interrupts disabled.
How typical. Hm I wonder how to fix this properly then.
Needs more thought. Btw, the uninstall path does not invoke irq_deactive() either.... I so hate that chained handler mess....
I think it is just extremely uncommon to remove a chained handler (I don't know if anything exercises that path even) that is why we don't see any fallout from it. It's probably just untested code.
Do you see that chained handlers have any merit at all or should they all be moved to nested? The question needs asking, but IIUC there are performance benefits with chaining as opposed to nesting. :/
Yours, Linus Walleij
Linus,
On Fri, 7 Dec 2018, Linus Walleij wrote:
On Fri, Dec 7, 2018 at 12:52 PM Thomas Gleixner tglx@linutronix.de wrote:
Needs more thought. Btw, the uninstall path does not invoke irq_deactive() either.... I so hate that chained handler mess....
I think it is just extremely uncommon to remove a chained handler (I don't know if anything exercises that path even) that is why we don't see any fallout from it. It's probably just untested code.
git grep irq_set_chained.*NULL
tells that there are users. Whether any of this is ever invoked is a different questions.
Do you see that chained handlers have any merit at all or should they all be moved to nested? The question needs asking, but IIUC there are performance benefits with chaining as opposed to nesting. :/
The nested case in gpio land is about nesting in irq thread context, which is obviously slower. So instead of nesting you can just use a regular interrupt for demultiplexing.
The thing about chained is:
1) It's hidden, i.e. not exposed in /proc/interrupt
2) It's not protected against runaway interrupts, which has happened in x86 land. We've converted those over to use regular interrupts to catch that case and shutdown the offending interrupt. e.g. ba714a9c1dea
3) All the chained handlers have their own flow control inside the handler.
The chained performance benefit compared to a regular handler is minimal. The main difference are a few conditionals and the desc->action indirection. You probably can measure it with a high interrupt load, but I'm still not convinced that it outweighs the downsides for a lot of the places which use chained interrupts. I can see the benefit for very low level interrupts, but even there we had hard to debug issues with some ARM SoCs which ran into interrupt storms.
But hey, we surely can fix that chained issue. It's not going to be pretty though :)
Thanks,
tglx
linux-stable-mirror@lists.linaro.org
-
Linus Walleij -
Thomas Gleixner