[PATCH v2 01/14] smp: Create a new function to shutdown nonboot cpus

[PATCH v2 01/14] smp: Create a new function to shutdown nonboot cpus

[Qais Yousef]

This function will be used later in machine_shutdown() for some archs.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Josh Poimboeuf <jpoimboe@redhat.com>
CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
CC: Jiri Kosina <jkosina@suse.cz>
CC: Nicholas Piggin <npiggin@gmail.com>
CC: Daniel Lezcano <daniel.lezcano@linaro.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Eiichi Tsukata <devel@etsukata.com>
CC: Zhenzhong Duan <zhenzhong.duan@oracle.com>
CC: Nadav Amit <namit@vmware.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
CC: Tony Luck <tony.luck@intel.com>
CC: Fenghua Yu <fenghua.yu@intel.com>
CC: Russell King <linux@armlinux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will@kernel.org>
CC: linux-arm-kernel@lists.infradead.org
CC: linux-ia64@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 include/linux/cpu.h |  2 ++
 kernel/cpu.c        | 17 +++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index bc6c879bd110..8229932fb053 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -118,6 +118,7 @@ extern void cpu_hotplug_disable(void);
 extern void cpu_hotplug_enable(void);
 void clear_tasks_mm_cpumask(int cpu);
 int cpu_down(unsigned int cpu);
+extern void smp_shutdown_nonboot_cpus(unsigned int primary_cpu);
 
 #else /* CONFIG_HOTPLUG_CPU */
 
@@ -129,6 +130,7 @@ static inline int  cpus_read_trylock(void) { return true; }
 static inline void lockdep_assert_cpus_held(void) { }
 static inline void cpu_hotplug_disable(void) { }
 static inline void cpu_hotplug_enable(void) { }
+static inline void smp_shutdown_nonboot_cpus(unsigned int primary_cpu) { }
 #endif	/* !CONFIG_HOTPLUG_CPU */
 
 /* Wrappers which go away once all code is converted */
diff --git a/kernel/cpu.c b/kernel/cpu.c
index e2cad3ee2ead..94055a0d989e 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1058,6 +1058,23 @@ int cpu_down(unsigned int cpu)
 }
 EXPORT_SYMBOL(cpu_down);
 
+void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
+{
+	unsigned int cpu;
+
+	if (!cpu_online(primary_cpu)) {
+		pr_info("Attempting to shutdodwn nonboot cpus while boot cpu is offline!\n");
+		cpu_online(primary_cpu);
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == primary_cpu)
+			continue;
+		if (cpu_online(cpu))
+			cpu_down(cpu);
+	}
+}
+
 #else
 #define takedown_cpu		NULL
 #endif /*CONFIG_HOTPLUG_CPU*/
-- 
2.17.1

Re: [PATCH v2 01/14] smp: Create a new function to shutdown nonboot cpus

[Russell King - ARM Linux admin]

On Mon, Nov 25, 2019 at 11:27:41AM +0000, Qais Yousef wrote:
> This function will be used later in machine_shutdown() for some archs.
> 
> Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Josh Poimboeuf <jpoimboe@redhat.com>
> CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> CC: Jiri Kosina <jkosina@suse.cz>
> CC: Nicholas Piggin <npiggin@gmail.com>
> CC: Daniel Lezcano <daniel.lezcano@linaro.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Eiichi Tsukata <devel@etsukata.com>
> CC: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> CC: Nadav Amit <namit@vmware.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> CC: Tony Luck <tony.luck@intel.com>
> CC: Fenghua Yu <fenghua.yu@intel.com>
> CC: Russell King <linux@armlinux.org.uk>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Will Deacon <will@kernel.org>
> CC: linux-arm-kernel@lists.infradead.org
> CC: linux-ia64@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  include/linux/cpu.h |  2 ++
>  kernel/cpu.c        | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index bc6c879bd110..8229932fb053 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -118,6 +118,7 @@ extern void cpu_hotplug_disable(void);
>  extern void cpu_hotplug_enable(void);
>  void clear_tasks_mm_cpumask(int cpu);
>  int cpu_down(unsigned int cpu);
> +extern void smp_shutdown_nonboot_cpus(unsigned int primary_cpu);
>  
>  #else /* CONFIG_HOTPLUG_CPU */
>  
> @@ -129,6 +130,7 @@ static inline int  cpus_read_trylock(void) { return true; }
>  static inline void lockdep_assert_cpus_held(void) { }
>  static inline void cpu_hotplug_disable(void) { }
>  static inline void cpu_hotplug_enable(void) { }
> +static inline void smp_shutdown_nonboot_cpus(unsigned int primary_cpu) { }
>  #endif	/* !CONFIG_HOTPLUG_CPU */
>  
>  /* Wrappers which go away once all code is converted */
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index e2cad3ee2ead..94055a0d989e 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1058,6 +1058,23 @@ int cpu_down(unsigned int cpu)
>  }
>  EXPORT_SYMBOL(cpu_down);
>  
> +void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
> +{
> +	unsigned int cpu;
> +
> +	if (!cpu_online(primary_cpu)) {
> +		pr_info("Attempting to shutdodwn nonboot cpus while boot cpu is offline!\n");
> +		cpu_online(primary_cpu);
> +	}
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == primary_cpu)
> +			continue;
> +		if (cpu_online(cpu))
> +			cpu_down(cpu);
> +	}

How does this avoid racing with userspace attempting to restart CPUs
that have already been taken down by this function?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v2 01/14] smp: Create a new function to shutdown nonboot cpus

[Qais Yousef]

