[PATCH 0/8] ipi and irq cleanups and fixes

[PATCH 0/8] ipi and irq cleanups and fixes

[Milton Miller]

Here are a few cleanups and fies mostly around the previous series
and a few more as I continue to explore exporting the irq_host concept
for other architectures.

milton

[PATCH 6/8] powerpc 8xx: cascade eoi will be performed by generic_handle_irq handler

[Milton Miller]

The 8xx cpm_cascade was calling irq_eoi for the cascaded irq,
but that will already have been called by the handle_fasteoi_irq
that generic_handle_irq will call.  The handler is set in
arch/powerpc/sysdev/cpm1.c by the host map routine.

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: work.git/arch/powerpc/platforms/8xx/m8xx_setup.c
===================================================================
--- work.git.orig/arch/powerpc/platforms/8xx/m8xx_setup.c	2011-05-18 22:50:38.983498572 -0500
+++ work.git/arch/powerpc/platforms/8xx/m8xx_setup.c	2011-05-18 22:52:48.920532258 -0500
@@ -221,15 +221,9 @@ static void cpm_cascade(unsigned int irq
 	struct irq_chip *chip;
 	int cascade_irq;
 
-	if ((cascade_irq = cpm_get_irq()) >= 0) {
-		struct irq_desc *cdesc = irq_to_desc(cascade_irq);
-
+	if ((cascade_irq = cpm_get_irq()) >= 0)
 		generic_handle_irq(cascade_irq);
 
-		chip = irq_desc_get_chip(cdesc);
-		chip->irq_eoi(&cdesc->irq_data);
-	}
-
 	chip = irq_desc_get_chip(desc);
 	chip->irq_eoi(&desc->irq_data);
 }

[PATCH 4/8] powerpc irq: always free duplicate IRQ_LEGACY hosts

[Milton Miller]

