All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paul Burton <paul.burton@imgtec.com>
To: Thomas Gleixner <tglx@linutronix.de>, Ralf Baechle <ralf@linux-mips.org>
Cc: <dianders@chromium.org>, James Hogan <james.hogan@imgtec.com>,
	Brian Norris <briannorris@chromium.org>,
	Jason Cooper <jason@lakedaemon.net>, <jeffy.chen@rock-chips.com>,
	Marc Zyngier <marc.zyngier@arm.com>,
	<linux-kernel@vger.kernel.org>, <linux-mips@linux-mips.org>,
	<tfiga@chromium.org>, Paul Burton <paul.burton@imgtec.com>
Subject: [RFC PATCH v1 1/9] genirq: Allow shared interrupt users to opt into IRQ_NOAUTOEN
Date: Thu, 7 Sep 2017 16:25:34 -0700	[thread overview]
Message-ID: <20170907232542.20589-2-paul.burton@imgtec.com> (raw)
In-Reply-To: <20170907232542.20589-1-paul.burton@imgtec.com>

Shared interrupts which aren't automatically enabled during setup (ie.
which have the IRQ_NOAUTOEN flag set) can be problematic if one or more
users of the shared interrupt aren't expecting the IRQ_NOAUTOEN
behaviour. This led to a warning being added when a combination of
IRQ_NOAUTOEN & IRQF_SHARED are used by commit 04c848d39879 ("genirq:
Warn when IRQ_NOAUTOEN is used with shared interrupts").

There are however legitimate cases where a shared interrupt which isn't
automatically enabled may make sense. One such case is shared percpu
interrupts, which we don't currently support but will with subsequent
patches. For percpu interrupts the IRQ_NOAUTOEN flag is automatically
set by irq_set_percpu_devid_flags() because it would be inconsistent to
automatically enable the interrupt on the CPU that calls
setup_percpu_irq() but not on others, and anything else would at best be
an expensive operation with few legitimate uses. The use of IRQ_NOAUTOEN
means that percpu interrupts cannot currently be shared without running
into the warning that commit 04c848d39879 ("genirq: Warn when
IRQ_NOAUTOEN is used with shared interrupts") introduced.

This patch allows for the future possibility of shared interrupts that
are not automatically enabled, by introducing an IRQF_NOAUTOEN flag
which users of a shared interrupt can set to state that they are
prepared for the IRQ_NOAUTOEN behaviour. We then require that all
actions associated with the shared interrupt share the same value for
the IRQF_NOAUTOEN flag, and that if IRQ_NOAUTOEN is set before any
actions with IRQF_SHARED are associated with the interrupt then any
actions without the IRQF_NOAUTOEN flag set are rejected, with
__setup_irq() returning -EINVAL.

In some ways this means that we go further than commit 04c848d39879
("genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts") did,
in that any existing users of shared interrupts with IRQ_NOAUTOEN won't
only trigger a warning but will fail to setup their interrupt handler
entirely. In others this leaves us with an out for legitimate users
which will be able to opt-in to the IRQ_NOAUTOEN behaviour by setting
IRQF_NOAUTOEN.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: James Hogan <james.hogan@imgtec.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
---

 include/linux/interrupt.h |  2 ++
 kernel/irq/manage.c       | 58 ++++++++++++++++++++++++++++++++++++-----------
 kernel/irq/settings.h     |  5 ++++
 3 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 59ba11661b6e..a27e22275ca7 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -62,6 +62,7 @@
  *                interrupt handler after suspending interrupts. For system
  *                wakeup devices users need to implement wakeup detection in
  *                their interrupt handlers.
+ * IRQF_NOAUTOEN - Don't automatically enable the interrupt during setup.
  */
 #define IRQF_SHARED		0x00000080
 #define IRQF_PROBE_SHARED	0x00000100
@@ -75,6 +76,7 @@
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
 #define IRQF_COND_SUSPEND	0x00040000
+#define IRQF_NOAUTOEN		0x00080000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 573dc52b0806..fb5445a4a359 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1229,15 +1229,19 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		 * agree on ONESHOT.
 		 */
 		unsigned int oldtype = irqd_get_trigger_type(&desc->irq_data);
+		unsigned int must_match;
+
+		/*
+		 * These flags must have the same value for all actions
+		 * registered for a shared interrupt.
+		 */
+		must_match = IRQF_ONESHOT |
+			     IRQF_PERCPU |
+			     IRQF_NOAUTOEN;
 
 		if (!((old->flags & new->flags) & IRQF_SHARED) ||
 		    (oldtype != (new->flags & IRQF_TRIGGER_MASK)) ||
-		    ((old->flags ^ new->flags) & IRQF_ONESHOT))
-			goto mismatch;
-
-		/* All handlers must agree on per-cpuness */
-		if ((old->flags & IRQF_PERCPU) !=
-		    (new->flags & IRQF_PERCPU))
+		    ((old->flags ^ new->flags) & must_match))
 			goto mismatch;
 
 		/* add new interrupt at end of irq queue */
@@ -1313,6 +1317,38 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		goto out_unlock;
 	}
 
