[PATCH] genirq: add IRQF_NO_AUTOEN for request_irq

STALE2049d

3 messages, 2 authors, 2021-01-05 · open the first message on its own page

[PATCH] genirq: add IRQF_NO_AUTOEN for request_irq

From: Barry Song <hidden>
Date: 2021-01-04 22:32:09

This patch originated from the discussion with Dmitry in the below thread:
https://lore.kernel.org/linux-input/20210102042902.41664-1-song.bao.hua@hisilicon.com/
there are many drivers which don't want interrupts enabled automatically
due to request_irq().
So they are handling this issue by either way of the below two:
(1)
irq_set_status_flags(irq, IRQ_NOAUTOEN);
request_irq(dev, irq...);
(2)
request_irq(dev, irq...);
disable_irq(irq);

The code in the second way is silly and unsafe. In the small time gap
between request_irq and disable_irq, interrupts can still come.
The code in the first way is safe though we might be able to do it in
the generic irq code.

I guess Dmitry also prefers genirq handles this as he said
"What I would like to see is to allow passing something like IRQF_DISABLED
to request_irq() so that we would not need neither irq_set_status_flags()
nor disable_irq()" in the original email thread.

If this one is accepted, hundreds of drivers with this problem will be
handled afterwards.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Barry Song <redacted>
---
 include/linux/interrupt.h |  3 +++
 kernel/irq/manage.c       |  3 +++
 kernel/irq/settings.h     | 10 ++++++++++
 3 files changed, 16 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index bb8ff9083e7d..0f22d277078c 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h

@@ -61,6 +61,8 @@
  *                interrupt handler after suspending interrupts. For system
  *                wakeup devices users need to implement wakeup detection in
  *                their interrupt handlers.
+ * IRQF_NO_AUTOEN - Don't enable IRQ automatically when users request it. Users
+ *                will enable it explicitly by enable_irq() later.
  */
 #define IRQF_SHARED		0x00000080
 #define IRQF_PROBE_SHARED	0x00000100

@@ -74,6 +76,7 @@
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
 #define IRQF_COND_SUSPEND	0x00040000
+#define IRQF_NO_AUTOEN		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 ab8567f32501..364e8b47d9ba 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c

@@ -1693,6 +1693,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			irqd_set(&desc->irq_data, IRQD_NO_BALANCING);
 		}
 
+		if (new->flags & IRQF_NO_AUTOEN)
+			irq_settings_set_noautoen(desc);
+
 		if (irq_settings_can_autoenable(desc)) {
 			irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
 		} else {

diff --git a/kernel/irq/settings.h b/kernel/irq/settings.h
index 403378b9947b..a28958a9c548 100644
--- a/kernel/irq/settings.h
+++ b/kernel/irq/settings.h

@@ -145,6 +145,16 @@ static inline bool irq_settings_can_move_pcntxt(struct irq_desc *desc)
 	return desc->status_use_accessors & _IRQ_MOVE_PCNTXT;
 }
 
