Re: [RFC patch 2/5] genirq: add a quick check handler

Previous thread: booting uncompressed kernel on x86 by Pazzo Da Legare on Wednesday, October 1, 2008 - 2:57 pm. (5 messages)

Next thread: [RFC patch 4/5] genirq: add a helper to check whether the irq thread should run by Thomas Gleixner on Wednesday, October 1, 2008 - 4:02 pm. (1 message)
From: Thomas Gleixner
Date: Wednesday, October 1, 2008 - 4:02 pm

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 <tglx@linutronix.de>
Reviewed-by: Ingo Molnar <mingo@elte.hu>
---
 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 ...
From: Jon Masters
Date: Wednesday, October 1, 2008 - 5:47 pm

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.


--

From: Steven Rostedt
Date: Wednesday, October 1, 2008 - 10:09 pm

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

--

From: Jon Masters
Date: Thursday, October 2, 2008 - 3:51 am

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.


--

From: Greg KH
Date: Thursday, October 2, 2008 - 3:06 pm

"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
--

From: Steven Rostedt
Date: Wednesday, October 1, 2008 - 9:52 pm

Thomas,

Nice work, thanks for doing this.

Little comments below.


It would be nice to still keep the name of the parameters here.


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.

--

From: Christoph Hellwig
Date: Friday, October 3, 2008 - 1:29 am

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..

--

From: Thomas Gleixner
Date: Friday, October 3, 2008 - 3:37 am

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

--

Previous thread: booting uncompressed kernel on x86 by Pazzo Da Legare on Wednesday, October 1, 2008 - 2:57 pm. (5 messages)

Next thread: [RFC patch 4/5] genirq: add a helper to check whether the irq thread should run by Thomas Gleixner on Wednesday, October 1, 2008 - 4:02 pm. (1 message)