| Message ID | 1407938238-21413-1-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote: > Commit 1a6b69b6548c (ARM: gic: add CPU migration support, > 2012-04-12) introduced an acquisition of the irq_controller_lock > in gic_raise_softirq() which can lead to a spinlock recursion if > the gic_arch_extn hooks call into the scheduler (via complete() > or wake_up(), etc.). This happens because gic_arch_extn hooks are > normally called with the irq_controller_lock held and calling > into the scheduler may cause us to call smp_send_reschedule() > which will grab the irq_controller_lock again. Here's an example > from a vendor kernel (note that the gic_arch_extn hook code here > isn't actually in mainline): Here's a question: why would you want to call into the scheduler from the gic_arch_extn code? Oh. My. God. Thomas, what have you done to the generic IRQ layer? This is /totally/ unsafe: void disable_irq(unsigned int irq) { if (!__disable_irq_nosync(irq)) synchronize_irq(irq); } static int __disable_irq_nosync(unsigned int irq) { unsigned long flags; struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL); if (!desc) return -EINVAL; __disable_irq(desc, irq, false); irq_put_desc_busunlock(desc, flags); return 0; } void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend) { if (suspend) { if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND)) return; desc->istate |= IRQS_SUSPENDED; } if (!desc->depth++) irq_disable(desc); } You realise that disable_irq() and enable_irq() can be called by concurrently by different drivers for the /same/ interrupt. For starters, that post-increment there is completely unprotected against races. Secondly, the above is completely racy against a concurrent enable_irq() - what if we're in disable_irq(), we've incremented depth, but have yet to call irq_disable(). The count now has a value of 1. We then preempt, and run another thread which calls enable_irq() on it. This results in the depth being decremented, and the IRQ is now enabled. We resume the original thread, and continue to call irq_disable(), resulting in the interrupt being disabled. That's not nice (the right answer is that it's strictly an unbalanced enable_irq(), but that's no excuse here.)
On 13/08/14 15:22, Russell King - ARM Linux wrote: > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote: >> Commit 1a6b69b6548c (ARM: gic: add CPU migration support, >> 2012-04-12) introduced an acquisition of the irq_controller_lock >> in gic_raise_softirq() which can lead to a spinlock recursion if >> the gic_arch_extn hooks call into the scheduler (via complete() >> or wake_up(), etc.). This happens because gic_arch_extn hooks are >> normally called with the irq_controller_lock held and calling >> into the scheduler may cause us to call smp_send_reschedule() >> which will grab the irq_controller_lock again. Here's an example >> from a vendor kernel (note that the gic_arch_extn hook code here >> isn't actually in mainline): > > Here's a question: why would you want to call into the scheduler from > the gic_arch_extn code? > > Oh. My. God. Thomas, what have you done to the generic IRQ layer? > This is /totally/ unsafe: > > void disable_irq(unsigned int irq) > { > if (!__disable_irq_nosync(irq)) > synchronize_irq(irq); > } > > static int __disable_irq_nosync(unsigned int irq) > { > unsigned long flags; > struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL); irq_get_desc_buslock() results in us owning the descriptor's lock (raw_spinlock_t). > > if (!desc) > return -EINVAL; > __disable_irq(desc, irq, false); > irq_put_desc_busunlock(desc, flags); > return 0; > } > > void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend) > { > if (suspend) { > if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND)) > return; > desc->istate |= IRQS_SUSPENDED; > } > > if (!desc->depth++) > irq_disable(desc); > } > > You realise that disable_irq() and enable_irq() can be called by > concurrently by different drivers for the /same/ interrupt. For > starters, that post-increment there is completely unprotected against > races. Secondly, the above is completely racy against a concurrent > enable_irq() - what if we're in disable_irq(), we've incremented > depth, but have yet to call irq_disable(). The count now has a > value of 1. > > We then preempt, and run another thread which calls enable_irq() > on it. This results in the depth being decremented, and the IRQ > is now enabled. We shouldn't get that far due to the spinlock taken during the disable. > We resume the original thread, and continue to call irq_disable(), > resulting in the interrupt being disabled. > > That's not nice (the right answer is that it's strictly an unbalanced > enable_irq(), but that's no excuse here.) > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/13, Russell King - ARM Linux wrote: > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote: > > Commit 1a6b69b6548c (ARM: gic: add CPU migration support, > > 2012-04-12) introduced an acquisition of the irq_controller_lock > > in gic_raise_softirq() which can lead to a spinlock recursion if > > the gic_arch_extn hooks call into the scheduler (via complete() > > or wake_up(), etc.). This happens because gic_arch_extn hooks are > > normally called with the irq_controller_lock held and calling > > into the scheduler may cause us to call smp_send_reschedule() > > which will grab the irq_controller_lock again. Here's an example > > from a vendor kernel (note that the gic_arch_extn hook code here > > isn't actually in mainline): > > Here's a question: why would you want to call into the scheduler from > the gic_arch_extn code? In this case we want to send a message to another processor when an interrupt is enabled that's only a wakeup interrupt in certain low power states. It's done sort of indirectly, but basically we block that low power state from being entered so we can ensure that the interrupt wakes us up from a lighter version of suspend. > > Oh. My. God. Thomas, what have you done to the generic IRQ layer? > This is /totally/ unsafe: > > void disable_irq(unsigned int irq) > { > if (!__disable_irq_nosync(irq)) > synchronize_irq(irq); > } > > static int __disable_irq_nosync(unsigned int irq) > { > unsigned long flags; > struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL); We got the lock here. > > if (!desc) > return -EINVAL; > __disable_irq(desc, irq, false); > irq_put_desc_busunlock(desc, flags); > return 0; > } > > void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend) > { > if (suspend) { > if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND)) > return; > desc->istate |= IRQS_SUSPENDED; > } > > if (!desc->depth++) We're still holding the lock here right? > irq_disable(desc); > } > > You realise that disable_irq() and enable_irq() can be called by > concurrently by different drivers for the /same/ interrupt. Sure. > For > starters, that post-increment there is completely unprotected against > races. Secondly, the above is completely racy against a concurrent > enable_irq() - what if we're in disable_irq(), we've incremented > depth, but have yet to call irq_disable(). The count now has a > value of 1. > > We then preempt, and run another thread which calls enable_irq() > on it. This results in the depth being decremented, and the IRQ > is now enabled. How? Aren't we holding the descriptor lock? > > We resume the original thread, and continue to call irq_disable(), > resulting in the interrupt being disabled. > > That's not nice (the right answer is that it's strictly an unbalanced > enable_irq(), but that's no excuse here.) >
On Wed, Aug 13, 2014 at 07:55:26AM -0700, Stephen Boyd wrote: > On 08/13, Russell King - ARM Linux wrote: > > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote: > > > Commit 1a6b69b6548c (ARM: gic: add CPU migration support, > > > 2012-04-12) introduced an acquisition of the irq_controller_lock > > > in gic_raise_softirq() which can lead to a spinlock recursion if > > > the gic_arch_extn hooks call into the scheduler (via complete() > > > or wake_up(), etc.). This happens because gic_arch_extn hooks are > > > normally called with the irq_controller_lock held and calling > > > into the scheduler may cause us to call smp_send_reschedule() > > > which will grab the irq_controller_lock again. Here's an example > > > from a vendor kernel (note that the gic_arch_extn hook code here > > > isn't actually in mainline): > > > > Here's a question: why would you want to call into the scheduler from > > the gic_arch_extn code? > > In this case we want to send a message to another processor when > an interrupt is enabled that's only a wakeup interrupt in certain > low power states. It's done sort of indirectly, but basically we > block that low power state from being entered so we can ensure > that the interrupt wakes us up from a lighter version of suspend. No, that's not the correct answer for the question I asked. I did not ask "why would you want to call into the IRQ code from the scheduler". I asked "why would you want to call into the scheduler from the gic_arch_extn code?" That's a completely different question. Let me rephrase to try and get an answer to my question: Why are you calling complete() or wake_up() from the gic_arch_extn code? > > static int __disable_irq_nosync(unsigned int irq) > > { > > unsigned long flags; > > struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL); > > We got the lock here. Yes, Daniel pointed that out, which makes this fine of course.
On 08/13, Russell King - ARM Linux wrote: > On Wed, Aug 13, 2014 at 07:55:26AM -0700, Stephen Boyd wrote: > > On 08/13, Russell King - ARM Linux wrote: > > > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote: > > > > Commit 1a6b69b6548c (ARM: gic: add CPU migration support, > > > > 2012-04-12) introduced an acquisition of the irq_controller_lock > > > > in gic_raise_softirq() which can lead to a spinlock recursion if > > > > the gic_arch_extn hooks call into the scheduler (via complete() > > > > or wake_up(), etc.). This happens because gic_arch_extn hooks are > > > > normally called with the irq_controller_lock held and calling > > > > into the scheduler may cause us to call smp_send_reschedule() > > > > which will grab the irq_controller_lock again. Here's an example > > > > from a vendor kernel (note that the gic_arch_extn hook code here > > > > isn't actually in mainline): > > > > > > Here's a question: why would you want to call into the scheduler from > > > the gic_arch_extn code? > > > > In this case we want to send a message to another processor when > > an interrupt is enabled that's only a wakeup interrupt in certain > > low power states. It's done sort of indirectly, but basically we > > block that low power state from being entered so we can ensure > > that the interrupt wakes us up from a lighter version of suspend. > > No, that's not the correct answer for the question I asked. I did not > ask "why would you want to call into the IRQ code from the scheduler". > I asked "why would you want to call into the scheduler from the > gic_arch_extn code?" > > That's a completely different question. Let me rephrase to try and get > an answer to my question: Why are you calling complete() or wake_up() > from the gic_arch_extn code? That's the answer. I guess you think "another processor" means some other CPU running linux? That isn't the case. We aren't calling into the IRQ code from the scheduler. The IRQ code is calling the gic_arch_extn code which is calling into the scheduler. Let me try to clarify. The path from the stacktrace I posted is: request_irq() irq_enable() gic_unmask_irq() msm_mpm_enable_irq() ... complete() In this case a driver is requesting an interrupt that is wakeup capable even in the lowest power state, so we wakeup a thread that's sitting around waiting on that completion to notify the non-linux running remote processor that we can go into the lowest power state. We can only communicate with the other processor via sleeping APIs. Another way to do it would be to schedule a work but we would get into the same situation as this because scheduling works may call into the scheduler too.
On Wed, 13 Aug 2014, Stephen Boyd wrote: > Commit 1a6b69b6548c (ARM: gic: add CPU migration support, > 2012-04-12) introduced an acquisition of the irq_controller_lock > in gic_raise_softirq() which can lead to a spinlock recursion if > the gic_arch_extn hooks call into the scheduler (via complete() > or wake_up(), etc.). This happens because gic_arch_extn hooks are > normally called with the irq_controller_lock held and calling > into the scheduler may cause us to call smp_send_reschedule() > which will grab the irq_controller_lock again. Here's an example > from a vendor kernel (note that the gic_arch_extn hook code here > isn't actually in mainline): > > BUG: spinlock recursion on CPU#0, swapper/0/1 > lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw > er_cpu: 0 > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e > > Call trace: > [<ffffffc000087e1c>] dump_backtrace+0x0/0x140 > [<ffffffc000087f6c>] show_stack+0x10/0x1c > [<ffffffc00064732c>] dump_stack+0x74/0xc4 > [<ffffffc0006446c0>] spin_dump+0x78/0x88 > [<ffffffc0006446f4>] spin_bug+0x24/0x34 > [<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148 > [<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38 > [<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc > [<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40 > [<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288 > [<ffffffc0000c1f4c>] default_wake_function+0xc/0x18 > [<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c > [<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c > [<ffffffc0000cf734>] complete+0x3c/0x5c > [<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8 > [<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c > [<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18 > [<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c > [<ffffffc0000de5f4>] irq_enable+0x2c/0x48 > [<ffffffc0000de65c>] irq_startup+0x4c/0x74 > [<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0 > [<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c > [<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4 > [<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4 > [<ffffffc000337374>] platform_drv_probe+0x20/0x54 > [<ffffffc00033598c>] driver_probe_device+0x158/0x340 > [<ffffffc000335c20>] __driver_attach+0x60/0x90 > [<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c > [<ffffffc000335304>] driver_attach+0x1c/0x28 > [<ffffffc000334f14>] bus_add_driver+0x120/0x204 > [<ffffffc0003362e4>] driver_register+0xbc/0x10c > [<ffffffc000337348>] __platform_driver_register+0x5c/0x68 > [<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c > [<ffffffc0000813ec>] do_one_initcall+0xa4/0x130 > [<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc > [<ffffffc000642578>] kernel_init+0xc/0xd4 > > We really just want to synchronize the sending of an SGI with the > update of the gic_cpu_map[], so introduce a new SGI lock that we > can use to synchronize the two code paths. Three main events are > happening that we have to consider: > > 1. We're updating the gic_cpu_mask to point to an > incoming CPU > > 2. We're (potentially) sending an SGI to the outgoing CPU > > 3. We're redirecting any pending SGIs for the outgoing > CPU to the incoming CPU. > > Events 1 and 3 are already ordered within the same CPU by means > of program order and use of I/O accessors. Events 1 and 2 don't > need to be ordered, but events 2 and 3 do because any SGIs for > the outgoing CPU need to be pending before we can redirect them. > Synchronize by acquiring a new lock around event 2 and before > event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure > that event 1 is seen before event 3 on other CPUs that may be > executing event 2. We put this all behind the b.L switcher config > option so that if we're not using this feature we don't have to > acquire any locks at all in the IPI path. > > Cc: Nicolas Pitre <nico@linaro.org> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> Russell's concerns notwithstanding, this is a worthwhile optimization and cleanup. Reviewed-by: Nicolas Pitre <nico@linaro.org> Of course it would be good to clarify things wrt Russell's remark independently from this patch. > --- > drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 7c131cf7cc13..b9e669cb1c1e 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -81,6 +81,16 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock); > #define NR_GIC_CPU_IF 8 > static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly; > > +#ifdef CONFIG_BL_SWITCHER > +/* Synchronize switching CPU interface and sending SGIs */ > +static DEFINE_RAW_SPINLOCK(gic_sgi_lock); > +#define sgi_map_lock(flags) raw_spin_lock_irqsave(&gic_sgi_lock, flags) > +#define sgi_map_unlock(flags) raw_spin_unlock_irqrestore(&gic_sgi_lock, flags) > +#else > +#define sgi_map_lock(flags) (void)(flags) > +#define sgi_map_unlock(flags) (void)(flags) > +#endif > + > /* > * Supported arch specific GIC irq extension. > * Default make them NULL. > @@ -658,7 +668,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) > int cpu; > unsigned long flags, map = 0; > > - raw_spin_lock_irqsave(&irq_controller_lock, flags); > + sgi_map_lock(flags); > > /* Convert our logical CPU mask into a physical one. */ > for_each_cpu(cpu, mask) > @@ -673,7 +683,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) > /* this always happens on GIC0 */ > writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT); > > - raw_spin_unlock_irqrestore(&irq_controller_lock, flags); > + sgi_map_unlock(flags); > } > #endif > > @@ -764,6 +774,15 @@ void gic_migrate_target(unsigned int new_cpu_id) > > raw_spin_unlock(&irq_controller_lock); > > + raw_spin_lock(&gic_sgi_lock); > + /* > + * Ensure that the gic_cpu_map update above is seen in > + * gic_raise_softirq() before we redirect any pending SGIs that > + * may have been raised for the outgoing CPU (cur_cpu_id) > + */ > + smp_mb__after_unlock_lock(); > + raw_spin_unlock(&gic_sgi_lock); > + > /* > * Now let's migrate and clear any potential SGIs that might be > * pending for us (cur_cpu_id). Since GIC_DIST_SGI_PENDING_SET > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation > > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen, On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote: > Commit 1a6b69b6548c (ARM: gic: add CPU migration support, > 2012-04-12) introduced an acquisition of the irq_controller_lock > in gic_raise_softirq() which can lead to a spinlock recursion if > the gic_arch_extn hooks call into the scheduler (via complete() > or wake_up(), etc.). This happens because gic_arch_extn hooks are > normally called with the irq_controller_lock held and calling > into the scheduler may cause us to call smp_send_reschedule() > which will grab the irq_controller_lock again. Here's an example > from a vendor kernel (note that the gic_arch_extn hook code here > isn't actually in mainline): > > BUG: spinlock recursion on CPU#0, swapper/0/1 > lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw > er_cpu: 0 > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e > > Call trace: > [<ffffffc000087e1c>] dump_backtrace+0x0/0x140 > [<ffffffc000087f6c>] show_stack+0x10/0x1c > [<ffffffc00064732c>] dump_stack+0x74/0xc4 > [<ffffffc0006446c0>] spin_dump+0x78/0x88 > [<ffffffc0006446f4>] spin_bug+0x24/0x34 > [<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148 > [<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38 > [<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc > [<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40 > [<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288 > [<ffffffc0000c1f4c>] default_wake_function+0xc/0x18 > [<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c > [<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c > [<ffffffc0000cf734>] complete+0x3c/0x5c > [<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8 > [<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c > [<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18 > [<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c > [<ffffffc0000de5f4>] irq_enable+0x2c/0x48 > [<ffffffc0000de65c>] irq_startup+0x4c/0x74 > [<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0 > [<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c > [<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4 > [<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4 > [<ffffffc000337374>] platform_drv_probe+0x20/0x54 > [<ffffffc00033598c>] driver_probe_device+0x158/0x340 > [<ffffffc000335c20>] __driver_attach+0x60/0x90 > [<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c > [<ffffffc000335304>] driver_attach+0x1c/0x28 > [<ffffffc000334f14>] bus_add_driver+0x120/0x204 > [<ffffffc0003362e4>] driver_register+0xbc/0x10c > [<ffffffc000337348>] __platform_driver_register+0x5c/0x68 > [<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c > [<ffffffc0000813ec>] do_one_initcall+0xa4/0x130 > [<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc > [<ffffffc000642578>] kernel_init+0xc/0xd4 > > We really just want to synchronize the sending of an SGI with the > update of the gic_cpu_map[], so introduce a new SGI lock that we > can use to synchronize the two code paths. Three main events are > happening that we have to consider: > > 1. We're updating the gic_cpu_mask to point to an > incoming CPU > > 2. We're (potentially) sending an SGI to the outgoing CPU > > 3. We're redirecting any pending SGIs for the outgoing > CPU to the incoming CPU. > > Events 1 and 3 are already ordered within the same CPU by means > of program order and use of I/O accessors. Events 1 and 2 don't > need to be ordered, but events 2 and 3 do because any SGIs for > the outgoing CPU need to be pending before we can redirect them. > Synchronize by acquiring a new lock around event 2 and before > event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure > that event 1 is seen before event 3 on other CPUs that may be > executing event 2. We put this all behind the b.L switcher config > option so that if we're not using this feature we don't have to > acquire any locks at all in the IPI path. > > Cc: Nicolas Pitre <nico@linaro.org> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) Applied to irqchip/urgent with Nico's Ack. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote: > Stephen, > > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote: > > Commit 1a6b69b6548c (ARM: gic: add CPU migration support, > > 2012-04-12) introduced an acquisition of the irq_controller_lock > > in gic_raise_softirq() which can lead to a spinlock recursion if > > the gic_arch_extn hooks call into the scheduler (via complete() > > or wake_up(), etc.). This happens because gic_arch_extn hooks are > > normally called with the irq_controller_lock held and calling > > into the scheduler may cause us to call smp_send_reschedule() > > which will grab the irq_controller_lock again. Here's an example > > from a vendor kernel (note that the gic_arch_extn hook code here > > isn't actually in mainline): > > > > BUG: spinlock recursion on CPU#0, swapper/0/1 > > lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw > > er_cpu: 0 > > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e > > > > Call trace: > > [<ffffffc000087e1c>] dump_backtrace+0x0/0x140 > > [<ffffffc000087f6c>] show_stack+0x10/0x1c > > [<ffffffc00064732c>] dump_stack+0x74/0xc4 > > [<ffffffc0006446c0>] spin_dump+0x78/0x88 > > [<ffffffc0006446f4>] spin_bug+0x24/0x34 > > [<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148 > > [<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38 > > [<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc > > [<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40 > > [<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288 > > [<ffffffc0000c1f4c>] default_wake_function+0xc/0x18 > > [<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c > > [<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c > > [<ffffffc0000cf734>] complete+0x3c/0x5c > > [<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8 > > [<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c > > [<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18 > > [<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c > > [<ffffffc0000de5f4>] irq_enable+0x2c/0x48 > > [<ffffffc0000de65c>] irq_startup+0x4c/0x74 > > [<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0 > > [<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c > > [<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4 > > [<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4 > > [<ffffffc000337374>] platform_drv_probe+0x20/0x54 > > [<ffffffc00033598c>] driver_probe_device+0x158/0x340 > > [<ffffffc000335c20>] __driver_attach+0x60/0x90 > > [<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c > > [<ffffffc000335304>] driver_attach+0x1c/0x28 > > [<ffffffc000334f14>] bus_add_driver+0x120/0x204 > > [<ffffffc0003362e4>] driver_register+0xbc/0x10c > > [<ffffffc000337348>] __platform_driver_register+0x5c/0x68 > > [<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c > > [<ffffffc0000813ec>] do_one_initcall+0xa4/0x130 > > [<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc > > [<ffffffc000642578>] kernel_init+0xc/0xd4 > > > > We really just want to synchronize the sending of an SGI with the > > update of the gic_cpu_map[], so introduce a new SGI lock that we > > can use to synchronize the two code paths. Three main events are > > happening that we have to consider: > > > > 1. We're updating the gic_cpu_mask to point to an > > incoming CPU > > > > 2. We're (potentially) sending an SGI to the outgoing CPU > > > > 3. We're redirecting any pending SGIs for the outgoing > > CPU to the incoming CPU. > > > > Events 1 and 3 are already ordered within the same CPU by means > > of program order and use of I/O accessors. Events 1 and 2 don't > > need to be ordered, but events 2 and 3 do because any SGIs for > > the outgoing CPU need to be pending before we can redirect them. > > Synchronize by acquiring a new lock around event 2 and before > > event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure > > that event 1 is seen before event 3 on other CPUs that may be > > executing event 2. We put this all behind the b.L switcher config > > option so that if we're not using this feature we don't have to > > acquire any locks at all in the IPI path. > > > > Cc: Nicolas Pitre <nico@linaro.org> > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > > --- > > drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++-- > > 1 file changed, 21 insertions(+), 2 deletions(-) > > Applied to irqchip/urgent with Nico's Ack. Interesting, so I'm discussing this patch, and it gets applied anyway... yes, that's great.
Russell, On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote: > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote: > > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote: > > > Commit 1a6b69b6548c (ARM: gic: add CPU migration support, > > > 2012-04-12) introduced an acquisition of the irq_controller_lock > > > in gic_raise_softirq() which can lead to a spinlock recursion if > > > the gic_arch_extn hooks call into the scheduler (via complete() > > > or wake_up(), etc.). This happens because gic_arch_extn hooks are > > > normally called with the irq_controller_lock held and calling > > > into the scheduler may cause us to call smp_send_reschedule() > > > which will grab the irq_controller_lock again. Here's an example > > > from a vendor kernel (note that the gic_arch_extn hook code here > > > isn't actually in mainline): > > > > > > BUG: spinlock recursion on CPU#0, swapper/0/1 > > > lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw > > > er_cpu: 0 > > > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e > > > > > > Call trace: > > > [<ffffffc000087e1c>] dump_backtrace+0x0/0x140 > > > [<ffffffc000087f6c>] show_stack+0x10/0x1c > > > [<ffffffc00064732c>] dump_stack+0x74/0xc4 > > > [<ffffffc0006446c0>] spin_dump+0x78/0x88 > > > [<ffffffc0006446f4>] spin_bug+0x24/0x34 > > > [<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148 > > > [<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38 > > > [<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc > > > [<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40 > > > [<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288 > > > [<ffffffc0000c1f4c>] default_wake_function+0xc/0x18 > > > [<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c > > > [<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c > > > [<ffffffc0000cf734>] complete+0x3c/0x5c > > > [<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8 > > > [<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c > > > [<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18 > > > [<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c > > > [<ffffffc0000de5f4>] irq_enable+0x2c/0x48 > > > [<ffffffc0000de65c>] irq_startup+0x4c/0x74 > > > [<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0 > > > [<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c > > > [<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4 > > > [<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4 > > > [<ffffffc000337374>] platform_drv_probe+0x20/0x54 > > > [<ffffffc00033598c>] driver_probe_device+0x158/0x340 > > > [<ffffffc000335c20>] __driver_attach+0x60/0x90 > > > [<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c > > > [<ffffffc000335304>] driver_attach+0x1c/0x28 > > > [<ffffffc000334f14>] bus_add_driver+0x120/0x204 > > > [<ffffffc0003362e4>] driver_register+0xbc/0x10c > > > [<ffffffc000337348>] __platform_driver_register+0x5c/0x68 > > > [<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c > > > [<ffffffc0000813ec>] do_one_initcall+0xa4/0x130 > > > [<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc > > > [<ffffffc000642578>] kernel_init+0xc/0xd4 > > > > > > We really just want to synchronize the sending of an SGI with the > > > update of the gic_cpu_map[], so introduce a new SGI lock that we > > > can use to synchronize the two code paths. Three main events are > > > happening that we have to consider: > > > > > > 1. We're updating the gic_cpu_mask to point to an > > > incoming CPU > > > > > > 2. We're (potentially) sending an SGI to the outgoing CPU > > > > > > 3. We're redirecting any pending SGIs for the outgoing > > > CPU to the incoming CPU. > > > > > > Events 1 and 3 are already ordered within the same CPU by means > > > of program order and use of I/O accessors. Events 1 and 2 don't > > > need to be ordered, but events 2 and 3 do because any SGIs for > > > the outgoing CPU need to be pending before we can redirect them. > > > Synchronize by acquiring a new lock around event 2 and before > > > event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure > > > that event 1 is seen before event 3 on other CPUs that may be > > > executing event 2. We put this all behind the b.L switcher config > > > option so that if we're not using this feature we don't have to > > > acquire any locks at all in the IPI path. > > > > > > Cc: Nicolas Pitre <nico@linaro.org> > > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > > > --- > > > drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++-- > > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > Applied to irqchip/urgent with Nico's Ack. > > Interesting, so I'm discussing this patch, and it gets applied anyway... > yes, that's great. Quoting Nico: "Of course it would be good to clarify things wrt Russell's remark independently from this patch." I took 'independently' to mean "This patch is ok, *and* we need to address Russell's concerns in a follow-up patch." Nico's Reviewed-by with that comment was sent August 13th. The most recent activity on this thread was also August 13th. After four days, I reasoned there were no objections to his comment. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 17, 2014 at 03:04:34PM -0400, Jason Cooper wrote: > Quoting Nico: > > "Of course it would be good to clarify things wrt Russell's remark > independently from this patch." > > I took 'independently' to mean "This patch is ok, *and* we need to > address Russell's concerns in a follow-up patch." > > Nico's Reviewed-by with that comment was sent August 13th. The most > recent activity on this thread was also August 13th. After four days, I > reasoned there were no objections to his comment. Right, during the merge window, and during merge windows, I tend to ignore almost all email now because people don't stop developing, and they don't take any notice where the mainline cycle is. In fact, I go off and do non-kernel work during a merge window and only briefly scan for bug fixes. However, I have other concerns with this patch, which I've yet to air. For example, I don't like this crappy conditional locking that people keep dreaming up - that kind of stuff makes the kernel much harder to statically check that everything is correct. It's an anti-lockdep strategy. Secondly, I don't like this: + raw_spin_lock(&gic_sgi_lock); + /* + * Ensure that the gic_cpu_map update above is seen in + * gic_raise_softirq() before we redirect any pending SGIs that + * may have been raised for the outgoing CPU (cur_cpu_id) + */ + smp_mb__after_unlock_lock(); + raw_spin_unlock(&gic_sgi_lock); That goes against the principle of locking, that you lock the data, not the code. I have no problem with changing gic_raise_softirq() to use a different lock, which gic_migrate_target(), and gic_set_affinity() can also use. There's no need for horrid locking here, because the only thing we're protecting is gic_map[] and the write to the register to trigger an IPI - and nothing using gic_arch_extn has any business knowing about SGIs. No need for these crappy sgi_map_lock() macros and all the ifdeffery.
On Sun, 17 Aug 2014, Jason Cooper wrote: > Russell, > > On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote: > > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote: > > > Applied to irqchip/urgent with Nico's Ack. > > > > Interesting, so I'm discussing this patch, and it gets applied anyway... > > yes, that's great. > > Quoting Nico: > > "Of course it would be good to clarify things wrt Russell's remark > independently from this patch." > > I took 'independently' to mean "This patch is ok, *and* we need to > address Russell's concerns in a follow-up patch." > > Nico's Reviewed-by with that comment was sent August 13th. The most > recent activity on this thread was also August 13th. After four days, I > reasoned there were no objections to his comment. Well... I mentioned this patch is a nice cleanup independently of the reason why it was created in the first place. Maybe that shouldn't be sorted as "urgent" in that case, especially when the code having problem with the current state of things is living out of mainline. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 17 Aug 2014, Russell King - ARM Linux wrote: > On Sun, Aug 17, 2014 at 03:04:34PM -0400, Jason Cooper wrote: > > Quoting Nico: > > > > "Of course it would be good to clarify things wrt Russell's remark > > independently from this patch." > > > > I took 'independently' to mean "This patch is ok, *and* we need to > > address Russell's concerns in a follow-up patch." > > > > Nico's Reviewed-by with that comment was sent August 13th. The most > > recent activity on this thread was also August 13th. After four days, I > > reasoned there were no objections to his comment. > > Right, during the merge window, and during merge windows, I tend to > ignore almost all email now because people don't stop developing, and > they don't take any notice where the mainline cycle is. In fact, I go > off and do non-kernel work during a merge window and only briefly scan > for bug fixes. > > However, I have other concerns with this patch, which I've yet to air. > For example, I don't like this crappy conditional locking that people > keep dreaming up - that kind of stuff makes the kernel much harder to > statically check that everything is correct. It's an anti-lockdep > strategy. > > Secondly, I don't like this: > > + raw_spin_lock(&gic_sgi_lock); > + /* > + * Ensure that the gic_cpu_map update above is seen in > + * gic_raise_softirq() before we redirect any pending SGIs that > + * may have been raised for the outgoing CPU (cur_cpu_id) > + */ > + smp_mb__after_unlock_lock(); > + raw_spin_unlock(&gic_sgi_lock); > > That goes against the principle of locking, that you lock the data, > not the code. I admit I didn't understand the point of that construct on the first read. Maybe I wouldn't be the only one. Using Stephen's initial version for that hunk would be preferable as it is straight forward and would mean locking the data instead. > I have no problem with changing gic_raise_softirq() to use a different > lock, which gic_migrate_target(), and gic_set_affinity() can also use. > There's no need for horrid locking here, because the only thing we're > protecting is gic_map[] and the write to the register to trigger an > IPI - and nothing using gic_arch_extn has any business knowing about > SGIs. > > No need for these crappy sgi_map_lock() macros and all the ifdeffery. Those macros are there only to conditionalize the locking in gic_raise_softirq() because no locking what so ever is needed there when gic_migrate_target() is configured out. I suggested the macros to cut down on the #ifdefery in the code. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 17, 2014 at 08:04:45PM -0400, Nicolas Pitre wrote: > On Sun, 17 Aug 2014, Jason Cooper wrote: > > On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote: > > > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote: > > > > Applied to irqchip/urgent with Nico's Ack. > > > > > > Interesting, so I'm discussing this patch, and it gets applied anyway... > > > yes, that's great. > > > > Quoting Nico: > > > > "Of course it would be good to clarify things wrt Russell's remark > > independently from this patch." > > > > I took 'independently' to mean "This patch is ok, *and* we need to > > address Russell's concerns in a follow-up patch." > > > > Nico's Reviewed-by with that comment was sent August 13th. The most > > recent activity on this thread was also August 13th. After four days, I > > reasoned there were no objections to his comment. > > Well... I mentioned this patch is a nice cleanup independently of the > reason why it was created in the first place. Ah, fair enough. > Maybe that shouldn't be sorted as "urgent" in that case, especially > when the code having problem with the current state of things is > living out of mainline. hmmm, yes. I've been grappling with the semantics of '/urgent' vice '/fixes'. With mvebu, /fixes is the branch for all changes needing to go into the current -rcX cycle. For irqchip, Thomas suggested /urgent for the equivalent branch. To me, they serve the same purpose. Unfortunately, I occasionally hear "Well, it's not _urgent_ ...". I suppose I'll put up with it for one more cycle and then change it to /fixes. :) wrt this patch, I need to drop it anyway. I was a bit rusty (it's been a few weeks) and forgot to add the Cc -stable and Fixes: tags. I do agree, though, it's certainly not urgent. As Russell has raised more issues with this patch as well, I'll hold off on re-applying until I see a new version. Hopefully it'll meet with everyones approval. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 17, 2014 at 10:41:23PM +0100, Russell King - ARM Linux wrote: > On Sun, Aug 17, 2014 at 03:04:34PM -0400, Jason Cooper wrote: > > Quoting Nico: > > > > "Of course it would be good to clarify things wrt Russell's remark > > independently from this patch." > > > > I took 'independently' to mean "This patch is ok, *and* we need to > > address Russell's concerns in a follow-up patch." > > > > Nico's Reviewed-by with that comment was sent August 13th. The most > > recent activity on this thread was also August 13th. After four days, I > > reasoned there were no objections to his comment. > > Right, during the merge window, and during merge windows, I tend to > ignore almost all email now because people don't stop developing, and > they don't take any notice where the mainline cycle is. In fact, I go > off and do non-kernel work during a merge window and only briefly scan > for bug fixes. Ok, now dropped. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 17 Aug 2014, Jason Cooper wrote: > On Sun, Aug 17, 2014 at 08:04:45PM -0400, Nicolas Pitre wrote: > > On Sun, 17 Aug 2014, Jason Cooper wrote: > > > On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote: > > > > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote: > > > > > Applied to irqchip/urgent with Nico's Ack. > > > > > > > > Interesting, so I'm discussing this patch, and it gets applied anyway... > > > > yes, that's great. > > > > > > Quoting Nico: > > > > > > "Of course it would be good to clarify things wrt Russell's remark > > > independently from this patch." > > > > > > I took 'independently' to mean "This patch is ok, *and* we need to > > > address Russell's concerns in a follow-up patch." > > > > > > Nico's Reviewed-by with that comment was sent August 13th. The most > > > recent activity on this thread was also August 13th. After four days, I > > > reasoned there were no objections to his comment. > > > > Well... I mentioned this patch is a nice cleanup independently of the > > reason why it was created in the first place. > > Ah, fair enough. > > > Maybe that shouldn't be sorted as "urgent" in that case, especially > > when the code having problem with the current state of things is > > living out of mainline. > > hmmm, yes. I've been grappling with the semantics of '/urgent' vice > '/fixes'. With mvebu, /fixes is the branch for all changes needing to go > into the current -rcX cycle. For irqchip, Thomas suggested /urgent for > the equivalent branch. To me, they serve the same purpose. > Unfortunately, I occasionally hear "Well, it's not _urgent_ ...". I > suppose I'll put up with it for one more cycle and then change it to > /fixes. :) > > wrt this patch, I need to drop it anyway. I was a bit rusty (it's been > a few weeks) and forgot to add the Cc -stable and Fixes: tags. I do > agree, though, it's certainly not urgent. Given the raised issue has to do with out-of-tree code, there is no need to CC stable in that case anyway. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 17, 2014 at 09:35:11PM -0400, Nicolas Pitre wrote: > On Sun, 17 Aug 2014, Jason Cooper wrote: > > > On Sun, Aug 17, 2014 at 08:04:45PM -0400, Nicolas Pitre wrote: > > > On Sun, 17 Aug 2014, Jason Cooper wrote: > > > > On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote: > > > > > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote: > > > > > > Applied to irqchip/urgent with Nico's Ack. > > > > > > > > > > Interesting, so I'm discussing this patch, and it gets applied anyway... > > > > > yes, that's great. > > > > > > > > Quoting Nico: > > > > > > > > "Of course it would be good to clarify things wrt Russell's remark > > > > independently from this patch." > > > > > > > > I took 'independently' to mean "This patch is ok, *and* we need to > > > > address Russell's concerns in a follow-up patch." > > > > > > > > Nico's Reviewed-by with that comment was sent August 13th. The most > > > > recent activity on this thread was also August 13th. After four days, I > > > > reasoned there were no objections to his comment. > > > > > > Well... I mentioned this patch is a nice cleanup independently of the > > > reason why it was created in the first place. > > > > Ah, fair enough. > > > > > Maybe that shouldn't be sorted as "urgent" in that case, especially > > > when the code having problem with the current state of things is > > > living out of mainline. > > > > hmmm, yes. I've been grappling with the semantics of '/urgent' vice > > '/fixes'. With mvebu, /fixes is the branch for all changes needing to go > > into the current -rcX cycle. For irqchip, Thomas suggested /urgent for > > the equivalent branch. To me, they serve the same purpose. > > Unfortunately, I occasionally hear "Well, it's not _urgent_ ...". I > > suppose I'll put up with it for one more cycle and then change it to > > /fixes. :) > > > > wrt this patch, I need to drop it anyway. I was a bit rusty (it's been > > a few weeks) and forgot to add the Cc -stable and Fixes: tags. I do > > agree, though, it's certainly not urgent. > > Given the raised issue has to do with out-of-tree code, there is no need > to CC stable in that case anyway. I could go either way here. On the one hand, a fix is a fix is a fix. On the other, if it can't be triggered in mainline, we shouldn't accept it at all. Stephen, is the out of tree code that triggered this bound for mainline? thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 17 Aug 2014, Jason Cooper wrote: > On Sun, Aug 17, 2014 at 09:35:11PM -0400, Nicolas Pitre wrote: > > On Sun, 17 Aug 2014, Jason Cooper wrote: > > > > > On Sun, Aug 17, 2014 at 08:04:45PM -0400, Nicolas Pitre wrote: > > > > On Sun, 17 Aug 2014, Jason Cooper wrote: > > > > > On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote: > > > > > > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote: > > > > > > > Applied to irqchip/urgent with Nico's Ack. > > > > > > > > > > > > Interesting, so I'm discussing this patch, and it gets applied anyway... > > > > > > yes, that's great. > > > > > > > > > > Quoting Nico: > > > > > > > > > > "Of course it would be good to clarify things wrt Russell's remark > > > > > independently from this patch." > > > > > > > > > > I took 'independently' to mean "This patch is ok, *and* we need to > > > > > address Russell's concerns in a follow-up patch." > > > > > > > > > > Nico's Reviewed-by with that comment was sent August 13th. The most > > > > > recent activity on this thread was also August 13th. After four days, I > > > > > reasoned there were no objections to his comment. > > > > > > > > Well... I mentioned this patch is a nice cleanup independently of the > > > > reason why it was created in the first place. > > > > > > Ah, fair enough. > > > > > > > Maybe that shouldn't be sorted as "urgent" in that case, especially > > > > when the code having problem with the current state of things is > > > > living out of mainline. > > > > > > hmmm, yes. I've been grappling with the semantics of '/urgent' vice > > > '/fixes'. With mvebu, /fixes is the branch for all changes needing to go > > > into the current -rcX cycle. For irqchip, Thomas suggested /urgent for > > > the equivalent branch. To me, they serve the same purpose. > > > Unfortunately, I occasionally hear "Well, it's not _urgent_ ...". I > > > suppose I'll put up with it for one more cycle and then change it to > > > /fixes. :) > > > > > > wrt this patch, I need to drop it anyway. I was a bit rusty (it's been > > > a few weeks) and forgot to add the Cc -stable and Fixes: tags. I do > > > agree, though, it's certainly not urgent. > > > > Given the raised issue has to do with out-of-tree code, there is no need > > to CC stable in that case anyway. > > I could go either way here. On the one hand, a fix is a fix is a fix. > On the other, if it can't be triggered in mainline, we shouldn't accept > it at all. For mainline, it should be accepted as a cleanup and minor optimization since no mainline code is currently affected by the absence of this patch. If there is a real bug being fixed by this patch, and whether the best way to fix it is by relying on this patch, is still up for debate. > Stephen, is the out of tree code that triggered this bound for mainline? Maybe "mainline", but certainly not "stable". Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/17/14 17:17, Nicolas Pitre wrote: > On Sun, 17 Aug 2014, Russell King - ARM Linux wrote: > >> I have no problem with changing gic_raise_softirq() to use a different >> lock, which gic_migrate_target(), and gic_set_affinity() can also use. >> There's no need for horrid locking here, because the only thing we're >> protecting is gic_map[] and the write to the register to trigger an >> IPI - and nothing using gic_arch_extn has any business knowing about >> SGIs. >> >> No need for these crappy sgi_map_lock() macros and all the ifdeffery. > Those macros are there only to conditionalize the locking in > gic_raise_softirq() because no locking what so ever is needed there when > gic_migrate_target() is configured out. I suggested the macros to cut > down on the #ifdefery in the code. Ok I can resend with the sgi lock around the gic_cpu_map updating code. Let's see how v5 goes.
On 08/17/14 18:54, Jason Cooper wrote: > > Stephen, is the out of tree code that triggered this bound for mainline? > It's bound for mainline eventually. We're actively working on enabling more low power modes and when that happens we'll need this patch. We can always carry this patch for now if you don't want to accept it, but I figured getting it mainlined reduces our carrying burden and also makes a minor improvement in sending IPIs when the BL switcher is used.
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 7c131cf7cc13..b9e669cb1c1e 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -81,6 +81,16 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock); #define NR_GIC_CPU_IF 8 static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly; +#ifdef CONFIG_BL_SWITCHER +/* Synchronize switching CPU interface and sending SGIs */ +static DEFINE_RAW_SPINLOCK(gic_sgi_lock); +#define sgi_map_lock(flags) raw_spin_lock_irqsave(&gic_sgi_lock, flags) +#define sgi_map_unlock(flags) raw_spin_unlock_irqrestore(&gic_sgi_lock, flags) +#else +#define sgi_map_lock(flags) (void)(flags) +#define sgi_map_unlock(flags) (void)(flags) +#endif + /* * Supported arch specific GIC irq extension. * Default make them NULL. @@ -658,7 +668,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) int cpu; unsigned long flags, map = 0; - raw_spin_lock_irqsave(&irq_controller_lock, flags); + sgi_map_lock(flags); /* Convert our logical CPU mask into a physical one. */ for_each_cpu(cpu, mask) @@ -673,7 +683,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) /* this always happens on GIC0 */ writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT); - raw_spin_unlock_irqrestore(&irq_controller_lock, flags); + sgi_map_unlock(flags); } #endif @@ -764,6 +774,15 @@ void gic_migrate_target(unsigned int new_cpu_id) raw_spin_unlock(&irq_controller_lock); + raw_spin_lock(&gic_sgi_lock); + /* + * Ensure that the gic_cpu_map update above is seen in + * gic_raise_softirq() before we redirect any pending SGIs that + * may have been raised for the outgoing CPU (cur_cpu_id) + */ + smp_mb__after_unlock_lock(); + raw_spin_unlock(&gic_sgi_lock); + /* * Now let's migrate and clear any potential SGIs that might be * pending for us (cur_cpu_id). Since GIC_DIST_SGI_PENDING_SET
Commit 1a6b69b6548c (ARM: gic: add CPU migration support, 2012-04-12) introduced an acquisition of the irq_controller_lock in gic_raise_softirq() which can lead to a spinlock recursion if the gic_arch_extn hooks call into the scheduler (via complete() or wake_up(), etc.). This happens because gic_arch_extn hooks are normally called with the irq_controller_lock held and calling into the scheduler may cause us to call smp_send_reschedule() which will grab the irq_controller_lock again. Here's an example from a vendor kernel (note that the gic_arch_extn hook code here isn't actually in mainline): BUG: spinlock recursion on CPU#0, swapper/0/1 lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw er_cpu: 0 CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e Call trace: [<ffffffc000087e1c>] dump_backtrace+0x0/0x140 [<ffffffc000087f6c>] show_stack+0x10/0x1c [<ffffffc00064732c>] dump_stack+0x74/0xc4 [<ffffffc0006446c0>] spin_dump+0x78/0x88 [<ffffffc0006446f4>] spin_bug+0x24/0x34 [<ffffffc0000d47d0>] do_raw_spin_lock+0x58/0x148 [<ffffffc00064d398>] _raw_spin_lock_irqsave+0x24/0x38 [<ffffffc0002c9d7c>] gic_raise_softirq+0x2c/0xbc [<ffffffc00008daa4>] smp_send_reschedule+0x34/0x40 [<ffffffc0000c1e94>] try_to_wake_up+0x224/0x288 [<ffffffc0000c1f4c>] default_wake_function+0xc/0x18 [<ffffffc0000ceef0>] __wake_up_common+0x50/0x8c [<ffffffc0000cef3c>] __wake_up_locked+0x10/0x1c [<ffffffc0000cf734>] complete+0x3c/0x5c [<ffffffc0002f0e78>] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8 [<ffffffc0002f0f58>] __msm_mpm_enable_irq+0x4c/0x7c [<ffffffc0002f0f94>] msm_mpm_enable_irq+0xc/0x18 [<ffffffc0002c9bb0>] gic_unmask_irq+0x40/0x7c [<ffffffc0000de5f4>] irq_enable+0x2c/0x48 [<ffffffc0000de65c>] irq_startup+0x4c/0x74 [<ffffffc0000dd2fc>] __setup_irq+0x264/0x3f0 [<ffffffc0000dd5e0>] request_threaded_irq+0xcc/0x11c [<ffffffc0000df254>] devm_request_threaded_irq+0x68/0xb4 [<ffffffc000471520>] msm_iommu_ctx_probe+0x124/0x2d4 [<ffffffc000337374>] platform_drv_probe+0x20/0x54 [<ffffffc00033598c>] driver_probe_device+0x158/0x340 [<ffffffc000335c20>] __driver_attach+0x60/0x90 [<ffffffc000333c9c>] bus_for_each_dev+0x6c/0x8c [<ffffffc000335304>] driver_attach+0x1c/0x28 [<ffffffc000334f14>] bus_add_driver+0x120/0x204 [<ffffffc0003362e4>] driver_register+0xbc/0x10c [<ffffffc000337348>] __platform_driver_register+0x5c/0x68 [<ffffffc00094c478>] msm_iommu_driver_init+0x54/0x7c [<ffffffc0000813ec>] do_one_initcall+0xa4/0x130 [<ffffffc00091d928>] kernel_init_freeable+0x138/0x1dc [<ffffffc000642578>] kernel_init+0xc/0xd4 We really just want to synchronize the sending of an SGI with the update of the gic_cpu_map[], so introduce a new SGI lock that we can use to synchronize the two code paths. Three main events are happening that we have to consider: 1. We're updating the gic_cpu_mask to point to an incoming CPU 2. We're (potentially) sending an SGI to the outgoing CPU 3. We're redirecting any pending SGIs for the outgoing CPU to the incoming CPU. Events 1 and 3 are already ordered within the same CPU by means of program order and use of I/O accessors. Events 1 and 2 don't need to be ordered, but events 2 and 3 do because any SGIs for the outgoing CPU need to be pending before we can redirect them. Synchronize by acquiring a new lock around event 2 and before event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure that event 1 is seen before event 3 on other CPUs that may be executing event 2. We put this all behind the b.L switcher config option so that if we're not using this feature we don't have to acquire any locks at all in the IPI path. Cc: Nicolas Pitre <nico@linaro.org> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)