Use freeze_secondary_cpus() instead of open coding using cpu_down()
directly.
This also prepares to make cpu_up/down a private interface for anything
but the cpu subsystem.
Signed-off-by: Qais Yousef <[email protected]>
CC: Tony Luck <[email protected]>
CC: Fenghua Yu <[email protected]>
CC: [email protected]
CC: [email protected]
---
arch/ia64/kernel/process.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 968b5f33e725..70b433eafa5c 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -647,12 +647,8 @@ cpu_halt (void)
void machine_shutdown(void)
{
#ifdef CONFIG_HOTPLUG_CPU
- int cpu;
-
- for_each_online_cpu(cpu) {
- if (cpu != smp_processor_id())
- cpu_down(cpu);
- }
+ /* TODO: Can we use disable_nonboot_cpus()? */
+ freeze_secondary_cpus(smp_processor_id());
#endif
#ifdef CONFIG_KEXEC
kexec_disable_iosapic();
--
2.17.1
Hi Qais,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.4-rc5 next-20191031]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Qais-Yousef/arm64-hibernate-c-create-a-new-function-to-handle-cpu_up-sleep_cpu/20191102-053138
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0dbe6cb8f7e05bc9611602ef45980a6c57b245a3
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
arch/ia64/kernel/process.c: In function 'machine_shutdown':
>> arch/ia64/kernel/process.c:651:2: error: implicit declaration of function 'freeze_secondary_cpus'; did you mean 'suspend_enable_secondary_cpus'? [-Werror=implicit-function-declaration]
freeze_secondary_cpus(smp_processor_id());
^~~~~~~~~~~~~~~~~~~~~
suspend_enable_secondary_cpus
cc1: some warnings being treated as errors
vim +651 arch/ia64/kernel/process.c
646
647 void machine_shutdown(void)
648 {
649 #ifdef CONFIG_HOTPLUG_CPU
650 /* TODO: Can we use disable_nonboot_cpus()? */
> 651 freeze_secondary_cpus(smp_processor_id());
652 #endif
653 #ifdef CONFIG_KEXEC
654 kexec_disable_iosapic();
655 #endif
656 }
657
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, 30 Oct 2019, Qais Yousef wrote:
> Use freeze_secondary_cpus() instead of open coding using cpu_down()
> directly.
>
> This also prepares to make cpu_up/down a private interface for anything
> but the cpu subsystem.
>
> Signed-off-by: Qais Yousef <[email protected]>
> CC: Tony Luck <[email protected]>
> CC: Fenghua Yu <[email protected]>
> CC: [email protected]
> CC: [email protected]
> ---
> arch/ia64/kernel/process.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
> index 968b5f33e725..70b433eafa5c 100644
> --- a/arch/ia64/kernel/process.c
> +++ b/arch/ia64/kernel/process.c
> @@ -647,12 +647,8 @@ cpu_halt (void)
> void machine_shutdown(void)
> {
> #ifdef CONFIG_HOTPLUG_CPU
> - int cpu;
> -
> - for_each_online_cpu(cpu) {
> - if (cpu != smp_processor_id())
> - cpu_down(cpu);
> - }
> + /* TODO: Can we use disable_nonboot_cpus()? */
> + freeze_secondary_cpus(smp_processor_id());
freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)
Thanks,
tglx
On 11/19/19 23:21, Thomas Gleixner wrote:
> On Wed, 30 Oct 2019, Qais Yousef wrote:
>
> > Use freeze_secondary_cpus() instead of open coding using cpu_down()
> > directly.
> >
> > This also prepares to make cpu_up/down a private interface for anything
> > but the cpu subsystem.
> >
> > Signed-off-by: Qais Yousef <[email protected]>
> > CC: Tony Luck <[email protected]>
> > CC: Fenghua Yu <[email protected]>
> > CC: [email protected]
> > CC: [email protected]
> > ---
> > arch/ia64/kernel/process.c | 8 ++------
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
> > index 968b5f33e725..70b433eafa5c 100644
> > --- a/arch/ia64/kernel/process.c
> > +++ b/arch/ia64/kernel/process.c
> > @@ -647,12 +647,8 @@ cpu_halt (void)
> > void machine_shutdown(void)
> > {
> > #ifdef CONFIG_HOTPLUG_CPU
> > - int cpu;
> > -
> > - for_each_online_cpu(cpu) {
> > - if (cpu != smp_processor_id())
> > - cpu_down(cpu);
> > - }
> > + /* TODO: Can we use disable_nonboot_cpus()? */
> > + freeze_secondary_cpus(smp_processor_id());
>
> freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
> disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)
I thought I replied to this :-(
My plan was to simply make freeze_secondary_cpus() available and protected by
CONFIG_SMP only instead.
Good plan?
I'll probably do it as a separate patch before this one.
Thanks
--
Qais Yousef
On Tue, 19 Nov 2019, Qais Yousef wrote:
> On 11/19/19 23:21, Thomas Gleixner wrote:
> > On Wed, 30 Oct 2019, Qais Yousef wrote:
> > > void machine_shutdown(void)
> > > {
> > > #ifdef CONFIG_HOTPLUG_CPU
> > > - int cpu;
> > > -
> > > - for_each_online_cpu(cpu) {
> > > - if (cpu != smp_processor_id())
> > > - cpu_down(cpu);
> > > - }
> > > + /* TODO: Can we use disable_nonboot_cpus()? */
> > > + freeze_secondary_cpus(smp_processor_id());
> >
> > freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
> > disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)
>
> I thought I replied to this :-(
>
> My plan was to simply make freeze_secondary_cpus() available and protected by
> CONFIG_SMP only instead.
>
> Good plan?
No. freeze_secondary_cpus() is really for hibernation. Look at the exit
conditions there.
So you really want a seperate function which depends on CONFIG_HOTPLUG_CPU
and provides an inline stub for the CONFIG_HOTPLUG_CPU=n case.
But I have a hard time to see how that stuff works at all on
ia64:
cpu_down() might sleep, i.e. it must be called from preemptible
context. smp_processor_id() is invalid from preemtible context.
As this is obviously broken and ia64 is in keep compile mode only, it
should just go away.
Thanks,
tglx
On 11/19/19 23:59, Thomas Gleixner wrote:
> On Tue, 19 Nov 2019, Qais Yousef wrote:
> > On 11/19/19 23:21, Thomas Gleixner wrote:
> > > On Wed, 30 Oct 2019, Qais Yousef wrote:
> > > > void machine_shutdown(void)
> > > > {
> > > > #ifdef CONFIG_HOTPLUG_CPU
> > > > - int cpu;
> > > > -
> > > > - for_each_online_cpu(cpu) {
> > > > - if (cpu != smp_processor_id())
> > > > - cpu_down(cpu);
> > > > - }
> > > > + /* TODO: Can we use disable_nonboot_cpus()? */
> > > > + freeze_secondary_cpus(smp_processor_id());
> > >
> > > freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
> > > disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)
> >
> > I thought I replied to this :-(
> >
> > My plan was to simply make freeze_secondary_cpus() available and protected by
> > CONFIG_SMP only instead.
> >
> > Good plan?
>
> No. freeze_secondary_cpus() is really for hibernation. Look at the exit
> conditions there.
Hmm do you mean the pm_wakeup_pending() abort?
In arm64 we machine_shutdown() calls disable_nonboot_cpus(), which in turn
a wrapper around freeze_secondary_cpus() with 0 passed as an argument.
IIUC this means arm64 could fail to offline all CPUs on machine_shutdown(),
correct?
>
> So you really want a seperate function which depends on CONFIG_HOTPLUG_CPU
> and provides an inline stub for the CONFIG_HOTPLUG_CPU=n case.
>
> But I have a hard time to see how that stuff works at all on
> ia64:
>
> cpu_down() might sleep, i.e. it must be called from preemptible
> context. smp_processor_id() is invalid from preemtible context.
>
> As this is obviously broken and ia64 is in keep compile mode only, it
> should just go away.
If arm64 is doing the wrong thing, then we need a new function anyway.
Thanks
--
Qais Yousef
On Tue, 19 Nov 2019, Qais Yousef wrote:
> On 11/19/19 23:59, Thomas Gleixner wrote:
> > On Tue, 19 Nov 2019, Qais Yousef wrote:
> > > My plan was to simply make freeze_secondary_cpus() available and protected by
> > > CONFIG_SMP only instead.
> > >
> > > Good plan?
> >
> > No. freeze_secondary_cpus() is really for hibernation. Look at the exit
> > conditions there.
>
> Hmm do you mean the pm_wakeup_pending() abort?
>
> In arm64 we machine_shutdown() calls disable_nonboot_cpus(), which in turn
> a wrapper around freeze_secondary_cpus() with 0 passed as an argument.
>
> IIUC this means arm64 could fail to offline all CPUs on machine_shutdown(),
> correct?
Looks like.
Thanks,
tglx
On 11/20/19 09:46, Thomas Gleixner wrote:
> On Tue, 19 Nov 2019, Qais Yousef wrote:
> > On 11/19/19 23:59, Thomas Gleixner wrote:
> > > On Tue, 19 Nov 2019, Qais Yousef wrote:
> > > > My plan was to simply make freeze_secondary_cpus() available and protected by
> > > > CONFIG_SMP only instead.
> > > >
> > > > Good plan?
> > >
> > > No. freeze_secondary_cpus() is really for hibernation. Look at the exit
> > > conditions there.
> >
> > Hmm do you mean the pm_wakeup_pending() abort?
> >
> > In arm64 we machine_shutdown() calls disable_nonboot_cpus(), which in turn
> > a wrapper around freeze_secondary_cpus() with 0 passed as an argument.
> >
> > IIUC this means arm64 could fail to offline all CPUs on machine_shutdown(),
> > correct?
>
> Looks like.
Okay I'll double check and introduce a new function to be called from
machine_down() for arm64 and ia64 if necessary.
Thanks
--
Qais Yousef