| Message ID | 1405617110-1136-1-git-send-email-romain.perier@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 17, 2014 at 05:11:50PM +0000, Romain Perier wrote: > +static DECLARE_COMPLETION(cpu_died); > + > +static int rockchip_cpu_kill(unsigned int cpu) > +{ > + if (!wait_for_completion_timeout(&cpu_died, msecs_to_jiffies(1000))) { > + pr_err("CPU%d: didn't die correctly\n", cpu); > + return 0; > + } Please explain the point of this cpu_died thing. I think you should read arch/arm/kernel/smp.c, searching for cpu_died in there.
Hi all, you're probably talking about __cpu_die at http://lxr.free-electrons.com/source/arch/arm/kernel/smp.c#L223 . The problem being that the thread below which executes cpu_die completes the completion before executing smp_ops.cpu_die. So smp_ops.cpu_kill might be executed before smp_ops.cpu_die (in some cases cache coherency would not be turned off). Note that almost all SoCs do the same thing. On 18/07/2014 00:14, Russell King - ARM Linux wrote: > On Thu, Jul 17, 2014 at 05:11:50PM +0000, Romain Perier wrote: >> +static DECLARE_COMPLETION(cpu_died); >> + >> +static int rockchip_cpu_kill(unsigned int cpu) >> +{ >> + if (!wait_for_completion_timeout(&cpu_died, msecs_to_jiffies(1000))) { >> + pr_err("CPU%d: didn't die correctly\n", cpu); >> + return 0; >> + } > Please explain the point of this cpu_died thing. > > I think you should read arch/arm/kernel/smp.c, searching for cpu_died > in there. >
On Fri, Jul 18, 2014 at 08:16:02AM +0200, Romain Perier wrote: > you're probably talking about __cpu_die at > http://lxr.free-electrons.com/source/arch/arm/kernel/smp.c#L223 . The > problem being that the thread below which executes cpu_die completes the > completion before executing smp_ops.cpu_die. So smp_ops.cpu_kill might > be executed before smp_ops.cpu_die (in some cases cache coherency would > not be turned off). Note that almost all SoCs do the same thing. Look at what's happening: CPU0 CPU1 wait_for_completion_timeout() idle_task_exit() flush_cache_louis() complete(&cpu_died); At this point, it is safe for CPU1 to be powered down. smp_ops.cpu_kill(cpu); flush_cache_louis(); smp_ops.cpu_die(cpu); If we include your code at that point, then the sequence in totality becomes: wait_for_completion_timeout() idle_task_exit() flush_cache_louis() complete(&cpu_died); --- rockchip_cpu_kill --- wait_for_completion_timeout() flush_cache_louis(); --- rockchip_cpu_die --- complete(&cpu_died); pmu_set_power_domain(0 + cpu, false); flush_cache_louis(); v7_exit_coherency_flush(louis); while(1) cpu_do_idle(); So, I repeat my question. What is the point of your additional wait in rockchip_cpu_kill() and complete in rockchip_cpu_die()?
Le 18/07/2014 10:29, Russell King - ARM Linux a écrit : > On Fri, Jul 18, 2014 at 08:16:02AM +0200, Romain Perier wrote: >> you're probably talking about __cpu_die at >> http://lxr.free-electrons.com/source/arch/arm/kernel/smp.c#L223 . The >> problem being that the thread below which executes cpu_die completes the >> completion before executing smp_ops.cpu_die. So smp_ops.cpu_kill might >> be executed before smp_ops.cpu_die (in some cases cache coherency would >> not be turned off). Note that almost all SoCs do the same thing. > Look at what's happening: > > CPU0 CPU1 > wait_for_completion_timeout() > idle_task_exit() > flush_cache_louis() > complete(&cpu_died); > > At this point, it is safe for CPU1 to be powered down. > > smp_ops.cpu_kill(cpu); > flush_cache_louis(); > smp_ops.cpu_die(cpu); > > If we include your code at that point, then the sequence in totality > becomes: > > wait_for_completion_timeout() > idle_task_exit() > flush_cache_louis() > complete(&cpu_died); > --- rockchip_cpu_kill --- > wait_for_completion_timeout() > flush_cache_louis(); > --- rockchip_cpu_die --- > complete(&cpu_died); > pmu_set_power_domain(0 + cpu, false); > flush_cache_louis(); > v7_exit_coherency_flush(louis); > while(1) > cpu_do_idle(); > > So, I repeat my question. What is the point of your additional wait > in rockchip_cpu_kill() and complete in rockchip_cpu_die()? > Mhhh... you're right... my bad ! So either smp_ops.cpu_kill(cpu) is executed before smp_ops.cpu_die and in this case it is safe to power down the cpu. or smp_ops.cpu_die(cpu) is executed before smp_ops.cpu_kill(due) and it is safe too. I will send a new patch (v2) Romain
diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c index 910835d..7c1299a 100644 --- a/arch/arm/mach-rockchip/platsmp.c +++ b/arch/arm/mach-rockchip/platsmp.c @@ -21,6 +21,7 @@ #include <linux/of_address.h> #include <asm/cacheflush.h> +#include <asm/cp15.h> #include <asm/smp_scu.h> #include <asm/smp_plat.h> #include <asm/mach/map.h> @@ -178,8 +179,38 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus) pmu_set_power_domain(0 + i, false); } +#ifdef CONFIG_HOTPLUG_CPU +static DECLARE_COMPLETION(cpu_died); + +static int rockchip_cpu_kill(unsigned int cpu) +{ + if (!wait_for_completion_timeout(&cpu_died, msecs_to_jiffies(1000))) { + pr_err("CPU%d: didn't die correctly\n", cpu); + return 0; + } + pmu_set_power_domain(0 + cpu, false); + return 1; +} + + +static void rockchip_cpu_die(unsigned int cpu) +{ + complete(&cpu_died); + flush_cache_louis(); + + v7_exit_coherency_flush(louis); + + while(1) + cpu_do_idle(); +} +#endif + static struct smp_operations rockchip_smp_ops __initdata = { .smp_prepare_cpus = rockchip_smp_prepare_cpus, .smp_boot_secondary = rockchip_boot_secondary, +#ifdef CONFIG_HOTPLUG_CPU + .cpu_kill = rockchip_cpu_kill, + .cpu_die = rockchip_cpu_die, +#endif }; CPU_METHOD_OF_DECLARE(rk3066_smp, "rockchip,rk3066-smp", &rockchip_smp_ops);
Adds ability to shutdown all CPUs except the first one (since it might be special for a lot of platforms). It is now possible to use kexec which requires such a feature. Signed-off-by: Romain Perier <romain.perier@gmail.com> --- arch/arm/mach-rockchip/platsmp.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)