[PATCH v4 0/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[PATCH v4 0/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[James Morse]

Hi all,

This series is a break-up/continuation of [v3]. These patches allow arm64 to
hibernate on any CPU saving the cpu-id in the arch header, then switch to it
during resume by hooking disable_nonboot_cpus(). Today we forbid
hibernate if CPU0 has been offlined but this is futile if we booted via
kexec, as this may have occured with CPU0 offline. The first sign something
has gone wrong is a hang during resume.

With this series applied I can offline CPU0, kexec, hibernate and then
resume[3], the implementation is as discussed on the list[2].

Patch one should be usable for Chen Yu's hlt/mwait problem too [1].

This series is aimed at the PM tree, is based on v4.7-rc6, and can be
retrieved from:
git://linux-arm.org/linux-jm.git -b hibernate/cpuN/v4

Changes since v3:
 * Split series, at the PM/Hibernate patch, merged arm64 patches to remove
   dependencies.
 * Changed Kconfig symbol name.
 * Fixed logic error '<= 0' when testing for an unrecognised CPU.

Changes since v2:
 * Split wrong-CPU logic into an earlier patch that just returns an error.
 * Changed core code patch to use macros instead of a weak function.
   CONFIG_ARCH_HIBERNATION_HEADER now implies an asm/suspend.h header.
 * Wording in error messages 'hibernate' not 'suspend'.

Changes since v1:
 * Fixed 'Brining' typo.


[v1] http://www.spinics.net/lists/arm-kernel/msg507805.html
[v2] http://permalink.gmane.org/gmane.linux.power-management.general/77467
[v3] http://www.spinics.net/lists/arm-kernel/msg514644.html

[0] http://www.spinics.net/lists/arm-kernel/msg499305.html
[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1176775.html
[2] http://www.spinics.net/lists/arm-kernel/msg499036.html
[3] offline cpu0, kexec, hibernate, resume
-------------------------%<-------------------------
root@ubuntu:~# echo disk > /sys/power/state
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.007 seconds) done.
PM: Preallocating image memory... done (allocated 10112 pages)
PM: Allocated 647168 kbytes in 2.69 seconds (240.58 MB/s)
Freezing remaining freezable tasks ... (elapsed 0.005 seconds) done.
PM: freeze of devices complete after 27.013 msecs
PM: late freeze of devices complete after 20.624 msecs
PM: noirq freeze of devices complete after 22.211 msecs
Disabling non-boot CPUs ...
PM: Creating hibernation image:
PM: Need to copy 10103 pages
PM: Hibernation image created (10103 pages copied)
PM: noirq thaw of devices complete after 22.350 msecs
PM: early thaw of devices complete after 23.680 msecs
PM: thaw of devices complete after 98.331 msecs
hibernate: Suspending on cpu 0 [mpidr:0x103]
PM: Using 1 thread(s) for compression.
PM: Compressing and saving image data (10105 pages)...
PM: Image saving progress:   0%
atkbd serio0: keyboard reset failed on 1c060000.kmi
atkbd serio1: keyboard reset failed on 1c070000.kmi
PM: Image saving progress:  10%
PM: Image saving progress:  20%
PM: Image saving progress:  30%
PM: Image saving progress:  40%
PM: Image saving progress:  50%
PM: Image saving progress:  60%
PM: Image saving progress:  70%
PM: Image saving progress:  80%
PM: Image saving progress:  90%
PM: Image saving progress: 100%
PM: Image saving done.
PM: Wrote 646720 kbytes in 93.08 seconds (6.94 MB/s)
PM: S|
reboot: Power down

[ ... ]

Freezing user space processes ... (elapsed 0.000 seconds) done.
PM: Using 1 thread(s) for decompression.
PM: Loading and decompressing image data (10105 pages)...
hibernate: Suspended on cpu 5 [mpidr:0x103]
hibernate: Suspended on a CPU that is offline! Brining CPU up.
Detected VIPT I-cache on CPU5
CPU5: Booted secondary processor [410fd033]
random: nonblocking pool is initialized
PM: Image loading progress:   0%
PM: Image loading progress:  10%
PM: Image loading progress:  20%
PM: Image loading progress:  30%
PM: Image loading progress:  40%
PM: Image loading progress:  50%
PM: Image loading progress:  60%
PM: Image loading progress:  70%
PM: Image loading progress:  80%
PM: Image loading progress:  90%
PM: Image loading progress: 100%
PM: Image loading done.
PM: Read 646720 kbytes in 30.47 seconds (21.22 MB/s)
PM: quiesce of devices complete after 32.958 msecs
PM: late quiesce of devices complete after 11.574 msecs
PM: noirq quiesce of devices complete after 24.918 msecs
hibernate: Disabling secondary CPUs ...
IRQ1 no longer affine to CPU0
IRQ6 no longer affine to CPU0
IRQ28 no longer affine to CPU0
IRQ29 no longer affine to CPU0
IRQ32 no longer affine to CPU0
IRQ34 no longer affine to CPU0
IRQ35 no longer affine to CPU0
IRQ37 no longer affine to CPU0
IRQ41 no longer affine to CPU0
IRQ48 no longer affine to CPU0
CPU0: shutdown

psci: CPU0 killed.
PM: noirq restore of devices complete after 27.419 msecs
PM: early restore of devices complete after 23.554 msecs
PM: restore of devices complete after 113.188 msecs
Restarting tasks ... done.
root@ubuntu:~# atkbd serio0: keyboard reset failed on 1c060000.kmi
atkbd serio1: keyboard reset failed on 1c070000.kmi
root@ubuntu:~# cat /proc/cpuinfo
processor       : 0
BogoMIPS        : 100.00
Features        : fp asimd evtstrm aes pmull sha1 sha2 crc32
CPU implementer : 0x41
CPU architecture: 8
CPU variant     : 0x0
CPU part        : 0xd03
CPU revision    : 3

root@ubuntu:~#
-------------------------%<-------------------------

James Morse (3):
  PM / Hibernate: Allow arch code to influence CPUs disabled during
    hibernate
  arm64: hibernate: Resume when hibernate image created on non-boot CPU
  Revert "arm64: hibernate: Refuse to hibernate if the boot cpu is
    offline"

 arch/arm64/Kconfig               |  4 ++
 arch/arm64/include/asm/suspend.h |  4 ++
 arch/arm64/kernel/hibernate.c    | 86 ++++++++++++++++++++++++++++++----------
 kernel/power/hibernate.c         | 14 +++++--
 4 files changed, 84 insertions(+), 24 deletions(-)

-- 
2.8.0.rc3

[PATCH v4 0/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[James Morse]

Hi all,

This series is a break-up/continuation of [v3]. These patches allow arm64 to
hibernate on any CPU saving the cpu-id in the arch header, then switch to it
during resume by hooking disable_nonboot_cpus(). Today we forbid
hibernate if CPU0 has been offlined but this is futile if we booted via
kexec, as this may have occured with CPU0 offline. The first sign something
has gone wrong is a hang during resume.

With this series applied I can offline CPU0, kexec, hibernate and then
resume[3], the implementation is as discussed on the list[2].

Patch one should be usable for Chen Yu's hlt/mwait problem too [1].

This series is aimed at the PM tree, is based on v4.7-rc6, and can be
retrieved from:
git://linux-arm.org/linux-jm.git -b hibernate/cpuN/v4

Changes since v3:
 * Split series, at the PM/Hibernate patch, merged arm64 patches to remove
   dependencies.
 * Changed Kconfig symbol name.
 * Fixed logic error '<= 0' when testing for an unrecognised CPU.

Changes since v2:
 * Split wrong-CPU logic into an earlier patch that just returns an error.
 * Changed core code patch to use macros instead of a weak function.
   CONFIG_ARCH_HIBERNATION_HEADER now implies an asm/suspend.h header.
 * Wording in error messages 'hibernate' not 'suspend'.

Changes since v1:
 * Fixed 'Brining' typo.


[v1] http://www.spinics.net/lists/arm-kernel/msg507805.html
[v2] http://permalink.gmane.org/gmane.linux.power-management.general/77467
[v3] http://www.spinics.net/lists/arm-kernel/msg514644.html

[0] http://www.spinics.net/lists/arm-kernel/msg499305.html
[1] https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1176775.html
[2] http://www.spinics.net/lists/arm-kernel/msg499036.html
[3] offline cpu0, kexec, hibernate, resume
-------------------------%<-------------------------
root@ubuntu:~# echo disk > /sys/power/state
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.007 seconds) done.
PM: Preallocating image memory... done (allocated 10112 pages)
PM: Allocated 647168 kbytes in 2.69 seconds (240.58 MB/s)
Freezing remaining freezable tasks ... (elapsed 0.005 seconds) done.
PM: freeze of devices complete after 27.013 msecs
PM: late freeze of devices complete after 20.624 msecs
PM: noirq freeze of devices complete after 22.211 msecs
Disabling non-boot CPUs ...
PM: Creating hibernation image:
PM: Need to copy 10103 pages
PM: Hibernation image created (10103 pages copied)
PM: noirq thaw of devices complete after 22.350 msecs
PM: early thaw of devices complete after 23.680 msecs
PM: thaw of devices complete after 98.331 msecs
hibernate: Suspending on cpu 0 [mpidr:0x103]
PM: Using 1 thread(s) for compression.
PM: Compressing and saving image data (10105 pages)...
PM: Image saving progress:   0%
atkbd serio0: keyboard reset failed on 1c060000.kmi
atkbd serio1: keyboard reset failed on 1c070000.kmi
PM: Image saving progress:  10%
PM: Image saving progress:  20%
PM: Image saving progress:  30%
PM: Image saving progress:  40%
PM: Image saving progress:  50%
PM: Image saving progress:  60%
PM: Image saving progress:  70%
PM: Image saving progress:  80%
PM: Image saving progress:  90%
PM: Image saving progress: 100%
PM: Image saving done.
PM: Wrote 646720 kbytes in 93.08 seconds (6.94 MB/s)
PM: S|
reboot: Power down

[ ... ]

Freezing user space processes ... (elapsed 0.000 seconds) done.
PM: Using 1 thread(s) for decompression.
PM: Loading and decompressing image data (10105 pages)...
hibernate: Suspended on cpu 5 [mpidr:0x103]
hibernate: Suspended on a CPU that is offline! Brining CPU up.
Detected VIPT I-cache on CPU5
CPU5: Booted secondary processor [410fd033]
random: nonblocking pool is initialized
PM: Image loading progress:   0%
PM: Image loading progress:  10%
PM: Image loading progress:  20%
PM: Image loading progress:  30%
PM: Image loading progress:  40%
PM: Image loading progress:  50%
PM: Image loading progress:  60%
PM: Image loading progress:  70%
PM: Image loading progress:  80%
PM: Image loading progress:  90%
PM: Image loading progress: 100%
PM: Image loading done.
PM: Read 646720 kbytes in 30.47 seconds (21.22 MB/s)
PM: quiesce of devices complete after 32.958 msecs
PM: late quiesce of devices complete after 11.574 msecs
PM: noirq quiesce of devices complete after 24.918 msecs
hibernate: Disabling secondary CPUs ...
IRQ1 no longer affine to CPU0
IRQ6 no longer affine to CPU0
IRQ28 no longer affine to CPU0
IRQ29 no longer affine to CPU0
IRQ32 no longer affine to CPU0
IRQ34 no longer affine to CPU0
IRQ35 no longer affine to CPU0
IRQ37 no longer affine to CPU0
IRQ41 no longer affine to CPU0
IRQ48 no longer affine to CPU0
CPU0: shutdown

psci: CPU0 killed.
PM: noirq restore of devices complete after 27.419 msecs
PM: early restore of devices complete after 23.554 msecs
PM: restore of devices complete after 113.188 msecs
Restarting tasks ... done.
root at ubuntu:~# atkbd serio0: keyboard reset failed on 1c060000.kmi
atkbd serio1: keyboard reset failed on 1c070000.kmi
root at ubuntu:~# cat /proc/cpuinfo
processor       : 0
BogoMIPS        : 100.00
Features        : fp asimd evtstrm aes pmull sha1 sha2 crc32
CPU implementer : 0x41
CPU architecture: 8
CPU variant     : 0x0
CPU part        : 0xd03
CPU revision    : 3

root at ubuntu:~#
-------------------------%<-------------------------

James Morse (3):
  PM / Hibernate: Allow arch code to influence CPUs disabled during
    hibernate
  arm64: hibernate: Resume when hibernate image created on non-boot CPU
  Revert "arm64: hibernate: Refuse to hibernate if the boot cpu is
    offline"

 arch/arm64/Kconfig               |  4 ++
 arch/arm64/include/asm/suspend.h |  4 ++
 arch/arm64/kernel/hibernate.c    | 86 ++++++++++++++++++++++++++++++----------
 kernel/power/hibernate.c         | 14 +++++--
 4 files changed, 84 insertions(+), 24 deletions(-)

-- 
2.8.0.rc3

[PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[James Morse]

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
the CPU down calls.
The macros should be defined in asm/suspend.h, and
ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.

Signed-off-by: James Morse <james.morse@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
---
Changes since v3:
 * Changed kconfig name to CONFIG_ARCH_HIBERNATION_CPU_HOOKS

Changes since v2:
 * Added CONFIG_ARCH_HIBERNATION_CPUHP include guard allowing
 * Switch to macro approach.

 kernel/power/hibernate.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index fca9254280ee..855a3a2374c8 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_CPU_HOOKS
+/* Arch definition of the arch_hibernation_disable_cpus() macros? */
+#include <asm/suspend.h>
+#endif
+
 #include "power.h"
 
+#ifndef arch_hibernation_disable_cpus
+#define arch_hibernation_disable_cpus(x) disable_nonboot_cpus()
+#endif
 
 static int nocompress;
 static int noresume;
@@ -279,7 +287,7 @@ static int create_image(int platform_mode)
 	if (error || hibernation_test(TEST_PLATFORM))
 		goto Platform_finish;
 
-	error = disable_nonboot_cpus();
+	error = arch_hibernation_disable_cpus(true);
 	if (error || hibernation_test(TEST_CPUS))
 		goto Enable_cpus;
 
@@ -433,7 +441,7 @@ static int resume_target_kernel(bool platform_mode)
 	if (error)
 		goto Cleanup;
 
-	error = disable_nonboot_cpus();
+	error = arch_hibernation_disable_cpus(false);
 	if (error)
 		goto Enable_cpus;
 
@@ -551,7 +559,7 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Platform_finish;
 
-	error = disable_nonboot_cpus();
+	error = arch_hibernation_disable_cpus(true);
 	if (error)
 		goto Enable_cpus;
 
-- 
2.8.0.rc3

[PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[James Morse]

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
the CPU down calls.
The macros should be defined in asm/suspend.h, and
ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.

Signed-off-by: James Morse <james.morse@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
---
Changes since v3:
 * Changed kconfig name to CONFIG_ARCH_HIBERNATION_CPU_HOOKS

Changes since v2:
 * Added CONFIG_ARCH_HIBERNATION_CPUHP include guard allowing
 * Switch to macro approach.

 kernel/power/hibernate.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index fca9254280ee..855a3a2374c8 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_CPU_HOOKS
+/* Arch definition of the arch_hibernation_disable_cpus() macros? */
+#include <asm/suspend.h>
+#endif
+
 #include "power.h"
 
+#ifndef arch_hibernation_disable_cpus
+#define arch_hibernation_disable_cpus(x) disable_nonboot_cpus()
+#endif
 
 static int nocompress;
 static int noresume;
@@ -279,7 +287,7 @@ static int create_image(int platform_mode)
 	if (error || hibernation_test(TEST_PLATFORM))
 		goto Platform_finish;
 
-	error = disable_nonboot_cpus();
+	error = arch_hibernation_disable_cpus(true);
 	if (error || hibernation_test(TEST_CPUS))
 		goto Enable_cpus;
 
@@ -433,7 +441,7 @@ static int resume_target_kernel(bool platform_mode)
 	if (error)
 		goto Cleanup;
 
-	error = disable_nonboot_cpus();
+	error = arch_hibernation_disable_cpus(false);
 	if (error)
 		goto Enable_cpus;
 
@@ -551,7 +559,7 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Platform_finish;
 
-	error = disable_nonboot_cpus();
+	error = arch_hibernation_disable_cpus(true);
 	if (error)
 		goto Enable_cpus;
 
-- 
2.8.0.rc3

[PATCH v4 2/3] arm64: hibernate: Resume when hibernate image created on non-boot CPU

[James Morse]

On arm64 the cpu with logical id 0 is assumed to be the boot CPU. If a
user hotplugs this CPU out, then uses kexec to boot a new kernel, the new
kernel will assign logical id 0 to a different physical CPU.
This breaks hibernate as hibernate and resume will be attempted on
different CPUs.

We currently forbid hibernate if CPU0 has been hotplugged out to avoid
this situation without kexec.

Save the MPIDR of the CPU we hibernated on in the hibernate arch-header,
use arch_hibernation_disable_cpus() to direct which CPU we should resume
on based on the MPIDR of the CPU we hibernated on. This allows us to
hibernate/resume on any CPU, even if the logical numbers have been
shuffled by kexec.

Signed-off-by: James Morse <james.morse@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
This patch should be merged via the PM tree, (potentially with another user
of patch one)

Changes since v3:
 * Changed kconfig name to CONFIG_ARCH_HIBERNATION_CPU_HOOKS
 * Merged the split storing/reading/checking sleep_cpu code back into this patch

Changes since v2:
 * Storing/reading/checking sleep_cpu moved into an earlier patch
 * Moved to macro approach.
 * Added hidden ARCH_HIBERNATION_CPUHP config option.

 arch/arm64/Kconfig               |  4 +++
 arch/arm64/include/asm/suspend.h |  4 +++
 arch/arm64/kernel/hibernate.c    | 70 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5a0a691d4220..e8f2d560f97f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1007,6 +1007,10 @@ config ARCH_HIBERNATION_HEADER
 	def_bool y
 	depends on HIBERNATION
 
+config ARCH_HIBERNATION_CPU_HOOKS
+	def_bool y
+	depends on HIBERNATION
+
 config ARCH_SUSPEND_POSSIBLE
 	def_bool y
 
diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h
index 024d623f662e..9b3e8d9bfc8c 100644
--- a/arch/arm64/include/asm/suspend.h
+++ b/arch/arm64/include/asm/suspend.h
@@ -47,4 +47,8 @@ int swsusp_arch_resume(void);
 int arch_hibernation_header_save(void *addr, unsigned int max_size);
 int arch_hibernation_header_restore(void *addr);
 
+/* Used to resume on the CPU we hibernated on */
+int _arch_hibernation_disable_cpus(bool suspend);
+#define arch_hibernation_disable_cpus(x) _arch_hibernation_disable_cpus(x)
+
 #endif
diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index 21ab5df9fa76..bae45abde7a2 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c
@@ -15,6 +15,7 @@
  * License terms: GNU General Public License (GPL) version 2
  */
 #define pr_fmt(x) "hibernate: " x
+#include <linux/cpu.h>
 #include <linux/kvm_host.h>
 #include <linux/mm.h>
 #include <linux/notifier.h>
@@ -26,6 +27,7 @@
 
 #include <asm/barrier.h>
 #include <asm/cacheflush.h>
+#include <asm/cputype.h>
 #include <asm/irqflags.h>
 #include <asm/memory.h>
 #include <asm/mmu_context.h>
@@ -34,6 +36,7 @@
 #include <asm/pgtable-hwdef.h>
 #include <asm/sections.h>
 #include <asm/smp.h>
+#include <asm/smp_plat.h>
 #include <asm/suspend.h>
 #include <asm/virt.h>
 
@@ -66,6 +69,12 @@ extern char hibernate_el2_vectors[];
 extern char __hyp_stub_vectors[];
 
 /*
+ * The logical cpu number we should resume on, initialised to a non-cpu
+ * number.
+ */
+static int sleep_cpu = -EINVAL;
+
+/*
  * Values that may not change over hibernate/resume. We put the build number
  * and date in here so that we guarantee not to resume with a different
  * kernel.
@@ -87,6 +96,8 @@ static struct arch_hibernate_hdr {
 	 * re-configure el2.
 	 */
 	phys_addr_t	__hyp_stub_vectors;
+
+	u64		sleep_cpu_mpidr;
 } resume_hdr;
 
 static inline void arch_hdr_invariants(struct arch_hibernate_hdr_invariants *i)
@@ -129,12 +140,18 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
 	else
 		hdr->__hyp_stub_vectors = 0;
 
+	/* Save the mpidr of the cpu we called cpu_suspend() on... */
+	hdr->sleep_cpu_mpidr = cpu_logical_map(sleep_cpu);
+	pr_info("Hibernating on CPU %d [mpidr:0x%llx]\n", sleep_cpu,
+		hdr->sleep_cpu_mpidr);
+
 	return 0;
 }
 EXPORT_SYMBOL(arch_hibernation_header_save);
 
 int arch_hibernation_header_restore(void *addr)
 {
+	int ret;
 	struct arch_hibernate_hdr_invariants invariants;
 	struct arch_hibernate_hdr *hdr = addr;
 
@@ -144,6 +161,24 @@ int arch_hibernation_header_restore(void *addr)
 		return -EINVAL;
 	}
 
+	sleep_cpu = get_logical_index(hdr->sleep_cpu_mpidr);
+	pr_info("Hibernated on CPU %d [mpidr:0x%llx]\n", sleep_cpu,
+		hdr->sleep_cpu_mpidr);
+	if (sleep_cpu < 0) {
+		pr_crit("Hibernated on a CPU not known to this kernel!\n");
+		sleep_cpu = -EINVAL;
+		return -EINVAL;
+	}
+	if (!cpu_online(sleep_cpu)) {
+		pr_info("Hibernated on a CPU that is offline! Bringing CPU up.\n");
+		ret = cpu_up(sleep_cpu);
+		if (ret) {
+			pr_err("Failed to bring hibernate-CPU up!\n");
+			sleep_cpu = -EINVAL;
+			return ret;
+		}
+	}
+
 	resume_hdr = *hdr;
 
 	return 0;
@@ -245,6 +280,7 @@ int swsusp_arch_suspend(void)
 	local_dbg_save(flags);
 
 	if (__cpu_suspend_enter(&state)) {
+		sleep_cpu = smp_processor_id();
 		ret = swsusp_save();
 	} else {
 		/* Clean kernel to PoC for secondary core startup */
@@ -256,6 +292,7 @@ int swsusp_arch_suspend(void)
 		 */
 		in_suspend = 0;
 
+		sleep_cpu = -EINVAL;
 		__cpu_suspend_exit();
 	}
 
@@ -491,3 +528,36 @@ static int __init check_boot_cpu_online_init(void)
 	return 0;
 }
 core_initcall(check_boot_cpu_online_init);
+
+int _arch_hibernation_disable_cpus(bool suspend)
+{
+	int cpu, ret;
+
+	if (suspend) {
+		/*
+		 * During hibernate we need frozen_cpus to be updated and saved.
+		 */
+		ret = disable_nonboot_cpus();
+	} else {
+		/*
+		 * Resuming from hibernate. From here, we can't race with
+		 * userspace, and don't need to update frozen_cpus.
+		 */
+		pr_info("Disabling secondary CPUs ...\n");
+
+		/* sleep_cpu must have been loaded from the arch header */
+		BUG_ON(sleep_cpu < 0);
+
+		for_each_online_cpu(cpu) {
+			if (cpu == sleep_cpu)
+				continue;
+			ret = cpu_down(cpu);
+			if (ret) {
+				pr_err("Secondary CPUs are not disabled\n");
+				break;
+			}
+		}
+	}
+
+	return ret;
+}
-- 
2.8.0.rc3

[PATCH v4 2/3] arm64: hibernate: Resume when hibernate image created on non-boot CPU

[James Morse]

On arm64 the cpu with logical id 0 is assumed to be the boot CPU. If a
user hotplugs this CPU out, then uses kexec to boot a new kernel, the new
kernel will assign logical id 0 to a different physical CPU.
This breaks hibernate as hibernate and resume will be attempted on
different CPUs.

We currently forbid hibernate if CPU0 has been hotplugged out to avoid
this situation without kexec.

Save the MPIDR of the CPU we hibernated on in the hibernate arch-header,
use arch_hibernation_disable_cpus() to direct which CPU we should resume
on based on the MPIDR of the CPU we hibernated on. This allows us to
hibernate/resume on any CPU, even if the logical numbers have been
shuffled by kexec.

Signed-off-by: James Morse <james.morse@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
This patch should be merged via the PM tree, (potentially with another user
of patch one)

Changes since v3:
 * Changed kconfig name to CONFIG_ARCH_HIBERNATION_CPU_HOOKS
 * Merged the split storing/reading/checking sleep_cpu code back into this patch

Changes since v2:
 * Storing/reading/checking sleep_cpu moved into an earlier patch
 * Moved to macro approach.
 * Added hidden ARCH_HIBERNATION_CPUHP config option.

 arch/arm64/Kconfig               |  4 +++
 arch/arm64/include/asm/suspend.h |  4 +++
 arch/arm64/kernel/hibernate.c    | 70 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5a0a691d4220..e8f2d560f97f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1007,6 +1007,10 @@ config ARCH_HIBERNATION_HEADER
 	def_bool y
 	depends on HIBERNATION
 
+config ARCH_HIBERNATION_CPU_HOOKS
+	def_bool y
+	depends on HIBERNATION
+
 config ARCH_SUSPEND_POSSIBLE
 	def_bool y
 
diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h
index 024d623f662e..9b3e8d9bfc8c 100644
--- a/arch/arm64/include/asm/suspend.h
+++ b/arch/arm64/include/asm/suspend.h
@@ -47,4 +47,8 @@ int swsusp_arch_resume(void);
 int arch_hibernation_header_save(void *addr, unsigned int max_size);
 int arch_hibernation_header_restore(void *addr);
 
+/* Used to resume on the CPU we hibernated on */
+int _arch_hibernation_disable_cpus(bool suspend);
+#define arch_hibernation_disable_cpus(x) _arch_hibernation_disable_cpus(x)
+
 #endif
diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index 21ab5df9fa76..bae45abde7a2 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c
@@ -15,6 +15,7 @@
  * License terms: GNU General Public License (GPL) version 2
  */
 #define pr_fmt(x) "hibernate: " x
+#include <linux/cpu.h>
 #include <linux/kvm_host.h>
 #include <linux/mm.h>
 #include <linux/notifier.h>
@@ -26,6 +27,7 @@
 
 #include <asm/barrier.h>
 #include <asm/cacheflush.h>
+#include <asm/cputype.h>
 #include <asm/irqflags.h>
 #include <asm/memory.h>
 #include <asm/mmu_context.h>
@@ -34,6 +36,7 @@
 #include <asm/pgtable-hwdef.h>
 #include <asm/sections.h>
 #include <asm/smp.h>
+#include <asm/smp_plat.h>
 #include <asm/suspend.h>
 #include <asm/virt.h>
 
@@ -66,6 +69,12 @@ extern char hibernate_el2_vectors[];
 extern char __hyp_stub_vectors[];
 
 /*
+ * The logical cpu number we should resume on, initialised to a non-cpu
+ * number.
+ */
+static int sleep_cpu = -EINVAL;
+
+/*
  * Values that may not change over hibernate/resume. We put the build number
  * and date in here so that we guarantee not to resume with a different
  * kernel.
@@ -87,6 +96,8 @@ static struct arch_hibernate_hdr {
 	 * re-configure el2.
 	 */
 	phys_addr_t	__hyp_stub_vectors;
+
+	u64		sleep_cpu_mpidr;
 } resume_hdr;
 
 static inline void arch_hdr_invariants(struct arch_hibernate_hdr_invariants *i)
