On 01/21/20 17:03, Russell King - ARM Linux admin wrote:
On Mon, Nov 25, 2019 at 11:27:41AM +0000, Qais Yousef wrote:
quoted
+void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
+{
+ unsigned int cpu;
+
+ if (!cpu_online(primary_cpu)) {
+ pr_info("Attempting to shutdodwn nonboot cpus while boot cpu is offline!\n");
+ cpu_online(primary_cpu);
Eh, that should be cpu_up(primary_cpu)!
Which I have to say I'm not if is the right thing to do.
migrate_to_reboot_cpu() picks the first online cpu if reboot_cpu (assumed 0) is
offline
migrate_to_reboot_cpu():
225 /* Make certain the cpu I'm about to reboot on is online */
226 if (!cpu_online(cpu))
227 cpu = cpumask_first(cpu_online_mask);
quoted
+ }
+
+ for_each_present_cpu(cpu) {
+ if (cpu == primary_cpu)
+ continue;
+ if (cpu_online(cpu))
+ cpu_down(cpu);
+ }
How does this avoid racing with userspace attempting to restart CPUs
that have already been taken down by this function?
This is meant to be called from machine_shutdown() only.
But you've got a point.
The previous logic that used disable_nonboot_cpus(), which in turn called
freeze_secondary_cpus() didn't hold hotplug lock. So I assumed the higher level
logic of machine_shutdown() ensures that hotplug lock is held to synchronize
with potential other hotplug operations.
But I can see now that it doesn't.
With this series that migrates users to use device_{online,offline}, holding
the lock_device_hotplug() should protect against such races.
Worth noting that this an existing problem in the code and not something
I introduced, of course it makes sense to fix it properly as part of this
series.
I'm not sure how the other archs deal with this TBH.
Thanks for having a look!
Cheers
--
Qais Yousef
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel