Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

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

On Sunday 21 February 2010, Brian King wrote:
> On 02/21/2010 04:37 PM, Rafael J. Wysocki wrote:
> > On Sunday 21 February 2010, Brian King wrote:
> >> On 02/21/2010 04:27 PM, Rafael J. Wysocki wrote:
> >>> On Sunday 21 February 2010, Brian King wrote:
> >>>> On 02/21/2010 04:08 PM, Rafael J. Wysocki wrote:
> >>>>> I'm not a big fan of __attribute__ ((weak)), though.  While we already use that
> >>>>> in the suspend code, I'm not particularly comfortable with it.
> >>>>>
> >>>>> Have you considered any alternative approaches?
> >>>>
> >>>> I suppose another option would be to implement this similar to how
> >>>> arch_free_page and arch_alloc_page do. Something like this:
> >>>>
> >>>> #ifndef CONFIG_HAVE_ARCH_SUSPEND_CPUS
> >>>> static inline int arch_suspend_disable_nonboot_cpus(void)
> >>>> {
> >>>> 	return disable_nonboot_cpus();
> >>>> }
> >>>>
> >>>> static inline void arch_suspend_enable_nonboot_cpus(void)
> >>>> {
> >>>> 	return enable_nonboot_cpus()'
> >>>> }
> >>>> #else
> >>>> extern int arch_suspend_disable_nonboot_cpus(void);
> >>>> extern void arch_suspend_enable_nonboot_cpus(void);
> >>>> #endif
> >>>>
> >>>> I figured I would just be consistent with arch_suspend_disable_irqs /
> >>>> arch_suspend_enable_irqs.
> >>>
> >>> I just think that doing arch_suspend_[enable|disable]_irqs() this way was
> >>> a mistake.
> >>
> >> Do you prefer the example above? I can send an updated patch. If not,
> >> any other suggestions you might have as to the way you would like this
> >> done would be greatly appreciated.
> > 
> > disable_nonboot_cpus() is also called by kernel_power_off().  Is that fine
> > with your architecture?
> 
> Yes. We only need a different behavior for the suspend/resume path.

OK

> Here is an alternative implementation of the patch. My test machine is
> currently unavailable, so it is not yet been tested. How does this one look?

Well, I'd like to do that cleanly from the start.

Now, the problem is that PM_SLEEP_SMP selects HOTPLUG_CPU, because
that's necessary for the other architectures to make SMP suspend work, but it's
not necessary on your architecture.  Moreover, you don't need to compile
enable_nonboot_cpus() at all.

I'm not sure how to untangle it at the moment, but I think it should be
untangled.

Preferably, on architectures that need HOTPLUG_CPU for the SMP resume to work
PM_SLEEP_SMP should depend on it instead of selecting it, but on your
architectures they may be independent from each other.

Rafael