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

[Brian King <brking@linux.vnet.ibm.com>]

On 02/22/2010 01:14 PM, Rafael J. Wysocki wrote:
> 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.

At least for the architecture I am enabling this support for (PPC_PSERIES), upon looking closer, it looks like PM_SLEEP_SMP was never defined, so enable_nonboot_cpus and disable_nonboot_cpus were always nooped before, which I didn't previously realize. We probably want to retain this behavior.

So perhaps a better way to solve this might be to change the Kconfig rules so that PM_SLEEP_SMP is not defined for PPC_PSERIES and then use ->prepare_late to put the other CPUs in H_JOIN state and ->wake to send H_PROD to them to wake them up. In that case, I suppose a simple patch like the one below would suffice.


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 kernel/power/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff -puN kernel/power/Kconfig~suspend_no_pm_sleep_smp kernel/power/Kconfig
--- linux-2.6/kernel/power/Kconfig~suspend_no_pm_sleep_smp	2010-02-22 17:05:25.000000000 -0600
+++ linux-2.6-bjking1/kernel/power/Kconfig	2010-02-22 17:07:00.000000000 -0600
@@ -77,6 +77,7 @@ config PM_SLEEP_SMP
 	depends on SMP
 	depends on ARCH_SUSPEND_POSSIBLE || ARCH_HIBERNATION_POSSIBLE
 	depends on PM_SLEEP
+	depends on !PPC_PSERIES
 	select HOTPLUG_CPU
 	default y
 
_