+static inline void irq_settings_clr_noautoen(struct irq_desc *desc)
+{
+	desc->status_use_accessors &= ~_IRQ_NOAUTOEN;
+}
+
+static inline void irq_settings_set_noautoen(struct irq_desc *desc)
+{
+	desc->status_use_accessors |= _IRQ_NOAUTOEN;
+}
+
 static inline bool irq_settings_can_autoenable(struct irq_desc *desc)
 {
 	return !(desc->status_use_accessors & _IRQ_NOAUTOEN);

-- 
2.25.1

Re: [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2021-01-04 23:30:23

On Tue, Jan 05, 2021 at 11:26:12AM +1300, Barry Song wrote:

This patch originated from the discussion with Dmitry in the below thread:
https://lore.kernel.org/linux-input/20210102042902.41664-1-song.bao.hua@hisilicon.com/
there are many drivers which don't want interrupts enabled automatically
due to request_irq().
So they are handling this issue by either way of the below two:
(1)
irq_set_status_flags(irq, IRQ_NOAUTOEN);
request_irq(dev, irq...);
(2)
request_irq(dev, irq...);
disable_irq(irq);

The code in the second way is silly and unsafe. In the small time gap
between request_irq and disable_irq, interrupts can still come.
The code in the first way is safe though we might be able to do it in
the generic irq code.

I guess Dmitry also prefers genirq handles this as he said
"What I would like to see is to allow passing something like IRQF_DISABLED
to request_irq() so that we would not need neither irq_set_status_flags()
nor disable_irq()" in the original email thread.

One of the reasons I dislike irq_set_status_flags() is that we have to
call it before we actually granted our IRQ request...

quoted hunk

If this one is accepted, hundreds of drivers with this problem will be
handled afterwards.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Barry Song <redacted>
---
 include/linux/interrupt.h |  3 +++
 kernel/irq/manage.c       |  3 +++
 kernel/irq/settings.h     | 10 ++++++++++
 3 files changed, 16 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index bb8ff9083e7d..0f22d277078c 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h

@@ -61,6 +61,8 @@
  *                interrupt handler after suspending interrupts. For system
  *                wakeup devices users need to implement wakeup detection in
  *                their interrupt handlers.
+ * IRQF_NO_AUTOEN - Don't enable IRQ automatically when users request it. Users
+ *                will enable it explicitly by enable_irq() later.
  */
 #define IRQF_SHARED		0x00000080
 #define IRQF_PROBE_SHARED	0x00000100

@@ -74,6 +76,7 @@
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
 #define IRQF_COND_SUSPEND	0x00040000
+#define IRQF_NO_AUTOEN		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 ab8567f32501..364e8b47d9ba 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c

@@ -1693,6 +1693,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			irqd_set(&desc->irq_data, IRQD_NO_BALANCING);
 		}
 
+		if (new->flags & IRQF_NO_AUTOEN)
+			irq_settings_set_noautoen(desc);

Can we make sure we refuse this request if the caller also specified
IRQF_SHARED?

Thanks.

-- 
Dmitry

RE: [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq

From: Song Bao Hua (Barry Song) <hidden>
Date: 2021-01-05 01:33:59

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Tuesday, January 5, 2021 12:01 PM
To: Song Bao Hua (Barry Song) <redacted>
Cc: tglx@linutronix.de; maz@kernel.org; gregkh@linuxfoundation.org;
linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
linuxarm@openeuler.org
Subject: Re: [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq

On Tue, Jan 05, 2021 at 11:26:12AM +1300, Barry Song wrote:

quoted

This patch originated from the discussion with Dmitry in the below thread:

https://lore.kernel.org/linux-input/20210102042902.41664-1-song.bao.hua@hi
silicon.com/

quoted

there are many drivers which don't want interrupts enabled automatically
due to request_irq().
So they are handling this issue by either way of the below two:
(1)
irq_set_status_flags(irq, IRQ_NOAUTOEN);
request_irq(dev, irq...);
(2)
request_irq(dev, irq...);
disable_irq(irq);

The code in the second way is silly and unsafe. In the small time gap
between request_irq and disable_irq, interrupts can still come.
The code in the first way is safe though we might be able to do it in
the generic irq code.

I guess Dmitry also prefers genirq handles this as he said
"What I would like to see is to allow passing something like IRQF_DISABLED
to request_irq() so that we would not need neither irq_set_status_flags()
nor disable_irq()" in the original email thread.

One of the reasons I dislike irq_set_status_flags() is that we have to
call it before we actually granted our IRQ request...

quoted

If this one is accepted, hundreds of drivers with this problem will be
handled afterwards.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Barry Song <redacted>
---
 include/linux/interrupt.h |  3 +++
 kernel/irq/manage.c       |  3 +++
 kernel/irq/settings.h     | 10 ++++++++++
 3 files changed, 16 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index bb8ff9083e7d..0f22d277078c 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h

@@ -61,6 +61,8 @@
  *                interrupt handler after suspending interrupts. For system
  *                wakeup devices users need to implement wakeup detection in
  *                their interrupt handlers.
+ * IRQF_NO_AUTOEN - Don't enable IRQ automatically when users request it.

Users

quoted

+ *                will enable it explicitly by enable_irq() later.
  */
 #define IRQF_SHARED		0x00000080
 #define IRQF_PROBE_SHARED	0x00000100

@@ -74,6 +76,7 @@
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
 #define IRQF_COND_SUSPEND	0x00040000
+#define IRQF_NO_AUTOEN		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 ab8567f32501..364e8b47d9ba 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c

@@ -1693,6 +1693,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,

struct irqaction *new)

quoted

 			irqd_set(&desc->irq_data, IRQD_NO_BALANCING);
 		}

+		if (new->flags & IRQF_NO_AUTOEN)
+			irq_settings_set_noautoen(desc);

Can we make sure we refuse this request if the caller also specified
IRQF_SHARED?

Right now, there is a warning for IRQF_SHARED + NOAUTOEN:

		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;
		}

Of course, this could also be clearly rejected in the sanity-check
of request_threaded_irq() if we want to totally prohibit this
behavior:

int request_threaded_irq(unsigned int irq, irq_handler_t handler,
			 irq_handler_t thread_fn, unsigned long irqflags,
			 const char *devname, void *dev_id)
{
	struct irqaction *action;
	struct irq_desc *desc;
	int retval;

	if (irq == IRQ_NOTCONNECTED)
		return -ENOTCONN;

	/*
	 * Sanity-check: shared interrupts must pass in a real dev-ID,
	 * otherwise we'll have trouble later trying to figure out
	 * which interrupt is which (messes up the interrupt freeing
	 * logic etc).
	 *
	 * Also IRQF_COND_SUSPEND only makes sense for shared interrupts and
	 * it cannot be set along with IRQF_NO_SUSPEND.
	 */
	if (((irqflags & IRQF_SHARED) && !dev_id) ||
	    (!(irqflags & IRQF_SHARED) && (irqflags & IRQF_COND_SUSPEND)) ||
	    ((irqflags & IRQF_NO_SUSPEND) && (irqflags & IRQF_COND_SUSPEND)))
		return -EINVAL;

Thanks.

Thanks
Barry

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help