| Message ID | 20200409112742.3581-1-qais.yousef@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/3] Revert "cpu/hotplug: Ignore pm_wakeup_pending() for disable_nonboot_cpus()" | expand |
On Thursday, April 9, 2020 1:27:40 PM CEST Qais Yousef wrote: > This issue was fixed already by: > > commit d66b16f5df4b ("arm64: Don't use disable_nonboot_cpus()") > commit dddf3578e0d4 ("ARM: Don't use disable_nonboot_cpus()") > > The only caller of disable_nonboot_cpus() is x86, which is in a proper > suspend/resume path and due to the reverted patch lost its ability to > early abort due to a pending wakeup. > > The fix that is being reverted is arguably a better one to backport to > stable trees. But it highlights the confusion about using > disable_nonboot_cpus() API. > > This is a preparation to remove disable_nonboot_cpus() in favor of > freeze_secondary_cpus(). > > This reverts commit e98eac6ff1b45e4e73f2e6031b37c256ccb5d36b. > > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > CC: "Rafael J. Wysocki" <rjw@rjwysocki.net> > CC: Len Brown <len.brown@intel.com> > CC: Pavel Machek <pavel@ucw.cz> > CC: Ingo Molnar <mingo@redhat.com> > CC: Borislav Petkov <bp@alien8.de> > CC: "H. Peter Anvin" <hpa@zytor.com> > CC: x86@kernel.org > CC: Todd E Brandt <todd.e.brandt@linux.intel.com> > CC: linux-pm@vger.kernel.org > CC: linux-kernel@vger.kernel.org > --- > include/linux/cpu.h | 12 +++--------- > kernel/cpu.c | 4 ++-- > 2 files changed, 5 insertions(+), 11 deletions(-) > > diff --git a/include/linux/cpu.h b/include/linux/cpu.h > index beaed2dc269e..9ead281157d3 100644 > --- a/include/linux/cpu.h > +++ b/include/linux/cpu.h > @@ -144,18 +144,12 @@ static inline void get_online_cpus(void) { cpus_read_lock(); } > static inline void put_online_cpus(void) { cpus_read_unlock(); } > > #ifdef CONFIG_PM_SLEEP_SMP > -int __freeze_secondary_cpus(int primary, bool suspend); > -static inline int freeze_secondary_cpus(int primary) > -{ > - return __freeze_secondary_cpus(primary, true); > -} > - > +extern int freeze_secondary_cpus(int primary); > static inline int disable_nonboot_cpus(void) > { > - return __freeze_secondary_cpus(0, false); > + return freeze_secondary_cpus(0); > } > - > -void enable_nonboot_cpus(void); > +extern void enable_nonboot_cpus(void); > > static inline int suspend_disable_secondary_cpus(void) > { > diff --git a/kernel/cpu.c b/kernel/cpu.c > index 12ae636e9cb6..30848496cbc7 100644 > --- a/kernel/cpu.c > +++ b/kernel/cpu.c > @@ -1327,7 +1327,7 @@ void bringup_nonboot_cpus(unsigned int setup_max_cpus) > #ifdef CONFIG_PM_SLEEP_SMP > static cpumask_var_t frozen_cpus; > > -int __freeze_secondary_cpus(int primary, bool suspend) > +int freeze_secondary_cpus(int primary) > { > int cpu, error = 0; > > @@ -1352,7 +1352,7 @@ int __freeze_secondary_cpus(int primary, bool suspend) > if (cpu == primary) > continue; > > - if (suspend && pm_wakeup_pending()) { > + if (pm_wakeup_pending()) { > pr_info("Wakeup pending. Abort CPU freeze\n"); > error = -EBUSY; > break; > I would do this the other way around: 1. Make x86 call freeze_secondary_cpus() directly, rename enable_nonboot_cpus() and drop disable_nonboot_cpus(). 2. Get rid of __freeze_secondary_cpus().
On 04/26/20 17:24, Rafael J. Wysocki wrote: > I would do this the other way around: > > 1. Make x86 call freeze_secondary_cpus() directly, rename > enable_nonboot_cpus() and drop disable_nonboot_cpus(). All of this in a single patch? > 2. Get rid of __freeze_secondary_cpus(). I guess you're implying to drop the revert too and manually unroll it instead. Could do. Thanks -- Qais Yousef
On Mon, Apr 27, 2020 at 12:29 PM Qais Yousef <qais.yousef@arm.com> wrote: > > On 04/26/20 17:24, Rafael J. Wysocki wrote: > > I would do this the other way around: > > > > 1. Make x86 call freeze_secondary_cpus() directly, rename > > enable_nonboot_cpus() and drop disable_nonboot_cpus(). > > All of this in a single patch? Well, why not? Calling freeze_secondary_cpus() directly causes disable_nonboot_cpus() to be unused (and so it can be dropped in the same patch) and it also introduces a name mismatch between freeze_ and enable_, which IMO needs to be addressed right away (also in the same patch). > > 2. Get rid of __freeze_secondary_cpus(). > > I guess you're implying to drop the revert too and manually unroll it instead. IMO the revert is just an extra step with no real value, so why do it? > Could do. Thanks!
On 04/29/20 12:40, Rafael J. Wysocki wrote: > On Mon, Apr 27, 2020 at 12:29 PM Qais Yousef <qais.yousef@arm.com> wrote: > > > > On 04/26/20 17:24, Rafael J. Wysocki wrote: > > > I would do this the other way around: > > > > > > 1. Make x86 call freeze_secondary_cpus() directly, rename > > > enable_nonboot_cpus() and drop disable_nonboot_cpus(). > > > > All of this in a single patch? > > Well, why not? I don't mind, was just clarifying. Usually it's requested to split patches :) > > Calling freeze_secondary_cpus() directly causes disable_nonboot_cpus() > to be unused (and so it can be dropped in the same patch) and it also > introduces a name mismatch between freeze_ and enable_, which IMO > needs to be addressed right away (also in the same patch). > > > > 2. Get rid of __freeze_secondary_cpus(). > > > > I guess you're implying to drop the revert too and manually unroll it instead. > > IMO the revert is just an extra step with no real value, so why do it? Works for me. Will send v2 ASAP. Thanks -- Qais Yousef
diff --git a/include/linux/cpu.h b/include/linux/cpu.h index beaed2dc269e..9ead281157d3 100644 --- a/include/linux/cpu.h +++ b/include/linux/cpu.h @@ -144,18 +144,12 @@ static inline void get_online_cpus(void) { cpus_read_lock(); } static inline void put_online_cpus(void) { cpus_read_unlock(); } #ifdef CONFIG_PM_SLEEP_SMP -int __freeze_secondary_cpus(int primary, bool suspend); -static inline int freeze_secondary_cpus(int primary) -{ - return __freeze_secondary_cpus(primary, true); -} - +extern int freeze_secondary_cpus(int primary); static inline int disable_nonboot_cpus(void) { - return __freeze_secondary_cpus(0, false); + return freeze_secondary_cpus(0); } - -void enable_nonboot_cpus(void); +extern void enable_nonboot_cpus(void); static inline int suspend_disable_secondary_cpus(void) { diff --git a/kernel/cpu.c b/kernel/cpu.c index 12ae636e9cb6..30848496cbc7 100644 --- a/kernel/cpu.c +++ b/kernel/cpu.c @@ -1327,7 +1327,7 @@ void bringup_nonboot_cpus(unsigned int setup_max_cpus) #ifdef CONFIG_PM_SLEEP_SMP static cpumask_var_t frozen_cpus; -int __freeze_secondary_cpus(int primary, bool suspend) +int freeze_secondary_cpus(int primary) { int cpu, error = 0; @@ -1352,7 +1352,7 @@ int __freeze_secondary_cpus(int primary, bool suspend) if (cpu == primary) continue; - if (suspend && pm_wakeup_pending()) { + if (pm_wakeup_pending()) { pr_info("Wakeup pending. Abort CPU freeze\n"); error = -EBUSY; break;
This issue was fixed already by: commit d66b16f5df4b ("arm64: Don't use disable_nonboot_cpus()") commit dddf3578e0d4 ("ARM: Don't use disable_nonboot_cpus()") The only caller of disable_nonboot_cpus() is x86, which is in a proper suspend/resume path and due to the reverted patch lost its ability to early abort due to a pending wakeup. The fix that is being reverted is arguably a better one to backport to stable trees. But it highlights the confusion about using disable_nonboot_cpus() API. This is a preparation to remove disable_nonboot_cpus() in favor of freeze_secondary_cpus(). This reverts commit e98eac6ff1b45e4e73f2e6031b37c256ccb5d36b. Signed-off-by: Qais Yousef <qais.yousef@arm.com> CC: "Rafael J. Wysocki" <rjw@rjwysocki.net> CC: Len Brown <len.brown@intel.com> CC: Pavel Machek <pavel@ucw.cz> CC: Ingo Molnar <mingo@redhat.com> CC: Borislav Petkov <bp@alien8.de> CC: "H. Peter Anvin" <hpa@zytor.com> CC: x86@kernel.org CC: Todd E Brandt <todd.e.brandt@linux.intel.com> CC: linux-pm@vger.kernel.org CC: linux-kernel@vger.kernel.org --- include/linux/cpu.h | 12 +++--------- kernel/cpu.c | 4 ++-- 2 files changed, 5 insertions(+), 11 deletions(-)