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;