ipi and irq cleanups and fixes
Subsystems (3)
cover ipi and irq cleanups and fixes
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 2/8 powerpc cell: rename ipi functions to match current abstractions
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.
arch/powerpc/platforms/cell/interrupt.c +8 -8
@@ -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); }
arch/powerpc/platforms/cell/interrupt.h +1 -1
@@ -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 );
arch/powerpc/platforms/cell/smp.c +1 -1
@@ -152,7 +152,7 @@ static int smp_cell_cpu_bootable(unsignereturn 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 3/8 powerpc irq: remove stale and misleading comment
In reply to Milton MillerThe 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).
arch/powerpc/kernel/irq.c +1 -3
@@ -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 4/8 powerpc irq: always free duplicate IRQ_LEGACY hosts
In reply to Milton MillerSince 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.
arch/powerpc/kernel/irq.c +2 -9
@@ -557,15 +557,8 @@ struct irq_host *irq_alloc_host(struct dif (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 5/8 powerpc: check desc in handle_one_irq and expand generic_handle_irq
In reply to Milton MillerLook 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.
arch/powerpc/kernel/irq.c +5 -2
@@ -295,17 +295,20 @@ static inline void handle_one_irq(unsignunsigned 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 6/8 powerpc 8xx: cascade eoi will be performed by generic_handle_irq handler
In reply to Milton MillerThe 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.
arch/powerpc/platforms/8xx/m8xx_setup.c +1 -7
@@ -221,15 +221,9 @@ static void cpm_cascade(unsigned int irqstruct 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
Quoted 4 lines · Milton Miller
The 8xx
cpm_cascadewas callingirq_eoifor the cascaded irq, but that will already have been called by thehandle_fasteoi_irqthatgeneric_handle_irqwill call. The handler is set inarch/powerpc/sysdev/cpm1.cby 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.
Quoted 23 lines
Signed-off-by Milton Miller <[email protected]>Index:
work.git/arch/powerpc/platforms/8xx/m8xx_setup.c
===================================================================arch/powerpc/platforms/8xx/m8xx_setup.c +1
-7@@ -221,15 +221,9 @@ static void cpm_cascade(unsigned int irqstruct 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
Quoted 6 lines · Benjamin Herrenschmidt
Quoted 6 lines · Milton Miller
Quoted 4 lines
The 8xx
cpm_cascadewas callingirq_eoifor the cascaded irq, but that will already have been called by thehandle_fasteoi_irqthatgeneric_handle_irqwill call. The handler is set inarch/powerpc/sysdev/cpm1.cby the host map routine.
Quoted 6 lines
Quoted 6 lines
No it won't unless I'm missing something. The flow handler (
handle_fasteoi_irq) is going to be replaced by the chained handler whenmpc8xx_pics_init()callsirq_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
Quoted 26 lines
Quoted 26 lines
Quoted 24 lines
Signed-off-by Milton Miller <[email protected]>Index:
work.git/arch/powerpc/platforms/8xx/m8xx_setup.c
===================================================================arch/powerpc/platforms/8xx/m8xx_setup.c +1
-7@@ -221,15 +221,9 @@ static void cpm_cascade(unsigned int irqstruct 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 7/8 powerpc irq: protect irq_radix_revmap_lookup against irq_free_virt
In reply to Milton MillerThe 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.
arch/powerpc/kernel/irq.c +5 -2
@@ -893,10 +893,13 @@ unsigned int irq_radix_revmap_lookup(strreturn 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
Quoted 39 lines · Milton Miller
The radix-tree code uses
call_rcuwhen 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_lookupwill be called, I realized that the radix tree was not locked.When asked
For a normal
call_rcuusage, is it allowed to read the structure inirq_enter/irq_exit, without additionalrcu_read_lock? Could an element freed withcall_rcuadvance with the cpu still between irq_enter/irq_exit (andirq_disabled())?Paul McKenney replied:
Absolutely illegal to do so. OK for
call_rcu_sched(), but a flaming bug forcall_rcu().And thank you very much for finding this!!!
Further analysis:
In the current
CONFIG_TREE_RCUimplementation.CONFIG_TREE_PREEMPT_RCU(andCONFIG_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 forrcu_read_lockand unlock are a local counter increment and decrement.
Quoted 4 lines
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.
Quoted 22 lines
Signed-off-by Milton Miller <[email protected]>Index:
work.git/arch/powerpc/kernel/irq.c
===================================================================arch/powerpc/kernel/irq.c +5
-2@@ -893,10 +893,13 @@ unsigned int irq_radix_revmap_lookup(strreturn 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.
PATCH 8/8 powerpc: fix irq_free_virt by adjusting bounds before loop
In reply to Milton MillerInstead 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.
---
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.
arch/powerpc/kernel/irq.c +13 -4
@@ -1007,14 +1007,23 @@ void irq_free_virt(unsigned int virq, unWARN_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 ();