@@ -129,12 +140,18 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
 	else
 		hdr->__hyp_stub_vectors = 0;
 
+	/* Save the mpidr of the cpu we called cpu_suspend() on... */
+	hdr->sleep_cpu_mpidr = cpu_logical_map(sleep_cpu);
+	pr_info("Hibernating on CPU %d [mpidr:0x%llx]\n", sleep_cpu,
+		hdr->sleep_cpu_mpidr);
+
 	return 0;
 }
 EXPORT_SYMBOL(arch_hibernation_header_save);
 
 int arch_hibernation_header_restore(void *addr)
 {
+	int ret;
 	struct arch_hibernate_hdr_invariants invariants;
 	struct arch_hibernate_hdr *hdr = addr;
 
@@ -144,6 +161,24 @@ int arch_hibernation_header_restore(void *addr)
 		return -EINVAL;
 	}
 
+	sleep_cpu = get_logical_index(hdr->sleep_cpu_mpidr);
+	pr_info("Hibernated on CPU %d [mpidr:0x%llx]\n", sleep_cpu,
+		hdr->sleep_cpu_mpidr);
+	if (sleep_cpu < 0) {
+		pr_crit("Hibernated on a CPU not known to this kernel!\n");
+		sleep_cpu = -EINVAL;
+		return -EINVAL;
+	}
+	if (!cpu_online(sleep_cpu)) {
+		pr_info("Hibernated on a CPU that is offline! Bringing CPU up.\n");
+		ret = cpu_up(sleep_cpu);
+		if (ret) {
+			pr_err("Failed to bring hibernate-CPU up!\n");
+			sleep_cpu = -EINVAL;
+			return ret;
+		}
+	}
+
 	resume_hdr = *hdr;
 
 	return 0;
