Re: [patch 3/3] genirq: introduce IRQF_ALLOW_NESTED flag for request_irq()

Re: [patch 3/3] genirq: introduce IRQF_ALLOW_NESTED flag for request_irq()

[Thomas Gleixner]

Back to LKML

On Thu, 11 Mar 2010, akpm@linux-foundation.org wrote:

Sorry, this dropped from my radar.

> From: Marc Zyngier <maz@misterjones.org>
> 
> Now that we enjoy threaded interrupts, we're starting to see irq_chip
> implementations (wm831x, pca953x) that make use of threaded interrupts for
> the controller, and nested interrupts for the client interrupt.  It all
> works very well, with one drawback:
> 
> Drivers requesting an IRQ must now know whether the handler will run in a
> thread context of not, and call request_threaded_irq() or request_irq()
> accordingly.
> 
> The problem is that the requesting driver sometimes doesn't know about the
> nature of the interrupt, specially when the interrupt controller is a
> discrete chip (typically a GPIO expander connected over I2C) that can be
> connected to a wide variety of otherwise perfectly supported hardware.
> 
> The following patch introduces the IRQF_ALLOW_NESTED flag, that acts as a
> "contract" between the driver and the genirq framework.  By using this
> flag as part of the request_irq() call, the driver informs the genirq
> framework that the handler will happily run either in hardirq or nested
> context, without any ill effect.
> 
> The benefit of this is a single API for drivers.  It still requires the
> driver to be audited, and the flag added to the request_irq() call.
> 
> Of course, this goes against Linus choice of having separate APIs for both
> cases.  The only alternative I can imagine for this is to use
> request_threaded_irq() with the same function provided for both the
> handler and the threaded handler.  Ugly...

In general I have no objections, but one thing bothers me. We have no
way to let a driver know whether it runs in a nested threaded context
or in hard irq context. There might be (future) drivers which would be
happy to know that to apply context dependent optimizations etc.

What about a new function which solves your problem and returns that
information ? Something along the line:

int request_any_context_irq(....)
{
	...
	if (desc->status & IRQ_NESTED_THREAD) {
	   	ret = request_threaded_irq();
		if (!ret)
			ret = IRQ_IS_NESTED;

	} else {
		.....
			ret = IRQ_IS_NONTHREADED;
		else
			ret = IRQ_IS_THREADED;
			
	}
	...
	return ret;
}

You get the idea, right ?

It's a bit more code, but less magic and more flexible for further use
cases.

Thanks,

	tglx
 
