genirq: Add a set_irq_handler_locked() function

genirq: Add a set_irq_handler_locked() function

[David Gibson]

Thomas, as discussed previously over IRC and private email.  Please
apply.

At present set_irq_handler() and all the existing variants take the
desc->lock for the irq in question before adjusting the irq's flow
handler.  This can cause problems for irq chips for which a given
interrupt can be either level or edge depending on what's attached.
The chip->set_type() callback, which is used to switch the interrupt
type in this case may need to alter the flow handler - but it is
called with the desc->lock already held, so it cannot safely call
set_irq_handler().

This patch pulls the core logic out of set_irq_handler() into a new
set_irq_handler_locked() which assumes the desc->lock is already held.
It can safely be used from the chip->set_type() callback.

Signed-off-by: David Gibson <dwg@au1.ibm.com>

Index: working-2.6/kernel/irq/chip.c
===================================================================
--- working-2.6.orig/kernel/irq/chip.c	2007-01-24 12:01:21.000000000 +1100
+++ working-2.6/kernel/irq/chip.c	2007-02-09 14:28:55.000000000 +1100
@@ -500,6 +500,43 @@ handle_percpu_irq(unsigned int irq, stru
 
 #endif /* CONFIG_SMP */
 
+/**
+ *	set_irq_handler_locked - Set irq flow handler. *
+ *	@irq:		the interrupt number
+ *	@handle:	the flow handler function
+ *	@is_chained:	is this a chained handler (used for cascade interrupts)
+ *	@name:		name for this handler, usually "level" or "edge"
+ *
+ *	This version of set_irq_handler() assumes that desc->lock for
+ *	the interrupt in question is already taken.  In particular,
+ *	this means it is safe to call from a chip->set_type() callback
+ *	function.
+ */
+void set_irq_handler_locked(unsigned int irq, irq_flow_handler_t handle,
+			    int is_chained, const char *name)
+{
+	struct irq_desc *desc = irq_desc + irq;
+
+	/* Uninstall? */
+	if (handle == handle_bad_irq) {
+		if (desc->chip != &no_irq_chip) {
+			desc->chip->mask(irq);
+			desc->chip->ack(irq);
+		}
+		desc->status |= IRQ_DISABLED;
+		desc->depth = 1;
+	}
+	desc->handle_irq = handle;
+	desc->name = name;
+
+	if (handle != handle_bad_irq && is_chained) {
+		desc->status &= ~IRQ_DISABLED;
+		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
+		desc->depth = 0;
+		desc->chip->unmask(irq);
+	}
+}
+
 void
 __set_irq_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
 		  const char *name)
@@ -531,25 +568,7 @@ __set_irq_handler(unsigned int irq, irq_
 	}
 
 	spin_lock_irqsave(&desc->lock, flags);
-
-	/* Uninstall? */
-	if (handle == handle_bad_irq) {
-		if (desc->chip != &no_irq_chip) {
-			desc->chip->mask(irq);
-			desc->chip->ack(irq);
-		}
-		desc->status |= IRQ_DISABLED;
-		desc->depth = 1;
-	}
-	desc->handle_irq = handle;
-	desc->name = name;
-
-	if (handle != handle_bad_irq && is_chained) {
-		desc->status &= ~IRQ_DISABLED;
-		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
-		desc->depth = 0;
-		desc->chip->unmask(irq);
-	}
+	set_irq_handler_locked(irq, handle, is_chained, name);
 	spin_unlock_irqrestore(&desc->lock, flags);
 }
 
Index: working-2.6/include/linux/irq.h
===================================================================
--- working-2.6.orig/include/linux/irq.h	2007-01-24 12:01:21.000000000 +1100
+++ working-2.6/include/linux/irq.h	2007-02-09 14:04:12.000000000 +1100
@@ -329,6 +329,9 @@ set_irq_chip_and_handler_name(unsigned i
 			      irq_flow_handler_t handle, const char *name);
 
 extern void
+set_irq_handler_locked(unsigned int irq, irq_flow_handler_t handle,
+		       int is_chained, const char *name);
+extern void
 __set_irq_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
 		  const char *name);
 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: genirq: Add a set_irq_handler_locked() function

[Russell King]

On Fri, Feb 09, 2007 at 02:48:42PM +1100, David Gibson wrote:
> At present set_irq_handler() and all the existing variants take the
> desc->lock for the irq in question before adjusting the irq's flow
> handler.  This can cause problems for irq chips for which a given
> interrupt can be either level or edge depending on what's attached.

Are you sure you need to change the flow handler depending on how
you program the device?

Since the outset of this design, I've had what are essentially edge
based interrupt sources using the "level" handlers because they haven't
had a "broken" edge implementation.  By that, I mean that the masking
is done in such a way that you miss edges when the source is masked.

If you do not miss edges while the source is masked, there's no point
in having the complexity of the "edge" based handler in the path - it
buys you nothing.  Just use the "level" handler instead.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: genirq: Add a set_irq_handler_locked() function

[David Gibson]

On Fri, Feb 09, 2007 at 07:36:40AM +0000, Russell King wrote:
> On Fri, Feb 09, 2007 at 02:48:42PM +1100, David Gibson wrote:
> > At present set_irq_handler() and all the existing variants take the
> > desc->lock for the irq in question before adjusting the irq's flow
> > handler.  This can cause problems for irq chips for which a given
> > interrupt can be either level or edge depending on what's attached.
> 
> Are you sure you need to change the flow handler depending on how
> you program the device?
> 
> Since the outset of this design, I've had what are essentially edge
> based interrupt sources using the "level" handlers because they haven't
> had a "broken" edge implementation.  By that, I mean that the masking
> is done in such a way that you miss edges when the source is masked.
> 
> If you do not miss edges while the source is masked, there's no point
> in having the complexity of the "edge" based handler in the path - it
> buys you nothing.  Just use the "level" handler instead.

I see... how terribly obvious.

As far as I know, the 4xx UIC does things correctly, though I don't
have handy any devices with edge interrupts to test it with.

It would still be nice to have this change, so we can use the
lazy-masking from handle_edge_irq(), but I guess I can do without it
for now.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: genirq: Add a set_irq_handler_locked() function

[Russell King]

On Mon, Feb 12, 2007 at 02:17:28PM +1100, David Gibson wrote:
> On Fri, Feb 09, 2007 at 07:36:40AM +0000, Russell King wrote:
> > On Fri, Feb 09, 2007 at 02:48:42PM +1100, David Gibson wrote:
> > > At present set_irq_handler() and all the existing variants take the
> > > desc->lock for the irq in question before adjusting the irq's flow
> > > handler.  This can cause problems for irq chips for which a given
> > > interrupt can be either level or edge depending on what's attached.
> > 
> > Are you sure you need to change the flow handler depending on how
> > you program the device?
> > 
> > Since the outset of this design, I've had what are essentially edge
> > based interrupt sources using the "level" handlers because they haven't
> > had a "broken" edge implementation.  By that, I mean that the masking
> > is done in such a way that you miss edges when the source is masked.
> > 
> > If you do not miss edges while the source is masked, there's no point
> > in having the complexity of the "edge" based handler in the path - it
> > buys you nothing.  Just use the "level" handler instead.
> 
> I see... how terribly obvious.

Do you have a better way of naming the functions?

> As far as I know, the 4xx UIC does things correctly, though I don't
> have handy any devices with edge interrupts to test it with.
> 
> It would still be nice to have this change, so we can use the
> lazy-masking from handle_edge_irq(), but I guess I can do without it
> for now.

If you have correct behaviour from your interrupt controller for edge
based interrupts, you don't need the lazy masking, so why make things
complicated in this way?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of: