Preparatory patch for threaded interrupt handlers.
Adds a quick check handler which is called before the real handler.
The quick check handler can decide whether the interrupt was originated
from the device or not. It can also declare the interrupt as handled
and subsequently avoid that the real handler is called.
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Ingo Molnar <[email protected]>
---
include/linux/interrupt.h | 14 +++++++++++++-
include/linux/irqreturn.h | 2 ++
kernel/irq/handle.c | 18 +++++++++++++++---
kernel/irq/manage.c | 19 +++++++++++++------
4 files changed, 43 insertions(+), 10 deletions(-)
Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================
--- linux-2.6-tip.orig/include/linux/interrupt.h
+++ linux-2.6-tip/include/linux/interrupt.h
@@ -58,6 +58,7 @@
typedef irqreturn_t (*irq_handler_t)(int, void *);
struct irqaction {
+ irq_handler_t quick_check_handler;
irq_handler_t handler;
unsigned long flags;
cpumask_t mask;
@@ -69,8 +70,19 @@ struct irqaction {
};
extern irqreturn_t no_action(int cpl, void *dev_id);
-extern int __must_check request_irq(unsigned int, irq_handler_t handler,
+
+extern int __must_check
+request_irq_quickcheck(unsigned int, irq_handler_t handler,
+ irq_handler_t quick_check_handler,
unsigned long, const char *, void *);
+
+static inline int __must_check
+request_irq(unsigned int irq, irq_handler_t handler, unsigned long flags,
+ const char *name, void *dev)
+{
+ return request_irq_quickcheck(irq, handler, NULL, flags, name, dev);
+}
+
extern void free_irq(unsigned int, void *);
struct device;
Index: linux-2.6-tip/include/linux/irqreturn.h
===================================================================
--- linux-2.6-tip.orig/include/linux/irqreturn.h
+++ linux-2.6-tip/include/linux/irqreturn.h
@@ -5,10 +5,12 @@
* enum irqreturn
* @IRQ_NONE interrupt was not from this device
* @IRQ_HANDLED interrupt was handled by this device
+ * @IRQ_NEEDS_HANDLING quick check handler requests to run real handler
*/
enum irqreturn {
IRQ_NONE,
IRQ_HANDLED,
+ IRQ_NEEDS_HANDLING,
};
#define irqreturn_t enum irqreturn
Index: linux-2.6-tip/kernel/irq/handle.c
===================================================================
--- linux-2.6-tip.orig/kernel/irq/handle.c
+++ linux-2.6-tip/kernel/irq/handle.c
@@ -137,9 +137,21 @@ irqreturn_t handle_IRQ_event(unsigned in
local_irq_enable_in_hardirq();
do {
- ret = action->handler(irq, action->dev_id);
- if (ret == IRQ_HANDLED)
- status |= action->flags;
+ if (action->quick_check_handler)
+ ret = action->quick_check_handler(irq, action->dev_id);
+ else
+ ret = IRQ_NEEDS_HANDLING;
+
+ switch (ret) {
+ default:
+ break;
+
+ case IRQ_NEEDS_HANDLING:
+ ret = action->handler(irq, action->dev_id);
+ if (ret == IRQ_HANDLED)
+ status |= action->flags;
+ break;
+ }
retval |= ret;
action = action->next;
} while (action);
Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
--- linux-2.6-tip.orig/kernel/irq/manage.c
+++ linux-2.6-tip/kernel/irq/manage.c
@@ -563,9 +563,14 @@ void free_irq(unsigned int irq, void *de
EXPORT_SYMBOL(free_irq);
/**
- * request_irq - allocate an interrupt line
+ * request_irq_quickcheck - allocate an interrupt line
* @irq: Interrupt line to allocate
- * @handler: Function to be called when the IRQ occurs
+ * @handler: Function to be called when the IRQ occurs.
+ * Primary handler for threaded interrupts
+ * @quick_check_handler: Function to be called when the interrupt
+ * before the real handler is called to check
+ * whether the interrupt originated from the device
+ * can be NULL
* @irqflags: Interrupt type flags
* @devname: An ascii name for the claiming device
* @dev_id: A cookie passed back to the handler function
@@ -589,10 +594,11 @@ EXPORT_SYMBOL(free_irq);
* IRQF_SHARED Interrupt is shared
* IRQF_DISABLED Disable local interrupts while processing
* IRQF_SAMPLE_RANDOM The interrupt can be used for entropy
- *
*/
-int request_irq(unsigned int irq, irq_handler_t handler,
- unsigned long irqflags, const char *devname, void *dev_id)
+int request_irq_quickcheck(unsigned int irq, irq_handler_t handler,
+ irq_handler_t quick_check_handler,
+ unsigned long irqflags, const char *devname,
+ void *dev_id)
{
struct irqaction *action;
int retval;
@@ -622,6 +628,7 @@ int request_irq(unsigned int irq, irq_ha
if (!action)
return -ENOMEM;
+ action->quick_check_handler = quick_check_handler;
action->handler = handler;
action->flags = irqflags;
cpus_clear(action->mask);
@@ -651,4 +658,4 @@ int request_irq(unsigned int irq, irq_ha
return retval;
}
-EXPORT_SYMBOL(request_irq);
+EXPORT_SYMBOL(request_irq_quickcheck);
On Wed, 2008-10-01 at 23:02 +0000, Thomas Gleixner wrote:
> struct irqaction {
> + irq_handler_t quick_check_handler;
> irq_handler_t handler;
When we originally discussed this, there was an idea to modify the
request_irq API to take this handler and an IRQF_THREADED type to mark
the interrupt accordingly. I understand why it's a separate function in
this implementation for ease of migration, but what do you think should
happen in the end? Also, I suggest calling this something like
"quiesce_device" because the quickcheck also needs to do that.
We probably need some documentation eventually so people realize what
this "quickcheck" handler is for and what it's not for - under no
circumstances should anything more than the bare minimum be done.
Otherwise it breaks the benefit of deferred threaded handling. It's hard
to enforce that - but this is *not* a return of top/bottom half handling
where you can do whatever crap you like in the quickcheck bit.
Jon.
Thomas,
Nice work, thanks for doing this.
Little comments below.
On Wed, 1 Oct 2008, Thomas Gleixner wrote:
>
> extern irqreturn_t no_action(int cpl, void *dev_id);
> -extern int __must_check request_irq(unsigned int, irq_handler_t handler,
> +
> +extern int __must_check
> +request_irq_quickcheck(unsigned int, irq_handler_t handler,
> + irq_handler_t quick_check_handler,
> unsigned long, const char *, void *);
It would be nice to still keep the name of the parameters here.
unsigned long flags, const char *name, void *dev
> +
> +static inline int __must_check
> +request_irq(unsigned int irq, irq_handler_t handler, unsigned long flags,
> + const char *name, void *dev)
> +{
> + return request_irq_quickcheck(irq, handler, NULL, flags, name, dev);
> +}
> +
> extern void free_irq(unsigned int, void *);
>
> struct device;
> Index: linux-2.6-tip/include/linux/irqreturn.h
> ===================================================================
> --- linux-2.6-tip.orig/include/linux/irqreturn.h
> +++ linux-2.6-tip/include/linux/irqreturn.h
> @@ -5,10 +5,12 @@
> * enum irqreturn
> * @IRQ_NONE interrupt was not from this device
> * @IRQ_HANDLED interrupt was handled by this device
> + * @IRQ_NEEDS_HANDLING quick check handler requests to run real handler
> */
> enum irqreturn {
> IRQ_NONE,
> IRQ_HANDLED,
> + IRQ_NEEDS_HANDLING,
> };
>
> #define irqreturn_t enum irqreturn
> Index: linux-2.6-tip/kernel/irq/handle.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/irq/handle.c
> +++ linux-2.6-tip/kernel/irq/handle.c
> @@ -137,9 +137,21 @@ irqreturn_t handle_IRQ_event(unsigned in
> local_irq_enable_in_hardirq();
>
> do {
> - ret = action->handler(irq, action->dev_id);
> - if (ret == IRQ_HANDLED)
> - status |= action->flags;
> + if (action->quick_check_handler)
> + ret = action->quick_check_handler(irq, action->dev_id);
> + else
> + ret = IRQ_NEEDS_HANDLING;
> +
> + switch (ret) {
> + default:
> + break;
> +
> + case IRQ_NEEDS_HANDLING:
> + ret = action->handler(irq, action->dev_id);
> + if (ret == IRQ_HANDLED)
> + status |= action->flags;
> + break;
> + }
> retval |= ret;
> action = action->next;
> } while (action);
> Index: linux-2.6-tip/kernel/irq/manage.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/irq/manage.c
> +++ linux-2.6-tip/kernel/irq/manage.c
> @@ -563,9 +563,14 @@ void free_irq(unsigned int irq, void *de
> EXPORT_SYMBOL(free_irq);
>
> /**
> - * request_irq - allocate an interrupt line
> + * request_irq_quickcheck - allocate an interrupt line
> * @irq: Interrupt line to allocate
> - * @handler: Function to be called when the IRQ occurs
> + * @handler: Function to be called when the IRQ occurs.
> + * Primary handler for threaded interrupts
> + * @quick_check_handler: Function to be called when the interrupt
> + * before the real handler is called to check
> + * whether the interrupt originated from the device
> + * can be NULL
The above reads funny. How about something like:
@quick_check_handler: Function called before the real interrupt
handler. It checks if the interrupt originated
from the device. This can be NULL.
-- Steve
> * @irqflags: Interrupt type flags
> * @devname: An ascii name for the claiming device
> * @dev_id: A cookie passed back to the handler function
> @@ -589,10 +594,11 @@ EXPORT_SYMBOL(free_irq);
> * IRQF_SHARED Interrupt is shared
> * IRQF_DISABLED Disable local interrupts while processing
> * IRQF_SAMPLE_RANDOM The interrupt can be used for entropy
> - *
> */
> -int request_irq(unsigned int irq, irq_handler_t handler,
> - unsigned long irqflags, const char *devname, void *dev_id)
> +int request_irq_quickcheck(unsigned int irq, irq_handler_t handler,
> + irq_handler_t quick_check_handler,
> + unsigned long irqflags, const char *devname,
> + void *dev_id)
> {
> struct irqaction *action;
> int retval;
> @@ -622,6 +628,7 @@ int request_irq(unsigned int irq, irq_ha
> if (!action)
> return -ENOMEM;
>
> + action->quick_check_handler = quick_check_handler;
> action->handler = handler;
> action->flags = irqflags;
> cpus_clear(action->mask);
> @@ -651,4 +658,4 @@ int request_irq(unsigned int irq, irq_ha
>
> return retval;
> }
> -EXPORT_SYMBOL(request_irq);
> +EXPORT_SYMBOL(request_irq_quickcheck);
>
>
>
On Wed, 1 Oct 2008, Jon Masters wrote:
>
> We probably need some documentation eventually so people realize what
> this "quickcheck" handler is for and what it's not for - under no
> circumstances should anything more than the bare minimum be done.
> Otherwise it breaks the benefit of deferred threaded handling. It's hard
> to enforce that - but this is *not* a return of top/bottom half handling
> where you can do whatever crap you like in the quickcheck bit.
>
We could always implement something similar to what I was told Microsoft
does (I was just told this, I don't know for fact). Time this function and
if it takes longer than, say 50us, print a warning and kill the device
;-)
-- Steve
On Thu, 2008-10-02 at 01:09 -0400, Steven Rostedt wrote:
> On Wed, 1 Oct 2008, Jon Masters wrote:
> >
> > We probably need some documentation eventually so people realize what
> > this "quickcheck" handler is for and what it's not for - under no
> > circumstances should anything more than the bare minimum be done.
> > Otherwise it breaks the benefit of deferred threaded handling. It's hard
> > to enforce that - but this is *not* a return of top/bottom half handling
> > where you can do whatever crap you like in the quickcheck bit.
> >
>
> We could always implement something similar to what I was told Microsoft
> does (I was just told this, I don't know for fact). Time this function and
> if it takes longer than, say 50us, print a warning and kill the device
> ;-)
You know, it's funny you suggested that because I thought about going
there. But there's probably some silly patent on that groundshattering
Microsoft solution to the halting problem.
Anyway, I like to think we in the Linux community trust developers to do
the right thing more than Microsoft does :)
Jon.
On Thu, Oct 02, 2008 at 06:51:49AM -0400, Jon Masters wrote:
> On Thu, 2008-10-02 at 01:09 -0400, Steven Rostedt wrote:
> > On Wed, 1 Oct 2008, Jon Masters wrote:
> > >
> > > We probably need some documentation eventually so people realize what
> > > this "quickcheck" handler is for and what it's not for - under no
> > > circumstances should anything more than the bare minimum be done.
> > > Otherwise it breaks the benefit of deferred threaded handling. It's hard
> > > to enforce that - but this is *not* a return of top/bottom half handling
> > > where you can do whatever crap you like in the quickcheck bit.
> > >
> >
> > We could always implement something similar to what I was told Microsoft
> > does (I was just told this, I don't know for fact). Time this function and
> > if it takes longer than, say 50us, print a warning and kill the device
> > ;-)
>
> You know, it's funny you suggested that because I thought about going
> there. But there's probably some silly patent on that groundshattering
> Microsoft solution to the halting problem.
>
> Anyway, I like to think we in the Linux community trust developers to do
> the right thing more than Microsoft does :)
"Trust, but verify" :)
We should just print out something if it takes longer than Xus, that way
we can fix the code, something that other operating systems can't do.
thanks,
greg k-h
On Wed, Oct 01, 2008 at 11:02:18PM -0000, Thomas Gleixner wrote:
> Preparatory patch for threaded interrupt handlers.
>
> Adds a quick check handler which is called before the real handler.
> The quick check handler can decide whether the interrupt was originated
> from the device or not. It can also declare the interrupt as handled
> and subsequently avoid that the real handler is called.
I'd rather leave the handler as a handle which could return
IRQ_NEEDS_HANDLING to get a separate thread_fn called. This seems more
intuitive and means less mess in the fastpath.
Maybe IRQ_NEEDS_HANDLING might better be named IRQ_THREADED os similar,
but that's really getting into nitpicking..
On Fri, 3 Oct 2008, Christoph Hellwig wrote:
> On Wed, Oct 01, 2008 at 11:02:18PM -0000, Thomas Gleixner wrote:
> > Preparatory patch for threaded interrupt handlers.
> >
> > Adds a quick check handler which is called before the real handler.
> > The quick check handler can decide whether the interrupt was originated
> > from the device or not. It can also declare the interrupt as handled
> > and subsequently avoid that the real handler is called.
>
> I'd rather leave the handler as a handle which could return
> IRQ_NEEDS_HANDLING to get a separate thread_fn called. This seems more
> intuitive and means less mess in the fastpath.
see patch 3/5 :)
> Maybe IRQ_NEEDS_HANDLING might better be named IRQ_THREADED os similar,
> but that's really getting into nitpicking..
The reason why I split this out is: when you start to convert your
driver, then you can split out the quick check handler first w/o
making the real handler threaded in the first place. Once you got that
right you can work on moving the real handler into a thread.
Thanks,
tglx