> This patch has been tested on ARM and Blackfin platforms.
> 
> Signed-off-by: Marc Zyngier <maz@misterjones.org>
> Tested-by: Joachim Eastwood <joachim.eastwood@jotron.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  include/linux/interrupt.h |    3 +++
>  kernel/irq/manage.c       |   12 +++++++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff -puN include/linux/interrupt.h~genirq-introduce-irqf_allow_nested-flag-for-request_irq include/linux/interrupt.h
> --- a/include/linux/interrupt.h~genirq-introduce-irqf_allow_nested-flag-for-request_irq
> +++ a/include/linux/interrupt.h
> @@ -52,6 +52,8 @@
>   * IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler finished.
>   *                Used by threaded interrupts which need to keep the
>   *                irq line disabled until the threaded handler has been run.
> + * IRQF_ALLOW_NESTED - Handler can be either run as hardirq or nested
> + *		       interrupt.
>   */
>  #define IRQF_DISABLED		0x00000020
>  #define IRQF_SAMPLE_RANDOM	0x00000040
> @@ -62,6 +64,7 @@
>  #define IRQF_NOBALANCING	0x00000800
>  #define IRQF_IRQPOLL		0x00001000
>  #define IRQF_ONESHOT		0x00002000
> +#define IRQF_ALLOW_NESTED	0x00004000
>  
>  /*
>   * Bits used by threaded handlers:
> diff -puN kernel/irq/manage.c~genirq-introduce-irqf_allow_nested-flag-for-request_irq kernel/irq/manage.c
> --- a/kernel/irq/manage.c~genirq-introduce-irqf_allow_nested-flag-for-request_irq
> +++ a/kernel/irq/manage.c
> @@ -641,12 +641,18 @@ __setup_irq(unsigned int irq, struct irq
>  
>  	/*
>  	 * Check whether the interrupt nests into another interrupt
> -	 * thread.
> +	 * thread. Nested interrupt must provide a thread function
> +	 * unless it raises the IRQF_ALLOW_NESTED flag.
>  	 */
>  	nested = desc->status & IRQ_NESTED_THREAD;
>  	if (nested) {
> -		if (!new->thread_fn)
> -			return -EINVAL;
> +		if (!new->thread_fn) {
> +			if (new->flags & IRQF_ALLOW_NESTED)
> +				new->thread_fn = new->handler;
> +			else
> +				return -EINVAL;
> +		}
> +
>  		/*
>  		 * Replace the primary handler which was provided from
>  		 * the driver for non nested interrupt handling by the
> _
> 

Re: [patch 3/3] genirq: introduce IRQF_ALLOW_NESTED flag for request_irq()

[Marc Zyngier]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

On Sat, 13 Mar 2010 20:56:19 +0100 (CET), Thomas Gleixner
<tglx@linutronix.de> wrote:

Hi Thomas,

> In general I have no objections, but one thing bothers me. We have no
> way to let a driver know whether it runs in a nested threaded context
> or in hard irq context. There might be (future) drivers which would be
> happy to know that to apply context dependent optimizations etc.
> 
> What about a new function which solves your problem and returns that
> information ? Something along the line:
> 
> int request_any_context_irq(....)
> {
> 	...
> 	if (desc->status & IRQ_NESTED_THREAD) {
> 	   	ret = request_threaded_irq();
> 		if (!ret)
> 			ret = IRQ_IS_NESTED;
> 
> 	} else {
> 		.....
> 			ret = IRQ_IS_NONTHREADED;
> 		else
> 			ret = IRQ_IS_THREADED;
> 
> 	}
> 	...
> 	return ret;
> }
> 
> You get the idea, right ?
> 
> It's a bit more code, but less magic and more flexible for further use
> cases.

What about the attached (sorry, webmail crap) patch? I deliberately left
IS_THREADED out of the picture, as I have the feeling that the caller has
to know if it really wants a threaded handler, and I couldn't see a way to
guess its intent.

Please note that this patch has only been compile-tested, as I'm traveling
for the rest of the week and don't have access to my boards.

Thanks,

        M.
-- 
Who you jivin' with that Cosmik Debris?

[-- Attachment #2: 0001-genirq-introduce-request_any_context_irq.patch --]
[-- Type: text/plain, Size: 4527 bytes --]

From b776526f1a0f0b3f7b2e8fd1b9cfad49985635e2 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@misterjones.org>
Date: Mon, 15 Mar 2010 22:56:33 +0000
Subject: [PATCH] genirq: introduce request_any_context_irq()

Now that we enjoy threaded interrupts, we're starting to see irq_chip
implementations (wm831x, pca953x) that make use of threaded interrupts
for the controller, and nested interrupts for the client interrupt. It
all works very well, with one drawback:

Drivers requesting an IRQ must now know whether the handler will
run in a thread context of not, and call request_threaded_irq() or
request_irq() accordingly.

The problem is that the requesting driver sometimes doesn't know
about the nature of the interrupt, specially when the interrupt
controller is a discrete chip (typically a GPIO expander connected
over I2C) that can be connected to a wide variety of otherwise perfectly
supported hardware.

This patch introduce the request_any_context_irq() function that mostly
mimics the usual request_irq(), except that it checks whether the irq
level is configured as nested or not, and calls the right backend.
On success, it also returns either IRQC_IS_HARDIRQ or IRQC_IS_NESTED.

Signed-off-by: Marc Zyngier <maz@misterjones.org>
---
 include/linux/interrupt.h |   26 ++++++++++++++++++++++++++
 kernel/irq/manage.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 75f3f00..8081e40 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -107,6 +107,16 @@ struct irqaction {
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
 
+/**
+ * These flags can be returned by request_any_context_irq() and
+ * describe the context the interrupt will be run in.
+ *
+ * IRQC_IS_HARDIRQ - interrupt runs in hardirq context
+ * IRQC_IS_NESTED - interrupt runs in a nested threaded context
+ */
+#define IRQC_IS_HARDIRQ		0x00000000
+#define IRQC_IS_NESTED		0x00000001
+
 #ifdef CONFIG_GENERIC_HARDIRQS
 extern int __must_check
 request_threaded_irq(unsigned int irq, irq_handler_t handler,
@@ -120,6 +130,10 @@ request_irq(unsigned int irq, irq_handler_t handler, unsigned long flags,
 	return request_threaded_irq(irq, handler, NULL, flags, name, dev);
 }
 
+extern int __must_check
+request_any_context_irq(unsigned int irq, irq_handler_t handler,
+			unsigned long flags, const char *name, void *dev_id);
+
 extern void exit_irq_thread(void);
 #else
 
@@ -141,6 +155,18 @@ request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	return request_irq(irq, handler, flags, name, dev);
 }
 
+static inline int __must_check
+request_any_context_irq(unsigned int irq, irq_handler_t handler,
+			unsigned long flags, const char *name, void *dev_id)
+{
+	int ret = request_irq(irq, handler, flags, name, dev_id);
+
+	if (!ret)
+		ret = IRQC_IS_HARDIRQ;
+
+	return ret;
+}
+
 static inline void exit_irq_thread(void) { }
 #endif
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index eb6078c..60b33bf 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1088,3 +1088,48 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	return retval;
 }
 EXPORT_SYMBOL(request_threaded_irq);
+
+/**
+ *	request_any_context_irq - allocate an interrupt line
+ *	@irq: Interrupt line to allocate
+ *	@handler: Function to be called when the IRQ occurs.
+ *		  Threaded handler for threaded interrupts.
+ *	@flags: Interrupt type flags
+ *	@name: An ascii name for the claiming device
+ *	@dev_id: A cookie passed back to the handler function
+ *
+ *	This call allocates interrupt resources and enables the
+ *	interrupt line and IRQ handling. It selects either a
+ *	hardirq or threaded handling method depending on the
+ *	context.
+ *
+ *	On failure, it returns a negative value. On success,
+ *	it returns either IRQC_IS_HARDIRQ or IRQC_IS_NESTED.
+ *
+ */
+int request_any_context_irq(unsigned int irq, irq_handler_t handler,
+			    unsigned long flags, const char *name, void *dev_id)
+{
+	struct irq_desc *desc;
+	int ret;
+
+	desc = irq_to_desc(irq);
+	if (!desc)
+		return -EINVAL;
+
+	if (desc->status & IRQ_NESTED_THREAD) {
+		ret = request_threaded_irq(irq, NULL, handler,
+					   flags, name, dev_id);
+		if (!ret)
+			ret = IRQC_IS_NESTED;
+
+		return ret;
+	}
+
+	ret = request_irq(irq, handler, flags, name, dev_id);
+	if (!ret)
+		ret = IRQC_IS_HARDIRQ;
+
+	return ret;
+}
+EXPORT_SYMBOL(request_any_context_irq);
-- 
1.7.0.2

Re: [patch 3/3] genirq: introduce IRQF_ALLOW_NESTED flag for request_irq()

[Marc Zyngier]

On Tue, 16 Mar 2010 09:36:59 +0100
Marc Zyngier <maz@misterjones.org> wrote:

> On Sat, 13 Mar 2010 20:56:19 +0100 (CET), Thomas Gleixner
> <tglx@linutronix.de> wrote:
> 
> Hi Thomas,
> 
> > In general I have no objections, but one thing bothers me. We have no
> > way to let a driver know whether it runs in a nested threaded context
> > or in hard irq context. There might be (future) drivers which would be
> > happy to know that to apply context dependent optimizations etc.
> > 
> > What about a new function which solves your problem and returns that
> > information ? Something along the line:
> > 
> > int request_any_context_irq(....)
> > {
> > 	...
> > 	if (desc->status & IRQ_NESTED_THREAD) {
> > 	   	ret = request_threaded_irq();
> > 		if (!ret)
> > 			ret = IRQ_IS_NESTED;
> > 
> > 	} else {
> > 		.....
> > 			ret = IRQ_IS_NONTHREADED;
> > 		else
> > 			ret = IRQ_IS_THREADED;
> > 
> > 	}
> > 	...
> > 	return ret;
> > }
> > 
> > You get the idea, right ?
> > 
> > It's a bit more code, but less magic and more flexible for further use
> > cases.
> 
> What about the attached (sorry, webmail crap) patch? I deliberately left
> IS_THREADED out of the picture, as I have the feeling that the caller has
> to know if it really wants a threaded handler, and I couldn't see a way to
> guess its intent.
> 
> Please note that this patch has only been compile-tested, as I'm traveling
> for the rest of the week and don't have access to my boards.
> 
> Thanks,
> 
>         M.

Any comment on this?

Thanks,

	M.
-- 
I'm the slime oozin' out from your TV set...

Re: [patch 3/3] genirq: introduce IRQF_ALLOW_NESTED flag for request_irq()

[Thomas Gleixner]

On Mon, 22 Mar 2010, Marc Zyngier wrote:
> > Please note that this patch has only been compile-tested, as I'm traveling
> > for the rest of the week and don't have access to my boards.
> > 
> > Thanks,
> > 
> >         M.
> 
> Any comment on this?

Yes, I was traveling last week and this is in my mail backlog :)

Thanks,

	tglx

Re: [patch 3/3] genirq: introduce IRQF_ALLOW_NESTED flag for request_irq()

[Trilok Soni]

Hi ,
On Mon, Mar 22, 2010 at 1:17 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 22 Mar 2010, Marc Zyngier wrote:
>> > Please note that this patch has only been compile-tested, as I'm traveling
>> > for the rest of the week and don't have access to my boards.
>> >
>> > Thanks,
>> >
>> >         M.
>>
>> Any comment on this?
>
> Yes, I was traveling last week and this is in my mail backlog :)
>
> Thanks,

Any updates on this patch?


-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

