diff mbox

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

Message ID 1467643950-11034-2-git-send-email-james.morse@arm.com (mailing list archive)
State Not Applicable, archived
Delegated to: Rafael Wysocki
Headers show

Commit Message

James Morse July 4, 2016, 2:52 p.m. UTC 
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(-)

Comments

Rafael J. Wysocki July 5, 2016, 12:28 p.m. UTC | #1 
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

--
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 July 6, 2016, 12:38 a.m. UTC | #2 
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

--
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 July 6, 2016, 9:16 a.m. UTC | #3 
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
--
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 July 6, 2016, 9:11 p.m. UTC | #4 
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

--
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 July 7, 2016, 8:29 a.m. UTC | #5 
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



--
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..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;