Message-Id: <20181129133119.29387-1-linus.walleij@linaro.org>
Date:   Thu, 29 Nov 2018 14:31:19 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <marc.zyngier@....com>,
        Jason Cooper <jason@...edaemon.net>
Cc:     linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        stable@...r.kernel.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Hans Verkuil <hverkuil@...all.nl>
Subject: [PATCH] irq: Request and release resources for chained IRQs

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@...r.kernel.org
Cc: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: Hans Verkuil <hverkuil@...all.nl>
Fixes: 461c1a7d4733d ("gpiolib: override irq_enable/disable")
Signed-off-by: Linus Walleij <linus.walleij@...aro.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;
-- 
2.19.1

Powered by blists - more mailing lists