| Message ID | 1380297282-13021-1-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 27 Sep 2013, Lorenzo Pieralisi wrote: > On the TC2 testchip, when all CPUs in a cluster enter standbywfi > and commit a power down request, the power controller will wait > for standbywfil2 coming from L2 cache controller to shut the > cluster down. > By the time all CPUs in a cluster commit a power down request > and enter wfi, the power controller cannot backtrack, or put it > another way, a CPU must not be allowed to complete execution > independently of the power controller, the only way for it to > resume properly must be upon wake-up IRQ pending and subsequent > reset triggered from the power controller. > > Current MCPM back-end for TC2 disables the GIC CPU IF only when > power down is committed through the tc2_pm_suspend() method, that > makes sense since a suspended CPU is still online and can receive > interrupts whereas a hotplugged CPU, since it is offline, > migrated all IRQs and shutdown the per-CPU peripherals, hence > their PPIs. > > The flaw with this reasoning is the following. If all CPUs in > a clusters are entering a power down state either through CPU > idle or CPU hotplug, when the last man successfully completes > the MCPM power down sequence (and executes wfi), power controller > waits for L2 wfi signal to quiesce the cluster and shut it down. > If, when all CPUs are sitting in wfi, an online CPU hotplugs back > in one of the CPUs in the cluster being shutdown, that CPU > receives an IPI that causes wfi to complete (since tc2_pm_down() > method does not disable the GIC CPU IF in that case - CPU being > hotplugged out, not idle) and the power controller will never see > the stanbywfil2 signal coming from L2 that is required for > shutdown to happen and the system deadlocks. > > Further to this issue, kexec hotplugs secondary CPUs out during > kernel reload/restart. > Because kexec may (deliberately) trash the old kernel text, it is > not OK for CPUs to follow the MCPM soft reboot path, since > instructions after the WFI may have been replaced by kexec. > > If tc2_pm_down() does not disable the GIC cpu interface, there is a > race between CPU powerdown in the old kernel and the IPI from the > new kernel that triggers secondary boot, particularly if the > powerdown is slow (due to L2 cache cleaning for example). If the > new kernel wins the race, the affected CPU(s) will not really be > reset and may execute garbage after the WFI. > > The only solution to this problem consists in disabling the GIC > CPU IF on a CPU committed to power down regardless of the power > down entry method (CPU hotplug or CPU idle). This way, CPU wake-up > is under power controller control, which prevents unexpected wfi > exit caused by a pending IRQ. > > This patch moves the GIC CPU IF disable call in the TC2 MCPM > implementation from the tc2_pm_suspend() method to the > tc2_pm_down() method to fix the mentioned race condition(s). > > Reviewed-by: Dave Martin <Dave.Martin@arm.com> > Tested-by: Dave Martin <Dave.Martin@arm.com> (for kexec) > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com> Acked-by: Nicolas Pitre <nico@linaro.org> ARM-SOC dudes: would you please pick this up? > --- > arch/arm/mach-vexpress/tc2_pm.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c > index 7aeb5d6..e6eb481 100644 > --- a/arch/arm/mach-vexpress/tc2_pm.c > +++ b/arch/arm/mach-vexpress/tc2_pm.c > @@ -131,6 +131,16 @@ static void tc2_pm_down(u64 residency) > } else > BUG(); > > + /* > + * If the CPU is committed to power down, make sure > + * the power controller will be in charge of waking it > + * up upon IRQ, ie IRQ lines are cut from GIC CPU IF > + * to the CPU by disabling the GIC CPU IF to prevent wfi > + * from completing execution behind power controller back > + */ > + if (!skip_wfi) > + gic_cpu_if_down(); > + > if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) { > arch_spin_unlock(&tc2_pm_lock); > > @@ -231,7 +241,6 @@ static void tc2_pm_suspend(u64 residency) > cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0); > cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1); > ve_spc_set_resume_addr(cluster, cpu, virt_to_phys(mcpm_entry_point)); > - gic_cpu_if_down(); > tc2_pm_down(residency); > } > > -- > 1.8.2.2 > >
On Fri, Sep 27, 2013 at 12:12:25PM -0400, Nicolas Pitre wrote: > Acked-by: Nicolas Pitre <nico@linaro.org> > > ARM-SOC dudes: would you please pick this up? I can't tell if this is actually needed for 3.12, since kexec isn't supported without the set of patches that Dave Martin just posted. Anyway, the fix is tiny so I applied it to the for-3.12 fixes branch anyway. -Olof
On Mon, Sep 30, 2013 at 09:29:04AM -0700, Olof Johansson wrote: > On Fri, Sep 27, 2013 at 12:12:25PM -0400, Nicolas Pitre wrote: > > Acked-by: Nicolas Pitre <nico@linaro.org> > > > > ARM-SOC dudes: would you please pick this up? > > I can't tell if this is actually needed for 3.12, since kexec isn't supported > without the set of patches that Dave Martin just posted. I believe some problems have been seen when mixing hotplug with idle, even if kexec is out of the picture. This was a known issue before I started playing with kexec. This should be considered a separate patch. Cheers ---Dave > > Anyway, the fix is tiny so I applied it to the for-3.12 fixes branch anyway. > > > -Olof > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c index 7aeb5d6..e6eb481 100644 --- a/arch/arm/mach-vexpress/tc2_pm.c +++ b/arch/arm/mach-vexpress/tc2_pm.c @@ -131,6 +131,16 @@ static void tc2_pm_down(u64 residency) } else BUG(); + /* + * If the CPU is committed to power down, make sure + * the power controller will be in charge of waking it + * up upon IRQ, ie IRQ lines are cut from GIC CPU IF + * to the CPU by disabling the GIC CPU IF to prevent wfi + * from completing execution behind power controller back + */ + if (!skip_wfi) + gic_cpu_if_down(); + if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) { arch_spin_unlock(&tc2_pm_lock); @@ -231,7 +241,6 @@ static void tc2_pm_suspend(u64 residency) cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0); cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1); ve_spc_set_resume_addr(cluster, cpu, virt_to_phys(mcpm_entry_point)); - gic_cpu_if_down(); tc2_pm_down(residency); }