[tip:irq/core] genirq: Introduce request_any_context_irq()

[tip-bot for Marc Zyngier]

Commit-ID:  ae731f8d0785ccd3380f511bae888933b6562e45
Gitweb:     http://git.kernel.org/tip/ae731f8d0785ccd3380f511bae888933b6562e45
Author:     Marc Zyngier <maz@misterjones.org>
AuthorDate: Mon, 15 Mar 2010 22:56:33 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 13 Apr 2010 16:36:39 +0200

genirq: Introduce request_any_context_irq()

Now that we enjoy threaded interrupts, we're starting to see irq_chip
implementations (wm831x, pca953x) that make use of threaded interrupts
for the controller, and nested interrupts for the client interrupt. It
all works very well, with one drawback:

Drivers requesting an IRQ must now know whether the handler will
run in a thread context or not, and call request_threaded_irq() or
request_irq() accordingly.

The problem is that the requesting driver sometimes doesn't know
about the nature of the interrupt, specially when the interrupt
controller is a discrete chip (typically a GPIO expander connected
over I2C) that can be connected to a wide variety of otherwise perfectly
supported hardware.

This patch introduces the request_any_context_irq() function that mostly
mimics the usual request_irq(), except that it checks whether the irq
level is configured as nested or not, and calls the right backend.
On success, it also returns either IRQC_IS_HARDIRQ or IRQC_IS_NESTED.

[ tglx: Made return value an enum, simplified code and made the export
  	of request_any_context_irq GPL ]

Signed-off-by: Marc Zyngier <maz@misterjones.org>
Cc: <joachim.eastwood@jotron.com>
LKML-Reference: <927ea285bd0c68934ddae1a47e44a9ba@localhost>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/interrupt.h |   23 +++++++++++++++++++++++
 kernel/irq/manage.c       |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 75f3f00..d7e7a76 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -77,6 +77,18 @@ enum {
 	IRQTF_AFFINITY,
 };
 
+/**
+ * These values can be returned by request_any_context_irq() and
+ * describe the context the interrupt will be run in.
+ *
+ * IRQC_IS_HARDIRQ - interrupt runs in hardirq context
+ * IRQC_IS_NESTED - interrupt runs in a nested threaded context
+ */
+enum {
+	IRQC_IS_HARDIRQ	= 0,
+	IRQC_IS_NESTED,
+};
+
 typedef irqreturn_t (*irq_handler_t)(int, void *);
 
 /**
@@ -120,6 +132,10 @@ request_irq(unsigned int irq, irq_handler_t handler, unsigned long flags,
 	return request_threaded_irq(irq, handler, NULL, flags, name, dev);
 }
 
+extern int __must_check
+request_any_context_irq(unsigned int irq, irq_handler_t handler,
+			unsigned long flags, const char *name, void *dev_id);
+
 extern void exit_irq_thread(void);
 #else
 
@@ -141,6 +157,13 @@ request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	return request_irq(irq, handler, flags, name, dev);
 }
 
+static inline int __must_check
+request_any_context_irq(unsigned int irq, irq_handler_t handler,
+			unsigned long flags, const char *name, void *dev_id)
+{
+	return request_irq(irq, handler, flags, name, dev_id);
+}
+
 static inline void exit_irq_thread(void) { }
 #endif
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 704e488..84f3227 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1120,3 +1120,40 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	return retval;
 }
 EXPORT_SYMBOL(request_threaded_irq);
+
+/**
+ *	request_any_context_irq - allocate an interrupt line
+ *	@irq: Interrupt line to allocate
+ *	@handler: Function to be called when the IRQ occurs.
+ *		  Threaded handler for threaded interrupts.
+ *	@flags: Interrupt type flags
+ *	@name: An ascii name for the claiming device
+ *	@dev_id: A cookie passed back to the handler function
+ *
+ *	This call allocates interrupt resources and enables the
+ *	interrupt line and IRQ handling. It selects either a
+ *	hardirq or threaded handling method depending on the
+ *	context.
+ *
+ *	On failure, it returns a negative value. On success,
+ *	it returns either IRQC_IS_HARDIRQ or IRQC_IS_NESTED.
+ */
+int request_any_context_irq(unsigned int irq, irq_handler_t handler,
+			    unsigned long flags, const char *name, void *dev_id)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+	int ret;
+
+	if (!desc)
+		return -EINVAL;
+
+	if (desc->status & IRQ_NESTED_THREAD) {
+		ret = request_threaded_irq(irq, NULL, handler,
+					   flags, name, dev_id);
+		return !ret ? IRQC_IS_NESTED : ret;
+	}
+
+	ret = request_irq(irq, handler, flags, name, dev_id);
+	return !ret ? IRQC_IS_HARDIRQ : ret;
+}
+EXPORT_SYMBOL_GPL(request_any_context_irq);