diff mbox

[v2,4/6] PM / Hibernate: Allow architectures to specify the hibernate/resume CPU

Message ID 1466012148-7674-5-git-send-email-james.morse@arm.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

James Morse June 15, 2016, 5:35 p.m. UTC 
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.

Define a weak symbol arch_hibernation_disable_cpus(), which defaults to
calling disable_nonboot_cpus). Architectures that allow CPU 0 to be
hotplugged can use this to control which CPU is used for hibernate/resume.

Signed-off-by: James Morse <james.morse@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
---
If this approach is acceptable, this patch should go with 4&5 via the arm64
tree.

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

Comments

Rafael J. Wysocki June 15, 2016, 9:10 p.m. UTC | #1 
On Wed, Jun 15, 2016 at 7:35 PM, James Morse <james.morse@arm.com> wrote:
> 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.
>
> Define a weak symbol arch_hibernation_disable_cpus(), which defaults to
> calling disable_nonboot_cpus). Architectures that allow CPU 0 to be
> hotplugged can use this to control which CPU is used for hibernate/resume.
>
> Signed-off-by: James Morse <james.morse@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Pavel Machek <pavel@ucw.cz>
> ---
> If this approach is acceptable, this patch should go with 4&5 via the arm64
> tree.

I'm not entirely happy with this, but if you absolutely need it and if
you can't think about any cleaner way to achieve the goal, feel free
to add my ACK to this patch if necessary.

>  kernel/power/hibernate.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index fca9254280ee..0e668a3207ec 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -70,6 +70,16 @@ bool hibernation_available(void)
>  }
>
>  /**
> + * arch_hibernation_disable_cpus - Allow architectures to direct which CPU
> + * is used for suspend or resume.
> + * @suspend: True during hibernate, false for resume.
> + */
> +int __weak arch_hibernation_disable_cpus(__maybe_unused bool suspend)
> +{
> +       return disable_nonboot_cpus();
> +}
> +
> +/**
>   * hibernation_set_ops - Set the global hibernate operations.
>   * @ops: Hibernation operations to use in subsequent hibernation transitions.
>   */
> @@ -279,7 +289,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 +443,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 +561,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;
>
> --
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Morse June 28, 2016, 2:51 p.m. UTC | #2 
Hi Rafael,

On 15/06/16 22:10, Rafael J. Wysocki wrote:
> On Wed, Jun 15, 2016 at 7:35 PM, James Morse <james.morse@arm.com> wrote:
>> 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.
>>
>> Define a weak symbol arch_hibernation_disable_cpus(), which defaults to
>> calling disable_nonboot_cpus). Architectures that allow CPU 0 to be
>> hotplugged can use this to control which CPU is used for hibernate/resume.
>>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Pavel Machek <pavel@ucw.cz>
>> ---
>> If this approach is acceptable, this patch should go with 4&5 via the arm64
>> tree.
> 
> I'm not entirely happy with this, but if you absolutely need it and if
> you can't think about any cleaner way to achieve the goal, feel free
> to add my ACK to this patch if necessary.

I would prefer you to be happy with it.
On the weak symbol: After staring at this some more I've made the macro approach
work, so the weak symbol can go. (a new version of this series should arrive soon)
On the arch-specific odour coming from this, it may also be useful for Chen Yu's
'Use hlt instead of mwait when resuming from hibernation' series[0]. (I don't
understand the x86 specifics)


We need this to allow kexec and hibernate to interact freely. Preventing kexec
if CPU0 is offline because of hibernate doesn't feel right. Fixing it is
necessary as the first hint that something is wrong is when we fail to resume
and the user looses any data they had in memory.


Thanks,

James


[0] https://lkml.org/lkml/2016/6/25/98
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki June 29, 2016, 12:03 a.m. UTC | #3 
On Tuesday, June 28, 2016 03:51:39 PM James Morse wrote:
> Hi Rafael,
> 
> On 15/06/16 22:10, Rafael J. Wysocki wrote:
> > On Wed, Jun 15, 2016 at 7:35 PM, James Morse <james.morse@arm.com> wrote:
> >> 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.
> >>
> >> Define a weak symbol arch_hibernation_disable_cpus(), which defaults to
> >> calling disable_nonboot_cpus). Architectures that allow CPU 0 to be
> >> hotplugged can use this to control which CPU is used for hibernate/resume.
> >>
> >> Signed-off-by: James Morse <james.morse@arm.com>
> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> >> Cc: Pavel Machek <pavel@ucw.cz>
> >> ---
> >> If this approach is acceptable, this patch should go with 4&5 via the arm64
> >> tree.
> > 
> > I'm not entirely happy with this, but if you absolutely need it and if
> > you can't think about any cleaner way to achieve the goal, feel free
> > to add my ACK to this patch if necessary.
> 
> I would prefer you to be happy with it.
> On the weak symbol: After staring at this some more I've made the macro approach
> work, so the weak symbol can go. (a new version of this series should arrive soon)
> On the arch-specific odour coming from this, it may also be useful for Chen Yu's
> 'Use hlt instead of mwait when resuming from hibernation' series[0]. (I don't
> understand the x86 specifics)

Yes, it looks like both x86 and ARM64 want/need to do something special around
disabling/enabling nonboot CPUs during hibernation/restore.

Let me comment on the new submission, though.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index fca9254280ee..0e668a3207ec 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -70,6 +70,16 @@  bool hibernation_available(void)
 }
 
 /**
+ * arch_hibernation_disable_cpus - Allow architectures to direct which CPU
+ * is used for suspend or resume.
+ * @suspend: True during hibernate, false for resume.
+ */
+int __weak arch_hibernation_disable_cpus(__maybe_unused bool suspend)
+{
+	return disable_nonboot_cpus();
+}
+
+/**
  * hibernation_set_ops - Set the global hibernate operations.
  * @ops: Hibernation operations to use in subsequent hibernation transitions.
  */
@@ -279,7 +289,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 +443,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 +561,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;