+	/*
+	 * Using shared interrupts with the IRQ_NOAUTOEN flag can be risky if
+	 * the flag is set when one or more of the drivers sharing the IRQ
+	 * don't expect it. For example if driver A does:
+	 *
+	 *   irq_set_status_flags(shared_irq, IRQ_NOAUTOEN);
+	 *   request_irq(shared_irq, handler_a, IRQF_SHARED, "a", dev_a);
+	 *
+	 * Then driver B does:
+	 *
+	 *   request_irq(shared_irq, handler_b, IRQF_SHARED, "b", dev_b);
+	 *
+	 * Driver B has no idea that driver A set the IRQ_NOAUTOEN flag, and
+	 * thus may expect that the shared_irq is enabled after its call to
+	 * request_irq(). It may then miss interrupts that it was expecting to
+	 * receive.
+	 *
+	 * We therefore require that if a shared IRQ is used with IRQ_NOAUTOEN
+	 * then all drivers sharing it explicitly declare that they are aware
+	 * of the fact that the interrupt won't be automatically enabled, by
+	 * setting the IRQF_NOAUTOEN flag in their struct irqaction or
+	 * providing it to request_irq().
+	 */
+	if (WARN((new->flags && IRQF_SHARED) &&
+		 !irq_settings_can_autoenable(desc) &&
+		 !(new->flags & IRQF_NOAUTOEN),
+		 "shared irq %d isn't automatically enabled, but the caller doesn't set IRQF_NOAUTOEN",
+		 irq)) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	if (!shared) {
 		init_waitqueue_head(&desc->wait_for_threads);
 
@@ -1343,16 +1379,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			irqd_set(&desc->irq_data, IRQD_NO_BALANCING);
 		}
 
+		if (new->flags & IRQF_NOAUTOEN)
+			irq_settings_set_noautoenable(desc);
+
 		if (irq_settings_can_autoenable(desc)) {
 			irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
 		} else {
-			/*
-			 * Shared interrupts do not go well with disabling
-			 * auto enable. The sharing interrupt might request
-			 * it while it's still disabled and then wait for
-			 * interrupts forever.
-			 */
-			WARN_ON_ONCE(new->flags & IRQF_SHARED);
 			/* Undo nested disables: */
 			desc->depth = 1;
 		}
diff --git a/kernel/irq/settings.h b/kernel/irq/settings.h
index 320579d89091..97edef1bd781 100644
--- a/kernel/irq/settings.h
+++ b/kernel/irq/settings.h
@@ -147,6 +147,11 @@ static inline bool irq_settings_can_autoenable(struct irq_desc *desc)
 	return !(desc->status_use_accessors & _IRQ_NOAUTOEN);
 }
 
