Support for irq chips on slow busses (i2c, spi)

RFC PATCH In review Thomas Gleixner 5 participants 16 messages Aug 13, 2009 – Aug 15, 2009 IRQ

Share & tools

Thomas Gleixner

author

2009-08-13 19:41 UTC #0 lore raw

cover Support for irq chips on slow busses (i2c, spi)

The support for irq chips on slow busses eg. i2c, spi has been discussed to great length several times. Most of the details can be found in this thread: http://lkml.org/lkml/2009/7/21/266

The following patch series is a round up of the various patch snippets sent out during the discussion and the ideas we agreed on.

There are some subtle differences to the initial test patches:

1) Instead of using a separate flow handler we allow to request the one shot feature with a flag on request_irq and integrate the oneshot feature into the existing level/edge handlers.

2) Instead of conditional locking which makes the patch smaller but hides the already known fact that a device sits behind a slow irq chip I implemented a set of separate management functions (see patch 2/3). The initial idea with the conditional locking can be found here:
http://lkml.org/lkml/2009/7/22/163

3) patch 3/3 implements a helper function which allows the demultiplexing thread to call the demux handlers in its own thread context with out creating a separate thread for each subdevice interrupt. The device driver needs to request the interrupt via request_nested_slowbus_irq() to prevent the creation of the extra thread.

Please have a thorough look and hopefully a test ride on your favourite slowbus irq chip implementation so we can get this into .32

Thanks,

tglx

Thomas Gleixner

author

2009-08-13 19:41 UTC #1 lore raw

RFC PATCH 1/3 genirq: Add oneshot support

For threaded interrupt handlers we expect the hard interrupt handler part to mask the interrupt on the originating device. The interrupt line itself is reenabled after the hard interrupt handler has executed.

This requires access to the originating device from hard interrupt context which is not always possible. There are devices which can only be accessed via a bus (i2c, spi, ...). The bus access requires thread context. For such devices we need to keep the interrupt line masked until the threaded handler has executed.

Add a new flag IRQF_ONESHOT which allows drivers to request that the interrupt is not unmasked after the hard interrupt context handler has been executed and the thread has been woken. The interrupt line is unmasked after the thread handler function has been executed.

Note that for now IRQF_ONESHOT cannot be used with IRQF_SHARED to avoid complex accounting mechanisms.

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/interrupt.h |    4 ++++
 include/linux/irq.h       |    1 +
 kernel/irq/chip.c         |   14 +++++++++++---
 kernel/irq/manage.c       |   29 +++++++++++++++++++++++++++--
 4 files changed, 43 insertions(+), 5 deletions(-)

include/linux/interrupt.h +4 -0

Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================
@@ -49,6 +49,9 @@
  * IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is
  *                registered first in an shared interrupt is considered for
  *                performance reasons)
+ * 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.
  */
 #define IRQF_DISABLED		0x00000020
 #define IRQF_SAMPLE_RANDOM	0x00000040

@@ -58,6 +61,7 @@
 #define IRQF_PERCPU		0x00000400
 #define IRQF_NOBALANCING	0x00000800
 #define IRQF_IRQPOLL		0x00001000
+#define IRQF_ONESHOT		0x00002000
 
 /*
  * Bits used by threaded handlers:

include/linux/irq.h +1 -0

Index: linux-2.6-tip/include/linux/irq.h
===================================================================
@@ -69,6 +69,7 @@ typedef	void (*irq_flow_handler_t)(unsig
 #define IRQ_MOVE_PCNTXT		0x01000000	/* IRQ migration from process context */
 #define IRQ_AFFINITY_SET	0x02000000	/* IRQ affinity was set from userspace*/
 #define IRQ_SUSPENDED		0x04000000	/* IRQ has gone through suspend sequence */
+#define IRQ_ONESHOT		0x08000000	/* IRQ is not unmasked after hardirq */
 
 #ifdef CONFIG_IRQ_PER_CPU
 # define CHECK_IRQ_PER_CPU(var) ((var) & IRQ_PER_CPU)

kernel/irq/chip.c +11 -3

