[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel