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

["Rafael J. Wysocki" <rjw@sisk.pl>]

Hi Nathan,

On Friday 28 July 2006 20:20, Nathan Lynch wrote:
> 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?

Sure, why not.
 
> > +#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.

Absolutely.  Thanks for pointing this out.

> 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;

I'm not quite sure if we can finish with CPU0 offline.  Perhaps it's better to
check if CPU0 is online and bring it up if not and then continue or return
an error if that fails?

Rafael