[patch 0/2] Add support for threaded interrupt handlers - V3

[patch 0/2] Add support for threaded interrupt handlers - V3

[Thomas Gleixner]

This patch series implements support for threaded irq handlers for the
generic IRQ layer.

Changes vs. V2:
	- review comments addressed
	- simplified design
	- synchronize_irq adapted to threaded handlers

Threaded interrupt handlers are not only interesting in the preempt-rt
context. Threaded interrupt handlers can help to address common
problems in the interrupt handling code:

 - move long running handlers out of the hard interrupt context

 - avoid complex hardirq -> tasklet/softirq interaction and locking
   problems by integration of this functionality into the threaded
   handler code

 - improved debugability of the kernel: faulty handlers do not take
   down the system.

 - allows prioritizing of the handlers which share an interrupt line

The implementation provides an opt-in mechanism to convert drivers to
the threaded interrupt handler model contrary to the preempt-rt patch
where the threaded handlers are enabled by a brute force switch. The
brute force switch is suboptimal as it does not change the interrupt
handler -> tasklet/softirq interaction problems, but was the only way
which was possible for the limited man power of the preempt-rt
developers.

Converting an interrupt to threaded makes only sense when the handler
code takes advantage of it by integrating tasklet/softirq
functionality and simplifying the locking.

When a driver wants to use threaded interrupt handlers it needs to
provide a separate thread function, which is called from the per
device thread created in irq_request_threaded(). The primary handler
still needs to be supplied. It checks whether the interrupt was
originated from the device or not.

In case it was originated from the device the primary handler must
disable the interrupt at the device level and return
IRQ_WAKE_THREAD. The generic interrupt handling core then sets the
IRQF_RUNTHREAD in the irqaction of the handler and wakes the
associated thread.

The irqaction is referenced in the threads task_struct. The reference
is used to prevent further referencing of the thread in the interrupt
code in the case of segfault to make sure that the system (minus the
now dead interrupt handler) survives and debug information can be
retrieved. In the best case the driver can be restarted, but dont
expect that as a given.

The code was tested with a converted USB EHCI driver. The EHCI shares
an interrupt line with another device and both the threaded and the
non threaded handlers coexist nicely. 

Further the AHCI driver for ICH chipsets was converted and
tested. Interestingly enough this conversion did not cause worse
benchmark results with iobench and fio compared to the non threaded
mainline handler.

I'm still looking into a clean solution for the threaded demultiplex
handler case which was brought up by Dave to allow both the handling
of the demultiplexed devices in the context of the demultiplexer
interrupt thread and the wakeup of separate handler threads. But this
is an orthogonal extension of the existing patch set and does not
change the general design.

The patches apply on 
    git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git irq/threaded

The irq/threaded branch contains the generic irq related changes which are
already queued for .30

Thanks,

	tglx

[patch 1/2] genirq: add threaded interrupt handler support

[Thomas Gleixner]

[-- Attachment #1: genirq-add-threaded-irq-handlers.patch --]
[-- Type: text/plain, Size: 16724 bytes --]

Add support for threaded interrupt handlers:

A device driver can request that its main interrupt handler runs in a
thread. To achive this the device driver requests the interrupt with
request_threaded_irq() and provides additionally to the handler a
thread function. The handler function is called in hard interrupt
context and needs to check whether the interrupt originated from the
device. If the interrupt originated from the device then the handler
can either return IRQ_HANDLED or IRQ_WAKE_THREAD. IRQ_HANDLED is
returned when no further action is required. IRQ_WAKE_THREAD causes
the genirq code to invoke the threaded (main) handler. When
IRQ_WAKE_THREAD is returned handler must have disabled the interrupt
on the device level. This is mandatory for shared interrupt handlers,
but we need to do it as well for obscure x86 hardware where disabling
an interrupt on the IO_APIC level redirects the interrupt to the
legacy PIC interrupt lines.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/hardirq.h   |    2 
 include/linux/interrupt.h |   37 ++++++++
 include/linux/irq.h       |    5 +
 include/linux/irqreturn.h |    2 
 include/linux/sched.h     |    5 +
 kernel/exit.c             |    2 
 kernel/irq/handle.c       |   31 +++++++
 kernel/irq/manage.c       |  193 ++++++++++++++++++++++++++++++++++++++++++----
 8 files changed, 260 insertions(+), 17 deletions(-)

Index: linux-2.6-tip/include/linux/hardirq.h
===================================================================
--- linux-2.6-tip.orig/include/linux/hardirq.h
+++ linux-2.6-tip/include/linux/hardirq.h
@@ -105,7 +105,7 @@
 # define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
 #endif
 
-#ifdef CONFIG_SMP
+#if defined(CONFIG_SMP) || defined(CONFIG_GENERIC_HARDIRQS)
 extern void synchronize_irq(unsigned int irq);
 #else
 # define synchronize_irq(irq)	barrier()
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
@@ -59,6 +59,16 @@
 #define IRQF_NOBALANCING	0x00000800
 #define IRQF_IRQPOLL		0x00001000
 
+/*
+ * Bits used by threaded handlers:
+ * IRQTF_RUNTHREAD - signals that the interrupt handler thread should run
+ * IRQTF_DIED      - handler thread died
+ */
+enum {
+	IRQTF_RUNTHREAD,
+	IRQTF_DIED,
+};
+
 typedef irqreturn_t (*irq_handler_t)(int, void *);
 
 /**
@@ -71,6 +81,9 @@ typedef irqreturn_t (*irq_handler_t)(int
  * @next:	pointer to the next irqaction for shared interrupts
  * @irq:	interrupt number
  * @dir:	pointer to the proc/irq/NN/name entry
+ * @thread_fn:	interupt handler function for threaded interrupts
+ * @thread:	thread pointer for threaded interrupts
+ * @thread_flags:	flags related to @thread
  */
 struct irqaction {
 	irq_handler_t handler;
@@ -81,11 +94,31 @@ struct irqaction {
 	struct irqaction *next;
 	int irq;
 	struct proc_dir_entry *dir;
+	irq_handler_t thread_fn;
+	struct task_struct *thread;
+	unsigned long thread_flags;
 };
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
-extern int __must_check request_irq(unsigned int, irq_handler_t handler,
-		       unsigned long, const char *, void *);
+
+extern int __must_check
+request_threaded_irq(unsigned int irq, irq_handler_t handler,
+		     irq_handler_t thread_fn,
+		     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_threaded_irq(irq, handler, NULL, flags, name, dev);
+}
+
+#ifdef CONFIG_GENERIC_HARDIRQS
+extern void exit_irq_thread(void);
+#else
+static inline void exit_irq_thread(void) { }
+#endif
+
 extern void free_irq(unsigned int, void *);
 
 struct device;
Index: linux-2.6-tip/include/linux/irq.h
===================================================================
--- linux-2.6-tip.orig/include/linux/irq.h
+++ linux-2.6-tip/include/linux/irq.h
@@ -20,6 +20,7 @@
 #include <linux/irqreturn.h>
 #include <linux/irqnr.h>
 #include <linux/errno.h>
+#include <linux/wait.h>
 
 #include <asm/irq.h>
 #include <asm/ptrace.h>
@@ -155,6 +156,8 @@ struct irq_2_iommu;
  * @affinity:		IRQ affinity on SMP
  * @cpu:		cpu index useful for balancing
  * @pending_mask:	pending rebalanced interrupts
+ * @threads_active:	number of irqaction threads currently running
+ * @wait_for_threads:	wait queue for sync_irq to wait for threaded handlers
  * @dir:		/proc/irq/ procfs entry
  * @name:		flow handler name for /proc/interrupts output
  */
@@ -186,6 +189,8 @@ struct irq_desc {
 	cpumask_var_t		pending_mask;
 #endif
 #endif
+	atomic_t		threads_active;
+	wait_queue_head_t       wait_for_threads;
 #ifdef CONFIG_PROC_FS
 	struct proc_dir_entry	*dir;
 #endif
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_WAKE_THREAD	handler requests to wake the handler thread
  */
 enum irqreturn {
 	IRQ_NONE,
 	IRQ_HANDLED,
+	IRQ_WAKE_THREAD,
 };
 
 typedef enum irqreturn irqreturn_t;
Index: linux-2.6-tip/include/linux/sched.h
===================================================================
--- linux-2.6-tip.orig/include/linux/sched.h
+++ linux-2.6-tip/include/linux/sched.h
@@ -1292,6 +1292,11 @@ struct task_struct {
 /* Protection of (de-)allocation: mm, files, fs, tty, keyrings */
 	spinlock_t alloc_lock;
 
+#ifdef CONFIG_GENERIC_HARDIRQS
+	/* IRQ handler threads */
+	struct irqaction *irqaction;
+#endif
+
 	/* Protection of the PI data structures: */
 	spinlock_t pi_lock;
 
Index: linux-2.6-tip/kernel/exit.c
===================================================================
--- linux-2.6-tip.orig/kernel/exit.c
+++ linux-2.6-tip/kernel/exit.c
@@ -1037,6 +1037,8 @@ NORET_TYPE void do_exit(long code)
 		schedule();
 	}
 
+	exit_irq_thread();
+
 	exit_signals(tsk);  /* sets PF_EXITING */
 	/*
 	 * tsk->flags are checked in the futex code to protect against
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
@@ -358,8 +358,37 @@ irqreturn_t handle_IRQ_event(unsigned in
 
 	do {
 		ret = action->handler(irq, action->dev_id);
-		if (ret == IRQ_HANDLED)
+
+		switch (ret) {
+		case IRQ_WAKE_THREAD:
+			/*
+			 * Wake up the handler thread for this
+			 * action. In case the thread crashed and was
+			 * killed we just pretend that we handled the
+			 * interrupt. The hardirq handler above has
+			 * disabled the device interrupt, so no irq
+			 * storm is lurking.
+			 */
+			if (likely(!test_bit(IRQTF_DIED,
+					     &action->thread_flags))) {
+				set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
+				wake_up_process(action->thread);
+			}
+
+			/*
+			 * Set it to handled so the spurious check
+			 * does not trigger.
+			 */
+			ret = IRQ_HANDLED;
+			/* Fall through to add to randomness */
+		case IRQ_HANDLED:
 			status |= action->flags;
+			break;
+
+		default:
+			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
@@ -8,16 +8,15 @@
  */
 
 #include <linux/irq.h>
+#include <linux/kthread.h>
 #include <linux/module.h>
 #include <linux/random.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/sched.h>
 
 #include "internals.h"
 
-#if defined(CONFIG_SMP) && defined(CONFIG_GENERIC_HARDIRQS)
-cpumask_var_t irq_default_affinity;
-
 /**
  *	synchronize_irq - wait for pending IRQ handlers (on other CPUs)
  *	@irq: interrupt number to wait for
@@ -53,9 +52,18 @@ void synchronize_irq(unsigned int irq)
 
 		/* Oops, that failed? */
 	} while (status & IRQ_INPROGRESS);
+
+	/*
+	 * We made sure that no hardirq handler is running. Now verify
+	 * that no threaded handlers are active.
+	 */
+	wait_event(desc->wait_for_threads, !atomic_read(&desc->threads_active));
 }
 EXPORT_SYMBOL(synchronize_irq);
 
+#ifdef CONFIG_SMP
+cpumask_var_t irq_default_affinity;
+
 /**
  *	irq_can_set_affinity - Check if the affinity of a given irq can be set
  *	@irq:		Interrupt to check
@@ -72,6 +80,18 @@ int irq_can_set_affinity(unsigned int ir
 	return 1;
 }
 
+static void
+irq_set_thread_affinity(struct irq_desc *desc, const struct cpumask *cpumask)
+{
+	struct irqaction *action = desc->action;
+
+	while (action) {
+		if (action->thread)
+			set_cpus_allowed_ptr(action->thread, cpumask);
+		action = action->next;
+	}
+}
+
 /**
  *	irq_set_affinity - Set the irq affinity of a given irq
  *	@irq:		Interrupt to set affinity
@@ -100,6 +120,7 @@ int irq_set_affinity(unsigned int irq, c
 	cpumask_copy(desc->affinity, cpumask);
 	desc->chip->set_affinity(irq, cpumask);
 #endif
+	irq_set_thread_affinity(desc, cpumask);
 	desc->status |= IRQ_AFFINITY_SET;
 	spin_unlock_irqrestore(&desc->lock, flags);
 	return 0;
@@ -150,6 +171,8 @@ int irq_select_affinity_usr(unsigned int
 
 	spin_lock_irqsave(&desc->lock, flags);
 	ret = setup_affinity(irq, desc);
+	if (!ret)
+		irq_set_thread_affinity(desc, desc->affinity);
 	spin_unlock_irqrestore(&desc->lock, flags);
 
 	return ret;
@@ -384,6 +407,93 @@ int __irq_set_trigger(struct irq_desc *d
 	return ret;
 }
 
+static inline int irq_thread_should_run(struct irqaction *action)
+{
+	return test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags);
+}
+
+static int irq_wait_for_interrupt(struct irqaction *action)
+{
+	while (!kthread_should_stop()) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (irq_thread_should_run(action)) {
+			__set_current_state(TASK_RUNNING);
+			return 0;
+		} else
+			schedule();
+	}
+	return -1;
+}
+
+/*
+ * Interrupt handler thread
+ */
+static int irq_thread(void *data)
+{
+	struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO/2, };
+	struct irqaction *action = data;
+	struct irq_desc *desc = irq_to_desc(action->irq);
+	int wake;
+
+	sched_setscheduler(current, SCHED_FIFO, &param);
+	current->irqaction = action;
+
+	while (!irq_wait_for_interrupt(action)) {
+
+		atomic_inc(&desc->threads_active);
+
+		spin_lock_irq(&desc->lock);
+		if (unlikely(desc->status & IRQ_DISABLED)) {
+			/*
+			 * CHECKME: We might need a dedicated
+			 * IRQ_THREAD_PENDING flag here, which
+			 * retriggers the thread in check_irq_resend()
+			 * but AFAICT IRQ_PENDING should be fine as it
+			 * retriggers the interrupt itself --- tglx
+			 */
+			desc->status |= IRQ_PENDING;
+			spin_unlock_irq(&desc->lock);
+		} else {
+			spin_unlock_irq(&desc->lock);
+
+			action->thread_fn(action->irq, action->dev_id);
+		}
+
+		wake = atomic_dec_and_test(&desc->threads_active);
+
+		if (wake && waitqueue_active(&desc->wait_for_threads))
+			wake_up(&desc->wait_for_threads);
+	}
+
+	/*
+	 * Clear irqaction. Otherwise exit_irq_thread() would make
+	 * fuzz about an active irq thread going into nirvana.
+	 */
+	current->irqaction = NULL;
+	return 0;
+}
+
+/*
+ * Called from do_exit()
+ */
+void exit_irq_thread(void)
+{
+	struct task_struct *tsk = current;
+
+	if (!tsk->irqaction)
+		return;
+
+	printk(KERN_ERR
+	       "exiting task \"%s\" (%d) is an active IRQ thread (irq %d)\n",
+	       tsk->comm ? tsk->comm : "", tsk->pid, tsk->irqaction->irq);
+
+	/*
+	 * Set the THREAD DIED flag to prevent further wakeups of the
+	 * soon to be gone threaded handler.
+	 */
+	set_bit(IRQTF_DIED, &tsk->irqaction->flags);
+}
+
 /*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
@@ -420,6 +531,26 @@ __setup_irq(unsigned int irq, struct irq
 	}
 
 	/*
+	 * Threaded handler ?
+	 */
+	if (new->thread_fn) {
+		struct task_struct *t;
+
+		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
+				   new->name);
+		if (IS_ERR(t))
+			return PTR_ERR(t);
+		/*
+		 * We keep the reference to the task struct even if
+		 * the thread dies to avoid that the interrupt code
+		 * references an already freed task_struct.
+		 */
+		get_task_struct(t);
+		new->thread = t;
+		wake_up_process(t);
+	}
+
+	/*
 	 * The following block of code has to be executed atomically
 	 */
 	spin_lock_irqsave(&desc->lock, flags);
@@ -456,15 +587,15 @@ __setup_irq(unsigned int irq, struct irq
 	if (!shared) {
 		irq_chip_set_defaults(desc->chip);
 
+		init_waitqueue_head(&desc->wait_for_threads);
+
 		/* Setup the type (level, edge polarity) if configured: */
 		if (new->flags & IRQF_TRIGGER_MASK) {
 			ret = __irq_set_trigger(desc, irq,
 					new->flags & IRQF_TRIGGER_MASK);
 
-			if (ret) {
-				spin_unlock_irqrestore(&desc->lock, flags);
-				return ret;
-			}
+			if (ret)
+				goto out_thread;
 		} else
 			compat_irq_chip_set_default_handler(desc);
 #if defined(CONFIG_IRQ_PER_CPU)
@@ -532,8 +663,19 @@ mismatch:
 		dump_stack();
 	}
 #endif
+	ret = -EBUSY;
+
+out_thread:
 	spin_unlock_irqrestore(&desc->lock, flags);
-	return -EBUSY;
+	if (new->thread) {
+		struct task_struct *t = new->thread;
+
+		new->thread = NULL;
+		if (likely(!test_bit(IRQTF_DIED, &new->thread_flags)))
+			kthread_stop(t);
+		put_task_struct(t);
+	}
+	return ret;
 }
 
 /**
@@ -559,6 +701,7 @@ static struct irqaction *__free_irq(unsi
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	struct irqaction *action, **action_ptr;
+	struct task_struct *irqthread;
 	unsigned long flags;
 
 	WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
@@ -605,6 +748,10 @@ static struct irqaction *__free_irq(unsi
 		else
 			desc->chip->disable(irq);
 	}
+
+	irqthread = action->thread;
+	action->thread = NULL;
+
 	spin_unlock_irqrestore(&desc->lock, flags);
 
 	unregister_handler_proc(irq, action);
@@ -612,6 +759,12 @@ static struct irqaction *__free_irq(unsi
 	/* Make sure it's not being used on another CPU: */
 	synchronize_irq(irq);
 
+	if (irqthread) {
+		if (!test_bit(IRQTF_DIED, &action->thread_flags))
+			kthread_stop(irqthread);
+		put_task_struct(irqthread);
+	}
+
 #ifdef CONFIG_DEBUG_SHIRQ
 	/*
 	 * It's a shared IRQ -- the driver ought to be prepared for an IRQ
@@ -664,9 +817,12 @@ void free_irq(unsigned int irq, void *de
 EXPORT_SYMBOL(free_irq);
 
 /**
- *	request_irq - allocate an interrupt line
+ *	request_threaded_irq - 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
+ *      @thread_fn: Function called from the irq handler thread
+ *                  If NULL, no irq thread is created
  *	@irqflags: Interrupt type flags
  *	@devname: An ascii name for the claiming device
  *	@dev_id: A cookie passed back to the handler function
@@ -678,6 +834,15 @@ EXPORT_SYMBOL(free_irq);
  *	raises, you must take care both to initialise your hardware
  *	and to set up the interrupt handler in the right order.
  *
+ *	If you want to set up a threaded irq handler for your device
+ *	then you need to supply @handler and @thread_fn. @handler ist
+ *	still called in hard interrupt context and has to check
+ *	whether the interrupt originates from the device. If yes it
+ *	needs to disable the interrupt on the device and return
+ *	IRQ_THREAD_WAKE which will wake up the handler thread and run
+ *	@thread_fn. This split handler design is necessary to support
+ *	shared interrupts.
+ *
  *	Dev_id must be globally unique. Normally the address of the
  *	device data structure is used as the cookie. Since the handler
  *	receives this value it makes sense to use it.
@@ -693,8 +858,9 @@ EXPORT_SYMBOL(free_irq);
  *	IRQF_TRIGGER_*		Specify active edge(s) or level
  *
  */
-int request_irq(unsigned int irq, irq_handler_t handler,
-		unsigned long irqflags, const char *devname, void *dev_id)
+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;
@@ -742,6 +908,7 @@ int request_irq(unsigned int irq, irq_ha
 		return -ENOMEM;
 
 	action->handler = handler;
+	action->thread_fn = thread_fn;
 	action->flags = irqflags;
 	action->name = devname;
 	action->dev_id = dev_id;
@@ -771,4 +938,4 @@ int request_irq(unsigned int irq, irq_ha
 #endif
 	return retval;
 }
-EXPORT_SYMBOL(request_irq);
+EXPORT_SYMBOL(request_threaded_irq);

[patch 2/2] genirq: add support for threaded interrupts to devres

[Thomas Gleixner]

[-- Attachment #1: genirq-add-support-for-threaded-to-devres.patch --]
[-- Type: text/plain, Size: 3444 bytes --]

Impact: support threaded interrupts in devres

Some devices use devres_request_irq() for to install their interrupt
handler. Add support for threaded interrupts to devres as well.

[tglx - simplified and adapted to latest threadirq version]

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/interrupt.h |   17 ++++++++++++++---
 kernel/irq/devres.c       |   16 ++++++++++------
 2 files changed, 24 insertions(+), 9 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
@@ -123,9 +123,20 @@ extern void free_irq(unsigned int, void 
 
 struct device;
 
-extern int __must_check devm_request_irq(struct device *dev, unsigned int irq,
-			    irq_handler_t handler, unsigned long irqflags,
-			    const char *devname, void *dev_id);
+extern int __must_check
+devm_request_threaded_irq(struct device *dev, unsigned int irq,
+			  irq_handler_t handler, irq_handler_t thread_fn,
+			  unsigned long irqflags, const char *devname,
+			  void *dev_id);
+
+static inline int __must_check
+devm_request_irq(struct device *dev, unsigned int irq, irq_handler_t handler,
+		 unsigned long irqflags, const char *devname, void *dev_id)
+{
+	return devm_request_threaded_irq(dev, irq, handler, NULL, irqflags,
+					 devname, dev_id);
+}
+
 extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
 
 /*
Index: linux-2.6-tip/kernel/irq/devres.c
===================================================================
--- linux-2.6-tip.orig/kernel/irq/devres.c
+++ linux-2.6-tip/kernel/irq/devres.c
@@ -26,10 +26,12 @@ static int devm_irq_match(struct device 
 }
 
 /**
- *	devm_request_irq - allocate an interrupt line for a managed device
+ *	devm_request_threaded_irq - allocate an interrupt line for a managed device
  *	@dev: device to request interrupt for
  *	@irq: Interrupt line to allocate
  *	@handler: Function to be called when the IRQ occurs
+ *	@thread_fn: function to be called in a threaded interrupt context. NULL
+ *		    for devices which handle everything in @handler
  *	@irqflags: Interrupt type flags
  *	@devname: An ascii name for the claiming device
  *	@dev_id: A cookie passed back to the handler function
@@ -42,9 +44,10 @@ static int devm_irq_match(struct device 
  *	If an IRQ allocated with this function needs to be freed
  *	separately, dev_free_irq() must be used.
  */
-int devm_request_irq(struct device *dev, unsigned int irq,
-		     irq_handler_t handler, unsigned long irqflags,
-		     const char *devname, void *dev_id)
+int devm_request_threaded_irq(struct device *dev, unsigned int irq,
+			      irq_handler_t handler, irq_handler_t thread_fn,
+			      unsigned long irqflags, const char *devname,
+			      void *dev_id)
 {
 	struct irq_devres *dr;
 	int rc;
@@ -54,7 +57,8 @@ int devm_request_irq(struct device *dev,
 	if (!dr)
 		return -ENOMEM;
 
-	rc = request_irq(irq, handler, irqflags, devname, dev_id);
+	rc = request_threaded_irq(irq, handler, thread_fn, irqflags, devname,
+				  dev_id);
 	if (rc) {
 		devres_free(dr);
 		return rc;
@@ -66,7 +70,7 @@ int devm_request_irq(struct device *dev,
 
 	return 0;
 }
-EXPORT_SYMBOL(devm_request_irq);
+EXPORT_SYMBOL(devm_request_threaded_irq);
 
 /**
  *	devm_free_irq - free an interrupt

Re: [patch 1/2] genirq: add threaded interrupt handler support

[Christoph Hellwig]

I like the API and concept of the patch a lot.

some minor comments:

> +		switch (ret) {
> +		case IRQ_WAKE_THREAD:
> +			/*
> +			 * Wake up the handler thread for this
> +			 * action. In case the thread crashed and was
> +			 * killed we just pretend that we handled the
> +			 * interrupt. The hardirq handler above has
> +			 * disabled the device interrupt, so no irq
> +			 * storm is lurking.
> +			 */

We probably wants some sort of error check to catch drivers returning
IRQ_WAKE_THREAD without actually defining a threaded handler here.

> +static inline int irq_thread_should_run(struct irqaction *action)
> +{
> +	return test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> +}
> +
> +static int irq_wait_for_interrupt(struct irqaction *action)
> +{
> +	while (!kthread_should_stop()) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		if (irq_thread_should_run(action)) {

Does adding the helper for one use really help readability?

> +			__set_current_state(TASK_RUNNING);
> +			return 0;
> +		} else
> +			schedule();

No need for the else here :)

> +	if (new->thread_fn) {
> +		struct task_struct *t;
> +
> +		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
> +				   new->name);
> +		if (IS_ERR(t))
> +			return PTR_ERR(t);
> +		/*
> +		 * We keep the reference to the task struct even if
> +		 * the thread dies to avoid that the interrupt code
> +		 * references an already freed task_struct.
> +		 */
> +		get_task_struct(t);
> +		new->thread = t;
> +		wake_up_process(t);
> +	}

We should really introduce a refcounted variant of the kthread helpers.
Not an argument against the patch, just a public mental note.

>  /**
> - *	request_irq - allocate an interrupt line
> + *	request_threaded_irq - 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
> + *      @thread_fn: Function called from the irq handler thread
> + *                  If NULL, no irq thread is created

The indentation is messed up here.

Re: [patch 1/2] genirq: add threaded interrupt handler support

[Thomas Gleixner]

Christoph,

On Mon, 23 Mar 2009, Christoph Hellwig wrote:

> I like the API and concept of the patch a lot.
> 
> some minor comments:
> 
> > +		switch (ret) {
> > +		case IRQ_WAKE_THREAD:
> > +			/*
> > +			 * Wake up the handler thread for this
> > +			 * action. In case the thread crashed and was
> > +			 * killed we just pretend that we handled the
> > +			 * interrupt. The hardirq handler above has
> > +			 * disabled the device interrupt, so no irq
> > +			 * storm is lurking.
> > +			 */
> 
> We probably wants some sort of error check to catch drivers returning
> IRQ_WAKE_THREAD without actually defining a threaded handler here.

Yep, got lost in the restructuring.

> > +static inline int irq_thread_should_run(struct irqaction *action)
> > +{
> > +	return test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> > +}
> > +
> > +static int irq_wait_for_interrupt(struct irqaction *action)
> > +{
> > +	while (!kthread_should_stop()) {
> > +		set_current_state(TASK_INTERRUPTIBLE);
> > +		if (irq_thread_should_run(action)) {
> 
> Does adding the helper for one use really help readability?

:)
 
> > +			__set_current_state(TASK_RUNNING);
> > +			return 0;
> > +		} else
> > +			schedule();
> 
> No need for the else here :)

Sigh. I was staring at this construct for a while because it looked
strange :)
 
> > +	if (new->thread_fn) {
> > +		struct task_struct *t;
> > +
> > +		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
> > +				   new->name);
> > +		if (IS_ERR(t))
> > +			return PTR_ERR(t);
> > +		/*
> > +		 * We keep the reference to the task struct even if
> > +		 * the thread dies to avoid that the interrupt code
> > +		 * references an already freed task_struct.
> > +		 */
> > +		get_task_struct(t);
> > +		new->thread = t;
> > +		wake_up_process(t);
> > +	}
> 
> We should really introduce a refcounted variant of the kthread helpers.
> Not an argument against the patch, just a public mental note.

Thought about that already, but did not come around to implement it.
 
> >  /**
> > - *	request_irq - allocate an interrupt line
> > + *	request_threaded_irq - 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
> > + *      @thread_fn: Function called from the irq handler thread
> > + *                  If NULL, no irq thread is created
> 
> The indentation is messed up here.

Thanks for the review ! 

       tglx

genirq: add threaded interrupt handler support - review fixups

[Thomas Gleixner]

Delta patch to address the review comments.

      - Implement warning when IRQ_WAKE_THREAD is requested and no
        thread handler installed
      - coding style fixes

Pointed-out-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20090323185618.GA11432@infradead.org>

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
@@ -63,10 +63,12 @@
  * Bits used by threaded handlers:
  * IRQTF_RUNTHREAD - signals that the interrupt handler thread should run
  * IRQTF_DIED      - handler thread died
+ * IRQTF_WARNED    - warning "IRQ_WAKE_THREAD w/o thread_fn" has been printed
  */
 enum {
 	IRQTF_RUNTHREAD,
 	IRQTF_DIED,
+	IRQTF_WARNED,
 };
 
 typedef irqreturn_t (*irq_handler_t)(int, void *);
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
@@ -338,6 +338,15 @@ irqreturn_t no_action(int cpl, void *dev
 	return IRQ_NONE;
 }
 
+static void warn_no_thread(unsigned int irq, struct irqaction *action)
+{
+	if (test_and_set_bit(IRQTF_WARNED, &action->thread_flags))
+		return;
+
+	printk(KERN_WARNING "IRQ %d device %s returned IRQ_WAKE_THREAD "
+	       "but no thread function available.", irq, action->name);
+}
+
 /**
  * handle_IRQ_event - irq action chain handler
  * @irq:	the interrupt number
@@ -361,6 +370,21 @@ irqreturn_t handle_IRQ_event(unsigned in
 		switch (ret) {
 		case IRQ_WAKE_THREAD:
 			/*
+			 * Set result to handled so the spurious check
+			 * does not trigger.
+			 */
+			ret = IRQ_HANDLED;
+
+			/*
+			 * Catch drivers which return WAKE_THREAD but
+			 * did not set up a thread function
+			 */
+			if (unlikely(!action->thread_fn)) {
+				warn_no_thread(irq, action);
+				break;
+			}
+
+			/*
 			 * Wake up the handler thread for this
 			 * action. In case the thread crashed and was
 			 * killed we just pretend that we handled the
@@ -374,11 +398,6 @@ irqreturn_t handle_IRQ_event(unsigned in
 				wake_up_process(action->thread);
 			}
 
-			/*
-			 * Set it to handled so the spurious check
-			 * does not trigger.
-			 */
-			ret = IRQ_HANDLED;
 			/* Fall through to add to randomness */
 		case IRQ_HANDLED:
 			status |= action->flags;
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
@@ -407,20 +407,17 @@ int __irq_set_trigger(struct irq_desc *d
 	return ret;
 }
 
-static inline int irq_thread_should_run(struct irqaction *action)
-{
-	return test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags);
-}
-
 static int irq_wait_for_interrupt(struct irqaction *action)
 {
 	while (!kthread_should_stop()) {
 		set_current_state(TASK_INTERRUPTIBLE);
-		if (irq_thread_should_run(action)) {
+
+		if (test_and_clear_bit(IRQTF_RUNTHREAD,
+				       &action->thread_flags)) {
 			__set_current_state(TASK_RUNNING);
 			return 0;
-		} else
-			schedule();
+		}
+		schedule();
 	}
 	return -1;
 }
@@ -820,8 +817,8 @@ EXPORT_SYMBOL(free_irq);
  *	@irq: Interrupt line to allocate
  *	@handler: Function to be called when the IRQ occurs.
  *		  Primary handler for threaded interrupts
- *      @thread_fn: Function called from the irq handler thread
- *                  If NULL, no irq thread is created
+ *	@thread_fn: Function called from the irq handler thread
+ *		    If NULL, no irq thread is created
  *	@irqflags: Interrupt type flags
  *	@devname: An ascii name for the claiming device
  *	@dev_id: A cookie passed back to the handler function

Re: [patch 0/2] Add support for threaded interrupt handlers - V3

[David Brownell]

It still looks OK as a basic extension handling one
common model.

Once this is on track to merge (when?), I'd hope various
I2C and SPI device drivers would start to convert.  The
most common example might be handling RTC alarms from the
numerous I2C and SPI based RTCs. 

(The ones that don't need to manage an irq_chip across
that queued-message bus, and demux its IRQs.  PMICs are
a bit more likely to need to demux lots of IRQs.)


On Monday 23 March 2009, Thomas Gleixner wrote:
> I'm still looking into a clean solution for the threaded demultiplex
> handler case which was brought up by Dave to allow both the handling
> of the demultiplexed devices in the context of the demultiplexer
> interrupt thread and the wakeup of separate handler threads. But this
> is an orthogonal extension of the existing patch set and does not
> change the general design.

No comments on the patch I sent?

  http://marc.info/?l=linux-kernel&m=123734579014392&w=2

Or is that what you meant by "orthogonal"?  Admittedly that
patch sort of begs the question about which request_irq()
variant should be used for such demuxed IRQs; the "current"
assumption is that request_irq() suffices, but that could
be improved so the handle_threaded_irq() flow handler could
use the action->thread_fn not action->handler.

- Dave

Re: [patch 0/2] Add support for threaded interrupt handlers - V3

[Thomas Gleixner]

On Tue, 24 Mar 2009, David Brownell wrote:
> On Monday 23 March 2009, Thomas Gleixner wrote:
> > I'm still looking into a clean solution for the threaded demultiplex
> > handler case which was brought up by Dave to allow both the handling
> > of the demultiplexed devices in the context of the demultiplexer
> > interrupt thread and the wakeup of separate handler threads. But this
> > is an orthogonal extension of the existing patch set and does not
> > change the general design.
> 
> No comments on the patch I sent?

Looked at it briefly, but I still try to figure out what the best
solution for this will be. As I said I'd like to support both
variants:

1) demux handlers run in the primary interrupt thread context
2) demux handlers kick their own handler threads