Since kmem caches are allocated before init_IRQ as noted in 3af259d155
(powerpc: Radix trees are available before init_IRQ), we now call
kmalloc in all cases and can can always call kfree if we are asked
to allocate a duplicate or conflicting IRQ_HOST_MAP_LEGACY host.

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: work.git/arch/powerpc/kernel/irq.c
===================================================================
--- work.git.orig/arch/powerpc/kernel/irq.c	2011-05-19 05:51:25.715500763 -0500
+++ work.git/arch/powerpc/kernel/irq.c	2011-05-19 05:57:30.776503350 -0500
@@ -557,15 +557,8 @@ struct irq_host *irq_alloc_host(struct d
 	if (revmap_type == IRQ_HOST_MAP_LEGACY) {
 		if (irq_map[0].host != NULL) {
 			raw_spin_unlock_irqrestore(&irq_big_lock, flags);
-			/* If we are early boot, we can't free the structure,
-			 * too bad...
-			 * this will be fixed once slab is made available early
-			 * instead of the current cruft
-			 */
-			if (mem_init_done) {
-				of_node_put(host->of_node);
-				kfree(host);
-			}
+			of_node_put(host->of_node);
+			kfree(host);
 			return NULL;
 		}
 		irq_map[0].host = host;

[PATCH 8/8] powerpc: fix irq_free_virt by adjusting bounds before loop

[Milton Miller]

Instead of looping over each irq and checking against the irq array
bounds, adjust the bounds before looping.

The old code will not free any irq if the irq + count is above
irq_virq_count because the test in the loop is testing irq + count
instead of irq + i.

This code checks the limits to avoid unsigned integer overflows.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
I am not aware of any call sites where the difference matters; I
prepared the refactor while continuing work on the irq host extraction
and noticed the logic error during the code movement.

Index: work.git/arch/powerpc/kernel/irq.c
===================================================================
--- work.git.orig/arch/powerpc/kernel/irq.c	2011-05-24 21:15:55.350096024 -0500
+++ work.git/arch/powerpc/kernel/irq.c	2011-05-24 21:17:16.000097884 -0500
@@ -1007,14 +1007,23 @@ void irq_free_virt(unsigned int virq, un
 	WARN_ON (virq < NUM_ISA_INTERRUPTS);
 	WARN_ON (count == 0 || (virq + count) > irq_virq_count);
 
+	if (virq < NUM_ISA_INTERRUPTS) {
+		if (virq + count < NUM_ISA_INTERRUPTS)
+			return;
+		count  =- NUM_ISA_INTERRUPTS - virq;
+		virq = NUM_ISA_INTERRUPTS;
+	}
+
+	if (count > irq_virq_count || virq > irq_virq_count - count) {
+		if (virq > irq_virq_count)
+			return;
+		count = irq_virq_count - virq;
+	}
+
 	raw_spin_lock_irqsave(&irq_big_lock, flags);
 	for (i = virq; i < (virq + count); i++) {
 		struct irq_host *host;
 
-		if (i < NUM_ISA_INTERRUPTS ||
-		    (virq + count) > irq_virq_count)
-			continue;
-
 		host = irq_map[i].host;
 		irq_map[i].hwirq = host->inval_irq;
 		smp_wmb();

[PATCH 3/8] powerpc irq: remove stale and misleading comment

[Milton Miller]

The comment claims we will call host->ops->map() to update the flags if
we find a previously established mapping, but we never did.  We used
to call remap, but that call was removed in da05198002 (powerpc: Remove
irq_host_ops->remap hook).

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: work.git/arch/powerpc/kernel/irq.c
===================================================================
--- work.git.orig/arch/powerpc/kernel/irq.c	2011-05-24 20:43:46.350096135 -0500
+++ work.git/arch/powerpc/kernel/irq.c	2011-05-24 21:03:49.520096058 -0500
@@ -727,9 +727,7 @@ unsigned int irq_create_mapping(struct i
 	}
 	pr_debug("irq: -> using host @%p\n", host);
 
-	/* Check if mapping already exist, if it does, call
-	 * host->ops->map() to update the flags
-	 */
+	/* Check if mapping already exists */
 	virq = irq_find_mapping(host, hwirq);
 	if (virq != NO_IRQ) {
 		pr_debug("irq: -> existing mapping on virq %d\n", virq);

[PATCH 5/8] powerpc: check desc in handle_one_irq and expand generic_handle_irq

[Milton Miller]

Look up the descriptor and check that it is found in handle_one_irq
before checking if we are on the irq stack, and call the handler
directly using the descriptor if we are on the stack.

We need check irq_to_desc finds the descriptor to avoid a NULL
pointer dereference.  It could have failed because the number from
ppc_md.get_irq was above NR_IRQS, or various exceptional conditions
with sparse irqs (eg race conditions while freeing an irq if its was
not shutdown in the controller).

fe12bc2c99 (genirq: Uninline and sanity check generic_handle_irq())
moved generic_handle_irq out of line to allow its use by interrupt
controllers in modules.  However, handle_one_irq is core arch code.
It already knows the details of struct irq_desc and handling irqs in
the nested irq case.  This will avoid the extra stack frame to return
the value we don't check.

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: work.git/arch/powerpc/kernel/irq.c
===================================================================
--- work.git.orig/arch/powerpc/kernel/irq.c	2011-05-21 01:06:49.042239939 -0500
+++ work.git/arch/powerpc/kernel/irq.c	2011-05-21 02:00:41.912586798 -0500
@@ -295,17 +295,20 @@ static inline void handle_one_irq(unsign
 	unsigned long saved_sp_limit;
 	struct irq_desc *desc;
 
+	desc = irq_to_desc(irq);
+	if (!desc)
+		return;
+
 	/* Switch to the irq stack to handle this */
 	curtp = current_thread_info();
 	irqtp = hardirq_ctx[smp_processor_id()];
 
 	if (curtp == irqtp) {
 		/* We're already on the irq stack, just handle it */
-		generic_handle_irq(irq);
+		desc->handle_irq(irq, desc);
 		return;
 	}
 
-	desc = irq_to_desc(irq);
 	saved_sp_limit = current->thread.ksp_limit;
 
 	irqtp->task = curtp->task;

[PATCH 2/8] powerpc cell: rename ipi functions to match current abstractions

[Milton Miller]

Rename functions and arguments to reflect current usage.  iic_cause_ipi
becomes iic_message_pass and iic_ipi_to_irq becomes iic_msg_to_irq,
and iic_request_ipi now takes a message (msg) instead of an ipi number.
Also mesg is renamed to msg.

Commit f1072939b6 (powerpc: Remove checks for MSG_ALL and
MSG_ALL_BUT_SELF) connected the smp_message_pass hook for cell to the
underlying iic_cause_IPI, a platform unique name.  Later 23d72bfd8f
(powerpc: Consolidate ipi message mux and demux) added a cause_ipi
hook to the smp_ops, also used in message passing, but for controllers
that can not send 4 unique messages and require multiplexing.  It is
even more confusing that the both take two arguments, but one is the
small message ordinal and the other is an opaque long data associated
with the cpu.

Since cell iic maps messages one to one to ipi irqs, rename the
function and argument to translate from ipi to message.  Also make it
clear that iic_request_ipi takes a message number as the argument
for which ipi to create and request.

No functionional change, just renames to avoid future confusion.

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: work.git/arch/powerpc/platforms/cell/interrupt.c
===================================================================
--- work.git.orig/arch/powerpc/platforms/cell/interrupt.c	2011-05-24 20:54:05.150096188 -0500
+++ work.git/arch/powerpc/platforms/cell/interrupt.c	2011-05-24 21:03:30.350097228 -0500
@@ -176,14 +176,14 @@ EXPORT_SYMBOL_GPL(iic_get_target_id);
 #ifdef CONFIG_SMP
 
 /* Use the highest interrupt priorities for IPI */
-static inline int iic_ipi_to_irq(int ipi)
+static inline int iic_msg_to_irq(int msg)
 {
-	return IIC_IRQ_TYPE_IPI + 0xf - ipi;
+	return IIC_IRQ_TYPE_IPI + 0xf - msg;
 }
 
-void iic_cause_IPI(int cpu, int mesg)
+void iic_message_pass(int cpu, int msg)
 {
-	out_be64(&per_cpu(cpu_iic, cpu).regs->generate, (0xf - mesg) << 4);
+	out_be64(&per_cpu(cpu_iic, cpu).regs->generate, (0xf - msg) << 4);
 }
 
 struct irq_host *iic_get_irq_host(int node)
@@ -192,14 +192,14 @@ struct irq_host *iic_get_irq_host(int no
 }
 EXPORT_SYMBOL_GPL(iic_get_irq_host);
 
-static void iic_request_ipi(int ipi)
+static void iic_request_ipi(int msg)
 {
 	int virq;
 
-	virq = irq_create_mapping(iic_host, iic_ipi_to_irq(ipi));
+	virq = irq_create_mapping(iic_host, iic_msg_to_irq(msg));
 	if (virq == NO_IRQ) {
 		printk(KERN_ERR
-		       "iic: failed to map IPI %s\n", smp_ipi_name[ipi]);
+		       "iic: failed to map IPI %s\n", smp_ipi_name[msg]);
 		return;
 	}
 
@@ -207,7 +207,7 @@ static void iic_request_ipi(int ipi)
 	 * If smp_request_message_ipi encounters an error it will notify
 	 * the error.  If a message is not needed it will return non-zero.
 	 */
-	if (smp_request_message_ipi(virq, ipi))
+	if (smp_request_message_ipi(virq, msg))
 		irq_dispose_mapping(virq);
 }
 
Index: work.git/arch/powerpc/platforms/cell/interrupt.h
===================================================================
--- work.git.orig/arch/powerpc/platforms/cell/interrupt.h	2011-05-24 20:47:00.580096118 -0500
+++ work.git/arch/powerpc/platforms/cell/interrupt.h	2011-05-24 20:59:28.580096158 -0500
@@ -75,7 +75,7 @@ enum {
 };
 
 extern void iic_init_IRQ(void);
-extern void iic_cause_IPI(int cpu, int mesg);
+extern void iic_message_pass(int cpu, int msg);
 extern void iic_request_IPIs(void);
 extern void iic_setup_cpu(void);
 
Index: work.git/arch/powerpc/platforms/cell/smp.c
===================================================================
--- work.git.orig/arch/powerpc/platforms/cell/smp.c	2011-05-24 20:47:00.570096092 -0500
+++ work.git/arch/powerpc/platforms/cell/smp.c	2011-05-24 20:56:53.350096175 -0500
@@ -152,7 +152,7 @@ static int smp_cell_cpu_bootable(unsigne
 	return 1;
 }
 static struct smp_ops_t bpa_iic_smp_ops = {
-	.message_pass	= iic_cause_IPI,
+	.message_pass	= iic_message_pass,
 	.probe		= smp_iic_probe,
 	.kick_cpu	= smp_cell_kick_cpu,
 	.setup_cpu	= smp_cell_setup_cpu,

[PATCH 7/8] powerpc irq: protect irq_radix_revmap_lookup against irq_free_virt

[Milton Miller]

The radix-tree code uses call_rcu when freeing internal elements.
We must protect against the elements being freed while we traverse
the tree, even if the returned pointer will still be valid.

While preparing a patch to expand the context in which
irq_radix_revmap_lookup will be called, I realized that the
radix tree was not locked.

When asked

    For a normal call_rcu usage, is it allowed to read the structure in
    irq_enter / irq_exit, without additional rcu_read_lock?  Could an
    element freed with call_rcu advance with the cpu still between
    irq_enter/irq_exit (and irq_disabled())?

Paul McKenney replied:

    Absolutely illegal to do so. OK for call_rcu_sched(), but a
    flaming bug for call_rcu().

    And thank you very much for finding this!!!

Further analysis:

In the current CONFIG_TREE_RCU implementation. CONFIG_TREE_PREEMPT_RCU
(and CONFIG_TINY_PREEMPT_RCU) uses explicit counters.

These counters are reflected from per-CPU to global in the
scheduling-clock-interrupt handler, so disabling irq does prevent the
grace period from completing. But there are real-time implementations
(such as the one use by the Concurrent guys) where disabling irq
does -not- prevent the grace period from completing.


While an alternative fix would be to switch radix-tree to rcu_sched, I
don't want to audit the other users of radix trees (nor put alternative
freeing in the library).  The normal overhead for rcu_read_lock and
unlock are a local counter increment and decrement.

This does not show up in the rcu lockdep because in 2.6.34 commit
2676a58c98 (radix-tree: Disable RCU lockdep checking in radix tree)
deemed it too hard to pass the condition of the protecting lock
to the library.

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: work.git/arch/powerpc/kernel/irq.c
===================================================================
--- work.git.orig/arch/powerpc/kernel/irq.c	2011-05-24 21:14:30.860096118 -0500
+++ work.git/arch/powerpc/kernel/irq.c	2011-05-24 21:15:55.350096024 -0500
@@ -893,10 +893,13 @@ unsigned int irq_radix_revmap_lookup(str
 		return irq_find_mapping(host, hwirq);
 
 	/*
-	 * No rcu_read_lock(ing) needed, the ptr returned can't go under us
-	 * as it's referencing an entry in the static irq_map table.
+	 * The ptr returned references the static global irq_map.
+	 * but freeing an irq can delete nodes along the path to
+	 * do the lookup via call_rcu.
 	 */
+	rcu_read_lock();
 	ptr = radix_tree_lookup(&host->revmap_data.tree, hwirq);
+	rcu_read_unlock();
 
 	/*
 	 * If found in radix tree, then fine.

Re: [PATCH 7/8] powerpc irq: protect irq_radix_revmap_lookup against irq_free_virt

[Paul E. McKenney]

On Wed, May 25, 2011 at 01:34:18AM -0500, Milton Miller wrote:
> The radix-tree code uses call_rcu when freeing internal elements.
> We must protect against the elements being freed while we traverse
> the tree, even if the returned pointer will still be valid.
> 
> While preparing a patch to expand the context in which
> irq_radix_revmap_lookup will be called, I realized that the
> radix tree was not locked.
> 
> When asked
> 
>     For a normal call_rcu usage, is it allowed to read the structure in
>     irq_enter / irq_exit, without additional rcu_read_lock?  Could an
>     element freed with call_rcu advance with the cpu still between
>     irq_enter/irq_exit (and irq_disabled())?
> 
> Paul McKenney replied:
> 
>     Absolutely illegal to do so. OK for call_rcu_sched(), but a
>     flaming bug for call_rcu().
> 
>     And thank you very much for finding this!!!
> 
> Further analysis:
> 
> In the current CONFIG_TREE_RCU implementation. CONFIG_TREE_PREEMPT_RCU
> (and CONFIG_TINY_PREEMPT_RCU) uses explicit counters.
> 
> These counters are reflected from per-CPU to global in the
> scheduling-clock-interrupt handler, so disabling irq does prevent the
> grace period from completing. But there are real-time implementations
> (such as the one use by the Concurrent guys) where disabling irq
> does -not- prevent the grace period from completing.
> 
> 
> While an alternative fix would be to switch radix-tree to rcu_sched, I
> don't want to audit the other users of radix trees (nor put alternative
> freeing in the library).  The normal overhead for rcu_read_lock and
> unlock are a local counter increment and decrement.
> 
> This does not show up in the rcu lockdep because in 2.6.34 commit
> 2676a58c98 (radix-tree: Disable RCU lockdep checking in radix tree)
> deemed it too hard to pass the condition of the protecting lock
> to the library.

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> Signed-off-by: Milton Miller <miltonm@bga.com>
> 
> Index: work.git/arch/powerpc/kernel/irq.c
> ===================================================================
> --- work.git.orig/arch/powerpc/kernel/irq.c	2011-05-24 21:14:30.860096118 -0500
> +++ work.git/arch/powerpc/kernel/irq.c	2011-05-24 21:15:55.350096024 -0500
> @@ -893,10 +893,13 @@ unsigned int irq_radix_revmap_lookup(str
>  		return irq_find_mapping(host, hwirq);
> 
>  	/*
> -	 * No rcu_read_lock(ing) needed, the ptr returned can't go under us
> -	 * as it's referencing an entry in the static irq_map table.
> +	 * The ptr returned references the static global irq_map.
> +	 * but freeing an irq can delete nodes along the path to
> +	 * do the lookup via call_rcu.
>  	 */
> +	rcu_read_lock();
>  	ptr = radix_tree_lookup(&host->revmap_data.tree, hwirq);
> +	rcu_read_unlock();
> 
>  	/*
>  	 * If found in radix tree, then fine.

Re: [PATCH 6/8] powerpc 8xx: cascade eoi will be performed by generic_handle_irq handler

[Benjamin Herrenschmidt]

On Wed, 2011-05-25 at 01:34 -0500, Milton Miller wrote:
> The 8xx cpm_cascade was calling irq_eoi for the cascaded irq,
> but that will already have been called by the handle_fasteoi_irq
> that generic_handle_irq will call.  The handler is set in
> arch/powerpc/sysdev/cpm1.c by the host map routine.

No it won't unless I'm missing something. The flow handler
(handle_fasteoi_irq) is going to be replaced by the chained handler when
mpc8xx_pics_init() calls irq_set_chained_handle(irq, cpm_cascade) no ?

Cheers,
Ben.

> Signed-off-by: Milton Miller <miltonm@bga.com>
> 
> Index: work.git/arch/powerpc/platforms/8xx/m8xx_setup.c
> ===================================================================
> --- work.git.orig/arch/powerpc/platforms/8xx/m8xx_setup.c	2011-05-18 22:50:38.983498572 -0500
> +++ work.git/arch/powerpc/platforms/8xx/m8xx_setup.c	2011-05-18 22:52:48.920532258 -0500
> @@ -221,15 +221,9 @@ static void cpm_cascade(unsigned int irq
>  	struct irq_chip *chip;
>  	int cascade_irq;
>  
> -	if ((cascade_irq = cpm_get_irq()) >= 0) {
> -		struct irq_desc *cdesc = irq_to_desc(cascade_irq);
> -
> +	if ((cascade_irq = cpm_get_irq()) >= 0)
>  		generic_handle_irq(cascade_irq);
>  
> -		chip = irq_desc_get_chip(cdesc);
> -		chip->irq_eoi(&cdesc->irq_data);
> -	}
> -
>  	chip = irq_desc_get_chip(desc);
>  	chip->irq_eoi(&desc->irq_data);
>  }

Re: [PATCH 6/8] powerpc 8xx: cascade eoi will be performed by generic_handle_irq handler

[Milton Miller]

On Thu, 26 May 2011 about 13:32:33 +1000, Benjamin Herrenschmidt wrote:
> > On Wed, 2011-05-25 at 01:34 -0500, Milton Miller wrote:
> > > The 8xx cpm_cascade was calling irq_eoi for the cascaded irq,
> > > but that will already have been called by the handle_fasteoi_irq
> > > that generic_handle_irq will call.  The handler is set in
> > > arch/powerpc/sysdev/cpm1.c by the host map routine.
> > 
> > No it won't unless I'm missing something. The flow handler
> > (handle_fasteoi_irq) is going to be replaced by the chained handler when
> > mpc8xx_pics_init() calls irq_set_chained_handle(irq, cpm_cascade) no ?
> > 
> > Cheers,
> > Ben.

No.   We set the chained handler on the top level irq (on the irq
to the primary irq controller).  The handler is set to this 8xx_cascade
function.  We don't change the handler on the subordnate irq that
is invoked via generic_handle_irq in this function.

Or that is how I understand things to work, and this makes the 8xx
code match the current 8xxx cpm1 cascade handler.

Of course, it would be good for someone with hardware to confirm
that it works with the patch (or even put a printk or counter in the
handler if that would not cause printk recursion -- don't do it on
your console).

milton

> > 
> > > Signed-off-by: Milton Miller <miltonm@bga.com>
> > > 
> > > Index: work.git/arch/powerpc/platforms/8xx/m8xx_setup.c
> > > ===================================================================
> > > --- work.git.orig/arch/powerpc/platforms/8xx/m8xx_setup.c	2011-05-18 22:50:38.983498572 -0500
> > > +++ work.git/arch/powerpc/platforms/8xx/m8xx_setup.c	2011-05-18 22:52:48.920532258 -0500
> > > @@ -221,15 +221,9 @@ static void cpm_cascade(unsigned int irq
> > >  	struct irq_chip *chip;
> > >  	int cascade_irq;
> > >  
> > > -	if ((cascade_irq = cpm_get_irq()) >= 0) {
> > > -		struct irq_desc *cdesc = irq_to_desc(cascade_irq);
> > > -
> > > +	if ((cascade_irq = cpm_get_irq()) >= 0)
> > >  		generic_handle_irq(cascade_irq);
> > >  
> > > -		chip = irq_desc_get_chip(cdesc);
> > > -		chip->irq_eoi(&cdesc->irq_data);
> > > -	}
> > > -
> > >  	chip = irq_desc_get_chip(desc);
> > >  	chip->irq_eoi(&desc->irq_data);
> > >  }
> > 
> >