Re: [PATCH -mm][resend] Disable CPU hotplug during suspend

Nathan Lynch
2006-07-28T18:21:17Z

Hi Rafael-

A couple of minor comments:

> +int cpu_down(unsigned int cpu)
> +{
> + int err = 0;
> +
> + mutex_lock(&cpu_add_remove_lock);
> + if (cpu_hotplug_disabled)
> + err = -EPERM;
> + else
> + err = __cpu_down(cpu);
> +
> mutex_unlock(&cpu_add_remove_lock);
> return err;
> }
> @@ -191,6 +203,11 @@ int __devinit cpu_up(unsigned int cpu)
> void *hcpu = (void *)(long)cpu;
>
> mutex_lock(&cpu_add_remove_lock);
> + if (cpu_hotplug_disabled) {
> + ret = -EPERM;
> + goto out;
> + }

I think -EBUSY would be more appropriate than -EPERM, perhaps?

> +#ifdef CONFIG_SUSPEND_SMP
> +static cpumask_t frozen_cpus;
> +
> +int disable_nonboot_cpus(void)
> +{
> + int cpu, error = 0;
> +
> + /* We take all of the non-boot CPUs down in one shot to avoid races
> + * with the userspace trying to use the CPU hotplug at the same time
> + */
> + mutex_lock(&cpu_add_remove_lock);
> + cpus_clear(frozen_cpus);
> + printk("Disabling non-boot CPUs ...\n");
> + for_each_online_cpu(cpu) {
> + if (cpu == 0)
> + continue;

Assuming cpu 0 is online is not okay in generic code. This should be
something like:

int cpu, first_cpu, error = 0;

/* We take all of the non-boot CPUs down in one shot to avoid races
* with the userspace trying to use the CPU hotplug at the same time
*/
mutex_lock(&cpu_add_remove_lock);
cpus_clear(frozen_cpus);
first_cpu = first_cpu(cpu_online_mask);
printk("Disabling non-boot CPUs ...\n");
for_each_online_cpu(cpu) {
if (cpu == first_cpu)
continue;

> + error = __cpu_down(cpu);
> + if (!error) {
> + cpu_set(cpu, frozen_cpus);
> + printk("CPU%d is down\n", cpu);
> + } else {
> + printk(KERN_ERR "Error taking CPU%d down: %d\n",
> + cpu, error);
> + break;
> + }
> + }
> + if (!error) {
> + BUG_ON(num_online_cpus() > 1);
> + BUG_ON(raw_smp_processor_id() != 0);

Same problem here.

Otherwise, I think the patch looks okay.