> Or is that what you meant by "orthogonal"?  Admittedly that
> patch sort of begs the question about which request_irq()
> variant should be used for such demuxed IRQs; the "current"
> assumption is that request_irq() suffices, but that could
> be improved so the handle_threaded_irq() flow handler could
> use the action->thread_fn not action->handler.

I don't want to special case that. See above.

Thanks,

	tglx

Re: [patch 0/2] Add support for threaded interrupt handlers - V3

[David Brownell]

On Tuesday 24 March 2009, Thomas Gleixner wrote:
> On Tue, 24 Mar 2009, David Brownell wrote:
> > On Monday 23 March 2009, Thomas Gleixner wrote:
> > > I'm still looking into a clean solution for the threaded demultiplex
> > > handler case which was brought up by Dave to allow both the handling
> > > of the demultiplexed devices in the context of the demultiplexer
> > > interrupt thread and the wakeup of separate handler threads. But this
> > > is an orthogonal extension of the existing patch set and does not
> > > change the general design.
> > 
> > No comments on the patch I sent?
> 
> Looked at it briefly, but I still try to figure out what the best
> solution for this will be. As I said I'd like to support both
> variants:
> 
> 1) demux handlers run in the primary interrupt thread context
> 2) demux handlers kick their own handler threads

I have no need for the latter, at least in current systems.
The IRQs are neither frequent nor time-critical, else they'd
not be managed via I2C in the first place ... even high speed
I2C (these mostly use 2.6 Mbit/sec, not 100 Kbit/sec) isn't
as quick as accessing any kind of local PIC.


The simplest case, FWIW, is with chips like the mcp23s08
GPIO expanders, or their I2C siblings:  just one level of
dispatch needed.  The Linux driver for those chips doesn't
yet support its IRQs, primarily because genirq doesn't cope
with such stuff very well yet.  (You might want to think
about something a lot simpler than TWL4030-family PMICs!)


> > Or is that what you meant by "orthogonal"?  Admittedly that
> > patch sort of begs the question about which request_irq()
> > variant should be used for such demuxed IRQs; the "current"
> > assumption is that request_irq() suffices, but that could
> > be improved so the handle_threaded_irq() flow handler could
> > use the action->thread_fn not action->handler.
> 
> I don't want to special case that. See above.

What's a special case though?  If you're serious about
wanting to support more than one case, it's always going
to be possible to call some of them "special".  As in,
"threaded IRQs are a special case in genirq".  That should
not mean they don't get handled.

- Dave

Re: [patch 0/2] Add support for threaded interrupt handlers - V3

[Thomas Gleixner]

On Tue, 24 Mar 2009, David Brownell wrote:

> On Tuesday 24 March 2009, Thomas Gleixner wrote:
> > On Tue, 24 Mar 2009, David Brownell wrote:
> > > On Monday 23 March 2009, Thomas Gleixner wrote:
> > > > I'm still looking into a clean solution for the threaded demultiplex
> > > > handler case which was brought up by Dave to allow both the handling
> > > > of the demultiplexed devices in the context of the demultiplexer
> > > > interrupt thread and the wakeup of separate handler threads. But this
> > > > is an orthogonal extension of the existing patch set and does not
> > > > change the general design.
> > > 
> > > No comments on the patch I sent?
> > 
> > Looked at it briefly, but I still try to figure out what the best
> > solution for this will be. As I said I'd like to support both
> > variants:
> > 
> > 1) demux handlers run in the primary interrupt thread context
> > 2) demux handlers kick their own handler threads
> 
> I have no need for the latter, at least in current systems.

