Hi Rafael,

On 29/06/16 01:10, Rafael J. Wysocki wrote:

On Tuesday, June 28, 2016 03:51:48 PM James Morse wrote:

quoted

Architecture code may need to do extra work when secondary CPUs are
disabled during hibernate and resume. This may include pushing sleeping
CPUs into a deeper power-saving state, or influencing which CPU resume
occurs on.

Define a macro arch_hibernation_disable_cpus(), which defaults to calling
disable_nonboot_cpus() if undefined. Architectures that need to do extra
work around these calls can use this to influence disable_nonboot_cpus()
behaviour. The macro should be defined in asm/suspend.h, and
ARCH_HIBERNATION_CPUHP should be added to Kconfig.

As you noted, this could be used to address the x86 issue that Yu is working on,
so I'd like it to go in as the first patch in the series and through the PM tree.

Sure, I will split the series here, and group the later patches so there are no
dependencies.

quoted

---
Changes since v2:
 * Added CONFIG_ARCH_HIBERNATION_CPUHP include guard allowing

What about calling it CONFIG_ARCH_HIBERNATION_CPU_OFFLINE?  It's not
hotplug really.

Even with the enable calls added? CONFIG_ARCH_HIBERNATION_CPU_HOOKS?

quoted

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index fca9254280ee..338745e78f7e 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c

@@ -31,8 +31,16 @@
 #include <linux/ktime.h>
 #include <trace/events/power.h>
 
+#ifdef CONFIG_ARCH_HIBERNATION_CPUHP
+/* Arch definition of the arch_hibernation_disable_cpus() macro? */
+#include <asm/suspend.h>
+#endif
+
 #include "power.h"
 
+#ifndef arch_hibernation_disable_cpus
+#define arch_hibernation_disable_cpus(x) disable_nonboot_cpus()

For the x86 case we'll also need the complementary "enable", so why don't
you add it here and then use it instead of the enable_nonboot_cpus()?

Done. I've added the 'in_suspend' argument so that it's symmetrical, this may be
slightly different to the behaviour of your 'in_resume_hibernate' flag.

quoted

+#endif
 
 static int nocompress;
 static int noresume;

quoted

@@ -551,7 +559,7 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Platform_finish;
 
-	error = disable_nonboot_cpus();
+	error = arch_hibernation_disable_cpus(true);

Does this one here actually matter?

I added it for completeness/symmetry (but not the enable call because it
wouldn't have any users). I assume its something to do with acpi...



Thanks,

James