| Message ID | 1467125510-18758-6-git-send-email-james.morse@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Tuesday, June 28, 2016 03:51:48 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 disable_nonboot_cpus() > behaviour. The macro should be defined in asm/suspend.h, and > ARCH_HIBERNATION_CPUHP 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> 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. A couple of nits below. > --- > 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. > * 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..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()? > +#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); Does this one here actually matter? > 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
On 2016年06月29日 08:10, Rafael J. Wysocki wrote: > On Tuesday, June 28, 2016 03:51:48 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 disable_nonboot_cpus() >> behaviour. The macro should be defined in asm/suspend.h, and >> ARCH_HIBERNATION_CPUHP 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> > 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. > This patch looks friendly for that fix :), thanks. > 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()? > Agreed. Yu -- 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
Hi Rafael, On 29/06/16 01:10, Rafael J. Wysocki wrote: > On Tuesday, June 28, 2016 03:51:48 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 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. > >> --- >> 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? >> 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. > >> +#endif >> >> static int nocompress; >> static int noresume; >> @@ -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 -- 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
On Monday, July 04, 2016 10:04:26 AM James Morse wrote: > Hi Rafael, > > On 29/06/16 01:10, Rafael J. Wysocki wrote: > > On Tuesday, June 28, 2016 03:51:48 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 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. > > > > > >> --- > >> 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? Fine by me. :-) > >> 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. Well, it turns out that I was wrong, we don't need the "enable" thing, so I guess it can stay the way it was. 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 --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() +#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;
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. Signed-off-by: James Morse <james.morse@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Pavel Machek <pavel@ucw.cz> --- 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(-)