Groan, the fact that you do not need it is definitely _not_ a good
reason to just add a irq_is_sufficient_for_dave_handler.

> > I don't want to special case that. See above.
> 
> What's a special case though?  If you're serious about
> wanting to support more than one case, it's always going
> to be possible to call some of them "special".  As in,
> "threaded IRQs are a special case in genirq".  That should
> not mean they don't get handled.

I don't like the idea of another action dispatcher in a special case
handler. The goal is to reuse the code i.e. simple_handler and
handle_IRQ_event. It just needs some thoughts to implement it in a
sane way.

Thanks,

	tglx

Re: [patch 0/2] Add support for threaded interrupt handlers - V3

[David Brownell]

On Wednesday 25 March 2009, Thomas Gleixner wrote:
> > I have no need for the latter, at least in current systems.
> 
> Groan, the fact that you do not need it is definitely _not_ a good
> reason to just add a irq_is_sufficient_for_dave_handler.

Groan ... don't *you* be puttin' words in *my* mouth.

When I've been in the position you're now in, I've
found it useful to know the actual requirements of
interface users.  Without knowing that, it's kind of
hard to deliver a usable solution, n'est-ce pas?

I'm fairly certain you've seen systems that needlessly
pulled in complicating requirements, and thereby caused
trouble.  If you can provide something to efficiently
address both modes, fine.  But "the latter" seems like
one of those needless complicating additions, which
could easily slow progress.


> > > I don't want to special case that. See above.
> > 
> > What's a special case though?  If you're serious about
> > wanting to support more than one case, it's always going
> > to be possible to call some of them "special".  As in,
> > "threaded IRQs are a special case in genirq".  That should
> > not mean they don't get handled.
> 
> I don't like the idea of another action dispatcher in a special case
> handler. The goal is to reuse the code i.e. simple_handler and
> handle_IRQ_event. It just needs some thoughts to implement it in a
> sane way.

You were the one to suggest a flow handler specifically
for cases like this, though ... you seem to have changed
your mind on this topic.  ISTR the rationale was to get
past the current IRQF_DISABLED special casing found in
handle_IRQ_event(), by using a flow handler which didn't
call that routine.

- Dave