On 08/15/2016 12:06 PM, Corey Minyard wrote: > On 08/15/2016 06:35 AM, ???? / KAWAI?HIDEHIRO wrote: >> Hi Corey, >> >>> From: Corey Minyard [mailto:cminyard at mvista.com] >>> Sent: Friday, August 12, 2016 10:56 PM >>> I'll try to test this, but I have one comment inline... >> Thank you very much! >> >>> On 08/11/2016 10:17 PM, Dave Young wrote: >>>> On 08/10/16 at 05:09pm, Hidehiro Kawai wrote: >> [snip] >>>>> diff --git a/arch/mips/kernel/crash.c b/arch/mips/kernel/crash.c >>>>> index 610f0f3..1723b17 100644 >>>>> --- a/arch/mips/kernel/crash.c >>>>> +++ b/arch/mips/kernel/crash.c >>>>> @@ -47,9 +47,14 @@ static void crash_shutdown_secondary(void >>>>> *passed_regs) >>>>> >>>>> static void crash_kexec_prepare_cpus(void) >>>>> { >>>>> + static int cpus_stopped; >>>>> unsigned int msecs; >>>>> + unsigned int ncpus; >>>>> >>>>> - unsigned int ncpus = num_online_cpus() - 1;/* Excluding the >>>>> panic cpu */ >>>>> + if (cpus_stopped) >>>>> + return; >>> Wouldn't you want an atomic operation and some special handling here to >>> ensure that only one CPU does this? So if a CPU comes in here and >>> another CPU is already in the process stopping the CPUs it won't >>> result in a >>> deadlock. >> Because this function can be called only one panicking CPU, >> there is no problem. >> >> There are two paths which crash_kexec_prepare_cpus is called. >> >> Path 1 (panic path): >> panic() >> crash_smp_send_stop() >> crash_kexec_prepare_cpus() >> >> Path 2 (oops path): >> crash_kexec() >> __crash_kexec() >> machine_crash_shutdown() >> default_machine_crash_shutdown() // for MIPS >> crash_kexec_prepare_cpus() >> >> Here, panic() and crash_kexec() run exclusively via >> panic_cpu atomic variable. So we can use cpus_stopped as >> normal variable. > > Ok, if the code can only be entered once, what's the purpose of > cpus_stopped? > I guess that's what confused me. You are right, the panic_cpu atomic > should > keep this on a single CPU. Never mind, I see the path through panic() where that is required. My question below still remains, though. -corey > > Also, panic() will call panic_smp_self_stop() if it finds another CPU > has already > called panic, which will just spin with interrupts off by default. I > didn't see a > definition for it in MIPS, wouldn't it need to be overridden to avoid > a deadlock? > > -corey > >> >> Best regards, >> >> Hidehiro Kawai >> >