@@ -245,6 +280,7 @@ int swsusp_arch_suspend(void)
 	local_dbg_save(flags);
 
 	if (__cpu_suspend_enter(&state)) {
+		sleep_cpu = smp_processor_id();
 		ret = swsusp_save();
 	} else {
 		/* Clean kernel to PoC for secondary core startup */
@@ -256,6 +292,7 @@ int swsusp_arch_suspend(void)
 		 */
 		in_suspend = 0;
 
+		sleep_cpu = -EINVAL;
 		__cpu_suspend_exit();
 	}
 
@@ -491,3 +528,36 @@ static int __init check_boot_cpu_online_init(void)
 	return 0;
 }
 core_initcall(check_boot_cpu_online_init);
+
+int _arch_hibernation_disable_cpus(bool suspend)
+{
+	int cpu, ret;
+
+	if (suspend) {
+		/*
+		 * During hibernate we need frozen_cpus to be updated and saved.
+		 */
+		ret = disable_nonboot_cpus();
+	} else {
+		/*
+		 * Resuming from hibernate. From here, we can't race with
+		 * userspace, and don't need to update frozen_cpus.
+		 */
+		pr_info("Disabling secondary CPUs ...\n");
+
+		/* sleep_cpu must have been loaded from the arch header */
+		BUG_ON(sleep_cpu < 0);
+
+		for_each_online_cpu(cpu) {
+			if (cpu == sleep_cpu)
+				continue;
+			ret = cpu_down(cpu);
+			if (ret) {
+				pr_err("Secondary CPUs are not disabled\n");
+				break;
+			}
+		}
+	}
+
+	return ret;
+}
-- 
2.8.0.rc3