+static inline void irq_settings_set_noautoenable(struct irq_desc *desc)
+{
+	desc->status_use_accessors |= _IRQ_NOAUTOEN;
+}
+
 static inline bool irq_settings_is_nested_thread(struct irq_desc *desc)
 {
 	return desc->status_use_accessors & _IRQ_NESTED_THREAD;
-- 
2.14.1

WARNING: multiple messages have this Message-ID (diff)
From: Paul Burton <paul.burton@imgtec.com>
To: Thomas Gleixner <tglx@linutronix.de>, Ralf Baechle <ralf@linux-mips.org>
Cc: dianders@chromium.org, James Hogan <james.hogan@imgtec.com>,
	Brian Norris <briannorris@chromium.org>,
	Jason Cooper <jason@lakedaemon.net>,
	jeffy.chen@rock-chips.com, Marc Zyngier <marc.zyngier@arm.com>,
	linux-kernel@vger.kernel.org, linux-mips@linux-mips.org,
	tfiga@chromium.org, Paul Burton <paul.burton@imgtec.com>
Subject: [RFC PATCH v1 1/9] genirq: Allow shared interrupt users to opt into IRQ_NOAUTOEN
Date: Thu, 7 Sep 2017 16:25:34 -0700	[thread overview]
Message-ID: <20170907232542.20589-2-paul.burton@imgtec.com> (raw)
Message-ID: <20170907232534.P4r3HBIN23piRyuEcZEKaVL3aKHuVhghtncZ1no1sEU@z> (raw)
In-Reply-To: <20170907232542.20589-1-paul.burton@imgtec.com>

Shared interrupts which aren't automatically enabled during setup (ie.
which have the IRQ_NOAUTOEN flag set) can be problematic if one or more
users of the shared interrupt aren't expecting the IRQ_NOAUTOEN
behaviour. This led to a warning being added when a combination of
IRQ_NOAUTOEN & IRQF_SHARED are used by commit 04c848d39879 ("genirq:
Warn when IRQ_NOAUTOEN is used with shared interrupts").

There are however legitimate cases where a shared interrupt which isn't
automatically enabled may make sense. One such case is shared percpu
interrupts, which we don't currently support but will with subsequent
patches. For percpu interrupts the IRQ_NOAUTOEN flag is automatically
set by irq_set_percpu_devid_flags() because it would be inconsistent to
automatically enable the interrupt on the CPU that calls
setup_percpu_irq() but not on others, and anything else would at best be
an expensive operation with few legitimate uses. The use of IRQ_NOAUTOEN
means that percpu interrupts cannot currently be shared without running
into the warning that commit 04c848d39879 ("genirq: Warn when
IRQ_NOAUTOEN is used with shared interrupts") introduced.

This patch allows for the future possibility of shared interrupts that
are not automatically enabled, by introducing an IRQF_NOAUTOEN flag
which users of a shared interrupt can set to state that they are
prepared for the IRQ_NOAUTOEN behaviour. We then require that all
actions associated with the shared interrupt share the same value for
the IRQF_NOAUTOEN flag, and that if IRQ_NOAUTOEN is set before any
actions with IRQF_SHARED are associated with the interrupt then any
actions without the IRQF_NOAUTOEN flag set are rejected, with
__setup_irq() returning -EINVAL.

In some ways this means that we go further than commit 04c848d39879
("genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts") did,
in that any existing users of shared interrupts with IRQ_NOAUTOEN won't
only trigger a warning but will fail to setup their interrupt handler
entirely. In others this leaves us with an out for legitimate users
which will be able to opt-in to the IRQ_NOAUTOEN behaviour by setting
IRQF_NOAUTOEN.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: James Hogan <james.hogan@imgtec.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
---

 include/linux/interrupt.h |  2 ++
 kernel/irq/manage.c       | 58 ++++++++++++++++++++++++++++++++++++-----------
 kernel/irq/settings.h     |  5 ++++
 3 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 59ba11661b6e..a27e22275ca7 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -62,6 +62,7 @@
  *                interrupt handler after suspending interrupts. For system
  *                wakeup devices users need to implement wakeup detection in
  *                their interrupt handlers.
+ * IRQF_NOAUTOEN - Don't automatically enable the interrupt during setup.
  */
 #define IRQF_SHARED		0x00000080
 #define IRQF_PROBE_SHARED	0x00000100
@@ -75,6 +76,7 @@
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
 #define IRQF_COND_SUSPEND	0x00040000
+#define IRQF_NOAUTOEN		0x00080000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 573dc52b0806..fb5445a4a359 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1229,15 +1229,19 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		 * agree on ONESHOT.
 		 */
 		unsigned int oldtype = irqd_get_trigger_type(&desc->irq_data);
+		unsigned int must_match;
+
+		/*
+		 * These flags must have the same value for all actions
+		 * registered for a shared interrupt.
+		 */
+		must_match = IRQF_ONESHOT |
+			     IRQF_PERCPU |
+			     IRQF_NOAUTOEN;
 
 		if (!((old->flags & new->flags) & IRQF_SHARED) ||
 		    (oldtype != (new->flags & IRQF_TRIGGER_MASK)) ||
-		    ((old->flags ^ new->flags) & IRQF_ONESHOT))
-			goto mismatch;
-
-		/* All handlers must agree on per-cpuness */
-		if ((old->flags & IRQF_PERCPU) !=
-		    (new->flags & IRQF_PERCPU))
+		    ((old->flags ^ new->flags) & must_match))
 			goto mismatch;
 
 		/* add new interrupt at end of irq queue */
@@ -1313,6 +1317,38 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		goto out_unlock;
 	}
 