On 01/21/20 17:03, Russell King - ARM Linux admin wrote:
> On Mon, Nov 25, 2019 at 11:27:41AM +0000, Qais Yousef wrote:
> > +void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
> > +{
> > +	unsigned int cpu;
> > +
> > +	if (!cpu_online(primary_cpu)) {
> > +		pr_info("Attempting to shutdodwn nonboot cpus while boot cpu is offline!\n");
> > +		cpu_online(primary_cpu);

Eh, that should be cpu_up(primary_cpu)!

Which I have to say I'm not if is the right thing to do.
migrate_to_reboot_cpu() picks the first online cpu if reboot_cpu (assumed 0) is
offline

migrate_to_reboot_cpu():
 225         /* Make certain the cpu I'm about to reboot on is online */
 226         if (!cpu_online(cpu))
 227                 cpu = cpumask_first(cpu_online_mask);

> > +	}
> > +
> > +	for_each_present_cpu(cpu) {
> > +		if (cpu == primary_cpu)
> > +			continue;
> > +		if (cpu_online(cpu))
> > +			cpu_down(cpu);
> > +	}
> 
> How does this avoid racing with userspace attempting to restart CPUs
> that have already been taken down by this function?

This is meant to be called from machine_shutdown() only.

But you've got a point.

The previous logic that used disable_nonboot_cpus(), which in turn called
freeze_secondary_cpus() didn't hold hotplug lock. So I assumed the higher level
logic of machine_shutdown() ensures that hotplug lock is held to synchronize
with potential other hotplug operations.

But I can see now that it doesn't.

With this series that migrates users to use device_{online,offline}, holding
the lock_device_hotplug() should protect against such races.

Worth noting that this an existing problem in the code and not something
I introduced, of course it makes sense to fix it properly as part of this
series.

I'm not sure how the other archs deal with this TBH.

Thanks for having a look!

Cheers

--
Qais Yousef

Re: [PATCH v2 01/14] smp: Create a new function to shutdown nonboot cpus

[Russell King - ARM Linux admin]

On Tue, Jan 21, 2020 at 05:47:52PM +0000, Qais Yousef wrote:
> On 01/21/20 17:03, Russell King - ARM Linux admin wrote:
> > On Mon, Nov 25, 2019 at 11:27:41AM +0000, Qais Yousef wrote:
> > > +void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
> > > +{
> > > +	unsigned int cpu;
> > > +
> > > +	if (!cpu_online(primary_cpu)) {
> > > +		pr_info("Attempting to shutdodwn nonboot cpus while boot cpu is offline!\n");
> > > +		cpu_online(primary_cpu);
> 
> Eh, that should be cpu_up(primary_cpu)!
> 
> Which I have to say I'm not if is the right thing to do.
> migrate_to_reboot_cpu() picks the first online cpu if reboot_cpu (assumed 0) is
> offline
> 
> migrate_to_reboot_cpu():
>  225         /* Make certain the cpu I'm about to reboot on is online */
>  226         if (!cpu_online(cpu))
>  227                 cpu = cpumask_first(cpu_online_mask);
> 
> > > +	}
> > > +
> > > +	for_each_present_cpu(cpu) {
> > > +		if (cpu == primary_cpu)
> > > +			continue;
> > > +		if (cpu_online(cpu))
> > > +			cpu_down(cpu);
> > > +	}
> > 
> > How does this avoid racing with userspace attempting to restart CPUs
> > that have already been taken down by this function?
> 
> This is meant to be called from machine_shutdown() only.
> 
> But you've got a point.
> 
> The previous logic that used disable_nonboot_cpus(), which in turn called
> freeze_secondary_cpus() didn't hold hotplug lock. So I assumed the higher level
> logic of machine_shutdown() ensures that hotplug lock is held to synchronize
> with potential other hotplug operations.

freeze_secondary_cpus() takes the CPU maps lock while it takes CPUs
down, and then disables cpu hotplug by incrementing
cpu_hotplug_disabled.  Incrementing that prevents cpu_up() and
cpu_down() being used, thereby preventing userspace from changing the
online state of any CPU in the system.

> But I can see now that it doesn't.
> 
> With this series that migrates users to use device_{online,offline}, holding
> the lock_device_hotplug() should protect against such races.
> 
> Worth noting that this an existing problem in the code and not something
> I introduced, of course it makes sense to fix it properly as part of this
> series.
> 
> I'm not sure how the other archs deal with this TBH.
> 
> Thanks for having a look!
> 
> Cheers
> 
> --
> Qais Yousef
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v2 01/14] smp: Create a new function to shutdown nonboot cpus

[Qais Yousef]

On 01/21/20 18:09, Russell King - ARM Linux admin wrote:
> On Tue, Jan 21, 2020 at 05:47:52PM +0000, Qais Yousef wrote:
> > On 01/21/20 17:03, Russell King - ARM Linux admin wrote:
> > > On Mon, Nov 25, 2019 at 11:27:41AM +0000, Qais Yousef wrote:
> > > > +void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
> > > > +{
> > > > +	unsigned int cpu;
> > > > +
> > > > +	if (!cpu_online(primary_cpu)) {
> > > > +		pr_info("Attempting to shutdodwn nonboot cpus while boot cpu is offline!\n");
> > > > +		cpu_online(primary_cpu);
> > 
> > Eh, that should be cpu_up(primary_cpu)!
> > 
> > Which I have to say I'm not if is the right thing to do.
> > migrate_to_reboot_cpu() picks the first online cpu if reboot_cpu (assumed 0) is
> > offline
> > 
> > migrate_to_reboot_cpu():
> >  225         /* Make certain the cpu I'm about to reboot on is online */
> >  226         if (!cpu_online(cpu))
> >  227                 cpu = cpumask_first(cpu_online_mask);
> > 
> > > > +	}
> > > > +
> > > > +	for_each_present_cpu(cpu) {
> > > > +		if (cpu == primary_cpu)
> > > > +			continue;
> > > > +		if (cpu_online(cpu))
> > > > +			cpu_down(cpu);
> > > > +	}
> > > 
> > > How does this avoid racing with userspace attempting to restart CPUs
> > > that have already been taken down by this function?
> > 
> > This is meant to be called from machine_shutdown() only.
> > 
> > But you've got a point.
> > 
> > The previous logic that used disable_nonboot_cpus(), which in turn called
> > freeze_secondary_cpus() didn't hold hotplug lock. So I assumed the higher level
> > logic of machine_shutdown() ensures that hotplug lock is held to synchronize
> > with potential other hotplug operations.
> 
> freeze_secondary_cpus() takes the CPU maps lock while it takes CPUs
> down, and then disables cpu hotplug by incrementing
> cpu_hotplug_disabled.  Incrementing that prevents cpu_up() and
> cpu_down() being used, thereby preventing userspace from changing the
> online state of any CPU in the system.

I see. Sorry I missed the CPU maps lock.

Yes this makes sense and should work here too.

Thanks for the help.

Thomas, I'll wait for your comment on this and potentially other patches before
sending v3.

Thanks

--
Qais Yousef

[PATCH v2 02/14] ia64: Replace cpu_down with smp_shutdown_nonboot_cpus()

[Qais Yousef]

Use the new smp_shutdown_nonboot_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 <qais.yousef@arm.com>
CC: Tony Luck <tony.luck@intel.com>
CC: Fenghua Yu <fenghua.yu@intel.com>
CC: linux-ia64@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/ia64/kernel/process.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 968b5f33e725..cc894d4900be 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -646,14 +646,8 @@ cpu_halt (void)
 
 void machine_shutdown(void)
 {
-#ifdef CONFIG_HOTPLUG_CPU
-	int cpu;
+	smp_shutdown_nonboot_cpus(0);
 
-	for_each_online_cpu(cpu) {
-		if (cpu != smp_processor_id())
-			cpu_down(cpu);
-	}
-#endif
 #ifdef CONFIG_KEXEC
 	kexec_disable_iosapic();
 #endif
-- 
2.17.1

[PATCH v2 03/14] arm: arm64: Don't use disable_nonboot_cpus()

[Qais Yousef]

disable_nonboot_cpus() is not safe to use when doing machine_down(),
because it relies on freeze_secondary_cpus() which in return is
a suspend/resume related freeze and could abort if the logic detects any
pending activities that can prevent finishing the offlining process.

Beside disable_nonboot_cpus() is dependent on CONFIG_PM_SLEEP_SMP which
is an othogonal config to rely on to ensure this function works
correctly.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Russell King <linux@armlinux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will@kernel.org>
CC: linux-arm-kernel@lists.infradead.org
CC: linux-kernel@vger.kernel.org
---
 arch/arm/kernel/reboot.c    | 4 ++--
 arch/arm64/kernel/process.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
index bb18ed0539f4..58ad1a70f770 100644
--- a/arch/arm/kernel/reboot.c
+++ b/arch/arm/kernel/reboot.c
@@ -88,11 +88,11 @@ void soft_restart(unsigned long addr)
  * to execute e.g. a RAM-based pin loop is not sufficient. This allows the
  * kexec'd kernel to use any and all RAM as it sees fit, without having to
  * avoid any code or data used by any SW CPU pin loop. The CPU hotplug
- * functionality embodied in disable_nonboot_cpus() to achieve this.
+ * functionality embodied in smp_shutdown_nonboot_cpus() to achieve this.
  */
 void machine_shutdown(void)
 {
-	disable_nonboot_cpus();
+	smp_shutdown_nonboot_cpus(0);
 }
 
 /*
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 71f788cd2b18..3bcc9bfc581e 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -141,11 +141,11 @@ void arch_cpu_idle_dead(void)
  * to execute e.g. a RAM-based pin loop is not sufficient. This allows the
  * kexec'd kernel to use any and all RAM as it sees fit, without having to
  * avoid any code or data used by any SW CPU pin loop. The CPU hotplug
- * functionality embodied in disable_nonboot_cpus() to achieve this.
+ * functionality embodied in smpt_shutdown_nonboot_cpus() to achieve this.
  */
 void machine_shutdown(void)
 {
-	disable_nonboot_cpus();
+	smp_shutdown_nonboot_cpus(0);
 }
 
 /*
-- 
2.17.1

Re: [PATCH v2 03/14] arm: arm64: Don't use disable_nonboot_cpus()

[Qais Yousef]

On 11/25/19 11:27, Qais Yousef wrote:
> disable_nonboot_cpus() is not safe to use when doing machine_down(),
> because it relies on freeze_secondary_cpus() which in return is
> a suspend/resume related freeze and could abort if the logic detects any
> pending activities that can prevent finishing the offlining process.
> 
> Beside disable_nonboot_cpus() is dependent on CONFIG_PM_SLEEP_SMP which
> is an othogonal config to rely on to ensure this function works
> correctly.
> 
> Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> CC: Russell King <linux@armlinux.org.uk>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Will Deacon <will@kernel.org>
> CC: linux-arm-kernel@lists.infradead.org
> CC: linux-kernel@vger.kernel.org
> ---

Ping :)

I'm missing ACKs on this patch and patch 4 for arm64. Hopefully none should be
controversial.

Thanks!

--
Qais Yousef

>  arch/arm/kernel/reboot.c    | 4 ++--
>  arch/arm64/kernel/process.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
> index bb18ed0539f4..58ad1a70f770 100644
> --- a/arch/arm/kernel/reboot.c
> +++ b/arch/arm/kernel/reboot.c
> @@ -88,11 +88,11 @@ void soft_restart(unsigned long addr)
>   * to execute e.g. a RAM-based pin loop is not sufficient. This allows the
>   * kexec'd kernel to use any and all RAM as it sees fit, without having to
>   * avoid any code or data used by any SW CPU pin loop. The CPU hotplug
> - * functionality embodied in disable_nonboot_cpus() to achieve this.
> + * functionality embodied in smp_shutdown_nonboot_cpus() to achieve this.
>   */
>  void machine_shutdown(void)
>  {
> -	disable_nonboot_cpus();
> +	smp_shutdown_nonboot_cpus(0);
>  }
>  
>  /*
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 71f788cd2b18..3bcc9bfc581e 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -141,11 +141,11 @@ void arch_cpu_idle_dead(void)
>   * to execute e.g. a RAM-based pin loop is not sufficient. This allows the
>   * kexec'd kernel to use any and all RAM as it sees fit, without having to
>   * avoid any code or data used by any SW CPU pin loop. The CPU hotplug
> - * functionality embodied in disable_nonboot_cpus() to achieve this.
> + * functionality embodied in smpt_shutdown_nonboot_cpus() to achieve this.
>   */
>  void machine_shutdown(void)
>  {
> -	disable_nonboot_cpus();
> +	smp_shutdown_nonboot_cpus(0);
>  }
>  
>  /*
> -- 
> 2.17.1
> 

Re: [PATCH v2 03/14] arm: arm64: Don't use disable_nonboot_cpus()

[Russell King - ARM Linux admin]

On Tue, Jan 21, 2020 at 04:50:31PM +0000, Qais Yousef wrote:
> On 11/25/19 11:27, Qais Yousef wrote:
> > disable_nonboot_cpus() is not safe to use when doing machine_down(),
> > because it relies on freeze_secondary_cpus() which in return is
> > a suspend/resume related freeze and could abort if the logic detects any
> > pending activities that can prevent finishing the offlining process.
> > 
> > Beside disable_nonboot_cpus() is dependent on CONFIG_PM_SLEEP_SMP which
> > is an othogonal config to rely on to ensure this function works
> > correctly.
> > 
> > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > CC: Russell King <linux@armlinux.org.uk>
> > CC: Catalin Marinas <catalin.marinas@arm.com>
> > CC: Will Deacon <will@kernel.org>
> > CC: linux-arm-kernel@lists.infradead.org
> > CC: linux-kernel@vger.kernel.org
> > ---
> 
> Ping :)
> 
> I'm missing ACKs on this patch and patch 4 for arm64. Hopefully none should be
> controversial.

ARM and ARM64 are maintained separately, so you can't submit a single
patch covering both.  Sorry.

> 
> Thanks!
> 
> --
> Qais Yousef
> 
> >  arch/arm/kernel/reboot.c    | 4 ++--
> >  arch/arm64/kernel/process.c | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
> > index bb18ed0539f4..58ad1a70f770 100644
> > --- a/arch/arm/kernel/reboot.c
> > +++ b/arch/arm/kernel/reboot.c
> > @@ -88,11 +88,11 @@ void soft_restart(unsigned long addr)
> >   * to execute e.g. a RAM-based pin loop is not sufficient. This allows the
> >   * kexec'd kernel to use any and all RAM as it sees fit, without having to
> >   * avoid any code or data used by any SW CPU pin loop. The CPU hotplug
> > - * functionality embodied in disable_nonboot_cpus() to achieve this.
> > + * functionality embodied in smp_shutdown_nonboot_cpus() to achieve this.
> >   */
> >  void machine_shutdown(void)
> >  {
> > -	disable_nonboot_cpus();
> > +	smp_shutdown_nonboot_cpus(0);
> >  }
> >  
> >  /*
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 71f788cd2b18..3bcc9bfc581e 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -141,11 +141,11 @@ void arch_cpu_idle_dead(void)
> >   * to execute e.g. a RAM-based pin loop is not sufficient. This allows the
> >   * kexec'd kernel to use any and all RAM as it sees fit, without having to
> >   * avoid any code or data used by any SW CPU pin loop. The CPU hotplug
> > - * functionality embodied in disable_nonboot_cpus() to achieve this.
> > + * functionality embodied in smpt_shutdown_nonboot_cpus() to achieve this.
> >   */
> >  void machine_shutdown(void)
> >  {
> > -	disable_nonboot_cpus();
> > +	smp_shutdown_nonboot_cpus(0);
> >  }
> >  
> >  /*
> > -- 
> > 2.17.1
> > 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v2 03/14] arm: arm64: Don't use disable_nonboot_cpus()

[Qais Yousef]

On 01/21/20 16:53, Russell King - ARM Linux admin wrote:
> On Tue, Jan 21, 2020 at 04:50:31PM +0000, Qais Yousef wrote:
> > On 11/25/19 11:27, Qais Yousef wrote:
> > > disable_nonboot_cpus() is not safe to use when doing machine_down(),
> > > because it relies on freeze_secondary_cpus() which in return is
> > > a suspend/resume related freeze and could abort if the logic detects any
> > > pending activities that can prevent finishing the offlining process.
> > > 
> > > Beside disable_nonboot_cpus() is dependent on CONFIG_PM_SLEEP_SMP which
> > > is an othogonal config to rely on to ensure this function works
> > > correctly.
> > > 
> > > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > > CC: Russell King <linux@armlinux.org.uk>
> > > CC: Catalin Marinas <catalin.marinas@arm.com>
> > > CC: Will Deacon <will@kernel.org>
> > > CC: linux-arm-kernel@lists.infradead.org
> > > CC: linux-kernel@vger.kernel.org
> > > ---
> > 
> > Ping :)
> > 
> > I'm missing ACKs on this patch and patch 4 for arm64. Hopefully none should be
> > controversial.
> 
> ARM and ARM64 are maintained separately, so you can't submit a single
> patch covering both.  Sorry.

Sure I'd be happy to split.

It was just a single line change and I expected Thomas to pick the whole series
up, so I didn't think there'd be an issue in combining them up since they're
identical.

Do I take it you have no objection to the code change itself and just would
like to see this split up?

Thanks

--
Qais Yousef

> 
> > 
> > Thanks!
> > 
> > --
> > Qais Yousef
> > 
> > >  arch/arm/kernel/reboot.c    | 4 ++--
> > >  arch/arm64/kernel/process.c | 4 ++--
> > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
> > > index bb18ed0539f4..58ad1a70f770 100644
> > > --- a/arch/arm/kernel/reboot.c
> > > +++ b/arch/arm/kernel/reboot.c
> > > @@ -88,11 +88,11 @@ void soft_restart(unsigned long addr)
> > >   * to execute e.g. a RAM-based pin loop is not sufficient. This allows the
> > >   * kexec'd kernel to use any and all RAM as it sees fit, without having to
> > >   * avoid any code or data used by any SW CPU pin loop. The CPU hotplug
> > > - * functionality embodied in disable_nonboot_cpus() to achieve this.
> > > + * functionality embodied in smp_shutdown_nonboot_cpus() to achieve this.
> > >   */
> > >  void machine_shutdown(void)
> > >  {
> > > -	disable_nonboot_cpus();
> > > +	smp_shutdown_nonboot_cpus(0);
> > >  }
> > >  
> > >  /*
> > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > index 71f788cd2b18..3bcc9bfc581e 100644
> > > --- a/arch/arm64/kernel/process.c
> > > +++ b/arch/arm64/kernel/process.c
> > > @@ -141,11 +141,11 @@ void arch_cpu_idle_dead(void)
> > >   * to execute e.g. a RAM-based pin loop is not sufficient. This allows the
> > >   * kexec'd kernel to use any and all RAM as it sees fit, without having to
> > >   * avoid any code or data used by any SW CPU pin loop. The CPU hotplug
> > > - * functionality embodied in disable_nonboot_cpus() to achieve this.
> > > + * functionality embodied in smpt_shutdown_nonboot_cpus() to achieve this.
> > >   */
> > >  void machine_shutdown(void)
> > >  {
> > > -	disable_nonboot_cpus();
> > > +	smp_shutdown_nonboot_cpus(0);
> > >  }
> > >  
> > >  /*
> > > -- 
> > > 2.17.1
> > > 
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v2 03/14] arm: arm64: Don't use disable_nonboot_cpus()

[Russell King - ARM Linux admin]

On Tue, Jan 21, 2020 at 04:58:09PM +0000, Qais Yousef wrote:
> On 01/21/20 16:53, Russell King - ARM Linux admin wrote:
> > On Tue, Jan 21, 2020 at 04:50:31PM +0000, Qais Yousef wrote:
> > > On 11/25/19 11:27, Qais Yousef wrote:
> > > > disable_nonboot_cpus() is not safe to use when doing machine_down(),
> > > > because it relies on freeze_secondary_cpus() which in return is
> > > > a suspend/resume related freeze and could abort if the logic detects any
> > > > pending activities that can prevent finishing the offlining process.
> > > > 
> > > > Beside disable_nonboot_cpus() is dependent on CONFIG_PM_SLEEP_SMP which
> > > > is an othogonal config to rely on to ensure this function works
> > > > correctly.
> > > > 
> > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > > > CC: Russell King <linux@armlinux.org.uk>
> > > > CC: Catalin Marinas <catalin.marinas@arm.com>
> > > > CC: Will Deacon <will@kernel.org>
> > > > CC: linux-arm-kernel@lists.infradead.org
> > > > CC: linux-kernel@vger.kernel.org
> > > > ---
> > > 
> > > Ping :)
> > > 
> > > I'm missing ACKs on this patch and patch 4 for arm64. Hopefully none should be
> > > controversial.
> > 
> > ARM and ARM64 are maintained separately, so you can't submit a single
> > patch covering both.  Sorry.
> 
> Sure I'd be happy to split.
> 
> It was just a single line change and I expected Thomas to pick the whole series
> up, so I didn't think there'd be an issue in combining them up since they're
> identical.
> 
> Do I take it you have no objection to the code change itself and just would
> like to see this split up?

I do have an objection to the new function you're introducing in patch
1.  See my reply to that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH v2 04/14] arm64: hibernate.c: Create a new function to handle cpu_up(sleep_cpu)

[Qais Yousef]

In preparation to make cpu_up/down private - move the user in arm64
hibernate.c to use a new generic function that provides what arm64
needs.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will@kernel.org>
CC: Steve Capper <steve.capper@arm.com>
CC: Richard Fontana <rfontana@redhat.com>
CC: James Morse <james.morse@arm.com>
CC: Mark Rutland <mark.rutland@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Josh Poimboeuf <jpoimboe@redhat.com>
CC: Ingo Molnar <mingo@kernel.org>
CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
CC: Nicholas Piggin <npiggin@gmail.com>
CC: Daniel Lezcano <daniel.lezcano@linaro.org>
CC: Jiri Kosina <jkosina@suse.cz>
CC: Pavankumar Kondeti <pkondeti@codeaurora.org>
CC: Zhenzhong Duan <zhenzhong.duan@oracle.com>
CC: linux-arm-kernel@lists.infradead.org
CC: linux-kernel@vger.kernel.org
---
 arch/arm64/kernel/hibernate.c | 13 +++++--------
 include/linux/cpu.h           |  1 +
 kernel/cpu.c                  | 24 ++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index a96b2921d22c..8ae348107f97 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c
@@ -166,14 +166,11 @@ int arch_hibernation_header_restore(void *addr)
 		sleep_cpu = -EINVAL;
 		return -EINVAL;
 	}
-	if (!cpu_online(sleep_cpu)) {
-		pr_info("Hibernated on a CPU that is offline! Bringing CPU up.\n");
-		ret = cpu_up(sleep_cpu);
-		if (ret) {
-			pr_err("Failed to bring hibernate-CPU up!\n");
-			sleep_cpu = -EINVAL;
-			return ret;
-		}
+
+	ret = bringup_hibernate_cpu(sleep_cpu);
+	if (ret) {
+		sleep_cpu = -EINVAL;
+		return ret;
 	}
 
 	resume_hdr = *hdr;
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 8229932fb053..f05168b49fab 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -92,6 +92,7 @@ int cpu_up(unsigned int cpu);
 void notify_cpu_starting(unsigned int cpu);
 extern void cpu_maps_update_begin(void);
 extern void cpu_maps_update_done(void);
+extern int bringup_hibernate_cpu(unsigned int sleep_cpu);
 
 #else	/* CONFIG_SMP */
 #define cpuhp_tasks_frozen	0
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 94055a0d989e..9610442c8dbc 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1221,6 +1221,30 @@ int cpu_up(unsigned int cpu)
 }
 EXPORT_SYMBOL_GPL(cpu_up);
 
+/**
+ * bringup_hibernate_cpu - Bring up the CPU that we hibernated on
+ * @sleep_cpu: The cpu we hibernated on and should be brought up.
+ *
+ * On some archs like arm64, we can hibernate on any CPU, but on wake up the
+ * CPU we hibernated on might be offline as a side effect of using maxcpus= for
+ * example.
+ */
+int bringup_hibernate_cpu(unsigned int sleep_cpu)
+{
+	int ret;
+
+	if (!cpu_online(sleep_cpu)) {
+		pr_info("Hibernated on a CPU that is offline! Bringing CPU up.\n");
+		ret = cpu_up(sleep_cpu);
+		if (ret) {
+			pr_err("Failed to bring hibernate-CPU up!\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP_SMP
 static cpumask_var_t frozen_cpus;
 
-- 
2.17.1

[PATCH v2 05/14] x86: Replace cpu_up/down with devcie_online/offline

[Qais Yousef]

The core device API performs extra housekeeping bits that are missing
from directly calling cpu_up/down.

See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
serialization during LPM") for an example description of what might go
wrong.

This also prepares to make cpu_up/down a private interface for anything
but the cpu subsystem.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/topology.c | 4 ++--
 arch/x86/mm/mmio-mod.c     | 8 ++++++--
 arch/x86/xen/smp.c         | 4 +++-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
index be5bc2e47c71..3b253088615e 100644
--- a/arch/x86/kernel/topology.c
+++ b/arch/x86/kernel/topology.c
@@ -69,7 +69,7 @@ int _debug_hotplug_cpu(int cpu, int action)
 
 	switch (action) {
 	case 0:
-		ret = cpu_down(cpu);
+		ret = device_offline(get_cpu_device(cpu));
 		if (!ret) {
 			pr_info("DEBUG_HOTPLUG_CPU0: CPU %u is now offline\n", cpu);
 			dev->offline = true;
@@ -78,7 +78,7 @@ int _debug_hotplug_cpu(int cpu, int action)
 			pr_debug("Can't offline CPU%d.\n", cpu);
 		break;
 	case 1:
-		ret = cpu_up(cpu);
+		ret = device_online(get_cpu_device(cpu));
 		if (!ret) {
 			dev->offline = false;
 			kobject_uevent(&dev->kobj, KOBJ_ONLINE);
diff --git a/arch/x86/mm/mmio-mod.c b/arch/x86/mm/mmio-mod.c
index b8ef8557d4b3..7ec7d05335ce 100644
--- a/arch/x86/mm/mmio-mod.c
+++ b/arch/x86/mm/mmio-mod.c
@@ -385,13 +385,15 @@ static void enter_uniprocessor(void)
 		pr_notice("Disabling non-boot CPUs...\n");
 	put_online_cpus();
 
+	lock_device_hotplug();
 	for_each_cpu(cpu, downed_cpus) {
-		err = cpu_down(cpu);
+		err = device_offline(get_cpu_device(cpu));
 		if (!err)
 			pr_info("CPU%d is down.\n", cpu);
 		else
 			pr_err("Error taking CPU%d down: %d\n", cpu, err);
 	}
+	unlock_device_hotplug();
 out:
 	if (num_online_cpus() > 1)
 		pr_warning("multiple CPUs still online, may miss events.\n");
@@ -405,13 +407,15 @@ static void leave_uniprocessor(void)
 	if (downed_cpus == NULL || cpumask_weight(downed_cpus) == 0)
 		return;
 	pr_notice("Re-enabling CPUs...\n");
+	lock_device_hotplug();
 	for_each_cpu(cpu, downed_cpus) {
-		err = cpu_up(cpu);
+		err = device_online(get_cpu_device(cpu));
 		if (!err)
 			pr_info("enabled CPU%d.\n", cpu);
 		else
 			pr_err("cannot re-enable CPU%d: %d\n", cpu, err);
 	}
+	unlock_device_hotplug();
 }
 
 #else /* !CONFIG_HOTPLUG_CPU */
diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 7a43b2ae19f1..aaa31100a31e 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -128,11 +128,12 @@ void __init xen_smp_cpus_done(unsigned int max_cpus)
 	if (xen_have_vcpu_info_placement)
 		return;
 
+	lock_device_hotplug();
 	for_each_online_cpu(cpu) {
 		if (xen_vcpu_nr(cpu) < MAX_VIRT_CPUS)
 			continue;
 
-		rc = cpu_down(cpu);
+		rc = device_offline(get_cpu_device(cpu));
 
 		if (rc == 0) {
 			/*
@@ -145,6 +146,7 @@ void __init xen_smp_cpus_done(unsigned int max_cpus)
 				__func__, cpu, rc);
 		}
 	}
+	unlock_device_hotplug();
 	WARN(count, "%s: brought %d CPUs offline\n", __func__, count);
 }
 
-- 
2.17.1

[PATCH v2 06/14] powerpc: Replace cpu_up/down with device_online/offline

[Qais Yousef]

The core device API performs extra housekeeping bits that are missing
from directly calling cpu_up/down.

See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
serialization during LPM") for an example description of what might go
wrong.

This also prepares to make cpu_up/down a private interface for anything
but the cpu subsystem.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Enrico Weigelt <info@metux.net>
CC: Ram Pai <linuxram@us.ibm.com>
CC: Nicholas Piggin <npiggin@gmail.com>
CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
CC: Christophe Leroy <christophe.leroy@c-s.fr>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: linuxppc-dev@lists.ozlabs.org
CC: linux-kernel@vger.kernel.org
---
 arch/powerpc/kernel/machine_kexec_64.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c
index 04a7cba58eff..ebf8cc7acc4d 100644
--- a/arch/powerpc/kernel/machine_kexec_64.c
+++ b/arch/powerpc/kernel/machine_kexec_64.c
@@ -208,13 +208,15 @@ static void wake_offline_cpus(void)
 {
 	int cpu = 0;
 
+	lock_device_hotplug();
 	for_each_present_cpu(cpu) {
 		if (!cpu_online(cpu)) {
 			printk(KERN_INFO "kexec: Waking offline cpu %d.\n",
 			       cpu);
-			WARN_ON(cpu_up(cpu));
+			WARN_ON(device_online(get_cpu_device(cpu)));
 		}
 	}
+	unlock_device_hotplug();
 }
 
 static void kexec_prepare_cpus(void)
-- 
2.17.1

[PATCH v2 07/14] sparc: Replace cpu_up/down with device_online/offline

[Qais Yousef]

The core device API performs extra housekeeping bits that are missing
from directly calling cpu_up/down.

See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
serialization during LPM") for an example description of what might go
wrong.

This also prepares to make cpu_up/down a private interface for anything
but the cpu subsystem.

Acked-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Sakari Ailus <sakari.ailus@linux.intel.com>
CC: sparclinux@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/sparc/kernel/ds.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/sparc/kernel/ds.c b/arch/sparc/kernel/ds.c
index bbf59b3b4af8..39350e5dbaf1 100644
--- a/arch/sparc/kernel/ds.c
+++ b/arch/sparc/kernel/ds.c
@@ -550,12 +550,13 @@ static int dr_cpu_configure(struct ds_info *dp, struct ds_cap_state *cp,
 	mdesc_populate_present_mask(mask);
 	mdesc_fill_in_cpu_data(mask);
 
+	lock_device_hotplug();
 	for_each_cpu(cpu, mask) {
 		int err;
 
 		printk(KERN_INFO "ds-%llu: Starting cpu %d...\n",
 		       dp->id, cpu);
-		err = cpu_up(cpu);
+		err = device_online(get_cpu_device(cpu));
 		if (err) {
 			__u32 res = DR_CPU_RES_FAILURE;
 			__u32 stat = DR_CPU_STAT_UNCONFIGURED;
@@ -574,6 +575,7 @@ static int dr_cpu_configure(struct ds_info *dp, struct ds_cap_state *cp,
 			dr_cpu_mark(resp, cpu, ncpus, res, stat);
 		}
 	}
+	unlock_device_hotplug();
 
 	spin_lock_irqsave(&ds_lock, flags);
 	__ds_send(dp->lp, resp, resp_len);
@@ -606,17 +608,19 @@ static int dr_cpu_unconfigure(struct ds_info *dp,
 			     resp_len, ncpus, mask,
 			     DR_CPU_STAT_UNCONFIGURED);
 
+	lock_device_hotplug();
 	for_each_cpu(cpu, mask) {
 		int err;
 
 		printk(KERN_INFO "ds-%llu: Shutting down cpu %d...\n",
 		       dp->id, cpu);
-		err = cpu_down(cpu);
+		err = device_offline(get_cpu_device(cpu));
 		if (err)
 			dr_cpu_mark(resp, cpu, ncpus,
 				    DR_CPU_RES_FAILURE,
 				    DR_CPU_STAT_CONFIGURED);
 	}
+	unlock_device_hotplug();
 
 	spin_lock_irqsave(&ds_lock, flags);
 	__ds_send(dp->lp, resp, resp_len);
-- 
2.17.1

[PATCH v2 08/14] parisc: Replace cpu_up/down with device_online/offline

[Qais Yousef]

The core device API performs extra housekeeping bits that are missing
from directly calling cpu_up/down.

See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
serialization during LPM") for an example description of what might go
wrong.

This also prepares to make cpu_up/down a private interface for anything
but the cpu subsystem.

Acked-by: Helge Deller <deller@gmx.de>
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
CC: Helge Deller <deller@gmx.de>
CC: Richard Fontana <rfontana@redhat.com>
CC: Armijn Hemel <armijn@tjaldur.nl>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: linux-parisc@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/parisc/kernel/processor.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/parisc/kernel/processor.c b/arch/parisc/kernel/processor.c
index 13f771f74ee3..4dde5fe78f0c 100644
--- a/arch/parisc/kernel/processor.c
+++ b/arch/parisc/kernel/processor.c
@@ -212,7 +212,9 @@ static int __init processor_probe(struct parisc_device *dev)
 #ifdef CONFIG_SMP
 	if (cpuid) {
 		set_cpu_present(cpuid, true);
-		cpu_up(cpuid);
+		lock_device_hotplug();
+		device_online(get_cpu_device(cpuid));
+		unlock_device_hotplug();
 	}
 #endif
 
-- 
2.17.1

[PATCH v2 09/14] driver: base: cpu: Export device_online/offline

[Qais Yousef]

And the {lock,unlock}_device_hotplug so that they can be used from
modules.

This is in preparation to replace cpu_up/down with
device_online/offline; which kernel/torture.c will require to be
exported to be built as a module.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: linux-kernel@vger.kernel.org
---
 drivers/base/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7bd9cd366d41..29b40c71b1ba 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -883,11 +883,13 @@ void lock_device_hotplug(void)
 {
 	mutex_lock(&device_hotplug_lock);
 }
+EXPORT_SYMBOL_GPL(lock_device_hotplug);
 
 void unlock_device_hotplug(void)
 {
 	mutex_unlock(&device_hotplug_lock);
 }
+EXPORT_SYMBOL_GPL(unlock_device_hotplug);
 
 int lock_device_hotplug_sysfs(void)
 {
@@ -2679,6 +2681,7 @@ int device_offline(struct device *dev)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(device_offline);
 
 /**
  * device_online - Put the device back online after successful device_offline().
@@ -2710,6 +2713,7 @@ int device_online(struct device *dev)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(device_online);
 
 struct root_device {
 	struct device dev;
-- 
2.17.1

[PATCH v2 10/14] driver: xen: Replace cpu_up/down with device_online/offline

[Qais Yousef]

The core device API performs extra housekeeping bits that are missing
from directly calling cpu_up/down.

See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
serialization during LPM") for an example description of what might go
wrong.

This also prepares to make cpu_up/down a private interface for anything
but the cpu subsystem.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Juergen Gross <jgross@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: xen-devel@lists.xenproject.org
CC: linux-kernel@vger.kernel.org
---
 drivers/xen/cpu_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/cpu_hotplug.c b/drivers/xen/cpu_hotplug.c
index f192b6f42da9..ec975decb5de 100644
--- a/drivers/xen/cpu_hotplug.c
+++ b/drivers/xen/cpu_hotplug.c
@@ -94,7 +94,7 @@ static int setup_cpu_watcher(struct notifier_block *notifier,
 
 	for_each_possible_cpu(cpu) {
 		if (vcpu_online(cpu) == 0) {
-			(void)cpu_down(cpu);
+			device_offline(get_cpu_device(cpu));
 			set_cpu_present(cpu, false);
 		}
 	}
-- 
2.17.1

Re: [PATCH v2 10/14] driver: xen: Replace cpu_up/down with device_online/offline

[Jürgen Groß]

On 25.11.19 12:27, Qais Yousef wrote:
> The core device API performs extra housekeeping bits that are missing
> from directly calling cpu_up/down.
> 
> See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
> serialization during LPM") for an example description of what might go
> wrong.
> 
> This also prepares to make cpu_up/down a private interface for anything
> but the cpu subsystem.
> 
> Signed-off-by: Qais Yousef <qais.yousef@arm.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[PATCH v2 11/14] firmware: psci: Replace cpu_up/down with device_online/offline

[Qais Yousef]

The core device API performs extra housekeeping bits that are missing
from directly calling cpu_up/down.

See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
serialization during LPM") for an example description of what might go
wrong.

This also prepares to make cpu_up/down a private interface for anything
but the cpu subsystem.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Mark Rutland <mark.rutland@arm.com>
CC: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
CC: linux-arm-kernel@lists.infradead.org
CC: linux-kernel@vger.kernel.org
---
 drivers/firmware/psci/psci_checker.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/psci/psci_checker.c b/drivers/firmware/psci/psci_checker.c
index 6a445397771c..9e4b1bade659 100644
--- a/drivers/firmware/psci/psci_checker.c
+++ b/drivers/firmware/psci/psci_checker.c
@@ -83,8 +83,9 @@ static unsigned int down_and_up_cpus(const struct cpumask *cpus,
 	cpumask_clear(offlined_cpus);
 
 	/* Try to power down all CPUs in the mask. */
+	lock_device_hotplug();
 	for_each_cpu(cpu, cpus) {
-		int ret = cpu_down(cpu);
+		int ret = device_offline(get_cpu_device(cpu));
 
 		/*
 		 * cpu_down() checks the number of online CPUs before the TOS
@@ -116,7 +117,7 @@ static unsigned int down_and_up_cpus(const struct cpumask *cpus,
 
 	/* Try to power up all the CPUs that have been offlined. */
 	for_each_cpu(cpu, offlined_cpus) {
-		int ret = cpu_up(cpu);
+		int ret = device_online(get_cpu_device(cpu));
 
 		if (ret != 0) {
 			pr_err("Error occurred (%d) while trying "
@@ -126,6 +127,7 @@ static unsigned int down_and_up_cpus(const struct cpumask *cpus,
 			cpumask_clear_cpu(cpu, offlined_cpus);
 		}
 	}
+	unlock_device_hotplug();
 
 	/*
 	 * Something went bad at some point and some CPUs could not be turned
-- 
2.17.1

[PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Qais Yousef]

The core device API performs extra housekeeping bits that are missing
from directly calling cpu_up/down.

See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
serialization during LPM") for an example description of what might go
wrong.

This also prepares to make cpu_up/down a private interface for anything
but the cpu subsystem.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Davidlohr Bueso <dave@stgolabs.net>
CC: "Paul E. McKenney" <paulmck@kernel.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: linux-kernel@vger.kernel.org
---
 kernel/torture.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/kernel/torture.c b/kernel/torture.c
index 7c13f5558b71..12115024feb2 100644
--- a/kernel/torture.c
+++ b/kernel/torture.c
@@ -97,7 +97,9 @@ bool torture_offline(int cpu, long *n_offl_attempts, long *n_offl_successes,
 			 torture_type, cpu);
 	starttime = jiffies;
 	(*n_offl_attempts)++;
-	ret = cpu_down(cpu);
+	lock_device_hotplug();
+	ret = device_offline(get_cpu_device(cpu));
+	unlock_device_hotplug();
 	if (ret) {
 		if (verbose)
 			pr_alert("%s" TORTURE_FLAG
@@ -148,7 +150,9 @@ bool torture_online(int cpu, long *n_onl_attempts, long *n_onl_successes,
 			 torture_type, cpu);
 	starttime = jiffies;
 	(*n_onl_attempts)++;
-	ret = cpu_up(cpu);
+	lock_device_hotplug();
+	ret = device_online(get_cpu_device(cpu));
+	unlock_device_hotplug();
 	if (ret) {
 		if (verbose)
 			pr_alert("%s" TORTURE_FLAG
@@ -192,17 +196,20 @@ torture_onoff(void *arg)
 	for_each_online_cpu(cpu)
 		maxcpu = cpu;
 	WARN_ON(maxcpu < 0);
-	if (!IS_MODULE(CONFIG_TORTURE_TEST))
+	if (!IS_MODULE(CONFIG_TORTURE_TEST)) {
+		lock_device_hotplug();
 		for_each_possible_cpu(cpu) {
 			if (cpu_online(cpu))
 				continue;
-			ret = cpu_up(cpu);
+			ret = device_online(get_cpu_device(cpu));
 			if (ret && verbose) {
 				pr_alert("%s" TORTURE_FLAG
 					 "%s: Initial online %d: errno %d\n",
 					 __func__, torture_type, cpu, ret);
 			}
 		}
+		unlock_device_hotplug();
+	}
 
 	if (maxcpu == 0) {
 		VERBOSE_TOROUT_STRING("Only one CPU, so CPU-hotplug testing is disabled");
-- 
2.17.1

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Paul E. McKenney]

On Mon, Nov 25, 2019 at 11:27:52AM +0000, Qais Yousef wrote:
> The core device API performs extra housekeeping bits that are missing
> from directly calling cpu_up/down.
> 
> See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
> serialization during LPM") for an example description of what might go
> wrong.
> 
> This also prepares to make cpu_up/down a private interface for anything
> but the cpu subsystem.
> 
> Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> CC: Davidlohr Bueso <dave@stgolabs.net>
> CC: "Paul E. McKenney" <paulmck@kernel.org>
> CC: Josh Triplett <josh@joshtriplett.org>
> CC: linux-kernel@vger.kernel.org

Looks fine from an rcutorture viewpoint, but why not provide an API
that pulled lock_device_hotplug() and unlock_device_hotplug() into the
online/offline calls?

							Thanx, Paul

> ---
>  kernel/torture.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/torture.c b/kernel/torture.c
> index 7c13f5558b71..12115024feb2 100644
> --- a/kernel/torture.c
> +++ b/kernel/torture.c
> @@ -97,7 +97,9 @@ bool torture_offline(int cpu, long *n_offl_attempts, long *n_offl_successes,
>  			 torture_type, cpu);
>  	starttime = jiffies;
>  	(*n_offl_attempts)++;
> -	ret = cpu_down(cpu);
> +	lock_device_hotplug();
> +	ret = device_offline(get_cpu_device(cpu));
> +	unlock_device_hotplug();
>  	if (ret) {
>  		if (verbose)
>  			pr_alert("%s" TORTURE_FLAG
> @@ -148,7 +150,9 @@ bool torture_online(int cpu, long *n_onl_attempts, long *n_onl_successes,
>  			 torture_type, cpu);
>  	starttime = jiffies;
>  	(*n_onl_attempts)++;
> -	ret = cpu_up(cpu);
> +	lock_device_hotplug();
> +	ret = device_online(get_cpu_device(cpu));
> +	unlock_device_hotplug();
>  	if (ret) {
>  		if (verbose)
>  			pr_alert("%s" TORTURE_FLAG
> @@ -192,17 +196,20 @@ torture_onoff(void *arg)
>  	for_each_online_cpu(cpu)
>  		maxcpu = cpu;
>  	WARN_ON(maxcpu < 0);
> -	if (!IS_MODULE(CONFIG_TORTURE_TEST))
> +	if (!IS_MODULE(CONFIG_TORTURE_TEST)) {
> +		lock_device_hotplug();
>  		for_each_possible_cpu(cpu) {
>  			if (cpu_online(cpu))
>  				continue;
> -			ret = cpu_up(cpu);
> +			ret = device_online(get_cpu_device(cpu));
>  			if (ret && verbose) {
>  				pr_alert("%s" TORTURE_FLAG
>  					 "%s: Initial online %d: errno %d\n",
>  					 __func__, torture_type, cpu, ret);
>  			}
>  		}
> +		unlock_device_hotplug();
> +	}
>  
>  	if (maxcpu == 0) {
>  		VERBOSE_TOROUT_STRING("Only one CPU, so CPU-hotplug testing is disabled");
> -- 
> 2.17.1
> 

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Qais Yousef]

On 11/27/19 13:47, Paul E. McKenney wrote:
> On Mon, Nov 25, 2019 at 11:27:52AM +0000, Qais Yousef wrote:
> > The core device API performs extra housekeeping bits that are missing
> > from directly calling cpu_up/down.
> > 
> > See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
> > serialization during LPM") for an example description of what might go
> > wrong.
> > 
> > This also prepares to make cpu_up/down a private interface for anything
> > but the cpu subsystem.
> > 
> > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > CC: Davidlohr Bueso <dave@stgolabs.net>
> > CC: "Paul E. McKenney" <paulmck@kernel.org>
> > CC: Josh Triplett <josh@joshtriplett.org>
> > CC: linux-kernel@vger.kernel.org
> 
> Looks fine from an rcutorture viewpoint, but why not provide an API
> that pulled lock_device_hotplug() and unlock_device_hotplug() into the
> online/offline calls?

I *think* the right way to do what you say is by doing lock_device_hotplug()
inside device_{online, offline}() - which affects all drivers not just the CPU.

And even then, I think we need to refcount it so nested calls won't deadlock.

I don't know if this can break any rule or not. If Greg thinks it's okay I'd be
happy to post some patches that do just that.

Thanks

--
Qais Yousef

> 
> 							Thanx, Paul
> 
> > ---
> >  kernel/torture.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/torture.c b/kernel/torture.c
> > index 7c13f5558b71..12115024feb2 100644
> > --- a/kernel/torture.c
> > +++ b/kernel/torture.c
> > @@ -97,7 +97,9 @@ bool torture_offline(int cpu, long *n_offl_attempts, long *n_offl_successes,
> >  			 torture_type, cpu);
> >  	starttime = jiffies;
> >  	(*n_offl_attempts)++;
> > -	ret = cpu_down(cpu);
> > +	lock_device_hotplug();
> > +	ret = device_offline(get_cpu_device(cpu));
> > +	unlock_device_hotplug();
> >  	if (ret) {
> >  		if (verbose)
> >  			pr_alert("%s" TORTURE_FLAG
> > @@ -148,7 +150,9 @@ bool torture_online(int cpu, long *n_onl_attempts, long *n_onl_successes,
> >  			 torture_type, cpu);
> >  	starttime = jiffies;
> >  	(*n_onl_attempts)++;
> > -	ret = cpu_up(cpu);
> > +	lock_device_hotplug();
> > +	ret = device_online(get_cpu_device(cpu));
> > +	unlock_device_hotplug();
> >  	if (ret) {
> >  		if (verbose)
> >  			pr_alert("%s" TORTURE_FLAG
> > @@ -192,17 +196,20 @@ torture_onoff(void *arg)
> >  	for_each_online_cpu(cpu)
> >  		maxcpu = cpu;
> >  	WARN_ON(maxcpu < 0);
> > -	if (!IS_MODULE(CONFIG_TORTURE_TEST))
> > +	if (!IS_MODULE(CONFIG_TORTURE_TEST)) {
> > +		lock_device_hotplug();
> >  		for_each_possible_cpu(cpu) {
> >  			if (cpu_online(cpu))
> >  				continue;
> > -			ret = cpu_up(cpu);
> > +			ret = device_online(get_cpu_device(cpu));
> >  			if (ret && verbose) {
> >  				pr_alert("%s" TORTURE_FLAG
> >  					 "%s: Initial online %d: errno %d\n",
> >  					 __func__, torture_type, cpu, ret);
> >  			}
> >  		}
> > +		unlock_device_hotplug();
> > +	}
> >  
> >  	if (maxcpu == 0) {
> >  		VERBOSE_TOROUT_STRING("Only one CPU, so CPU-hotplug testing is disabled");
> > -- 
> > 2.17.1
> > 

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Qais Yousef]

On 11/28/19 16:56, Qais Yousef wrote:
> On 11/27/19 13:47, Paul E. McKenney wrote:
> > On Mon, Nov 25, 2019 at 11:27:52AM +0000, Qais Yousef wrote:
> > > The core device API performs extra housekeeping bits that are missing
> > > from directly calling cpu_up/down.
> > > 
> > > See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
> > > serialization during LPM") for an example description of what might go
> > > wrong.
> > > 
> > > This also prepares to make cpu_up/down a private interface for anything
> > > but the cpu subsystem.
> > > 
> > > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > > CC: Davidlohr Bueso <dave@stgolabs.net>
> > > CC: "Paul E. McKenney" <paulmck@kernel.org>
> > > CC: Josh Triplett <josh@joshtriplett.org>
> > > CC: linux-kernel@vger.kernel.org
> > 
> > Looks fine from an rcutorture viewpoint, but why not provide an API
> > that pulled lock_device_hotplug() and unlock_device_hotplug() into the
> > online/offline calls?
> 
> I *think* the right way to do what you say is by doing lock_device_hotplug()
> inside device_{online, offline}() - which affects all drivers not just the CPU.
> 
> And even then, I think we need to refcount it so nested calls won't deadlock.

Forget that. I don't think nesting here makes actually any sense.

Thanks

--
Qais Yousef

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Paul E. McKenney]

On Thu, Nov 28, 2019 at 05:00:26PM +0000, Qais Yousef wrote:
> On 11/28/19 16:56, Qais Yousef wrote:
> > On 11/27/19 13:47, Paul E. McKenney wrote:
> > > On Mon, Nov 25, 2019 at 11:27:52AM +0000, Qais Yousef wrote:
> > > > The core device API performs extra housekeeping bits that are missing
> > > > from directly calling cpu_up/down.
> > > > 
> > > > See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
> > > > serialization during LPM") for an example description of what might go
> > > > wrong.
> > > > 
> > > > This also prepares to make cpu_up/down a private interface for anything
> > > > but the cpu subsystem.
> > > > 
> > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > > > CC: Davidlohr Bueso <dave@stgolabs.net>
> > > > CC: "Paul E. McKenney" <paulmck@kernel.org>
> > > > CC: Josh Triplett <josh@joshtriplett.org>
> > > > CC: linux-kernel@vger.kernel.org
> > > 
> > > Looks fine from an rcutorture viewpoint, but why not provide an API
> > > that pulled lock_device_hotplug() and unlock_device_hotplug() into the
> > > online/offline calls?
> > 
> > I *think* the right way to do what you say is by doing lock_device_hotplug()
> > inside device_{online, offline}() - which affects all drivers not just the CPU.

Or there could be a CPU-specific wrapper function that did the needed
locking.  (Whether this is worth it or not of course depends on the
number of invocations.)

							Thanx, Paul

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Qais Yousef]

On 11/28/19 13:02, Paul E. McKenney wrote:
> On Thu, Nov 28, 2019 at 05:00:26PM +0000, Qais Yousef wrote:
> > On 11/28/19 16:56, Qais Yousef wrote:
> > > On 11/27/19 13:47, Paul E. McKenney wrote:
> > > > On Mon, Nov 25, 2019 at 11:27:52AM +0000, Qais Yousef wrote:
> > > > > The core device API performs extra housekeeping bits that are missing
> > > > > from directly calling cpu_up/down.
> > > > > 
> > > > > See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
> > > > > serialization during LPM") for an example description of what might go
> > > > > wrong.
> > > > > 
> > > > > This also prepares to make cpu_up/down a private interface for anything
> > > > > but the cpu subsystem.
> > > > > 
> > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > > > > CC: Davidlohr Bueso <dave@stgolabs.net>
> > > > > CC: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > CC: Josh Triplett <josh@joshtriplett.org>
> > > > > CC: linux-kernel@vger.kernel.org
> > > > 
> > > > Looks fine from an rcutorture viewpoint, but why not provide an API
> > > > that pulled lock_device_hotplug() and unlock_device_hotplug() into the
> > > > online/offline calls?
> > > 
> > > I *think* the right way to do what you say is by doing lock_device_hotplug()
> > > inside device_{online, offline}() - which affects all drivers not just the CPU.
> 
> Or there could be a CPU-specific wrapper function that did the needed
> locking.  (Whether this is worth it or not of course depends on the
> number of invocations.)

Okay I see what you mean now. driver/base/memory.c have {add,remove}_memory()
that does what you say. I think we can replicate this in driver/base/cpu.c too.

I can certainly do that, better as an improvement on top as I need to audit the
code to make sure the critical sections weren't relying on this lock to protect
something else beside the online/offline operation.

Thanks!

--
Qais Yousef

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Paul E. McKenney]

On Fri, Nov 29, 2019 at 09:13:45AM +0000, Qais Yousef wrote:
> On 11/28/19 13:02, Paul E. McKenney wrote:
> > On Thu, Nov 28, 2019 at 05:00:26PM +0000, Qais Yousef wrote:
> > > On 11/28/19 16:56, Qais Yousef wrote:
> > > > On 11/27/19 13:47, Paul E. McKenney wrote:
> > > > > On Mon, Nov 25, 2019 at 11:27:52AM +0000, Qais Yousef wrote:
> > > > > > The core device API performs extra housekeeping bits that are missing
> > > > > > from directly calling cpu_up/down.
> > > > > > 
> > > > > > See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
> > > > > > serialization during LPM") for an example description of what might go
> > > > > > wrong.
> > > > > > 
> > > > > > This also prepares to make cpu_up/down a private interface for anything
> > > > > > but the cpu subsystem.
> > > > > > 
> > > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > > > > > CC: Davidlohr Bueso <dave@stgolabs.net>
> > > > > > CC: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > CC: Josh Triplett <josh@joshtriplett.org>
> > > > > > CC: linux-kernel@vger.kernel.org
> > > > > 
> > > > > Looks fine from an rcutorture viewpoint, but why not provide an API
> > > > > that pulled lock_device_hotplug() and unlock_device_hotplug() into the
> > > > > online/offline calls?
> > > > 
> > > > I *think* the right way to do what you say is by doing lock_device_hotplug()
> > > > inside device_{online, offline}() - which affects all drivers not just the CPU.
> > 
> > Or there could be a CPU-specific wrapper function that did the needed
> > locking.  (Whether this is worth it or not of course depends on the
> > number of invocations.)
> 
> Okay I see what you mean now. driver/base/memory.c have {add,remove}_memory()
> that does what you say. I think we can replicate this in driver/base/cpu.c too.
> 
> I can certainly do that, better as an improvement on top as I need to audit the
> code to make sure the critical sections weren't relying on this lock to protect
> something else beside the online/offline operation.

Works for me!

							Thanx, Paul

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Qais Yousef]

On 11/29/19 12:38, Paul E. McKenney wrote:
> On Fri, Nov 29, 2019 at 09:13:45AM +0000, Qais Yousef wrote:
> > On 11/28/19 13:02, Paul E. McKenney wrote:
> > > On Thu, Nov 28, 2019 at 05:00:26PM +0000, Qais Yousef wrote:
> > > > On 11/28/19 16:56, Qais Yousef wrote:
> > > > > On 11/27/19 13:47, Paul E. McKenney wrote:
> > > > > > On Mon, Nov 25, 2019 at 11:27:52AM +0000, Qais Yousef wrote:
> > > > > > > The core device API performs extra housekeeping bits that are missing
> > > > > > > from directly calling cpu_up/down.
> > > > > > > 
> > > > > > > See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
> > > > > > > serialization during LPM") for an example description of what might go
> > > > > > > wrong.
> > > > > > > 
> > > > > > > This also prepares to make cpu_up/down a private interface for anything
> > > > > > > but the cpu subsystem.
> > > > > > > 
> > > > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > > > > > > CC: Davidlohr Bueso <dave@stgolabs.net>
> > > > > > > CC: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > > CC: Josh Triplett <josh@joshtriplett.org>
> > > > > > > CC: linux-kernel@vger.kernel.org
> > > > > > 
> > > > > > Looks fine from an rcutorture viewpoint, but why not provide an API
> > > > > > that pulled lock_device_hotplug() and unlock_device_hotplug() into the
> > > > > > online/offline calls?
> > > > > 
> > > > > I *think* the right way to do what you say is by doing lock_device_hotplug()
> > > > > inside device_{online, offline}() - which affects all drivers not just the CPU.
> > > 
> > > Or there could be a CPU-specific wrapper function that did the needed
> > > locking.  (Whether this is worth it or not of course depends on the
> > > number of invocations.)
> > 
> > Okay I see what you mean now. driver/base/memory.c have {add,remove}_memory()
> > that does what you say. I think we can replicate this in driver/base/cpu.c too.
> > 
> > I can certainly do that, better as an improvement on top as I need to audit the
> > code to make sure the critical sections weren't relying on this lock to protect
> > something else beside the online/offline operation.
> 
> Works for me!

I'm taking that as reviewed-by, which I'll add to v3. Please shout if you still
need to have a look further.

Once this is taken I'll add the suggested API!

Thanks

--
Qais Yousef

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Paul E. McKenney]

On Thu, Feb 20, 2020 at 03:31:59PM +0000, Qais Yousef wrote:
> On 11/29/19 12:38, Paul E. McKenney wrote:
> > On Fri, Nov 29, 2019 at 09:13:45AM +0000, Qais Yousef wrote:
> > > On 11/28/19 13:02, Paul E. McKenney wrote:
> > > > On Thu, Nov 28, 2019 at 05:00:26PM +0000, Qais Yousef wrote:
> > > > > On 11/28/19 16:56, Qais Yousef wrote:
> > > > > > On 11/27/19 13:47, Paul E. McKenney wrote:
> > > > > > > On Mon, Nov 25, 2019 at 11:27:52AM +0000, Qais Yousef wrote:
> > > > > > > > The core device API performs extra housekeeping bits that are missing
> > > > > > > > from directly calling cpu_up/down.
> > > > > > > > 
> > > > > > > > See commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and
> > > > > > > > serialization during LPM") for an example description of what might go
> > > > > > > > wrong.
> > > > > > > > 
> > > > > > > > This also prepares to make cpu_up/down a private interface for anything
> > > > > > > > but the cpu subsystem.
> > > > > > > > 
> > > > > > > > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> > > > > > > > CC: Davidlohr Bueso <dave@stgolabs.net>
> > > > > > > > CC: "Paul E. McKenney" <paulmck@kernel.org>
> > > > > > > > CC: Josh Triplett <josh@joshtriplett.org>
> > > > > > > > CC: linux-kernel@vger.kernel.org
> > > > > > > 
> > > > > > > Looks fine from an rcutorture viewpoint, but why not provide an API
> > > > > > > that pulled lock_device_hotplug() and unlock_device_hotplug() into the
> > > > > > > online/offline calls?
> > > > > > 
> > > > > > I *think* the right way to do what you say is by doing lock_device_hotplug()
> > > > > > inside device_{online, offline}() - which affects all drivers not just the CPU.
> > > > 
> > > > Or there could be a CPU-specific wrapper function that did the needed
> > > > locking.  (Whether this is worth it or not of course depends on the
> > > > number of invocations.)
> > > 
> > > Okay I see what you mean now. driver/base/memory.c have {add,remove}_memory()
> > > that does what you say. I think we can replicate this in driver/base/cpu.c too.
> > > 
> > > I can certainly do that, better as an improvement on top as I need to audit the
> > > code to make sure the critical sections weren't relying on this lock to protect
> > > something else beside the online/offline operation.
> > 
> > Works for me!
> 
> I'm taking that as reviewed-by, which I'll add to v3. Please shout if you still
> need to have a look further.
> 
> Once this is taken I'll add the suggested API!

OK, I will bite...

Why not right now?

							Thanx, Paul

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Qais Yousef]

On 02/20/20 16:26, Paul E. McKenney wrote:
> > I'm taking that as reviewed-by, which I'll add to v3. Please shout if you still
> > need to have a look further.
> > 
> > Once this is taken I'll add the suggested API!
> 
> OK, I will bite...
> 
> Why not right now?

Sigh. Good question. Probably I'm just being lame; it just felt the series is a
bit fragile spanning that many archs and was wary introducing some extra
changes on top will make it even harder to get merged soon.

Let me go and do it. You're probably right and it shouldn't really create a big
ripple on the series.

Thanks!

--
Qais Yousef

Re: [PATCH v2 12/14] torture: Replace cpu_up/down with device_online/offline

[Paul E. McKenney]

On Fri, Feb 21, 2020 at 09:35:05AM +0000, Qais Yousef wrote:
> On 02/20/20 16:26, Paul E. McKenney wrote:
> > > I'm taking that as reviewed-by, which I'll add to v3. Please shout if you still
> > > need to have a look further.
> > > 
> > > Once this is taken I'll add the suggested API!
> > 
> > OK, I will bite...
> > 
> > Why not right now?
> 
> Sigh. Good question. Probably I'm just being lame; it just felt the series is a
> bit fragile spanning that many archs and was wary introducing some extra
> changes on top will make it even harder to get merged soon.
> 
> Let me go and do it. You're probably right and it shouldn't really create a big
> ripple on the series.

Very good, thank you!

							Thanx, Paul

[PATCH v2 13/14] smp: Create a new function to bringup nonboot cpus online

[Qais Yousef]

This is the last direct user of cpu_up() before we can hide it now as
internal implementation detail of the cpu subsystem.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Josh Poimboeuf <jpoimboe@redhat.com>
CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
CC: Jiri Kosina <jkosina@suse.cz>
CC: Nicholas Piggin <npiggin@gmail.com>
CC: Daniel Lezcano <daniel.lezcano@linaro.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Eiichi Tsukata <devel@etsukata.com>
CC: Zhenzhong Duan <zhenzhong.duan@oracle.com>
CC: Nadav Amit <namit@vmware.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
CC: linux-kernel@vger.kernel.org
---
 include/linux/cpu.h |  1 +
 kernel/cpu.c        | 12 ++++++++++++
 kernel/smp.c        |  9 +--------
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index f05168b49fab..b5d9287899d1 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -93,6 +93,7 @@ void notify_cpu_starting(unsigned int cpu);
 extern void cpu_maps_update_begin(void);
 extern void cpu_maps_update_done(void);
 extern int bringup_hibernate_cpu(unsigned int sleep_cpu);
+extern void smp_bringup_nonboot_cpus(unsigned int setup_max_cpus);
 
 #else	/* CONFIG_SMP */
 #define cpuhp_tasks_frozen	0
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 9610442c8dbc..3631184a284f 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1245,6 +1245,18 @@ int bringup_hibernate_cpu(unsigned int sleep_cpu)
 	return 0;
 }
 
+void smp_bringup_nonboot_cpus(unsigned int setup_max_cpus)
+{
+	unsigned int cpu;
+
+	for_each_present_cpu(cpu) {
+		if (num_online_cpus() >= setup_max_cpus)
+			break;
+		if (!cpu_online(cpu))
+			cpu_up(cpu);
+	}
+}
+
 #ifdef CONFIG_PM_SLEEP_SMP
 static cpumask_var_t frozen_cpus;
 
diff --git a/kernel/smp.c b/kernel/smp.c
index 7dbcb402c2fc..74134272b5aa 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -578,20 +578,13 @@ void __init setup_nr_cpu_ids(void)
 void __init smp_init(void)
 {
 	int num_nodes, num_cpus;
-	unsigned int cpu;
 
 	idle_threads_init();
 	cpuhp_threads_init();
 
 	pr_info("Bringing up secondary CPUs ...\n");
 
-	/* FIXME: This should be done in userspace --RR */
-	for_each_present_cpu(cpu) {
-		if (num_online_cpus() >= setup_max_cpus)
-			break;
-		if (!cpu_online(cpu))
-			cpu_up(cpu);
-	}
+	smp_bringup_nonboot_cpus(setup_max_cpus);
 
 	num_nodes = num_online_nodes();
 	num_cpus  = num_online_cpus();
-- 
2.17.1

[PATCH v2 14/14] cpu: Hide cpu_up/down

[Qais Yousef]

Provide a special exported function for the device core to bring a cpu
up/down and hide the real cpu_up/down as they are treated as private
functions. cpu_up/down are lower level API and users outside the cpu
subsystem should use device_online/offline which will take care of extra
housekeeping work like keeping sysfs in sync.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Josh Poimboeuf <jpoimboe@redhat.com>
CC: Nicholas Piggin <npiggin@gmail.com>
CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
CC: Jiri Kosina <jkosina@suse.cz>
CC: Daniel Lezcano <daniel.lezcano@linaro.org>
CC: Eiichi Tsukata <devel@etsukata.com>
CC: Zhenzhong Duan <zhenzhong.duan@oracle.com>
CC: Ingo Molnar <mingo@kernel.org>
CC: Pavankumar Kondeti <pkondeti@codeaurora.org>
CC: linux-kernel@vger.kernel.org
---
 drivers/base/cpu.c  |  4 ++--
 include/linux/cpu.h |  4 ++--
 kernel/cpu.c        | 32 ++++++++++++++++++++++++++++----
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 6265871a4af2..9a134cd362ee 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -55,7 +55,7 @@ static int cpu_subsys_online(struct device *dev)
 	if (from_nid == NUMA_NO_NODE)
 		return -ENODEV;
 
-	ret = cpu_up(cpuid);
+	ret = cpu_subsys_up(dev);
 	/*
 	 * When hot adding memory to memoryless node and enabling a cpu
 	 * on the node, node number of the cpu may internally change.
@@ -69,7 +69,7 @@ static int cpu_subsys_online(struct device *dev)
 
 static int cpu_subsys_offline(struct device *dev)
 {
-	return cpu_down(dev->id);
+	return cpu_subsys_down(dev);
 }
 
 void unregister_cpu(struct cpu *cpu)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index b5d9287899d1..bd9c5870c26f 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -88,7 +88,7 @@ extern ssize_t arch_cpu_release(const char *, size_t);
 
 #ifdef CONFIG_SMP
 extern bool cpuhp_tasks_frozen;
-int cpu_up(unsigned int cpu);
+int cpu_subsys_up(struct device *dev);
 void notify_cpu_starting(unsigned int cpu);
 extern void cpu_maps_update_begin(void);
 extern void cpu_maps_update_done(void);
@@ -119,7 +119,7 @@ extern void lockdep_assert_cpus_held(void);
 extern void cpu_hotplug_disable(void);
 extern void cpu_hotplug_enable(void);
 void clear_tasks_mm_cpumask(int cpu);
-int cpu_down(unsigned int cpu);
+int cpu_subsys_down(struct device *dev);
 extern void smp_shutdown_nonboot_cpus(unsigned int primary_cpu);
 
 #else /* CONFIG_HOTPLUG_CPU */
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 3631184a284f..8b29e0aa95ba 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1052,11 +1052,23 @@ static int do_cpu_down(unsigned int cpu, enum cpuhp_state target)
 	return err;
 }
 
-int cpu_down(unsigned int cpu)
+static int cpu_down(unsigned int cpu)
 {
 	return do_cpu_down(cpu, CPUHP_OFFLINE);
 }
-EXPORT_SYMBOL(cpu_down);
+
+/**
+ * cpu_subsys_down - Bring down a cpu device
+ * @dev: Pointer to the cpu device to offline
+ *
+ * This function is meant to be used by device core cpu subsystem only.
+ *
+ * Other subsystems should use device_offline(get_cpu_device(cpu)) instead.
+ */
+int cpu_subsys_down(struct device *dev)
+{
+	return cpu_down(dev->id);
+}
 
 void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
 {
@@ -1215,11 +1227,23 @@ static int do_cpu_up(unsigned int cpu, enum cpuhp_state target)
 	return err;
 }
 
-int cpu_up(unsigned int cpu)
+static int cpu_up(unsigned int cpu)
 {
 	return do_cpu_up(cpu, CPUHP_ONLINE);
 }
-EXPORT_SYMBOL_GPL(cpu_up);
+
+/**
+ * cpu_subsys_up - Bring up a cpu device
+ * @dev: Pointer to the cpu device to online
+ *
+ * This function is meant to be used by device core cpu subsystem only.
+ *
+ * Other subsystems should use device_online(get_cpu_device(cpu)) instead.
+ */
+int cpu_subsys_up(struct device *dev)
+{
+	return cpu_up(dev->id);
+}
 
 /**
  * bringup_hibernate_cpu - Bring up the CPU that we hibernated on
-- 
2.17.1