* [PATCH 1/9] irq: add irq_set_chained_handler_and_data()
@ 2015-06-16 22:06 Russell King
2015-06-16 22:29 ` [PATCH] GPU: ipu: fix lockup caused by pending chained interrupts Russell King
2015-06-17 14:10 ` [PATCH 1/9] irq: add irq_set_chained_handler_and_data() Thomas Gleixner
0 siblings, 2 replies; 5+ messages in thread
From: Russell King @ 2015-06-16 22:06 UTC (permalink / raw
To: linux-arm-kernel
Driver authors seem to get the ordering of irq_set_chained_handler()
and irq_set_handler_data() wrong - ordering the former before the
latter. This opens a race window where, if there is an interrupt
pending, the handler will be called between these two calls,
potentially resulting in an oops.
Provide a single interface to set both of these together, especially
as that's commonly what is required.
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
It probably makes sense to convert everything over to this new
registration function, and kill all users of irq_set_chained_handler()
to prevent this problem recurring. Thoughts?
include/linux/irq.h | 9 +++++++++
kernel/irq/chip.c | 45 ++++++++++++++++++++++++++++++++++-----------
2 files changed, 43 insertions(+), 11 deletions(-)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 62c6901cab55..4942cbc379bb 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -517,6 +517,15 @@ irq_set_chained_handler(unsigned int irq, irq_flow_handler_t handle)
__irq_set_handler(irq, handle, 1, NULL);
}
+/*
+ * Set a highlevel chained flow handler and its data for a given IRQ.
+ * (a chained handler is automatically enabled and set to
+ * IRQ_NOREQUEST, IRQ_NOPROBE, and IRQ_NOTHREAD)
+ */
+void
+irq_set_chained_handler_and_data(unsigned int irq, irq_flow_handler_t handle,
+ void *data);
+
void irq_modify_status(unsigned int irq, unsigned long clr, unsigned long set);
static inline void irq_set_status_flags(unsigned int irq, unsigned long set)
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index eb9a4ea394ab..92bed9010bc6 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -719,15 +719,9 @@ void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc)
}
void
-__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
- const char *name)
+__irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
+ int is_chained, const char *name)
{
- unsigned long flags;
- struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, 0);
-
- if (!desc)
- return;
-
if (!handle) {
handle = handle_bad_irq;
} else {
@@ -749,13 +743,13 @@ __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
* right away.
*/
if (WARN_ON(is_chained))
- goto out;
+ return;
/* Try the parent */
irq_data = irq_data->parent_data;
}
#endif
if (WARN_ON(!irq_data || irq_data->chip == &no_irq_chip))
- goto out;
+ return;
}
/* Uninstall? */
@@ -774,12 +768,41 @@ __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
irq_settings_set_nothread(desc);
irq_startup(desc, true);
}
-out:
+}
+
+void
+__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
+ const char *name)
+{
+ unsigned long flags;
+ struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, 0);
+
+ if (!desc)
+ return;
+
+ __irq_do_set_handler(desc, handle, is_chained, name);
irq_put_desc_busunlock(desc, flags);
}
EXPORT_SYMBOL_GPL(__irq_set_handler);
void
+irq_set_chained_handler_and_data(unsigned int irq, irq_flow_handler_t handle,
+ void *data)
+{
+ unsigned long flags;
+ struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, 0);
+
+ if (!desc)
+ return;
+
+ __irq_do_set_handler(desc, handle, 1, NULL);
+ desc->irq_data.handler_data = data;
+
+ irq_put_desc_busunlock(desc, flags);
+}
+EXPORT_SYMBOL_GPL(irq_set_chained_handler_and_data);
+
+void
irq_set_chip_and_handler_name(unsigned int irq, struct irq_chip *chip,
irq_flow_handler_t handle, const char *name)
{
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] GPU: ipu: fix lockup caused by pending chained interrupts
2015-06-16 22:06 [PATCH 1/9] irq: add irq_set_chained_handler_and_data() Russell King
@ 2015-06-16 22:29 ` Russell King
2015-06-19 14:36 ` Philipp Zabel
2015-06-17 14:10 ` [PATCH 1/9] irq: add irq_set_chained_handler_and_data() Thomas Gleixner
1 sibling, 1 reply; 5+ messages in thread
From: Russell King @ 2015-06-16 22:29 UTC (permalink / raw
To: linux-arm-kernel
Even with the oops fixed by a previous patch, the system still fails to
kexec, due to a stuck chained interrupt locking the system. We must
disable the child interrupts prior to setting up the irq chip to ensure
we don't get stuck here.
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
Maybe this is all that's really needed... would also be nice if this
path had an etry in MAINTAINERS...
drivers/gpu/ipu-v3/ipu-common.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
index 6d2f39d36e44..00f2058944e5 100644
--- a/drivers/gpu/ipu-v3/ipu-common.c
+++ b/drivers/gpu/ipu-v3/ipu-common.c
@@ -1107,6 +1107,9 @@ static int ipu_irq_init(struct ipu_soc *ipu)
return ret;
}
+ for (i = 0; i < IPU_NUM_IRQS; i += 32)
+ ipu_cm_write(ipu, 0, IPU_INT_CTRL(i / 32));
+
for (i = 0; i < IPU_NUM_IRQS; i += 32) {
gc = irq_get_domain_generic_chip(ipu->domain, i);
gc->reg_base = ipu->cm_reg;
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 1/9] irq: add irq_set_chained_handler_and_data()
2015-06-16 22:06 [PATCH 1/9] irq: add irq_set_chained_handler_and_data() Russell King
2015-06-16 22:29 ` [PATCH] GPU: ipu: fix lockup caused by pending chained interrupts Russell King
@ 2015-06-17 14:10 ` Thomas Gleixner
2015-06-17 19:38 ` Russell King - ARM Linux
1 sibling, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2015-06-17 14:10 UTC (permalink / raw
To: linux-arm-kernel
On Tue, 16 Jun 2015, Russell King wrote:
> Driver authors seem to get the ordering of irq_set_chained_handler()
> and irq_set_handler_data() wrong - ordering the former before the
> latter. This opens a race window where, if there is an interrupt
> pending, the handler will be called between these two calls,
> potentially resulting in an oops.
>
> Provide a single interface to set both of these together, especially
> as that's commonly what is required.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> It probably makes sense to convert everything over to this new
> registration function, and kill all users of irq_set_chained_handler()
> to prevent this problem recurring. Thoughts?
Yes. Classic coccinelle problem. Here is the semantic patch:
@@
expression E1, E2, E3;
@@
-irq_set_chained_handler(E1, E3);
...
-irq_set_handler_data(E1, E2);
+irq_set_chained_handler_and_data(E1, E3, E2);
This finds and corrects all instances which get it wrong:
arch:
arm/common/locomo.c | 3 +--
arm/common/sa1111.c | 3 +--
arm/mach-gemini/gpio.c | 4 ++--
avr32/mach-at32ap/extint.c | 3 +--
m68k/mac/psc.c | 12 ++++--------
mips/ath25/ar2315.c | 4 ++--
mips/ath25/ar5312.c | 4 ++--
mips/pci/pci-ar2315.c | 4 ++--
mips/ralink/irq.c | 3 +--
sh/intc/core.c | 5 +++--
drivers:
dma/ipu/ipu_irq.c | 6 ++----
gpio/gpio-bcm-kona.c | 5 +++--
gpio/gpio-davinci.c | 10 ++--------
gpio/gpio-dwapb.c | 4 ++--
gpio/gpio-msic.c | 3 +--
gpio/gpio-mxc.c | 10 +++++-----
gpio/gpio-mxs.c | 4 ++--
gpio/gpio-tegra.c | 4 ++--
gpu/ipu-v3/ipu-common.c | 13 +++++--------
irqchip/irq-keystone.c | 4 ++--
irqchip/spear-shirq.c | 3 +--
mfd/pm8921-core.c | 6 ++----
mfd/t7l66xb.c | 3 +--
mfd/tc6393xb.c | 3 +--
pci/host/pci-keystone.c | 7 +++----
pinctrl/mediatek/pinctrl-mtk-common.c | 3 +--
pinctrl/pinctrl-adi2.c | 4 ++--
pinctrl/pinctrl-st.c | 4 ++--
pinctrl/samsung/pinctrl-exynos.c | 4 ++--
pinctrl/samsung/pinctrl-s3c24xx.c | 3 +--
pinctrl/samsung/pinctrl-s3c64xx.c | 8 ++++----
pinctrl/sunxi/pinctrl-sunxi.c | 6 +++---
spmi/spmi-pmic-arb.c | 6 ++----
33 files changed, 70 insertions(+), 98 deletions(-)
If we revert the search pattern we get the ones which got it right:
@@
expression E1, E2, E3;
@@
-irq_set_handler_data(E1, E2);
...
-irq_set_chained_handler(E1, E3);
+irq_set_chained_handler_and_data(E1, E3, E2);
That handles another bunch and leaves us with 135 instances of
irq_set_chained_handler() which do not set handler data. So its
trivial to change them to
irq_set_chained_handler_and_data(irq, handler, NULL);
and then remove irq_set_chained_handler()
If thats ok for you, then i apply the lot you sent and run the cocci
scripts right at rc1. I have another set of transformations in that
area pending.
Thanks,
tglx
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/9] irq: add irq_set_chained_handler_and_data()
2015-06-17 14:10 ` [PATCH 1/9] irq: add irq_set_chained_handler_and_data() Thomas Gleixner
@ 2015-06-17 19:38 ` Russell King - ARM Linux
0 siblings, 0 replies; 5+ messages in thread
From: Russell King - ARM Linux @ 2015-06-17 19:38 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jun 17, 2015 at 04:10:02PM +0200, Thomas Gleixner wrote:
> On Tue, 16 Jun 2015, Russell King wrote:
>
> > Driver authors seem to get the ordering of irq_set_chained_handler()
> > and irq_set_handler_data() wrong - ordering the former before the
> > latter. This opens a race window where, if there is an interrupt
> > pending, the handler will be called between these two calls,
> > potentially resulting in an oops.
> >
> > Provide a single interface to set both of these together, especially
> > as that's commonly what is required.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> > It probably makes sense to convert everything over to this new
> > registration function, and kill all users of irq_set_chained_handler()
> > to prevent this problem recurring. Thoughts?
>
> Yes. Classic coccinelle problem. Here is the semantic patch:
>
> @@
> expression E1, E2, E3;
> @@
> -irq_set_chained_handler(E1, E3);
> ...
> -irq_set_handler_data(E1, E2);
> +irq_set_chained_handler_and_data(E1, E3, E2);
>
> This finds and corrects all instances which get it wrong:
>
> arch:
> arm/common/locomo.c | 3 +--
> arm/common/sa1111.c | 3 +--
> arm/mach-gemini/gpio.c | 4 ++--
> avr32/mach-at32ap/extint.c | 3 +--
> m68k/mac/psc.c | 12 ++++--------
> mips/ath25/ar2315.c | 4 ++--
> mips/ath25/ar5312.c | 4 ++--
> mips/pci/pci-ar2315.c | 4 ++--
> mips/ralink/irq.c | 3 +--
> sh/intc/core.c | 5 +++--
>
> drivers:
> dma/ipu/ipu_irq.c | 6 ++----
> gpio/gpio-bcm-kona.c | 5 +++--
> gpio/gpio-davinci.c | 10 ++--------
> gpio/gpio-dwapb.c | 4 ++--
> gpio/gpio-msic.c | 3 +--
> gpio/gpio-mxc.c | 10 +++++-----
> gpio/gpio-mxs.c | 4 ++--
> gpio/gpio-tegra.c | 4 ++--
> gpu/ipu-v3/ipu-common.c | 13 +++++--------
> irqchip/irq-keystone.c | 4 ++--
> irqchip/spear-shirq.c | 3 +--
> mfd/pm8921-core.c | 6 ++----
> mfd/t7l66xb.c | 3 +--
> mfd/tc6393xb.c | 3 +--
> pci/host/pci-keystone.c | 7 +++----
> pinctrl/mediatek/pinctrl-mtk-common.c | 3 +--
> pinctrl/pinctrl-adi2.c | 4 ++--
> pinctrl/pinctrl-st.c | 4 ++--
> pinctrl/samsung/pinctrl-exynos.c | 4 ++--
> pinctrl/samsung/pinctrl-s3c24xx.c | 3 +--
> pinctrl/samsung/pinctrl-s3c64xx.c | 8 ++++----
> pinctrl/sunxi/pinctrl-sunxi.c | 6 +++---
> spmi/spmi-pmic-arb.c | 6 ++----
> 33 files changed, 70 insertions(+), 98 deletions(-)
>
> If we revert the search pattern we get the ones which got it right:
>
> @@
> expression E1, E2, E3;
> @@
> -irq_set_handler_data(E1, E2);
> ...
> -irq_set_chained_handler(E1, E3);
> +irq_set_chained_handler_and_data(E1, E3, E2);
>
> That handles another bunch and leaves us with 135 instances of
> irq_set_chained_handler() which do not set handler data. So its
> trivial to change them to
>
> irq_set_chained_handler_and_data(irq, handler, NULL);
>
> and then remove irq_set_chained_handler()
>
> If thats ok for you, then i apply the lot you sent and run the cocci
> scripts right at rc1. I have another set of transformations in that
> area pending.
Totally fine with that.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] GPU: ipu: fix lockup caused by pending chained interrupts
2015-06-16 22:29 ` [PATCH] GPU: ipu: fix lockup caused by pending chained interrupts Russell King
@ 2015-06-19 14:36 ` Philipp Zabel
0 siblings, 0 replies; 5+ messages in thread
From: Philipp Zabel @ 2015-06-19 14:36 UTC (permalink / raw
To: linux-arm-kernel
Am Dienstag, den 16.06.2015, 23:29 +0100 schrieb Russell King:
> Even with the oops fixed by a previous patch, the system still fails to
> kexec, due to a stuck chained interrupt locking the system. We must
> disable the child interrupts prior to setting up the irq chip to ensure
> we don't get stuck here.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Applied, thank you.
> Maybe this is all that's really needed... would also be nice if this
> path had an etry in MAINTAINERS...
I'll send a patch to add it to the imx-drm driver section.
regards
Philipp
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-06-19 14:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-16 22:06 [PATCH 1/9] irq: add irq_set_chained_handler_and_data() Russell King
2015-06-16 22:29 ` [PATCH] GPU: ipu: fix lockup caused by pending chained interrupts Russell King
2015-06-19 14:36 ` Philipp Zabel
2015-06-17 14:10 ` [PATCH 1/9] irq: add irq_set_chained_handler_and_data() Thomas Gleixner
2015-06-17 19:38 ` Russell King - ARM Linux
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).