+	/*
+	 * Using shared interrupts with the IRQ_NOAUTOEN flag can be risky if
+	 * the flag is set when one or more of the drivers sharing the IRQ
+	 * don't expect it. For example if driver A does:
+	 *
+	 *   irq_set_status_flags(shared_irq, IRQ_NOAUTOEN);
+	 *   request_irq(shared_irq, handler_a, IRQF_SHARED, "a", dev_a);
+	 *
+	 * Then driver B does:
+	 *
+	 *   request_irq(shared_irq, handler_b, IRQF_SHARED, "b", dev_b);
+	 *
+	 * Driver B has no idea that driver A set the IRQ_NOAUTOEN flag, and
+	 * thus may expect that the shared_irq is enabled after its call to
+	 * request_irq(). It may then miss interrupts that it was expecting to
+	 * receive.
+	 *
+	 * We therefore require that if a shared IRQ is used with IRQ_NOAUTOEN
+	 * then all drivers sharing it explicitly declare that they are aware
+	 * of the fact that the interrupt won't be automatically enabled, by
+	 * setting the IRQF_NOAUTOEN flag in their struct irqaction or
+	 * providing it to request_irq().
+	 */
+	if (WARN((new->flags && IRQF_SHARED) &&
+		 !irq_settings_can_autoenable(desc) &&
+		 !(new->flags & IRQF_NOAUTOEN),
+		 "shared irq %d isn't automatically enabled, but the caller doesn't set IRQF_NOAUTOEN",
+		 irq)) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	if (!shared) {
 		init_waitqueue_head(&desc->wait_for_threads);
 
@@ -1343,16 +1379,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			irqd_set(&desc->irq_data, IRQD_NO_BALANCING);
 		}
 
+		if (new->flags & IRQF_NOAUTOEN)
+			irq_settings_set_noautoenable(desc);
+
 		if (irq_settings_can_autoenable(desc)) {
 			irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
 		} else {
-			/*
-			 * Shared interrupts do not go well with disabling
-			 * auto enable. The sharing interrupt might request
-			 * it while it's still disabled and then wait for
-			 * interrupts forever.
-			 */
-			WARN_ON_ONCE(new->flags & IRQF_SHARED);
 			/* Undo nested disables: */
 			desc->depth = 1;
 		}
diff --git a/kernel/irq/settings.h b/kernel/irq/settings.h
index 320579d89091..97edef1bd781 100644
--- a/kernel/irq/settings.h
+++ b/kernel/irq/settings.h
@@ -147,6 +147,11 @@ static inline bool irq_settings_can_autoenable(struct irq_desc *desc)
 	return !(desc->status_use_accessors & _IRQ_NOAUTOEN);
 }
 