Index: linux-2.6-tip/kernel/irq/chip.c
===================================================================
@@ -382,7 +382,10 @@ handle_level_irq(unsigned int irq, struc
 
 	spin_lock(&desc->lock);
 	desc->status &= ~IRQ_INPROGRESS;
-	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
+
+	if (unlikely(desc->status & IRQ_ONESHOT))
+		desc->status |= IRQ_MASKED;
+	else if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
 		desc->chip->unmask(irq);
 out_unlock:
 	spin_unlock(&desc->lock);

@@ -478,8 +481,13 @@ handle_edge_irq(unsigned int irq, struct
 	kstat_incr_irqs_this_cpu(irq, desc);
 
 	/* Start handling the irq */
-	if (desc->chip->ack)
-		desc->chip->ack(irq);
+	if (unlikely(desc->status & IRQ_ONESHOT)) {
+		desc->status |= IRQ_MASKED;
+		mask_ack_irq(desc, irq);
+	} else {
+		if (desc->chip->ack)
+			desc->chip->ack(irq);
+	}
 
 	/* Mark the IRQ currently in progress.*/
 	desc->status |= IRQ_INPROGRESS;

kernel/irq/manage.c +27 -2

Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
@@ -444,6 +444,21 @@ static int irq_wait_for_interrupt(struct
 }
 
 /*
+ * Oneshot interrupts keep the irq line masked until the threaded
+ * handler finished. unmask if the interrupt has not been disabled and
+ * is marked MASKED.
+ */
+static void irq_finalize_oneshot(unsigned int irq, struct irq_desc *desc)
+{
+	spin_lock_irq(&desc->lock);
+	if (!(desc->status & IRQ_DISABLED) && (desc->status & IRQ_MASKED)) {
+		desc->status &= ~IRQ_MASKED;
+		desc->chip->unmask(irq);
+	}
+	spin_unlock_irq(&desc->lock);
+}
+
+/*
  * Interrupt handler thread
  */
 static int irq_thread(void *data)

@@ -451,7 +466,7 @@ 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;
+	int wake, oneshot = desc->status & IRQ_ONESHOT;
 
 	sched_setscheduler(current, SCHED_FIFO, &param);
 	current->irqaction = action;

@@ -475,6 +490,9 @@ static int irq_thread(void *data)
 			spin_unlock_irq(&desc->lock);
 
 			action->thread_fn(action->irq, action->dev_id);
+
+			if (oneshot)
+				irq_finalize_oneshot(action->irq, desc);
 		}
 
 		wake = atomic_dec_and_test(&desc->threads_active);

@@ -547,6 +565,10 @@ __setup_irq(unsigned int irq, struct irq
 		rand_initialize_irq(irq);
 	}
 
+	/* Oneshot interrupts are not allowed with shared */
+	if ((new->flags & IRQF_ONESHOT) && (new->flags & IRQF_SHARED))
+		return -EINVAL;
+
 	/*
 	 * Threaded handler ?
 	 */

@@ -620,9 +642,12 @@ __setup_irq(unsigned int irq, struct irq
 			desc->status |= IRQ_PER_CPU;
 #endif
 
-		desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING |
+		desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING | IRQ_ONESHOT |
 				  IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);
 
+		if (new->flags & IRQF_ONESHOT)
+			desc->status |= IRQ_ONESHOT;
+
 		if (!(desc->status & IRQ_NOAUTOEN)) {
 			desc->depth = 0;
 			desc->status &= ~IRQ_DISABLED;

Thomas Gleixner

author

2009-08-13 19:41 UTC #2 lore raw

RFC PATCH 2/3 genirq: Add buslock support for irq chips on slow busses

In reply to Thomas Gleixner

Some interrupt chips are connected to a "slow" bus (i2c, spi ...). The bus access needs to sleep and therefor cannot be called in atomic contexts.

Some of the generic interrupt management functions like disable_irq(), enable_irq() ... call interrupt chip functions with the irq_desc->lock held and interrupts disabled. This does not work for such devices.

Provide a separate synchronization mechanism for such interrupt chips. The irq_chip structure is extended by two optional functions (bus_lock and bus_sync_and_unlock).

The idea is to serialize the bus access for those operations in the core code so that drivers which are behind that bus operated interrupt controller do not have to worry about it and just can use the normal interfaces. To achieve this we add two function pointers to the
irq_chip: bus_lock and bus_sync_unlock.

bus_lock() is called to serialize access to the interrupt controller bus.

Now the core code can issue chip->mask/unmask ... commands without changing the fast path code at all. The chip implementation merily stores that information in a chip private data structure and returns. No bus interaction as these functions are called from atomic context.

After that bus_sync_unlock() is called outside the atomic context. Now the chip implementation issues the bus commands, waits for completion and unlocks the interrupt controller bus.

The irq_chip implementation as pseudo code:

struct irq_chip_data {
       struct mutex   mutex;
       unsigned int   irq_offset;
       unsigned long  mask;
       unsigned long  mask_status;
}

static void bus_lock(unsigned int irq)

{
        struct irq_chip_data *data = get_irq_desc_chip_data(irq);

        mutex_lock(&data->mutex);
}

static void mask(unsigned int irq)

{
        struct irq_chip_data *data = get_irq_desc_chip_data(irq);

        irq -= data->irq_offset;
        data->mask |= (1 << irq);
}

static void unmask(unsigned int irq)

{
        struct irq_chip_data *data = get_irq_desc_chip_data(irq);

        irq -= data->irq_offset;
        data->mask &= ~(1 << irq);
}

static void bus_sync_unlock(unsigned int irq)

{
        struct irq_chip_data *data = get_irq_desc_chip_data(irq);

        if (data->mask != data->mask_status) {
                do_bus_magic_to_set_mask(data->mask);
                data->mask_status = data->mask;
        }
        mutex_unlock(&data->mutex);
}

For the device drivers a new set of functions is provided:
request_threaded_slowbus_irq, free_slowbus_irq, disable_slowbus_irq and enable_slowbus_irq. They work the same as the !slowbus variants with the only restriction that the calls need to come from non atomic context.

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/interrupt.h |    9 +++
 include/linux/irq.h       |    6 ++
 kernel/irq/manage.c       |  117 +++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 131 insertions(+), 1 deletion(-)

include/linux/interrupt.h +9 -0

Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================
@@ -113,6 +113,11 @@ request_threaded_irq(unsigned int irq, i
 		     irq_handler_t thread_fn,
 		     unsigned long flags, const char *name, void *dev);
 
+extern int __must_check
+request_threaded_slowbus_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)

@@ -145,6 +150,7 @@ static inline void exit_irq_thread(void)
 #endif
 
 extern void free_irq(unsigned int, void *);
+extern void free_slowbus_irq(unsigned int irq, void *dev_id);
 
 struct device;

@@ -186,6 +192,9 @@ extern void disable_irq_nosync(unsigned 
 extern void disable_irq(unsigned int irq);
 extern void enable_irq(unsigned int irq);
 
+extern void disable_slowbus_irq(unsigned int irq);
+extern void enable_slowbus_irq(unsigned int irq);
+
 /* The following three functions are for the core kernel use only. */
 #ifdef CONFIG_GENERIC_HARDIRQS
 extern void suspend_device_irqs(void);

include/linux/irq.h +6 -0

Index: linux-2.6-tip/include/linux/irq.h
===================================================================
@@ -101,6 +101,9 @@ struct msi_desc;
  * @set_type:		set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
  * @set_wake:		enable/disable power-management wake-on of an IRQ
  *
+ * @bus_lock:		function to lock access to slow bus (i2c) chips
+ * @bus_sync_unlock:	function to sync and unlock slow bus (i2c) chips
+ *
  * @release:		release function solely used by UML
  * @typename:		obsoleted by name, kept as migration helper
  */

@@ -124,6 +127,9 @@ struct irq_chip {
 	int		(*set_type)(unsigned int irq, unsigned int flow_type);
 	int		(*set_wake)(unsigned int irq, unsigned int on);
 
+	void		(*bus_lock)(unsigned int irq);
+	void		(*bus_sync_unlock)(unsigned int irq);
+
 	/* Currently used only by UML, might disappear one day.*/
 #ifdef CONFIG_IRQ_RELEASE_METHOD
 	void		(*release)(unsigned int irq, void *dev_id);

kernel/irq/manage.c +114 -0

135 lines not on this page · raw message

Peter Zijlstra

2009-08-14 08:51 UTC #3 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

Quoted 17 lines · Thomas Gleixner

kernel/irq/manage.c +16 -0

+/**
+ *	disable_slowbus_irq - disable an slowbus irq and wait for completion
+ *	@irq: Interrupt to disable
+ *
+ *	Disable the selected interrupt line.  Enables and Disables are
+ *	nested.
+ *	This function waits for any pending IRQ handlers for this interrupt
+ *	to complete before returning. If you use this function while
+ *	holding a resource the IRQ handler may need you will deadlock.
+ *
+ *	This function must not be called from IRQ context.
+ */
+void disable_slowbus_irq(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+

kernel/irq/manage.c void disable_irq(unsigned int irq) · line 281

Quoted 10 lines

kernel/irq/manage.c +10 -0

+	if (!desc || !desc->chip || !desc->chip->bus_lock)
+		return;
+
+	desc->chip->bus_lock(irq);
+	disable_irq_nosync(irq);
+	if (desc->action)
+		synchronize_irq(irq);
+	desc->chip->bus_sync_unlock(irq);
+}
+EXPORT_SYMBOL(disable_slowbus_irq);

Should we also not check that desc->chip->bus_lock is not set for the regular function disable_irq()?

It seems to me mixing disable_irq() and disable_slowbus_irq() is a recipe for disaster.

Same for the other slowbus functions of course.

Thomas Gleixner

author

2009-08-14 09:04 UTC #4 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

Quoted 32 lines · Peter Zijlstra

Quoted 17 lines · Thomas Gleixner

kernel/irq/manage.c +16 -0

+/**
+ *	disable_slowbus_irq - disable an slowbus irq and wait for completion
+ *	@irq: Interrupt to disable
+ *
+ *	Disable the selected interrupt line.  Enables and Disables are
+ *	nested.
+ *	This function waits for any pending IRQ handlers for this interrupt
+ *	to complete before returning. If you use this function while
+ *	holding a resource the IRQ handler may need you will deadlock.
+ *
+ *	This function must not be called from IRQ context.
+ */
+void disable_slowbus_irq(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+

kernel/irq/manage.c void disable_irq(unsigned int irq) · line 281

Quoted 10 lines

kernel/irq/manage.c +10 -0

+	if (!desc || !desc->chip || !desc->chip->bus_lock)
+		return;
+
+	desc->chip->bus_lock(irq);
+	disable_irq_nosync(irq);
+	if (desc->action)
+		synchronize_irq(irq);
+	desc->chip->bus_sync_unlock(irq);
+}
+EXPORT_SYMBOL(disable_slowbus_irq);

Should we also not check that desc->chip->bus_lock is not set for the regular function disable_irq()?

Quoted 4 lines

It seems to me mixing disable_irq() and disable_slowbus_irq() is a recipe for disaster.
Same for the other slowbus functions of course.

Yeah, that's what I wanted to avoid with the first version, which did the conditional locking and did not require a separate API, but Ingo frowned upon the conditional lock.

Thanks,

tglx

Ingo Molnar

2009-08-14 09:18 UTC #5 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

Quoted 33 lines · Thomas Gleixner

Quoted 32 lines · Peter Zijlstra

Quoted 17 lines · Thomas Gleixner

kernel/irq/manage.c +16 -0

+/**
+ *	disable_slowbus_irq - disable an slowbus irq and wait for completion
+ *	@irq: Interrupt to disable
+ *
+ *	Disable the selected interrupt line.  Enables and Disables are
+ *	nested.
+ *	This function waits for any pending IRQ handlers for this interrupt
+ *	to complete before returning. If you use this function while
+ *	holding a resource the IRQ handler may need you will deadlock.
+ *
+ *	This function must not be called from IRQ context.
+ */
+void disable_slowbus_irq(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+

kernel/irq/manage.c void disable_irq(unsigned int irq) · line 281

Quoted 10 lines

kernel/irq/manage.c +10 -0

+	if (!desc || !desc->chip || !desc->chip->bus_lock)
+		return;
+
+	desc->chip->bus_lock(irq);
+	disable_irq_nosync(irq);
+	if (desc->action)
+		synchronize_irq(irq);
+	desc->chip->bus_sync_unlock(irq);
+}
+EXPORT_SYMBOL(disable_slowbus_irq);

Should we also not check that desc->chip->bus_lock is not set for the regular function disable_irq()?

Quoted 8 lines

Quoted 4 lines
It seems to me mixing disable_irq() and disable_slowbus_irq() is a recipe for disaster.
Same for the other slowbus functions of course.
Yeah, that's what I wanted to avoid with the first version, which did the conditional locking and did not require a separate API, but Ingo frowned upon the conditional lock.

Mind posting that version too?

Conditional locking is really nasty but if the only other option is nastier there's not much we can do, is there?

Ingo

Peter Zijlstra

2009-08-14 09:30 UTC #6 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

Quoted 40 lines · Ingo Molnar

Quoted 38 lines · Thomas Gleixner
Quoted 36 lines · Peter Zijlstra
Quoted 17 lines · Thomas Gleixner
kernel/irq/manage.c +16 -0
+/**
+ *	disable_slowbus_irq - disable an slowbus irq and wait for completion
+ *	@irq: Interrupt to disable
+ *
+ *	Disable the selected interrupt line.  Enables and Disables are
+ *	nested.
+ *	This function waits for any pending IRQ handlers for this interrupt
+ *	to complete before returning. If you use this function while
+ *	holding a resource the IRQ handler may need you will deadlock.
+ *
+ *	This function must not be called from IRQ context.
+ */
+void disable_slowbus_irq(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+
kernel/irq/manage.c void disable_irq(unsigned int irq) · line 281
Quoted 10 lines
kernel/irq/manage.c +10 -0
+	if (!desc || !desc->chip || !desc->chip->bus_lock)
+		return;
+
+	desc->chip->bus_lock(irq);
+	disable_irq_nosync(irq);
+	if (desc->action)
+		synchronize_irq(irq);
+	desc->chip->bus_sync_unlock(irq);
+}
+EXPORT_SYMBOL(disable_slowbus_irq);
Should we also not check that desc->chip->bus_lock is not set for the regular function disable_irq()?
It seems to me mixing disable_irq() and disable_slowbus_irq() is a recipe for disaster.
Same for the other slowbus functions of course.

Quoted 8 lines

Quoted 3 lines
Yeah, that's what I wanted to avoid with the first version, which did the conditional locking and did not require a separate API, but Ingo frowned upon the conditional lock.
Mind posting that version too?
Conditional locking is really nasty but if the only other option is nastier there's not much we can do, is there?

Ah what I meant was that we can make the posted version better by adding

  BUG_ON(desc && desc->chip && desc->chip->bus_lock);

to disable_irq/enable_irq/free_irq.

Thomas Gleixner

author

2009-08-14 10:03 UTC #7 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

In reply to Ingo Molnar

Quoted 8 lines · Ingo Molnar

Quoted 3 lines
Yeah, that's what I wanted to avoid with the first version, which did the conditional locking and did not require a separate API, but Ingo frowned upon the conditional lock.
Mind posting that version too?
Conditional locking is really nasty but if the only other option is nastier there's not much we can do, is there?

Here you go.

Thanks,

tglx
----
Subject: genirq: Add buslock support
From: Thomas Gleixner <[email protected]>
Date: Thu, 13 Aug 2009 12:17:48 +0200

Some interrupt chips are connected to a "slow" bus (i2c, spi ...). The bus access needs to sleep and therefor cannot be called in atomic contexts.

Provide a separate synchronization mechanism for such interrupt chips. The irq_chip structure is extended by two optional functions (bus_lock and bus_sync_and_unlock).

bus_lock() is called to serialize access to the interrupt controller bus.

After that bus_sync_unlock() is called outside the atomic context. Now the chip implementation issues the bus commands, waits for completion and unlocks the interrupt controller bus.

The irq_chip implementation as pseudo code:

struct irq_chip_data {
       struct mutex   mutex;
       unsigned int   irq_offset;
       unsigned long  mask;
       unsigned long  mask_status;
}

static void bus_lock(unsigned int irq)

{
        struct irq_chip_data *data = get_irq_desc_chip_data(irq);

        mutex_lock(&data->mutex);
}

static void mask(unsigned int irq)

{
        struct irq_chip_data *data = get_irq_desc_chip_data(irq);

        irq -= data->irq_offset;
        data->mask |= (1 << irq);
}

static void unmask(unsigned int irq)

{
        struct irq_chip_data *data = get_irq_desc_chip_data(irq);

        irq -= data->irq_offset;
        data->mask &= ~(1 << irq);
}

static void bus_sync_unlock(unsigned int irq)

{
        struct irq_chip_data *data = get_irq_desc_chip_data(irq);

        if (data->mask != data->mask_status) {
                do_bus_magic_to_set_mask(data->mask);
                data->mask_status = data->mask;
        }
        mutex_unlock(&data->mutex);
}

The device drivers can use request_threaded_irq, free_irq, disable_irq and enable_irq as usual with the only restriction that the calls need to come from non atomic context.

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/irq.h |    6 ++++++
 kernel/irq/manage.c |   34 +++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

include/linux/irq.h +6 -0

Index: linux-2.6-tip/include/linux/irq.h
===================================================================
@@ -101,6 +101,9 @@ struct msi_desc;
  * @set_type:		set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
  * @set_wake:		enable/disable power-management wake-on of an IRQ
  *
+ * @bus_lock:		function to lock access to slow bus (i2c) chips
+ * @bus_sync_unlock:	function to sync and unlock slow bus (i2c) chips
+ *
  * @release:		release function solely used by UML
  * @typename:		obsoleted by name, kept as migration helper
  */

@@ -124,6 +127,9 @@ struct irq_chip {
 	int		(*set_type)(unsigned int irq, unsigned int flow_type);
 	int		(*set_wake)(unsigned int irq, unsigned int on);
 
+	void		(*bus_lock)(unsigned int irq);
+	void		(*bus_sync_unlock)(unsigned int irq);
+
 	/* Currently used only by UML, might disappear one day.*/
 #ifdef CONFIG_IRQ_RELEASE_METHOD
 	void		(*release)(unsigned int irq, void *dev_id);

kernel/irq/manage.c +33 -1

Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
@@ -17,6 +17,22 @@
 
 #include "internals.h"
 
+static inline void chip_bus_lock(unsigned int irq, struct irq_desc *desc)
+{
+	if (unlikely(desc->chip->bus_lock)) {
+		might_sleep();
+		desc->chip->bus_lock(irq);
+	}
+}
+
+static inline void chip_bus_sync_unlock(unsigned int irq, struct irq_desc *desc)
+{
+	if (unlikely(desc->chip->bus_sync_unlock)) {
+		might_sleep();
+		desc->chip->bus_sync_unlock(irq);
+	}
+}
+
 /**
  *	synchronize_irq - wait for pending IRQ handlers (on other CPUs)
  *	@irq: interrupt number to wait for

@@ -222,9 +238,11 @@ void disable_irq_nosync(unsigned int irq
 	if (!desc)
 		return;
 
+	chip_bus_lock(irq, desc);
 	spin_lock_irqsave(&desc->lock, flags);
 	__disable_irq(desc, irq, false);
 	spin_unlock_irqrestore(&desc->lock, flags);
+	chip_bus_sync_unlock(irq, desc);
 }
 EXPORT_SYMBOL(disable_irq_nosync);

@@ -286,7 +304,8 @@ void __enable_irq(struct irq_desc *desc,
  *	matches the last disable, processing of interrupts on this
  *	IRQ line is re-enabled.
  *
- *	This function may be called from IRQ context.
+ *	This function may be called from IRQ context only when
+ *	desc->chip->bus_lock and desc->chip->bus_sync_unlock are NULL !
  */
 void enable_irq(unsigned int irq)
 {

@@ -296,9 +315,11 @@ void enable_irq(unsigned int irq)
 	if (!desc)
 		return;
 
+	chip_bus_lock(irq, desc);
 	spin_lock_irqsave(&desc->lock, flags);
 	__enable_irq(desc, irq, false);
 	spin_unlock_irqrestore(&desc->lock, flags);
+	chip_bus_sync_unlock(irq, desc);
 }
 EXPORT_SYMBOL(enable_irq);

@@ -854,7 +875,14 @@ EXPORT_SYMBOL_GPL(remove_irq);
  */
 void free_irq(unsigned int irq, void *dev_id)
 {
+	struct irq_desc *desc = irq_to_desc(irq);
+
+	if (!desc)
+		return;
+
+	chip_bus_lock(irq, desc);
 	kfree(__free_irq(irq, dev_id));
+	chip_bus_sync_unlock(irq, desc);
 }
 EXPORT_SYMBOL(free_irq);

@@ -955,10 +983,14 @@ int request_threaded_irq(unsigned int ir
 	action->name = devname;
 	action->dev_id = dev_id;
 
+	chip_bus_lock(irq, desc);
+
 	retval = __setup_irq(irq, desc, action);
 	if (retval)
 		kfree(action);
 
+	chip_bus_sync_unlock(irq, desc);
+
 #ifdef CONFIG_DEBUG_SHIRQ
 	if (irqflags & IRQF_SHARED) {
 		/*

Pavel Machek

2009-08-14 10:17 UTC #8 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

In reply to Thomas Gleixner

Hi!

Quoted 25 lines

Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================

include/linux/interrupt.h +6 -0

@@ -113,6 +113,11 @@ request_threaded_irq(unsigned int irq, i
 		     irq_handler_t thread_fn,
 		     unsigned long flags, const char *name, void *dev);
 
+extern int __must_check
+request_threaded_slowbus_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)

@@ -145,6 +150,7 @@ static inline void exit_irq_thread(void)
 #endif
 
 extern void free_irq(unsigned int, void *);
+extern void free_slowbus_irq(unsigned int irq, void *dev_id);
 
 struct device;

include/linux/interrupt.h extern void disable_irq_nosync(unsigned · line 197

Quoted 6 lines

include/linux/interrupt.h +3 -0

@@ -186,6 +192,9 @@ extern void disable_irq_nosync(unsigned 
 extern void disable_irq(unsigned int irq);
 extern void enable_irq(unsigned int irq);
 
+extern void disable_slowbus_irq(unsigned int irq);
+extern void enable_slowbus_irq(unsigned int irq);
+

AFAICT this means that driver would need to know what kind of IRQ it is hooked to, right? That will lead to some ugly code in drivers that can handle both normal and slowbus irqs, right?

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Thomas Gleixner

author

2009-08-14 10:21 UTC #9 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

Quoted 7 lines · Pavel Machek

include/linux/interrupt.h extern void disable_irq_nosync(unsigned · line 197
Quoted 3 lines
include/linux/interrupt.h +3 -0
+extern void disable_slowbus_irq(unsigned int irq);
+extern void enable_slowbus_irq(unsigned int irq);
+
AFAICT this means that driver would need to know what kind of IRQ it is hooked to, right? That will lead to some ugly code in drivers that can handle both normal and slowbus irqs, right?

Are there such drivers in reality ?

Thanks,

tglx

Pavel Machek

2009-08-14 10:30 UTC #10 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

Quoted 10 lines · Thomas Gleixner

Quoted 7 lines · Pavel Machek
include/linux/interrupt.h extern void disable_irq_nosync(unsigned · line 197
Quoted 3 lines
include/linux/interrupt.h +3 -0
+extern void disable_slowbus_irq(unsigned int irq);
+extern void enable_slowbus_irq(unsigned int irq);
+
AFAICT this means that driver would need to know what kind of IRQ it is hooked to, right? That will lead to some ugly code in drivers that can handle both normal and slowbus irqs, right?
Are there such drivers in reality ?

I think so. Between Spitz and Akita, some gpios were moved to slow i2c extender. For stuff like buttons...

Now... it is probably already fixed somehow for Spitz/Akita, but I believe more hardware designers do such things....
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Mark Brown

2009-08-14 11:21 UTC #11 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

In reply to Thomas Gleixner

Quoted 7 lines · Thomas Gleixner

Quoted 3 lines · Pavel Machek
AFAICT this means that driver would need to know what kind of IRQ it is hooked to, right? That will lead to some ugly code in drivers that can handle both normal and slowbus irqs, right?
Are there such drivers in reality ?

Yes. The GPIO based stuff is the prime example but there's other examples - one is the WM831x touchscreen (no driver in mainline yet) which can use interrupts via the main interrupt controller on the CPU but also has the option of bringing the interrupt signals out to dedicated pins on the chip for direct connection to the CPU precisely to avoid the overheads of these slow interrupt controllers.

Ingo Molnar

2009-08-15 09:55 UTC #12 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

Quoted 9 lines · Mark Brown

Quoted 7 lines · Thomas Gleixner
Quoted 3 lines · Pavel Machek
AFAICT this means that driver would need to know what kind of IRQ it is hooked to, right? That will lead to some ugly code in drivers that can handle both normal and slowbus irqs, right?
Are there such drivers in reality ?

Quoted 7 lines

Yes. The GPIO based stuff is the prime example but there's other examples - one is the WM831x touchscreen (no driver in mainline yet) which can use interrupts via the main interrupt controller on the CPU but also has the option of bringing the interrupt signals out to dedicated pins on the chip for direct connection to the CPU precisely to avoid the overheads of these slow interrupt controllers.

This would call for Thomas's first version of the patch, that is transparent to drivers - the IRQ subsystem will know how to lock access to the line.

How about implementing that first patch in a cleaner way - can we somehow express the slow-bus property purely via the irqchip? Or is that too lowlevel?

Ingo

Thomas Gleixner

author

2009-08-15 11:38 UTC #13 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

Quoted 19 lines · Ingo Molnar

Quoted 9 lines · Mark Brown
Quoted 7 lines · Thomas Gleixner
Quoted 3 lines · Pavel Machek
AFAICT this means that driver would need to know what kind of IRQ it is hooked to, right? That will lead to some ugly code in drivers that can handle both normal and slowbus irqs, right?
Are there such drivers in reality ?
Quoted 7 lines
Yes. The GPIO based stuff is the prime example but there's other examples - one is the WM831x touchscreen (no driver in mainline yet) which can use interrupts via the main interrupt controller on the CPU but also has the option of bringing the interrupt signals out to dedicated pins on the chip for direct connection to the CPU precisely to avoid the overheads of these slow interrupt controllers.

Quoted 7 lines

This would call for Thomas's first version of the patch, that is transparent to drivers - the IRQ subsystem will know how to lock access to the line.
How about implementing that first patch in a cleaner way - can we somehow express the slow-bus property purely via the irqchip? Or is that too lowlevel?

The problem here is that the management functions serialize via irqdesc->lock and call the chip level function with the lock held (preemption and interrupts disabled). So we can not access the slow bus chips from these low level functions.

If the low level functions just store the information and schedule it for bus access then there is no serialization anymore. Lets look at disable_irq():

	spin_lock_irqsave(&desc->lock);
	....
	desc->chip->mask();
		schedule bus access;
	...
	spin_lock_irqsave(&desc->lock);

So now we return, but the mask has not reached the chip.

The idea of bus_lock/bus_sync_unlock() was to provide well defined synchronization points for the chip level implementation to do the bus update and have this serialized against other management functions.

	desc->chip->bus_lock();
		Take the chip->bus mutex

	spin_lock_irqsave(&desc->lock);
	....
	desc->chip->mask();
		store mask information
	...
	spin_lock_irqsave(&desc->lock);

	desc->chip->bus_sync_unlock();
		Update the mask via the slow bus
		Release chip->bus mutex

That way we have made sure that the change to the chip actually hits the hardware before we allow further management operations and it simplifies the code for the chip implementation as the bus access can be done in the context of the caller w/o the need of an extra thread/workqueue ...

Thanks,

tglx

Ingo Molnar

2009-08-15 14:29 UTC #14 lore raw

Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses

Quoted 64 lines · Thomas Gleixner

Quoted 19 lines · Ingo Molnar
Quoted 9 lines · Mark Brown
Quoted 7 lines · Thomas Gleixner
Quoted 3 lines · Pavel Machek
AFAICT this means that driver would need to know what kind of IRQ it is hooked to, right? That will lead to some ugly code in drivers that can handle both normal and slowbus irqs, right?
Are there such drivers in reality ?
Quoted 7 lines
Yes. The GPIO based stuff is the prime example but there's other examples - one is the WM831x touchscreen (no driver in mainline yet) which can use interrupts via the main interrupt controller on the CPU but also has the option of bringing the interrupt signals out to dedicated pins on the chip for direct connection to the CPU precisely to avoid the overheads of these slow interrupt controllers.
Quoted 7 lines
This would call for Thomas's first version of the patch, that is transparent to drivers - the IRQ subsystem will know how to lock access to the line.
How about implementing that first patch in a cleaner way - can we somehow express the slow-bus property purely via the irqchip? Or is that too lowlevel?
The problem here is that the management functions serialize via irqdesc->lock and call the chip level function with the lock held (preemption and interrupts disabled). So we can not access the slow bus chips from these low level functions.
If the low level functions just store the information and schedule it for bus access then there is no serialization anymore. Lets look at disable_irq():
	spin_lock_irqsave(&desc->lock);
	....
	desc->chip->mask();
		schedule bus access;
	...
	spin_lock_irqsave(&desc->lock);
So now we return, but the mask has not reached the chip.
The idea of bus_lock/bus_sync_unlock() was to provide well defined synchronization points for the chip level implementation to do the bus update and have this serialized against other management functions.
	desc->chip->bus_lock();
		Take the chip->bus mutex

	spin_lock_irqsave(&desc->lock);
	....
	desc->chip->mask();
		store mask information
	...
	spin_lock_irqsave(&desc->lock);

	desc->chip->bus_sync_unlock();
		Update the mask via the slow bus
		Release chip->bus mutex

Quoted 5 lines

That way we have made sure that the change to the chip actually hits the hardware before we allow further management operations and it simplifies the code for the chip implementation as the bus access can be done in the context of the caller w/o the need of an extra thread/workqueue ...

Given the alternatives i'd prefer this one now ...

Ingo

Thomas Gleixner

author

2009-08-13 19:41 UTC #15 lore raw

RFC PATCH 3/3 genirq: Support nested threaded irq handling

In reply to Thomas Gleixner

Interrupt chips which are behind a slow bus (i2c, spi ...) and demultiplex other interrupt sources need to run their interrupt handler in a thread.

The demultiplexed interrupt handlers need to run in thread context as well and need to finish before the demux handler thread can reenable the interrupt line. So instead of creating separate threads which need to synchronize with the demux handler thread we provide a function which calls the interrupt thread function of the sub device in the context of the demux handler thread.

To avoid that a separate thread is created for the subdevices the function request_nested_slowbus_irq() is provided which sets a dummy handler for the primary handler which should never be called. A flag prevents the creation of the separate handler thread in __setup_irq().

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/interrupt.h |    9 +++++++++
 kernel/irq/chip.c         |   44 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/irq/internals.h    |    2 ++
 kernel/irq/manage.c       |   28 +++++++++++++++++++++++++++-
 4 files changed, 82 insertions(+), 1 deletion(-)

include/linux/interrupt.h +9 -0

Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================
@@ -52,6 +52,9 @@
  * 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_NESTED -  Interrupt is nested into another threaded interrupt. Handler
+ *		  is called in the parent interrupt thread context
+ *
  */
 #define IRQF_DISABLED		0x00000020
 #define IRQF_SAMPLE_RANDOM	0x00000040

@@ -62,6 +65,7 @@
 #define IRQF_NOBALANCING	0x00000800
 #define IRQF_IRQPOLL		0x00001000
 #define IRQF_ONESHOT		0x00002000
+#define IRQF_NESTED		0x00004000
 
 /*
  * Bits used by threaded handlers:

@@ -125,6 +129,11 @@ request_irq(unsigned int irq, irq_handle
 	return request_threaded_irq(irq, handler, NULL, flags, name, dev);
 }
 
+extern int __must_check
+request_nested_slowbus_irq(unsigned int irq, irq_handler_t thread_fn,
+			   unsigned long irqflags, const char *devname,
+			   void *dev_id);
+
 extern void exit_irq_thread(void);
 #else

kernel/irq/chip.c +44 -0

50 lines not on this page · raw message

kernel/irq/internals.h +2 -0

Index: linux-2.6-tip/kernel/irq/internals.h
===================================================================
@@ -45,6 +45,8 @@ extern int irq_select_affinity_usr(unsig
 extern void
 irq_set_thread_affinity(struct irq_desc *desc, const struct cpumask *cpumask);
 
+extern irqreturn_t handle_nested_irq_primary(int irq, void *dev_id);
+
 /*
  * Debugging printout:
  */

kernel/irq/manage.c +27 -1

Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
@@ -625,7 +625,7 @@ __setup_irq(unsigned int irq, struct irq
 	/*
 	 * Threaded handler ?
 	 */
-	if (new->thread_fn) {
+	if (new->thread_fn && !(new->flags & IRQF_NESTED)) {
 		struct task_struct *t;
 
 		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,

@@ -1095,3 +1095,29 @@ int request_slowbus_threaded_irq(unsigne
 	return res;
 }
 EXPORT_SYMBOL(request_slowbus_threaded_irq);
+
+/**
+ *	request_nested_slowbus_irq - allocate an interrupt line for a nested irq
+ *	@irq: Interrupt line to allocate
+ *	@thread_fn: Function called from the irq handler thread
+ *	@irqflags: Interrupt type flags
+ *	@devname: An ascii name for the claiming device
+ *	@dev_id: A cookie passed back to the handler function
+ *
+ *	This function is used for interrupts which nest into another
+ *	threaded interrupt handler, e.g. subdevice interrupts of an
+ *	interrupt controller which is connected via i2c/spi or other
+ *	slow path mechanisms. The thread_fn is called from the
+ *	demultiplexing interrupt thread via handle_nested_irq().
+ */
+int request_nested_slowbus_irq(unsigned int irq, irq_handler_t thread_fn,
+			       unsigned long irqflags, const char *devname,
+			       void *dev_id)
+{
+	irqflags |= IRQF_NESTED;
+
+	return request_threaded_slowbus_irq(irq, handle_nested_irq_primary,
+					    thread_fn, irqflags, devname,
+					    dev_id);
+}
+EXPORT_SYMBOL(request_nested_slowbus_irq);