[PATCH v4 3/3] Revert "arm64: hibernate: Refuse to hibernate if the boot cpu is offline"

[James Morse]

Now that we use the MPIDR to resume on the same CPU that we hibernated on,
we no longer need to refuse to hibernate if the boot cpu is offline. (Which
we can't possibly know if kexec causes logical CPUs to be renumbered).

This reverts commit 1fe492ce6482b77807b25d29690a48c46456beee.

Signed-off-by: James Morse <james.morse@arm.com>
---
This patch should be merged via the PM tree, (potentially with another user
of patch one)

 arch/arm64/kernel/hibernate.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index bae45abde7a2..ea1acf323b61 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c
@@ -18,7 +18,6 @@
 #include <linux/cpu.h>
 #include <linux/kvm_host.h>
 #include <linux/mm.h>
-#include <linux/notifier.h>
 #include <linux/pm.h>
 #include <linux/sched.h>
 #include <linux/suspend.h>
@@ -504,31 +503,6 @@ out:
 	return rc;
 }
 
-static int check_boot_cpu_online_pm_callback(struct notifier_block *nb,
-					     unsigned long action, void *ptr)
-{
-	if (action == PM_HIBERNATION_PREPARE &&
-	     cpumask_first(cpu_online_mask) != 0) {
-		pr_warn("CPU0 is offline.\n");
-		return notifier_from_errno(-ENODEV);
-	}
-
-	return NOTIFY_OK;
-}
-
-static int __init check_boot_cpu_online_init(void)
-{
-	/*
-	 * Set this pm_notifier callback with a lower priority than
-	 * cpu_hotplug_pm_callback, so that cpu_hotplug_pm_callback will be
-	 * called earlier to disable cpu hotplug before the cpu online check.
-	 */
-	pm_notifier(check_boot_cpu_online_pm_callback, -INT_MAX);
-
-	return 0;
-}
-core_initcall(check_boot_cpu_online_init);
-
 int _arch_hibernation_disable_cpus(bool suspend)
 {
 	int cpu, ret;
-- 
2.8.0.rc3

[PATCH v4 3/3] Revert "arm64: hibernate: Refuse to hibernate if the boot cpu is offline"

[James Morse]

Now that we use the MPIDR to resume on the same CPU that we hibernated on,
we no longer need to refuse to hibernate if the boot cpu is offline. (Which
we can't possibly know if kexec causes logical CPUs to be renumbered).

This reverts commit 1fe492ce6482b77807b25d29690a48c46456beee.

Signed-off-by: James Morse <james.morse@arm.com>
---
This patch should be merged via the PM tree, (potentially with another user
of patch one)

 arch/arm64/kernel/hibernate.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index bae45abde7a2..ea1acf323b61 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c
@@ -18,7 +18,6 @@
 #include <linux/cpu.h>
 #include <linux/kvm_host.h>
 #include <linux/mm.h>
-#include <linux/notifier.h>
 #include <linux/pm.h>
 #include <linux/sched.h>
 #include <linux/suspend.h>
@@ -504,31 +503,6 @@ out:
 	return rc;
 }
 
-static int check_boot_cpu_online_pm_callback(struct notifier_block *nb,
-					     unsigned long action, void *ptr)
-{
-	if (action == PM_HIBERNATION_PREPARE &&
-	     cpumask_first(cpu_online_mask) != 0) {
-		pr_warn("CPU0 is offline.\n");
-		return notifier_from_errno(-ENODEV);
-	}
-
-	return NOTIFY_OK;
-}
-
-static int __init check_boot_cpu_online_init(void)
-{
-	/*
-	 * Set this pm_notifier callback with a lower priority than
-	 * cpu_hotplug_pm_callback, so that cpu_hotplug_pm_callback will be
-	 * called earlier to disable cpu hotplug before the cpu online check.
-	 */
-	pm_notifier(check_boot_cpu_online_pm_callback, -INT_MAX);
-
-	return 0;
-}
-core_initcall(check_boot_cpu_online_init);
-
 int _arch_hibernation_disable_cpus(bool suspend)
 {
 	int cpu, ret;
-- 
2.8.0.rc3

Re: [PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[Rafael J. Wysocki]

On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
> 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
> the CPU down calls.
> The macros should be defined in asm/suspend.h, and
> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Pavel Machek <pavel@ucw.cz>

I'm going to apply this one later today.

If you want me to apply the other two as well, they need to be ACKed by the
ARM64 maintainers.

Thanks,
Rafael

[PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[Rafael J. Wysocki]

On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
> 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
> the CPU down calls.
> The macros should be defined in asm/suspend.h, and
> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Pavel Machek <pavel@ucw.cz>

I'm going to apply this one later today.

If you want me to apply the other two as well, they need to be ACKed by the
ARM64 maintainers.

Thanks,
Rafael

Re: [PATCH v4 2/3] arm64: hibernate: Resume when hibernate image created on non-boot CPU

[Catalin Marinas]

On Mon, Jul 04, 2016 at 03:52:29PM +0100, James Morse wrote:
> diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h
> index 024d623f662e..9b3e8d9bfc8c 100644
> --- a/arch/arm64/include/asm/suspend.h
> +++ b/arch/arm64/include/asm/suspend.h
> @@ -47,4 +47,8 @@ int swsusp_arch_resume(void);
>  int arch_hibernation_header_save(void *addr, unsigned int max_size);
>  int arch_hibernation_header_restore(void *addr);
>  
> +/* Used to resume on the CPU we hibernated on */
> +int _arch_hibernation_disable_cpus(bool suspend);
> +#define arch_hibernation_disable_cpus(x) _arch_hibernation_disable_cpus(x)

Nitpick: we normally tend to use the same name for the function an macro
but it's fine like this as well:

+int arch_hibernation_disable_cpus(bool suspend);
+#define arch_hibernation_disable_cpus arch_hibernation_disable_cpus

BTW, do you expect an architecture to define ARCH_HIBERNATION_CPU_HOOKS
but not arch_hibernation_disable_cpus? Or you'd expect more hooks in the
future?

> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
> index 21ab5df9fa76..bae45abde7a2 100644
> --- a/arch/arm64/kernel/hibernate.c
> +++ b/arch/arm64/kernel/hibernate.c
> @@ -15,6 +15,7 @@
>   * License terms: GNU General Public License (GPL) version 2
>   */
>  #define pr_fmt(x) "hibernate: " x
> +#include <linux/cpu.h>
>  #include <linux/kvm_host.h>
>  #include <linux/mm.h>
>  #include <linux/notifier.h>
> @@ -26,6 +27,7 @@
>  
>  #include <asm/barrier.h>
>  #include <asm/cacheflush.h>
> +#include <asm/cputype.h>
>  #include <asm/irqflags.h>
>  #include <asm/memory.h>
>  #include <asm/mmu_context.h>
> @@ -34,6 +36,7 @@
>  #include <asm/pgtable-hwdef.h>
>  #include <asm/sections.h>
>  #include <asm/smp.h>
> +#include <asm/smp_plat.h>
>  #include <asm/suspend.h>
>  #include <asm/virt.h>
>  
> @@ -66,6 +69,12 @@ extern char hibernate_el2_vectors[];
>  extern char __hyp_stub_vectors[];
>  
>  /*
> + * The logical cpu number we should resume on, initialised to a non-cpu
> + * number.
> + */
> +static int sleep_cpu = -EINVAL;
> +
> +/*
>   * Values that may not change over hibernate/resume. We put the build number
>   * and date in here so that we guarantee not to resume with a different
>   * kernel.
> @@ -87,6 +96,8 @@ static struct arch_hibernate_hdr {
>  	 * re-configure el2.
>  	 */
>  	phys_addr_t	__hyp_stub_vectors;
> +
> +	u64		sleep_cpu_mpidr;
>  } resume_hdr;
>  
>  static inline void arch_hdr_invariants(struct arch_hibernate_hdr_invariants *i)
> @@ -129,12 +140,18 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
>  	else
>  		hdr->__hyp_stub_vectors = 0;
>  
> +	/* Save the mpidr of the cpu we called cpu_suspend() on... */
> +	hdr->sleep_cpu_mpidr = cpu_logical_map(sleep_cpu);
> +	pr_info("Hibernating on CPU %d [mpidr:0x%llx]\n", sleep_cpu,
> +		hdr->sleep_cpu_mpidr);

Do we have a guarantee that sleep_cpu != -EINVAL here?

If the above is fine, feel free to add:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH v4 2/3] arm64: hibernate: Resume when hibernate image created on non-boot CPU

[Catalin Marinas]

On Mon, Jul 04, 2016 at 03:52:29PM +0100, James Morse wrote:
> diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h
> index 024d623f662e..9b3e8d9bfc8c 100644
> --- a/arch/arm64/include/asm/suspend.h
> +++ b/arch/arm64/include/asm/suspend.h
> @@ -47,4 +47,8 @@ int swsusp_arch_resume(void);
>  int arch_hibernation_header_save(void *addr, unsigned int max_size);
>  int arch_hibernation_header_restore(void *addr);
>  
> +/* Used to resume on the CPU we hibernated on */
> +int _arch_hibernation_disable_cpus(bool suspend);
> +#define arch_hibernation_disable_cpus(x) _arch_hibernation_disable_cpus(x)

Nitpick: we normally tend to use the same name for the function an macro
but it's fine like this as well:

+int arch_hibernation_disable_cpus(bool suspend);
+#define arch_hibernation_disable_cpus arch_hibernation_disable_cpus

BTW, do you expect an architecture to define ARCH_HIBERNATION_CPU_HOOKS
but not arch_hibernation_disable_cpus? Or you'd expect more hooks in the
future?

> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
> index 21ab5df9fa76..bae45abde7a2 100644
> --- a/arch/arm64/kernel/hibernate.c
> +++ b/arch/arm64/kernel/hibernate.c
> @@ -15,6 +15,7 @@
>   * License terms: GNU General Public License (GPL) version 2
>   */
>  #define pr_fmt(x) "hibernate: " x
> +#include <linux/cpu.h>
>  #include <linux/kvm_host.h>
>  #include <linux/mm.h>
>  #include <linux/notifier.h>
> @@ -26,6 +27,7 @@
>  
>  #include <asm/barrier.h>
>  #include <asm/cacheflush.h>
> +#include <asm/cputype.h>
>  #include <asm/irqflags.h>
>  #include <asm/memory.h>
>  #include <asm/mmu_context.h>
> @@ -34,6 +36,7 @@
>  #include <asm/pgtable-hwdef.h>
>  #include <asm/sections.h>
>  #include <asm/smp.h>
> +#include <asm/smp_plat.h>
>  #include <asm/suspend.h>
>  #include <asm/virt.h>
>  
> @@ -66,6 +69,12 @@ extern char hibernate_el2_vectors[];
>  extern char __hyp_stub_vectors[];
>  
>  /*
> + * The logical cpu number we should resume on, initialised to a non-cpu
> + * number.
> + */
> +static int sleep_cpu = -EINVAL;
> +
> +/*
>   * Values that may not change over hibernate/resume. We put the build number
>   * and date in here so that we guarantee not to resume with a different
>   * kernel.
> @@ -87,6 +96,8 @@ static struct arch_hibernate_hdr {
>  	 * re-configure el2.
>  	 */
>  	phys_addr_t	__hyp_stub_vectors;
> +
> +	u64		sleep_cpu_mpidr;
>  } resume_hdr;
>  
>  static inline void arch_hdr_invariants(struct arch_hibernate_hdr_invariants *i)
> @@ -129,12 +140,18 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
>  	else
>  		hdr->__hyp_stub_vectors = 0;
>  
> +	/* Save the mpidr of the cpu we called cpu_suspend() on... */
> +	hdr->sleep_cpu_mpidr = cpu_logical_map(sleep_cpu);
> +	pr_info("Hibernating on CPU %d [mpidr:0x%llx]\n", sleep_cpu,
> +		hdr->sleep_cpu_mpidr);

Do we have a guarantee that sleep_cpu != -EINVAL here?

If the above is fine, feel free to add:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v4 3/3] Revert "arm64: hibernate: Refuse to hibernate if the boot cpu is offline"