+static inline void irq_settings_set_noautoenable(struct irq_desc *desc)
+{
+	desc->status_use_accessors |= _IRQ_NOAUTOEN;
+}
+
 static inline bool irq_settings_is_nested_thread(struct irq_desc *desc)
 {
 	return desc->status_use_accessors & _IRQ_NESTED_THREAD;
-- 
2.14.1

  reply	other threads:[~2017-09-07 23:26 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-31  9:58 [patch 0/2] genirq: Handle NOAUTOEN interrupts correctly Thomas Gleixner
2017-05-31  9:58 ` [patch 1/2] genirq: Handle NOAUTOEN interrupt setup proper Thomas Gleixner
2017-05-31 13:54   ` Marc Zyngier
2017-05-31 15:18     ` Thomas Gleixner
2017-06-04 12:47   ` [tip:irq/core] " tip-bot for Thomas Gleixner
2017-05-31  9:58 ` [patch 2/2] genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts Thomas Gleixner
2017-06-04 12:48   ` [tip:irq/core] " tip-bot for Thomas Gleixner
2017-09-06  6:00   ` [2/2] " Paul Burton
2017-09-06  8:16     ` Thomas Gleixner
2017-09-06 14:01       ` Paul Burton
2017-09-06 14:14         ` Thomas Gleixner
2017-09-07  1:18           ` Paul Burton
2017-09-07 23:25             ` [RFC PATCH v1 0/9] Support shared percpu interrupts; clean up MIPS hacks Paul Burton
2017-09-07 23:25               ` Paul Burton
2017-09-07 23:25               ` Paul Burton [this message]
2017-09-07 23:25                 ` [RFC PATCH v1 1/9] genirq: Allow shared interrupt users to opt into IRQ_NOAUTOEN Paul Burton
2017-09-07 23:25               ` [RFC PATCH v1 2/9] genirq: Support shared per_cpu_devid interrupts Paul Burton
2017-09-07 23:25                 ` Paul Burton
2017-09-25 21:06                 ` Thomas Gleixner
2017-09-26 12:00                   ` Thomas Gleixner
2017-10-19 14:08                     ` Thomas Gleixner
2017-09-07 23:25               ` [RFC PATCH v1 3/9] genirq: Introduce irq_is_percpu_devid() Paul Burton
2017-09-07 23:25                 ` Paul Burton
2017-09-07 23:25               ` [RFC PATCH v1 4/9] MIPS: Remove perf_irq interrupt sharing fallback Paul Burton
2017-09-07 23:25                 ` Paul Burton
2017-09-07 23:25               ` [RFC PATCH v1 5/9] MIPS: Remove perf_irq Paul Burton
2017-09-07 23:25                 ` Paul Burton
2017-09-07 23:25               ` [RFC PATCH v1 6/9] MIPS: perf: percpu_devid interrupt support Paul Burton
2017-09-07 23:25                 ` Paul Burton
2017-10-19 14:12                 ` Thomas Gleixner
2017-09-07 23:25               ` [RFC PATCH v1 7/9] MIPS: cevt-r4k: " Paul Burton
2017-09-07 23:25                 ` Paul Burton
2017-09-07 23:25               ` [RFC PATCH v1 8/9] irqchip: mips-cpu: Set timer, FDC & perf interrupts percpu_devid Paul Burton
2017-09-07 23:25                 ` Paul Burton
2017-09-07 23:25               ` [RFC PATCH v1 9/9] irqchip: mips-gic: Remove gic_all_vpes_local_irq_controller Paul Burton
2017-09-07 23:25                 ` Paul Burton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170907232542.20589-2-paul.burton@imgtec.com \
    --to=paul.burton@imgtec.com \
    --cc=briannorris@chromium.org \
    --cc=dianders@chromium.org \
    --cc=james.hogan@imgtec.com \
    --cc=jason@lakedaemon.net \
    --cc=jeffy.chen@rock-chips.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@linux-mips.org \
    --cc=marc.zyngier@arm.com \
    --cc=ralf@linux-mips.org \
    --cc=tfiga@chromium.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.