On 06/07/2013 04:15 PM, Russell King - ARM Linux wrote: > On Fri, Jun 07, 2013 at 03:28:38PM -0600, Stephen Warren wrote: >> On 06/07/2013 12:56 PM, Stephen Warren wrote: >> ... >>> [1] Perhaps the issue is why ipi_send_stop() calls down into >>> tegra_cpu_kill() rather than tegra_cpu_die(), since die() is what >>> should be run on the killed CPU, and kill() on the killing CPU? >> >> Scratch that; I don't think it's calling down to /either/; I was >> confused. It seems like it /should/ call cpu_die() though, at least if >> hotplug is enabled, right? > > The problem is really complex. > > CPU hotplug is done in paths where we're relatively confident that the > system is working correctly. So all the features such as scheduling > are available, the timer ticks work and so forth. > > However, reboot is a totally different environment. This can happen > from almost any context with the system in any state what so ever. A > CPU could be stuck. A CPU could have oopsed. The CPU which is in > the reboot code could be the CPU which has oopsed. It could be called > from within an interrupt... > > What that means is the usual CPU hotplug methods can't be used in the > reboot path. Well, they can, but it will be fragile. > > For reboot, the real solution there is not to use software-based > reboot, but bring the other cores to a halt (which is what > ipi_send_stop is doing) and then issue a hardware reset to the whole > system, including the other CPUs. Ignoring the issues with oops in reboot, I think there's a bug in that when hotplug is enabled, smp_kill_cpus() calls platform_cpu_kill(), but nothing causes the failing CPU to ever execute smp_ops.cpu_die(). Hence, if the implementation of smp_ops.cpu_kill() relies on the target CPU having run smp_ops.cpu_die(), then smp_ops.cpu_kill() may not operate correctly. Or, must smp_ops.cpu_kill() not assume that smp_ops.cpu_die() will be called on the target CPU? What are the semantics here? Will mentioned that __cpu_die and cpu_die are a pair, but what about is the smp_ops are used directly; are they also supposed to be a pair? The change below solves the pairing issue, by making ipi_cpu_stop() perform the low-level part of hotplug that matches what smp_kill_cpus() call to platform_cpu_kill(). This certainly fixes the hang-in-reboot-or-shutdown problem on Tegra. > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c > index 550d63c..541f667 100644 > --- a/arch/arm/kernel/smp.c > +++ b/arch/arm/kernel/smp.c > @@ -581,11 +581,20 @@ static void ipi_cpu_stop(unsigned int cpu) > > set_cpu_online(cpu, false); > > +#ifdef CONFIG_HOTPLUG_CPU > +#if 0 > + arch_cpu_idle_dead(); > +#else > + /* The body of arch_cpu_idle_dead() - which is better? */ > + cpu_die(); > +#endif > +#else > local_fiq_disable(); > local_irq_disable(); > > while (1) > cpu_relax(); > +#endif > } Some things I'm not sure of here: * cpu_die() calls idle_task_exit(). That's probably wrong if it's triggered from an IPI; who knows what task it's executing. That said, if migrate_to_reboot_cpu() did set_cpus_allowed_ptr(current, cpumask_of(cpu)), perhaps that guarantees the CPU is running the idle task since there's nothing else that could be running? * ipi_cpu_stop() currently calls local_fiq_disable(), but cpu_die() doesn't. Should both functions call both local_fiq_disable() and local_irq_disable()? * Perhaps smp_kill_cpus() should also be changed, to call cpu_die() not platform_cpu_kill(), to keep the pairing correct at that level too. Plus, I ignored any issues you raised for the oops case on reboot... -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