[Catalin Marinas]

On Mon, Jul 04, 2016 at 03:52:30PM +0100, James Morse wrote:
> Now that we use the MPIDR to resume on the same CPU that we hibernated on,
> we no longer need to refuse to hibernate if the boot cpu is offline. (Which
> we can't possibly know if kexec causes logical CPUs to be renumbered).
> 
> This reverts commit 1fe492ce6482b77807b25d29690a48c46456beee.
> 
> Signed-off-by: James Morse <james.morse@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH v4 3/3] Revert "arm64: hibernate: Refuse to hibernate if the boot cpu is offline"

[Catalin Marinas]

On Mon, Jul 04, 2016 at 03:52:30PM +0100, James Morse wrote:
> Now that we use the MPIDR to resume on the same CPU that we hibernated on,
> we no longer need to refuse to hibernate if the boot cpu is offline. (Which
> we can't possibly know if kexec causes logical CPUs to be renumbered).
> 
> This reverts commit 1fe492ce6482b77807b25d29690a48c46456beee.
> 
> Signed-off-by: James Morse <james.morse@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[Rafael J. Wysocki]

On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
> 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
> the CPU down calls.
> The macros should be defined in asm/suspend.h, and
> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Pavel Machek <pavel@ucw.cz>
> ---
> Changes since v3:
>  * Changed kconfig name to CONFIG_ARCH_HIBERNATION_CPU_HOOKS
> 
> Changes since v2:
>  * Added CONFIG_ARCH_HIBERNATION_CPUHP include guard allowing
>  * Switch to macro approach.
> 
>  kernel/power/hibernate.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index fca9254280ee..855a3a2374c8 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_CPU_HOOKS
> +/* Arch definition of the arch_hibernation_disable_cpus() macros? */
> +#include <asm/suspend.h>
> +#endif
> +
>  #include "power.h"
>  
> +#ifndef arch_hibernation_disable_cpus
> +#define arch_hibernation_disable_cpus(x) disable_nonboot_cpus()
> +#endif
>  
>  static int nocompress;
>  static int noresume;
> @@ -279,7 +287,7 @@ static int create_image(int platform_mode)
>  	if (error || hibernation_test(TEST_PLATFORM))
>  		goto Platform_finish;
>  
> -	error = disable_nonboot_cpus();
> +	error = arch_hibernation_disable_cpus(true);
>  	if (error || hibernation_test(TEST_CPUS))
>  		goto Enable_cpus;
>  
> @@ -433,7 +441,7 @@ static int resume_target_kernel(bool platform_mode)
>  	if (error)
>  		goto Cleanup;
>  
> -	error = disable_nonboot_cpus();
> +	error = arch_hibernation_disable_cpus(false);

Why "false"?

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

I have the same question about this hunk I had before.

Is it really necessary to do the arch thing here?

It shouldn't really matter AFAICS.

>  	if (error)
>  		goto Enable_cpus;

Thanks,
Rafael

[PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[Rafael J. Wysocki]

On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
> 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
> the CPU down calls.
> The macros should be defined in asm/suspend.h, and
> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Pavel Machek <pavel@ucw.cz>
> ---
> Changes since v3:
>  * Changed kconfig name to CONFIG_ARCH_HIBERNATION_CPU_HOOKS
> 
> Changes since v2:
>  * Added CONFIG_ARCH_HIBERNATION_CPUHP include guard allowing
>  * Switch to macro approach.
> 
>  kernel/power/hibernate.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index fca9254280ee..855a3a2374c8 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_CPU_HOOKS
> +/* Arch definition of the arch_hibernation_disable_cpus() macros? */
> +#include <asm/suspend.h>
> +#endif
> +
>  #include "power.h"
>  
> +#ifndef arch_hibernation_disable_cpus
> +#define arch_hibernation_disable_cpus(x) disable_nonboot_cpus()
> +#endif
>  
>  static int nocompress;
>  static int noresume;
> @@ -279,7 +287,7 @@ static int create_image(int platform_mode)
>  	if (error || hibernation_test(TEST_PLATFORM))
>  		goto Platform_finish;
>  
> -	error = disable_nonboot_cpus();
> +	error = arch_hibernation_disable_cpus(true);
>  	if (error || hibernation_test(TEST_CPUS))
>  		goto Enable_cpus;
>  
> @@ -433,7 +441,7 @@ static int resume_target_kernel(bool platform_mode)
>  	if (error)
>  		goto Cleanup;
>  
> -	error = disable_nonboot_cpus();
> +	error = arch_hibernation_disable_cpus(false);

Why "false"?

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

I have the same question about this hunk I had before.

Is it really necessary to do the arch thing here?

It shouldn't really matter AFAICS.

>  	if (error)
>  		goto Enable_cpus;

Thanks,
Rafael

Re: [PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[James Morse]

Hi Rafael,

On 05/07/16 13:28, Rafael J. Wysocki wrote:
> On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
>> 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
>> the CPU down calls.
>> The macros should be defined in asm/suspend.h, and
>> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.
>>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Pavel Machek <pavel@ucw.cz>
> 
> I'm going to apply this one later today.
> 
> If you want me to apply the other two as well, they need to be ACKed by the
> ARM64 maintainers.

Thanks. I'd like to get to the bottom of Lorenzo's comments[0] before the arm64
patches go any further though!

Thanks,


James


[0] http://www.spinics.net/lists/arm-kernel/msg516244.html

[PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[James Morse]

Hi Rafael,

On 05/07/16 13:28, Rafael J. Wysocki wrote:
> On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
>> 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
>> the CPU down calls.
>> The macros should be defined in asm/suspend.h, and
>> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.
>>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Pavel Machek <pavel@ucw.cz>
> 
> I'm going to apply this one later today.
> 
> If you want me to apply the other two as well, they need to be ACKed by the
> ARM64 maintainers.

Thanks. I'd like to get to the bottom of Lorenzo's comments[0] before the arm64
patches go any further though!

Thanks,


James


[0] http://www.spinics.net/lists/arm-kernel/msg516244.html

Re: [PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[Rafael J. Wysocki]

On Wednesday, July 06, 2016 10:16:15 AM James Morse wrote:
> Hi Rafael,
> 
> On 05/07/16 13:28, Rafael J. Wysocki wrote:
> > On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
> >> 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
> >> the CPU down calls.
> >> The macros should be defined in asm/suspend.h, and
> >> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.
> >>
> >> Signed-off-by: James Morse <james.morse@arm.com>
> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> >> Cc: Pavel Machek <pavel@ucw.cz>
> > 
> > I'm going to apply this one later today.
> > 
> > If you want me to apply the other two as well, they need to be ACKed by the
> > ARM64 maintainers.
> 
> Thanks. I'd like to get to the bottom of Lorenzo's comments[0] before the arm64
> patches go any further though!

OK

Can you please answer my questions regarding patch [1/3] in the
meantime (posted separately)?

Thanks,
Rafael

[PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[Rafael J. Wysocki]

On Wednesday, July 06, 2016 10:16:15 AM James Morse wrote:
> Hi Rafael,
> 
> On 05/07/16 13:28, Rafael J. Wysocki wrote:
> > On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
> >> 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
> >> the CPU down calls.
> >> The macros should be defined in asm/suspend.h, and
> >> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.
> >>
> >> Signed-off-by: James Morse <james.morse@arm.com>
> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> >> Cc: Pavel Machek <pavel@ucw.cz>
> > 
> > I'm going to apply this one later today.
> > 
> > If you want me to apply the other two as well, they need to be ACKed by the
> > ARM64 maintainers.
> 
> Thanks. I'd like to get to the bottom of Lorenzo's comments[0] before the arm64
> patches go any further though!

OK

Can you please answer my questions regarding patch [1/3] in the
meantime (posted separately)?

Thanks,
Rafael

Re: [PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[James Morse]

On 06/07/16 01:38, Rafael J. Wysocki wrote:
> On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
>> 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
>> the CPU down calls.
>> The macros should be defined in asm/suspend.h, and
>> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.

>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>> index fca9254280ee..855a3a2374c8 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_CPU_HOOKS
>> +/* Arch definition of the arch_hibernation_disable_cpus() macros? */
>> +#include <asm/suspend.h>
>> +#endif
>> +
>>  #include "power.h"
>>  
>> +#ifndef arch_hibernation_disable_cpus
>> +#define arch_hibernation_disable_cpus(x) disable_nonboot_cpus()
>> +#endif
>>  
>>  static int nocompress;
>>  static int noresume;
>> @@ -279,7 +287,7 @@ static int create_image(int platform_mode)
>>  	if (error || hibernation_test(TEST_PLATFORM))
>>  		goto Platform_finish;
>>  
>> -	error = disable_nonboot_cpus();
>> +	error = arch_hibernation_disable_cpus(true);
>>  	if (error || hibernation_test(TEST_CPUS))
>>  		goto Enable_cpus;
>>  
>> @@ -433,7 +441,7 @@ static int resume_target_kernel(bool platform_mode)
>>  	if (error)
>>  		goto Cleanup;
>>  
>> -	error = disable_nonboot_cpus();
>> +	error = arch_hibernation_disable_cpus(false);
> 
> Why "false"?

To indicate whether this is suspend or resume. On suspend we just call
disable_nonboot_cpus(), this ensures frozen_cpus and the potential races with
userspace are covered properly. At this point we don't care which CPU it picks.

On resume we know which CPU we want, so cpu_down() all the others. I thought the
frozen_cpus and user-space race wouldn't be a problem here, but Lorenzo
suggested it may confuse some device drivers to receive a CPU_DOWN_PREPARE etc
followed by CPU_UP_PREPARE_FROZEN etc.

I haven't found any drivers in the tree where this would be a problem (~95% of
notifiers either mask out the frozen bits, or fall-through in those cases). But
I'm still going through the list...


> 
>>  	if (error)
>>  		goto Enable_cpus;
>>  
>> @@ -551,7 +559,7 @@ int hibernation_platform_enter(void)
>>  	if (error)
>>  		goto Platform_finish;
>>  
>> -	error = disable_nonboot_cpus();
>> +	error = arch_hibernation_disable_cpus(true);
> 
> I have the same question about this hunk I had before.
> 
> Is it really necessary to do the arch thing here?

Ah, sorry I didn't understand what this did before. This is used when ACPI
drives hibernate/resume instead of swsusp_arch_suspend().

No, its not needed.


> It shouldn't really matter AFAICS.
> 
>>  	if (error)
>>  		goto Enable_cpus;

Thanks,

James

[PATCH v4 1/3] PM / Hibernate: Allow arch code to influence CPUs disabled during hibernate

[James Morse]

On 06/07/16 01:38, Rafael J. Wysocki wrote:
> On Monday, July 04, 2016 03:52:28 PM James Morse wrote:
>> 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
>> the CPU down calls.
>> The macros should be defined in asm/suspend.h, and
>> ARCH_HIBERNATION_CPU_HOOKS should be added to Kconfig.

>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>> index fca9254280ee..855a3a2374c8 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_CPU_HOOKS
>> +/* Arch definition of the arch_hibernation_disable_cpus() macros? */
>> +#include <asm/suspend.h>
>> +#endif
>> +
>>  #include "power.h"
>>  
>> +#ifndef arch_hibernation_disable_cpus
>> +#define arch_hibernation_disable_cpus(x) disable_nonboot_cpus()
>> +#endif
>>  
>>  static int nocompress;
>>  static int noresume;
>> @@ -279,7 +287,7 @@ static int create_image(int platform_mode)
>>  	if (error || hibernation_test(TEST_PLATFORM))
>>  		goto Platform_finish;
>>  
>> -	error = disable_nonboot_cpus();
>> +	error = arch_hibernation_disable_cpus(true);
>>  	if (error || hibernation_test(TEST_CPUS))
>>  		goto Enable_cpus;
>>  
>> @@ -433,7 +441,7 @@ static int resume_target_kernel(bool platform_mode)
>>  	if (error)
>>  		goto Cleanup;
>>  
>> -	error = disable_nonboot_cpus();
>> +	error = arch_hibernation_disable_cpus(false);
> 
> Why "false"?

To indicate whether this is suspend or resume. On suspend we just call
disable_nonboot_cpus(), this ensures frozen_cpus and the potential races with
userspace are covered properly. At this point we don't care which CPU it picks.

On resume we know which CPU we want, so cpu_down() all the others. I thought the
frozen_cpus and user-space race wouldn't be a problem here, but Lorenzo
suggested it may confuse some device drivers to receive a CPU_DOWN_PREPARE etc
followed by CPU_UP_PREPARE_FROZEN etc.

I haven't found any drivers in the tree where this would be a problem (~95% of
notifiers either mask out the frozen bits, or fall-through in those cases). But
I'm still going through the list...


> 
>>  	if (error)
>>  		goto Enable_cpus;
>>  
>> @@ -551,7 +559,7 @@ int hibernation_platform_enter(void)
>>  	if (error)
>>  		goto Platform_finish;
>>  
>> -	error = disable_nonboot_cpus();
>> +	error = arch_hibernation_disable_cpus(true);
> 
> I have the same question about this hunk I had before.
> 
> Is it really necessary to do the arch thing here?

Ah, sorry I didn't understand what this did before. This is used when ACPI
drives hibernate/resume instead of swsusp_arch_suspend().

No, its not needed.


> It shouldn't really matter AFAICS.
> 
>>  	if (error)
>>  		goto Enable_cpus;

Thanks,

James

Re: [PATCH v4 2/3] arm64: hibernate: Resume when hibernate image created on non-boot CPU

[James Morse]

Hi Catalin,

On 05/07/16 18:49, Catalin Marinas wrote:
> On Mon, Jul 04, 2016 at 03:52:29PM +0100, James Morse wrote:
>> diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h
>> index 024d623f662e..9b3e8d9bfc8c 100644
>> --- a/arch/arm64/include/asm/suspend.h
>> +++ b/arch/arm64/include/asm/suspend.h
>> @@ -47,4 +47,8 @@ int swsusp_arch_resume(void);
>>  int arch_hibernation_header_save(void *addr, unsigned int max_size);
>>  int arch_hibernation_header_restore(void *addr);
>>  
>> +/* Used to resume on the CPU we hibernated on */
>> +int _arch_hibernation_disable_cpus(bool suspend);
>> +#define arch_hibernation_disable_cpus(x) _arch_hibernation_disable_cpus(x)
> 
> Nitpick: we normally tend to use the same name for the function an macro
> but it's fine like this as well:
> 
> +int arch_hibernation_disable_cpus(bool suspend);
> +#define arch_hibernation_disable_cpus arch_hibernation_disable_cpus
> 
> BTW, do you expect an architecture to define ARCH_HIBERNATION_CPU_HOOKS
> but not arch_hibernation_disable_cpus? Or you'd expect more hooks in the
> future?

The macro and kconfig symbol are gone in the new version... they were both part
of avoiding a weak symbol.


>> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
>> index 21ab5df9fa76..bae45abde7a2 100644
>> --- a/arch/arm64/kernel/hibernate.c
>> +++ b/arch/arm64/kernel/hibernate.c

>> @@ -66,6 +69,12 @@ extern char hibernate_el2_vectors[];
>>  extern char __hyp_stub_vectors[];
>>  
>>  /*
>> + * The logical cpu number we should resume on, initialised to a non-cpu
>> + * number.
>> + */
>> +static int sleep_cpu = -EINVAL;
>> +
>> +/*
>>   * Values that may not change over hibernate/resume. We put the build number
>>   * and date in here so that we guarantee not to resume with a different
>>   * kernel.
>> @@ -87,6 +96,8 @@ static struct arch_hibernate_hdr {
>>  	 * re-configure el2.
>>  	 */
>>  	phys_addr_t	__hyp_stub_vectors;
>> +
>> +	u64		sleep_cpu_mpidr;
>>  } resume_hdr;
>>  
>>  static inline void arch_hdr_invariants(struct arch_hibernate_hdr_invariants *i)
>> @@ -129,12 +140,18 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
>>  	else
>>  		hdr->__hyp_stub_vectors = 0;
>>  
>> +	/* Save the mpidr of the cpu we called cpu_suspend() on... */
>> +	hdr->sleep_cpu_mpidr = cpu_logical_map(sleep_cpu);
>> +	pr_info("Hibernating on CPU %d [mpidr:0x%llx]\n", sleep_cpu,
>> +		hdr->sleep_cpu_mpidr);
> 
> Do we have a guarantee that sleep_cpu != -EINVAL here?

This depends on the order the core code calls these functions, I will add a
check and return an error just in case it ever changes.


> If the above is fine, feel free to add:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks!



James

[PATCH v4 2/3] arm64: hibernate: Resume when hibernate image created on non-boot CPU

[James Morse]

Hi Catalin,

On 05/07/16 18:49, Catalin Marinas wrote:
> On Mon, Jul 04, 2016 at 03:52:29PM +0100, James Morse wrote:
>> diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h
>> index 024d623f662e..9b3e8d9bfc8c 100644
>> --- a/arch/arm64/include/asm/suspend.h
>> +++ b/arch/arm64/include/asm/suspend.h
>> @@ -47,4 +47,8 @@ int swsusp_arch_resume(void);
>>  int arch_hibernation_header_save(void *addr, unsigned int max_size);
>>  int arch_hibernation_header_restore(void *addr);
>>  
>> +/* Used to resume on the CPU we hibernated on */
>> +int _arch_hibernation_disable_cpus(bool suspend);
>> +#define arch_hibernation_disable_cpus(x) _arch_hibernation_disable_cpus(x)
> 
> Nitpick: we normally tend to use the same name for the function an macro
> but it's fine like this as well:
> 
> +int arch_hibernation_disable_cpus(bool suspend);
> +#define arch_hibernation_disable_cpus arch_hibernation_disable_cpus
> 
> BTW, do you expect an architecture to define ARCH_HIBERNATION_CPU_HOOKS
> but not arch_hibernation_disable_cpus? Or you'd expect more hooks in the
> future?

The macro and kconfig symbol are gone in the new version... they were both part
of avoiding a weak symbol.


>> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
>> index 21ab5df9fa76..bae45abde7a2 100644
>> --- a/arch/arm64/kernel/hibernate.c
>> +++ b/arch/arm64/kernel/hibernate.c

>> @@ -66,6 +69,12 @@ extern char hibernate_el2_vectors[];
>>  extern char __hyp_stub_vectors[];
>>  
>>  /*
>> + * The logical cpu number we should resume on, initialised to a non-cpu
>> + * number.
>> + */
>> +static int sleep_cpu = -EINVAL;
>> +
>> +/*
>>   * Values that may not change over hibernate/resume. We put the build number
>>   * and date in here so that we guarantee not to resume with a different
>>   * kernel.
>> @@ -87,6 +96,8 @@ static struct arch_hibernate_hdr {
>>  	 * re-configure el2.
>>  	 */
>>  	phys_addr_t	__hyp_stub_vectors;
>> +
>> +	u64		sleep_cpu_mpidr;
>>  } resume_hdr;
>>  
>>  static inline void arch_hdr_invariants(struct arch_hibernate_hdr_invariants *i)
>> @@ -129,12 +140,18 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
>>  	else
>>  		hdr->__hyp_stub_vectors = 0;
>>  
>> +	/* Save the mpidr of the cpu we called cpu_suspend() on... */
>> +	hdr->sleep_cpu_mpidr = cpu_logical_map(sleep_cpu);
>> +	pr_info("Hibernating on CPU %d [mpidr:0x%llx]\n", sleep_cpu,
>> +		hdr->sleep_cpu_mpidr);
> 
> Do we have a guarantee that sleep_cpu != -EINVAL here?

This depends on the order the core code calls these functions, I will add a
check and return an error just in case it ever changes.


> If the above is fine, feel free to add:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks!



James