[PATCH 00/12] Convert cpu_up/down to device_online/offline

[PATCH 00/12] Convert cpu_up/down to device_online/offline

[Qais Yousef]

Using cpu_up/down directly to bring cpus online/offline loses synchronization
with sysfs and could suffer from a race similar to what is described in
commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and serialization
during LPM").

cpu_up/down seem to be more of a internal implementation detail for the cpu
subsystem to use to boot up cpus, perform suspend/resume and low level hotplug
operations. Users outside of the cpu subsystem would be better using the device
core API to bring a cpu online/offline which is the interface used to hotplug
memory and other system devices.

Several users have already migrated to use the device core API, this series
converts the remaining users and hides cpu_up/down from internal users at the
end.

I still need to update the documentation to remove references to cpu_up/down
and advocate for device_online/offline instead if this series will make its way
through.

I noticed this problem while working on a hack to disable offlining
a particular CPU but noticed that setting the offline_disabled attribute in the
device struct isn't enough because users can easily bypass the device core.
While my hack isn't a valid use case but it did highlight the inconsistency in
the way cpus are being onlined/offlined and this attempt hopefully improves on
this.

The first 6 patches fixes arch users.

The next 5 patches fixes generic code users. Particularly creating a new
special exported API for the device core to use instead of cpu_up/down.
Maybe we can do something more restrictive than that.

The last patch removes cpu_up/down from cpu.h and unexport the functions.

In some cases where the use of cpu_up/down seemed legitimate, I encapsulated
the logic in a higher level - special purposed function; and converted the code
to use that instead.

I did run the rcu torture, lock torture and psci checker tests and no problem
was noticed. I did perform build tests on all arch affected except for parisc.

Hopefully I got the CC list right for all the patches. Apologies in advance if
some people were omitted from some patches but they should have been CCed.

CC: Armijn Hemel <armijn@tjaldur.nl>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Christophe Leroy <christophe.leroy@c-s.fr>
CC: Daniel Lezcano <daniel.lezcano@linaro.org>
CC: Davidlohr Bueso <dave@stgolabs.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eiichi Tsukata <devel@etsukata.com>
CC: Enrico Weigelt <info@metux.net>
CC: Fenghua Yu <fenghua.yu@intel.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Helge Deller <deller@gmx.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@kernel.org>
CC: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
CC: James Morse <james.morse@arm.com>
CC: Jiri Kosina <jkosina@suse.cz>
CC: Josh Poimboeuf <jpoimboe@redhat.com>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Juergen Gross <jgross@suse.com>
CC: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
CC: Mark Rutland <mark.rutland@arm.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Nadav Amit <namit@vmware.com>
CC: Nicholas Piggin <npiggin@gmail.com>
CC: "Paul E. McKenney" <paulmck@kernel.org>
CC: Paul Mackerras <paulus@samba.org>
CC: Pavankumar Kondeti <pkondeti@codeaurora.org>
CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Ram Pai <linuxram@us.ibm.com>
CC: Richard Fontana <rfontana@redhat.com>
CC: Sakari Ailus <sakari.ailus@linux.intel.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Steve Capper <steve.capper@arm.com>
CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Tony Luck <tony.luck@intel.com>
CC: Will Deacon <will@kernel.org>
CC: Zhenzhong Duan <zhenzhong.duan@oracle.com>
CC: linux-arm-kernel@lists.infradead.org
CC: linux-ia64@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-parisc@vger.kernel.org
CC: linuxppc-dev@lists.ozlabs.org
CC: sparclinux@vger.kernel.org
CC: x86@kernel.org
CC: xen-devel@lists.xenproject.org


Qais Yousef (12):
  arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)
  x86: Replace cpu_up/down with devcie_online/offline
  powerpc: Replace cpu_up/down with device_online/offline
  ia64: Replace cpu_down with freeze_secondary_cpus
  sparc: Replace cpu_up/down with device_online/offline
  parisc: Replace cpu_up/down with device_online/offline
  driver: base: cpu: export device_online/offline
  driver: xen: Replace cpu_up/down with device_online/offline
  firmware: psci: Replace cpu_up/down with device_online/offline
  torture: Replace cpu_up/down with device_online/offline
  smp: Create a new function to bringup nonboot cpus online
  cpu: Hide cpu_up/down

 arch/arm64/kernel/hibernate.c          | 13 +++----
 arch/ia64/kernel/process.c             |  8 +---
 arch/parisc/kernel/processor.c         |  4 +-
 arch/powerpc/kernel/machine_kexec_64.c |  4 +-
 arch/sparc/kernel/ds.c                 |  8 +++-
 arch/x86/kernel/topology.c             |  4 +-
 arch/x86/mm/mmio-mod.c                 |  8 +++-
 arch/x86/xen/smp.c                     |  4 +-
 drivers/base/core.c                    |  4 ++
 drivers/base/cpu.c                     |  4 +-
 drivers/firmware/psci/psci_checker.c   |  6 ++-
 drivers/xen/cpu_hotplug.c              |  2 +-
 include/linux/cpu.h                    |  6 ++-
 kernel/cpu.c                           | 53 ++++++++++++++++++++++++--
 kernel/smp.c                           |  9 +----
 kernel/torture.c                       | 15 ++++++--
 16 files changed, 106 insertions(+), 46 deletions(-)

-- 
2.17.1

[Xen-devel] [PATCH 00/12] Convert cpu_up/down to device_online/offline

[Qais Yousef]

Using cpu_up/down directly to bring cpus online/offline loses synchronization
with sysfs and could suffer from a race similar to what is described in
commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and serialization
during LPM").

cpu_up/down seem to be more of a internal implementation detail for the cpu
subsystem to use to boot up cpus, perform suspend/resume and low level hotplug
operations. Users outside of the cpu subsystem would be better using the device
core API to bring a cpu online/offline which is the interface used to hotplug
memory and other system devices.

Several users have already migrated to use the device core API, this series
converts the remaining users and hides cpu_up/down from internal users at the
end.

I still need to update the documentation to remove references to cpu_up/down
and advocate for device_online/offline instead if this series will make its way
through.

I noticed this problem while working on a hack to disable offlining
a particular CPU but noticed that setting the offline_disabled attribute in the
device struct isn't enough because users can easily bypass the device core.
While my hack isn't a valid use case but it did highlight the inconsistency in
the way cpus are being onlined/offlined and this attempt hopefully improves on
this.

The first 6 patches fixes arch users.

The next 5 patches fixes generic code users. Particularly creating a new
special exported API for the device core to use instead of cpu_up/down.
Maybe we can do something more restrictive than that.

The last patch removes cpu_up/down from cpu.h and unexport the functions.

In some cases where the use of cpu_up/down seemed legitimate, I encapsulated
the logic in a higher level - special purposed function; and converted the code
to use that instead.

I did run the rcu torture, lock torture and psci checker tests and no problem
was noticed. I did perform build tests on all arch affected except for parisc.

Hopefully I got the CC list right for all the patches. Apologies in advance if
some people were omitted from some patches but they should have been CCed.

CC: Armijn Hemel <armijn@tjaldur.nl>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Christophe Leroy <christophe.leroy@c-s.fr>
CC: Daniel Lezcano <daniel.lezcano@linaro.org>
CC: Davidlohr Bueso <dave@stgolabs.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eiichi Tsukata <devel@etsukata.com>
CC: Enrico Weigelt <info@metux.net>
CC: Fenghua Yu <fenghua.yu@intel.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Helge Deller <deller@gmx.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@kernel.org>
CC: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
CC: James Morse <james.morse@arm.com>
CC: Jiri Kosina <jkosina@suse.cz>
CC: Josh Poimboeuf <jpoimboe@redhat.com>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Juergen Gross <jgross@suse.com>
CC: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
CC: Mark Rutland <mark.rutland@arm.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Nadav Amit <namit@vmware.com>
CC: Nicholas Piggin <npiggin@gmail.com>
CC: "Paul E. McKenney" <paulmck@kernel.org>
CC: Paul Mackerras <paulus@samba.org>
CC: Pavankumar Kondeti <pkondeti@codeaurora.org>
CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>
CC: Ram Pai <linuxram@us.ibm.com>
CC: Richard Fontana <rfontana@redhat.com>
CC: Sakari Ailus <sakari.ailus@linux.intel.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Steve Capper <steve.capper@arm.com>
CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Tony Luck <tony.luck@intel.com>
CC: Will Deacon <will@kernel.org>
CC: Zhenzhong Duan <zhenzhong.duan@oracle.com>
CC: linux-arm-kernel@lists.infradead.org
CC: linux-ia64@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-parisc@vger.kernel.org
CC: linuxppc-dev@lists.ozlabs.org
CC: sparclinux@vger.kernel.org
CC: x86@kernel.org
CC: xen-devel@lists.xenproject.org


Qais Yousef (12):
  arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)
  x86: Replace cpu_up/down with devcie_online/offline
  powerpc: Replace cpu_up/down with device_online/offline
  ia64: Replace cpu_down with freeze_secondary_cpus
  sparc: Replace cpu_up/down with device_online/offline
  parisc: Replace cpu_up/down with device_online/offline
  driver: base: cpu: export device_online/offline
  driver: xen: Replace cpu_up/down with device_online/offline
  firmware: psci: Replace cpu_up/down with device_online/offline
  torture: Replace cpu_up/down with device_online/offline
  smp: Create a new function to bringup nonboot cpus online
  cpu: Hide cpu_up/down

 arch/arm64/kernel/hibernate.c          | 13 +++----
 arch/ia64/kernel/process.c             |  8 +---
 arch/parisc/kernel/processor.c         |  4 +-
 arch/powerpc/kernel/machine_kexec_64.c |  4 +-
 arch/sparc/kernel/ds.c                 |  8 +++-
 arch/x86/kernel/topology.c             |  4 +-
 arch/x86/mm/mmio-mod.c                 |  8 +++-
 arch/x86/xen/smp.c                     |  4 +-
 drivers/base/core.c                    |  4 ++
 drivers/base/cpu.c                     |  4 +-
 drivers/firmware/psci/psci_checker.c   |  6 ++-
 drivers/xen/cpu_hotplug.c              |  2 +-
 include/linux/cpu.h                    |  6 ++-
 kernel/cpu.c                           | 53 ++++++++++++++++++++++++--
 kernel/smp.c                           |  9 +----
 kernel/torture.c                       | 15 ++++++--
 16 files changed, 106 insertions(+), 46 deletions(-)

-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 01/12] 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
---

AFAICT we can't use device_online() directly here because suspend happens via
cpu_down() not device_offline(). If it is actually safe to use device_online()
then that would be simpler than creating the new function. Although the
operation seems generic enough to me and could benefit another arch user in the
future so the new function makes sense.


 arch/arm64/kernel/hibernate.c | 13 +++++--------
 include/linux/cpu.h           |  1 +
 kernel/cpu.c                  | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index e0a7fce0e01c..3b178055022f 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 = hibernation_bringup_sleep_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 88dc0c653925..3b1fbe192989 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -87,6 +87,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 hibernation_bringup_sleep_cpu(unsigned int sleep_cpu);
 
 #else	/* CONFIG_SMP */
 #define cpuhp_tasks_frozen	0
diff --git a/kernel/cpu.c b/kernel/cpu.c
index e1967e9eddc2..219f9033f438 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1197,6 +1197,20 @@ int cpu_up(unsigned int cpu)
 }
 EXPORT_SYMBOL_GPL(cpu_up);
 
+int hibernation_bringup_sleep_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;
+		}
+	}
+}
+
 #ifdef CONFIG_PM_SLEEP_SMP
 static cpumask_var_t frozen_cpus;
 
-- 
2.17.1

[PATCH 01/12] 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
---

AFAICT we can't use device_online() directly here because suspend happens via
cpu_down() not device_offline(). If it is actually safe to use device_online()
then that would be simpler than creating the new function. Although the
operation seems generic enough to me and could benefit another arch user in the
future so the new function makes sense.


 arch/arm64/kernel/hibernate.c | 13 +++++--------
 include/linux/cpu.h           |  1 +
 kernel/cpu.c                  | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index e0a7fce0e01c..3b178055022f 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 = hibernation_bringup_sleep_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 88dc0c653925..3b1fbe192989 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -87,6 +87,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 hibernation_bringup_sleep_cpu(unsigned int sleep_cpu);
 
 #else	/* CONFIG_SMP */
 #define cpuhp_tasks_frozen	0
diff --git a/kernel/cpu.c b/kernel/cpu.c
index e1967e9eddc2..219f9033f438 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1197,6 +1197,20 @@ int cpu_up(unsigned int cpu)
 }
 EXPORT_SYMBOL_GPL(cpu_up);
 
+int hibernation_bringup_sleep_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;
+		}
+	}
+}
+
 #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

Re: [PATCH 01/12] arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)

[Thomas Gleixner]

On Wed, 30 Oct 2019, Qais Yousef wrote:
>  
> +int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)

That function name is horrible. Aside of that I really have to ask how you
end up hibernating on an offline 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;
> +		}
> +	}
> +}

Thanks,

	tglx

Re: [PATCH 01/12] arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)

[Thomas Gleixner]

On Wed, 30 Oct 2019, Qais Yousef wrote:
>  
> +int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)

That function name is horrible. Aside of that I really have to ask how you
end up hibernating on an offline 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;
> +		}
> +	}
> +}

Thanks,

	tglx

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

Re: [PATCH 01/12] arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)

[Qais Yousef]

On 11/19/19 23:31, Thomas Gleixner wrote:
> On Wed, 30 Oct 2019, Qais Yousef wrote:
> >  
> > +int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)
> 
> That function name is horrible. Aside of that I really have to ask how you
> end up hibernating on an offline CPU?

James Morse can probably explain better.

But AFAIU we could sleep on any CPU, but on the next cold boot that CPU could
become offline as a side effect of using maxcpus= for example.

How about bringup_hibernate_cpu() as a name? I could add the above as an
explanation of why we need this call too.

It does seem to me that this is a generic problem that we might be able to
handle generically, but I'm not sure how.

Thanks

--
Qais Yousef

Re: [PATCH 01/12] arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)

[Qais Yousef]

On 11/19/19 23:31, Thomas Gleixner wrote:
> On Wed, 30 Oct 2019, Qais Yousef wrote:
> >  
> > +int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)
> 
> That function name is horrible. Aside of that I really have to ask how you
> end up hibernating on an offline CPU?

James Morse can probably explain better.

But AFAIU we could sleep on any CPU, but on the next cold boot that CPU could
become offline as a side effect of using maxcpus= for example.

How about bringup_hibernate_cpu() as a name? I could add the above as an
explanation of why we need this call too.

It does seem to me that this is a generic problem that we might be able to
handle generically, but I'm not sure how.

Thanks

--
Qais Yousef

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

Re: [PATCH 01/12] arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)

[Thomas Gleixner]

On Tue, 19 Nov 2019, Qais Yousef wrote:
> On 11/19/19 23:31, Thomas Gleixner wrote:
> > On Wed, 30 Oct 2019, Qais Yousef wrote:
> > >  
> > > +int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)
> > 
> > That function name is horrible. Aside of that I really have to ask how you
> > end up hibernating on an offline CPU?
> 
> James Morse can probably explain better.
> 
> But AFAIU we could sleep on any CPU, but on the next cold boot that CPU could
> become offline as a side effect of using maxcpus= for example.
> 
> How about bringup_hibernate_cpu() as a name? I could add the above as an
> explanation of why we need this call too.
> 
> It does seem to me that this is a generic problem that we might be able to
> handle generically, but I'm not sure how.

Don't know about other architectures, but x86 does not have that issue as
we force hibernation on CPU0 for historical reasons (Broken BIOSes etc.).

Thanks,

	tglx

Re: [PATCH 01/12] arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)

[Thomas Gleixner]

On Tue, 19 Nov 2019, Qais Yousef wrote:
> On 11/19/19 23:31, Thomas Gleixner wrote:
> > On Wed, 30 Oct 2019, Qais Yousef wrote:
> > >  
> > > +int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)
> > 
> > That function name is horrible. Aside of that I really have to ask how you
> > end up hibernating on an offline CPU?
> 
> James Morse can probably explain better.
> 
> But AFAIU we could sleep on any CPU, but on the next cold boot that CPU could
> become offline as a side effect of using maxcpus= for example.
> 
> How about bringup_hibernate_cpu() as a name? I could add the above as an
> explanation of why we need this call too.
> 
> It does seem to me that this is a generic problem that we might be able to
> handle generically, but I'm not sure how.

Don't know about other architectures, but x86 does not have that issue as
we force hibernation on CPU0 for historical reasons (Broken BIOSes etc.).

Thanks,

	tglx

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

Re: [PATCH 01/12] arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)

[Qais Yousef]

On 11/20/19 00:01, Thomas Gleixner wrote:
> On Tue, 19 Nov 2019, Qais Yousef wrote:
> > On 11/19/19 23:31, Thomas Gleixner wrote:
> > > On Wed, 30 Oct 2019, Qais Yousef wrote:
> > > >  
> > > > +int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)
> > > 
> > > That function name is horrible. Aside of that I really have to ask how you
> > > end up hibernating on an offline CPU?
> > 
> > James Morse can probably explain better.
> > 
> > But AFAIU we could sleep on any CPU, but on the next cold boot that CPU could
> > become offline as a side effect of using maxcpus= for example.
> > 
> > How about bringup_hibernate_cpu() as a name? I could add the above as an
> > explanation of why we need this call too.
> > 
> > It does seem to me that this is a generic problem that we might be able to
> > handle generically, but I'm not sure how.
> 
> Don't know about other architectures, but x86 does not have that issue as
> we force hibernation on CPU0 for historical reasons (Broken BIOSes etc.).

I'll avoid making this series bigger then.

Thanks

--
Qais Yousef

Re: [PATCH 01/12] arm64: hibernate.c: create a new function to handle cpu_up(sleep_cpu)

[Qais Yousef]

On 11/20/19 00:01, Thomas Gleixner wrote:
> On Tue, 19 Nov 2019, Qais Yousef wrote:
> > On 11/19/19 23:31, Thomas Gleixner wrote:
> > > On Wed, 30 Oct 2019, Qais Yousef wrote:
> > > >  
> > > > +int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)
> > > 
> > > That function name is horrible. Aside of that I really have to ask how you
> > > end up hibernating on an offline CPU?
> > 
> > James Morse can probably explain better.
> > 
> > But AFAIU we could sleep on any CPU, but on the next cold boot that CPU could
> > become offline as a side effect of using maxcpus= for example.
> > 
> > How about bringup_hibernate_cpu() as a name? I could add the above as an
> > explanation of why we need this call too.
> > 
> > It does seem to me that this is a generic problem that we might be able to
> > handle generically, but I'm not sure how.
> 
> Don't know about other architectures, but x86 does not have that issue as
> we force hibernation on CPU0 for historical reasons (Broken BIOSes etc.).

I'll avoid making this series bigger then.

Thanks

--
Qais Yousef

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

[PATCH 02/12] 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 03/12] 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.

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 03/12] 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.

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

Re: [PATCH 03/12] powerpc: Replace cpu_up/down with device_online/offline

[Michael Ellerman]

Qais Yousef <qais.yousef@arm.com> writes:
> 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: 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(-)

My initial though is "what about kdump", but that function is not called
during kdump, so there should be no issue with the extra locking leading
to deadlocks in a crash.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

I assume you haven't actually tested it?

cheers

> 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

Re: [PATCH 03/12] powerpc: Replace cpu_up/down with device_online/offline

[Michael Ellerman]

Qais Yousef <qais.yousef@arm.com> writes:
> 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: 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(-)

My initial though is "what about kdump", but that function is not called
during kdump, so there should be no issue with the extra locking leading
to deadlocks in a crash.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

I assume you haven't actually tested it?

cheers

> 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

Re: [PATCH 03/12] powerpc: Replace cpu_up/down with device_online/offline

[Qais Yousef]

On 11/19/19 12:19, Michael Ellerman wrote:
> Qais Yousef <qais.yousef@arm.com> writes:
> > 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: 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(-)
> 
> My initial though is "what about kdump", but that function is not called
> during kdump, so there should be no issue with the extra locking leading
> to deadlocks in a crash.
> 
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>

Thanks.

> 
> I assume you haven't actually tested it?

Only compile tested it I'm afraid. Would appreciate if you can give it a spin.
Otherwise I'd be happy to try it out on qemu.

Cheers

--
Qais Yousef

Re: [PATCH 03/12] powerpc: Replace cpu_up/down with device_online/offline

[Qais Yousef]

On 11/19/19 12:19, Michael Ellerman wrote:
> Qais Yousef <qais.yousef@arm.com> writes:
> > 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: 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(-)
> 
> My initial though is "what about kdump", but that function is not called
> during kdump, so there should be no issue with the extra locking leading
> to deadlocks in a crash.
> 
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>

Thanks.

> 
> I assume you haven't actually tested it?

Only compile tested it I'm afraid. Would appreciate if you can give it a spin.
Otherwise I'd be happy to try it out on qemu.

Cheers

--
Qais Yousef

[PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Qais Yousef]

Use freeze_secondary_cpus() instead of open coding using cpu_down()
directly.

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

Signed-off-by: Qais Yousef <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, 2 insertions(+), 6 deletions(-)

diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 968b5f33e725..70b433eafa5c 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -647,12 +647,8 @@ cpu_halt (void)
 void machine_shutdown(void)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-	int cpu;
-
-	for_each_online_cpu(cpu) {
-		if (cpu != smp_processor_id())
-			cpu_down(cpu);
-	}
+	/* TODO: Can we use disable_nonboot_cpus()? */
+	freeze_secondary_cpus(smp_processor_id());
 #endif
 #ifdef CONFIG_KEXEC
 	kexec_disable_iosapic();
-- 
2.17.1

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2028 bytes --]

Hi Qais,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.4-rc5 next-20191031]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Qais-Yousef/arm64-hibernate-c-create-a-new-function-to-handle-cpu_up-sleep_cpu/20191102-053138
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0dbe6cb8f7e05bc9611602ef45980a6c57b245a3
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/ia64/kernel/process.c: In function 'machine_shutdown':
>> arch/ia64/kernel/process.c:651:2: error: implicit declaration of function 'freeze_secondary_cpus'; did you mean 'suspend_enable_secondary_cpus'? [-Werror=implicit-function-declaration]
     freeze_secondary_cpus(smp_processor_id());
     ^~~~~~~~~~~~~~~~~~~~~
     suspend_enable_secondary_cpus
   cc1: some warnings being treated as errors

vim +651 arch/ia64/kernel/process.c

   646	
   647	void machine_shutdown(void)
   648	{
   649	#ifdef CONFIG_HOTPLUG_CPU
   650		/* TODO: Can we use disable_nonboot_cpus()? */
 > 651		freeze_secondary_cpus(smp_processor_id());
   652	#endif
   653	#ifdef CONFIG_KEXEC
   654		kexec_disable_iosapic();
   655	#endif
   656	}
   657	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54922 bytes --]

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2028 bytes --]

Hi Qais,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.4-rc5 next-20191031]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Qais-Yousef/arm64-hibernate-c-create-a-new-function-to-handle-cpu_up-sleep_cpu/20191102-053138
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0dbe6cb8f7e05bc9611602ef45980a6c57b245a3
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/ia64/kernel/process.c: In function 'machine_shutdown':
>> arch/ia64/kernel/process.c:651:2: error: implicit declaration of function 'freeze_secondary_cpus'; did you mean 'suspend_enable_secondary_cpus'? [-Werror=implicit-function-declaration]
     freeze_secondary_cpus(smp_processor_id());
     ^~~~~~~~~~~~~~~~~~~~~
     suspend_enable_secondary_cpus
   cc1: some warnings being treated as errors

vim +651 arch/ia64/kernel/process.c

   646	
   647	void machine_shutdown(void)
   648	{
   649	#ifdef CONFIG_HOTPLUG_CPU
   650		/* TODO: Can we use disable_nonboot_cpus()? */
 > 651		freeze_secondary_cpus(smp_processor_id());
   652	#endif
   653	#ifdef CONFIG_KEXEC
   654		kexec_disable_iosapic();
   655	#endif
   656	}
   657	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54922 bytes --]

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2078 bytes --]

Hi Qais,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.4-rc5 next-20191031]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Qais-Yousef/arm64-hibernate-c-create-a-new-function-to-handle-cpu_up-sleep_cpu/20191102-053138
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0dbe6cb8f7e05bc9611602ef45980a6c57b245a3
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/ia64/kernel/process.c: In function 'machine_shutdown':
>> arch/ia64/kernel/process.c:651:2: error: implicit declaration of function 'freeze_secondary_cpus'; did you mean 'suspend_enable_secondary_cpus'? [-Werror=implicit-function-declaration]
     freeze_secondary_cpus(smp_processor_id());
     ^~~~~~~~~~~~~~~~~~~~~
     suspend_enable_secondary_cpus
   cc1: some warnings being treated as errors

vim +651 arch/ia64/kernel/process.c

   646	
   647	void machine_shutdown(void)
   648	{
   649	#ifdef CONFIG_HOTPLUG_CPU
   650		/* TODO: Can we use disable_nonboot_cpus()? */
 > 651		freeze_secondary_cpus(smp_processor_id());
   652	#endif
   653	#ifdef CONFIG_KEXEC
   654		kexec_disable_iosapic();
   655	#endif
   656	}
   657	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 54922 bytes --]

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Thomas Gleixner]

On Wed, 30 Oct 2019, Qais Yousef wrote:

> Use freeze_secondary_cpus() instead of open coding using cpu_down()
> directly.
> 
> This also prepares to make cpu_up/down a private interface for anything
> but the cpu subsystem.
> 
> Signed-off-by: Qais Yousef <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, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
> index 968b5f33e725..70b433eafa5c 100644
> --- a/arch/ia64/kernel/process.c
> +++ b/arch/ia64/kernel/process.c
> @@ -647,12 +647,8 @@ cpu_halt (void)
>  void machine_shutdown(void)
>  {
>  #ifdef CONFIG_HOTPLUG_CPU
> -	int cpu;
> -
> -	for_each_online_cpu(cpu) {
> -		if (cpu != smp_processor_id())
> -			cpu_down(cpu);
> -	}
> +	/* TODO: Can we use disable_nonboot_cpus()? */
> +	freeze_secondary_cpus(smp_processor_id());

freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)

Thanks,

	tglx

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Thomas Gleixner]

On Wed, 30 Oct 2019, Qais Yousef wrote:

> Use freeze_secondary_cpus() instead of open coding using cpu_down()
> directly.
> 
> This also prepares to make cpu_up/down a private interface for anything
> but the cpu subsystem.
> 
> Signed-off-by: Qais Yousef <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, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
> index 968b5f33e725..70b433eafa5c 100644
> --- a/arch/ia64/kernel/process.c
> +++ b/arch/ia64/kernel/process.c
> @@ -647,12 +647,8 @@ cpu_halt (void)
>  void machine_shutdown(void)
>  {
>  #ifdef CONFIG_HOTPLUG_CPU
> -	int cpu;
> -
> -	for_each_online_cpu(cpu) {
> -		if (cpu != smp_processor_id())
> -			cpu_down(cpu);
> -	}
> +	/* TODO: Can we use disable_nonboot_cpus()? */
> +	freeze_secondary_cpus(smp_processor_id());

freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)

Thanks,

	tglx

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Qais Yousef]

On 11/19/19 23:21, Thomas Gleixner wrote:
> On Wed, 30 Oct 2019, Qais Yousef wrote:
> 
> > Use freeze_secondary_cpus() instead of open coding using cpu_down()
> > directly.
> > 
> > This also prepares to make cpu_up/down a private interface for anything
> > but the cpu subsystem.
> > 
> > Signed-off-by: Qais Yousef <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, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
> > index 968b5f33e725..70b433eafa5c 100644
> > --- a/arch/ia64/kernel/process.c
> > +++ b/arch/ia64/kernel/process.c
> > @@ -647,12 +647,8 @@ cpu_halt (void)
> >  void machine_shutdown(void)
> >  {
> >  #ifdef CONFIG_HOTPLUG_CPU
> > -	int cpu;
> > -
> > -	for_each_online_cpu(cpu) {
> > -		if (cpu != smp_processor_id())
> > -			cpu_down(cpu);
> > -	}
> > +	/* TODO: Can we use disable_nonboot_cpus()? */
> > +	freeze_secondary_cpus(smp_processor_id());
> 
> freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
> disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)

I thought I replied to this :-(

My plan was to simply make freeze_secondary_cpus() available and protected by
CONFIG_SMP only instead.

Good plan?

I'll probably do it as a separate patch before this one.

Thanks

--
Qais Yousef

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Thomas Gleixner]

On Tue, 19 Nov 2019, Qais Yousef wrote:
> On 11/19/19 23:21, Thomas Gleixner wrote:
> > On Wed, 30 Oct 2019, Qais Yousef wrote:
> > >  void machine_shutdown(void)
> > >  {
> > >  #ifdef CONFIG_HOTPLUG_CPU
> > > -	int cpu;
> > > -
> > > -	for_each_online_cpu(cpu) {
> > > -		if (cpu != smp_processor_id())
> > > -			cpu_down(cpu);
> > > -	}
> > > +	/* TODO: Can we use disable_nonboot_cpus()? */
> > > +	freeze_secondary_cpus(smp_processor_id());
> > 
> > freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
> > disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)
> 
> I thought I replied to this :-(
> 
> My plan was to simply make freeze_secondary_cpus() available and protected by
> CONFIG_SMP only instead.
> 
> Good plan?

No. freeze_secondary_cpus() is really for hibernation. Look at the exit
conditions there.

So you really want a seperate function which depends on CONFIG_HOTPLUG_CPU
and provides an inline stub for the CONFIG_HOTPLUG_CPU=n case.

But I have a hard time to see how that stuff works at all on
ia64:

  cpu_down() might sleep, i.e. it must be called from preemptible
  context. smp_processor_id() is invalid from preemtible context.

As this is obviously broken and ia64 is in keep compile mode only, it
should just go away.

Thanks,

	tglx

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Thomas Gleixner]

On Tue, 19 Nov 2019, Qais Yousef wrote:
> On 11/19/19 23:21, Thomas Gleixner wrote:
> > On Wed, 30 Oct 2019, Qais Yousef wrote:
> > >  void machine_shutdown(void)
> > >  {
> > >  #ifdef CONFIG_HOTPLUG_CPU
> > > -	int cpu;
> > > -
> > > -	for_each_online_cpu(cpu) {
> > > -		if (cpu != smp_processor_id())
> > > -			cpu_down(cpu);
> > > -	}
> > > +	/* TODO: Can we use disable_nonboot_cpus()? */
> > > +	freeze_secondary_cpus(smp_processor_id());
> > 
> > freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
> > disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)
> 
> I thought I replied to this :-(
> 
> My plan was to simply make freeze_secondary_cpus() available and protected by
> CONFIG_SMP only instead.
> 
> Good plan?

No. freeze_secondary_cpus() is really for hibernation. Look at the exit
conditions there.

So you really want a seperate function which depends on CONFIG_HOTPLUG_CPU
and provides an inline stub for the CONFIG_HOTPLUG_CPU=n case.

But I have a hard time to see how that stuff works at all on
ia64:

  cpu_down() might sleep, i.e. it must be called from preemptible
  context. smp_processor_id() is invalid from preemtible context.

As this is obviously broken and ia64 is in keep compile mode only, it
should just go away.

Thanks,

	tglx

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Qais Yousef]

On 11/19/19 23:59, Thomas Gleixner wrote:
> On Tue, 19 Nov 2019, Qais Yousef wrote:
> > On 11/19/19 23:21, Thomas Gleixner wrote:
> > > On Wed, 30 Oct 2019, Qais Yousef wrote:
> > > >  void machine_shutdown(void)
> > > >  {
> > > >  #ifdef CONFIG_HOTPLUG_CPU
> > > > -	int cpu;
> > > > -
> > > > -	for_each_online_cpu(cpu) {
> > > > -		if (cpu != smp_processor_id())
> > > > -			cpu_down(cpu);
> > > > -	}
> > > > +	/* TODO: Can we use disable_nonboot_cpus()? */
> > > > +	freeze_secondary_cpus(smp_processor_id());
> > > 
> > > freeze_secondary_cpus() is only available for CONFIG_PM_SLEEP_SMP=y and
> > > disable_nonboot_cpus() is a NOOP for CONFIG_PM_SLEEP_SMP=n :)
> > 
> > I thought I replied to this :-(
> > 
> > My plan was to simply make freeze_secondary_cpus() available and protected by
> > CONFIG_SMP only instead.
> > 
> > Good plan?
> 
> No. freeze_secondary_cpus() is really for hibernation. Look at the exit
> conditions there.

Hmm do you mean the pm_wakeup_pending() abort?

In arm64 we machine_shutdown() calls disable_nonboot_cpus(), which in turn
a wrapper around freeze_secondary_cpus() with 0 passed as an argument.

IIUC this means arm64 could fail to offline all CPUs on machine_shutdown(),
correct?

> 
> So you really want a seperate function which depends on CONFIG_HOTPLUG_CPU
> and provides an inline stub for the CONFIG_HOTPLUG_CPU=n case.
> 
> But I have a hard time to see how that stuff works at all on
> ia64:
> 
>   cpu_down() might sleep, i.e. it must be called from preemptible
>   context. smp_processor_id() is invalid from preemtible context.
> 
> As this is obviously broken and ia64 is in keep compile mode only, it
> should just go away.

If arm64 is doing the wrong thing, then we need a new function anyway.

Thanks

--
Qais Yousef

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Thomas Gleixner]

On Tue, 19 Nov 2019, Qais Yousef wrote:
> On 11/19/19 23:59, Thomas Gleixner wrote:
> > On Tue, 19 Nov 2019, Qais Yousef wrote:
> > > My plan was to simply make freeze_secondary_cpus() available and protected by
> > > CONFIG_SMP only instead.
> > > 
> > > Good plan?
> > 
> > No. freeze_secondary_cpus() is really for hibernation. Look at the exit
> > conditions there.
> 
> Hmm do you mean the pm_wakeup_pending() abort?
> 
> In arm64 we machine_shutdown() calls disable_nonboot_cpus(), which in turn
> a wrapper around freeze_secondary_cpus() with 0 passed as an argument.
> 
> IIUC this means arm64 could fail to offline all CPUs on machine_shutdown(),
> correct?

Looks like.

Thanks,

	tglx

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Thomas Gleixner]

On Tue, 19 Nov 2019, Qais Yousef wrote:
> On 11/19/19 23:59, Thomas Gleixner wrote:
> > On Tue, 19 Nov 2019, Qais Yousef wrote:
> > > My plan was to simply make freeze_secondary_cpus() available and protected by
> > > CONFIG_SMP only instead.
> > > 
> > > Good plan?
> > 
> > No. freeze_secondary_cpus() is really for hibernation. Look at the exit
> > conditions there.
> 
> Hmm do you mean the pm_wakeup_pending() abort?
> 
> In arm64 we machine_shutdown() calls disable_nonboot_cpus(), which in turn
> a wrapper around freeze_secondary_cpus() with 0 passed as an argument.
> 
> IIUC this means arm64 could fail to offline all CPUs on machine_shutdown(),
> correct?

Looks like.

Thanks,

	tglx

Re: [PATCH 04/12] ia64: Replace cpu_down with freeze_secondary_cpus

[Qais Yousef]

On 11/20/19 09:46, Thomas Gleixner wrote:
> On Tue, 19 Nov 2019, Qais Yousef wrote:
> > On 11/19/19 23:59, Thomas Gleixner wrote:
> > > On Tue, 19 Nov 2019, Qais Yousef wrote:
> > > > My plan was to simply make freeze_secondary_cpus() available and protected by
> > > > CONFIG_SMP only instead.
> > > > 
> > > > Good plan?
> > > 
> > > No. freeze_secondary_cpus() is really for hibernation. Look at the exit
> > > conditions there.
> > 
> > Hmm do you mean the pm_wakeup_pending() abort?
> > 
> > In arm64 we machine_shutdown() calls disable_nonboot_cpus(), which in turn
> > a wrapper around freeze_secondary_cpus() with 0 passed as an argument.
> > 
> > IIUC this means arm64 could fail to offline all CPUs on machine_shutdown(),
> > correct?
> 
> Looks like.

Okay I'll double check and introduce a new function to be called from
machine_down() for arm64 and ia64 if necessary.

Thanks

--
Qais Yousef

[PATCH 05/12] 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.

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

Re: [PATCH 05/12] sparc: Replace cpu_up/down with device_online/offline

[David Miller]

From: Qais Yousef <qais.yousef@arm.com>
Date: Wed, 30 Oct 2019 15:38:30 +0000

> 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>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 05/12] sparc: Replace cpu_up/down with device_online/offline

[David Miller]

From: Qais Yousef <qais.yousef@arm.com>
Date: Wed, 30 Oct 2019 15:38:30 +0000

> 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>

Acked-by: David S. Miller <davem@davemloft.net>

[PATCH 06/12] 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.

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
---

Couldn't compile test this one.

I'm not confident that this is a correct patch to be honest. This __init
indicates we're booting the secondary cpus and that might be too early in the
process to use the core API..?


 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

Re: [PATCH 06/12] parisc: Replace cpu_up/down with device_online/offline

[Qais Yousef]

On 10/30/19 15:38, 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: "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
> ---
> 
> Couldn't compile test this one.
> 
> I'm not confident that this is a correct patch to be honest. This __init
> indicates we're booting the secondary cpus and that might be too early in the
> process to use the core API..?

Helge, James

Do you have any comment on this? I have no means to test it and I'd
appreciate if you can spin it through one of your systems.

Thanks

--
Qais Yousef

> 
> 
>  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
> 

Re: [PATCH 06/12] parisc: Replace cpu_up/down with device_online/offline

[Helge Deller]

On 20.11.19 12:09, Qais Yousef wrote:
> On 10/30/19 15:38, 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: "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
>> ---
>>
>> Couldn't compile test this one.
>>
>> I'm not confident that this is a correct patch to be honest. This __init
>> indicates we're booting the secondary cpus and that might be too early in the
>> process to use the core API..?
>
> Helge, James
>
> Do you have any comment on this? I have no means to test it and I'd
> appreciate if you can spin it through one of your systems.

I pulled your cpu-hp-cleanup branch from git://linux-arm.org/linux-qy
and compiled a 32- and a 64-bit parisc kernel.

I faced one compile warning:
linux-2.6/kernel/cpu.c: In function ‘hibernation_bringup_sleep_cpu’:
linux-2.6/kernel/cpu.c:1237:1: warning: control reaches end of non-void function [-Wreturn-type]

Other than that the 32- and 64-bit SMP kernel booted nicely.
You may add to the series:
Acked-by: Helge Deller <deller@gmx.de>   # parisc

Thanks,
Helge


>>
>>  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
>>

Re: [PATCH 06/12] parisc: Replace cpu_up/down with device_online/offline

[Qais Yousef]

On 11/22/19 20:51, Helge Deller wrote:
> On 20.11.19 12:09, Qais Yousef wrote:
> > On 10/30/19 15:38, 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: "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
> >> ---
> >>
> >> Couldn't compile test this one.
> >>
> >> I'm not confident that this is a correct patch to be honest. This __init
> >> indicates we're booting the secondary cpus and that might be too early in the
> >> process to use the core API..?
> >
> > Helge, James
> >
> > Do you have any comment on this? I have no means to test it and I'd
> > appreciate if you can spin it through one of your systems.
> 
> I pulled your cpu-hp-cleanup branch from git://linux-arm.org/linux-qy
> and compiled a 32- and a 64-bit parisc kernel.
> 
> I faced one compile warning:
> linux-2.6/kernel/cpu.c: In function ‘hibernation_bringup_sleep_cpu’:
> linux-2.6/kernel/cpu.c:1237:1: warning: control reaches end of non-void function [-Wreturn-type]
> 
> Other than that the 32- and 64-bit SMP kernel booted nicely.
> You may add to the series:
> Acked-by: Helge Deller <deller@gmx.de>   # parisc

Great! Thanks for testing it and reporting the warning.

Cheers

--
Qais Yousef

> 
> Thanks,
> Helge
> 
> 
> >>
> >>  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 07/12] 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.

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 2db62d98e395..3431cd0c9eac 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -882,11 +882,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)
 {
@@ -2678,6 +2680,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().
@@ -2709,6 +2712,7 @@ int device_online(struct device *dev)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(device_online);
 
 struct root_device {
 	struct device dev;
-- 
2.17.1

Re: [PATCH 07/12] driver: base: cpu: export device_online/offline

[Greg Kroah-Hartman]

On Wed, Oct 30, 2019 at 03:38:32PM +0000, Qais Yousef wrote:
> 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.
> 
> 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(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 08/12] 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

[Xen-devel] [PATCH 08/12] 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 09/12] 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 09/12] 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

[PATCH 10/12] 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

[PATCH 11/12] 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        | 13 +++++++++++++
 kernel/smp.c        |  9 +--------
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 3b1fbe192989..b1c7b788b8e9 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -88,6 +88,7 @@ void notify_cpu_starting(unsigned int cpu);
 extern void cpu_maps_update_begin(void);
 extern void cpu_maps_update_done(void);
 extern int hibernation_bringup_sleep_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 219f9033f438..e16695b841de 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1211,6 +1211,19 @@ int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)
 	}
 }
 
+void smp_bringup_nonboot_cpus(unsigned int setup_max_cpus)
+{
+	unsigned int cpu;
+
+	/* 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);
+	}
+}
+
 #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

Re: [PATCH 11/12] smp: Create a new function to bringup nonboot cpus online

[Thomas Gleixner]

On Wed, 30 Oct 2019, Qais Yousef wrote:
> +void smp_bringup_nonboot_cpus(unsigned int setup_max_cpus)
> +{
> +	unsigned int cpu;
> +
> +	/* FIXME: This should be done in userspace --RR */

This fixme is stale and should just go away.

> +	for_each_present_cpu(cpu) {
> +		if (num_online_cpus() >= setup_max_cpus)
> +			break;
> +		if (!cpu_online(cpu))
> +			cpu_up(cpu);
> +	}
> +}

Thanks,

	tglx

[PATCH 12/12] 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        | 26 ++++++++++++++++++++++----
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index cc37511de866..96c69c5fbfff 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 b1c7b788b8e9..6822e676f420 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -83,7 +83,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);
@@ -114,7 +114,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);
 
 #else /* CONFIG_HOTPLUG_CPU */
 
diff --git a/kernel/cpu.c b/kernel/cpu.c
index e16695b841de..087c10dbfb99 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1045,11 +1045,20 @@ 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);
+
+/*
+ * This function is meant to be used by device core cpu subsystem.
+ *
+ * Other subsystems should use device_offline(get_cpu_device(cpu)) instead.
+ */
+int cpu_subsys_down(struct device *dev)
+{
+	return cpu_down(dev->id);
+}
 
 #else
 #define takedown_cpu		NULL
@@ -1191,11 +1200,20 @@ 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);
+
+/*
+ * This function is meant to be used by device core cpu subsystem.
+ *
+ * Other subsystems should use device_online(get_cpu_device(cpu)) instead.
+ */
+int cpu_subsys_up(struct device *dev)
+{
+	return cpu_up(dev->id);
+}
 
 int hibernation_bringup_sleep_cpu(unsigned int sleep_cpu)
 {
-- 
2.17.1

Re: [PATCH 12/12] cpu: Hide cpu_up/down

[Thomas Gleixner]

On Wed, 30 Oct 2019, Qais Yousef wrote:
> -int cpu_down(unsigned int cpu)
> +static int cpu_down(unsigned int cpu)
>  {
>  	return do_cpu_down(cpu, CPUHP_OFFLINE);
>  }
> -EXPORT_SYMBOL(cpu_down);

The exports should be gone already at this point, right?

> +/*
> + * This function is meant to be used by device core cpu subsystem.
> + *
> + * Other subsystems should use device_offline(get_cpu_device(cpu)) instead.
> + */

Can you please use proper kernel-doc function documentation?

Thanks,

	tglx

Re: [PATCH 12/12] cpu: Hide cpu_up/down

[Qais Yousef]

On 11/19/19 23:25, Thomas Gleixner wrote:
> On Wed, 30 Oct 2019, Qais Yousef wrote:
> > -int cpu_down(unsigned int cpu)
> > +static int cpu_down(unsigned int cpu)
> >  {
> >  	return do_cpu_down(cpu, CPUHP_OFFLINE);
> >  }
> > -EXPORT_SYMBOL(cpu_down);
> 
> The exports should be gone already at this point, right?

Yes. The only in-kernel user was the torture test.

> > +/*
> > + * This function is meant to be used by device core cpu subsystem.
> > + *
> > + * Other subsystems should use device_offline(get_cpu_device(cpu)) instead.
> > + */
> 
> Can you please use proper kernel-doc function documentation?

Will do.

Thanks

--
Qais Yousef

Re: [PATCH 00/12] Convert cpu_up/down to device_online/offline

[Qais Yousef]

Hi Thomas

On 10/30/19 15:38, Qais Yousef wrote:
> Using cpu_up/down directly to bring cpus online/offline loses synchronization
> with sysfs and could suffer from a race similar to what is described in
> commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and serialization
> during LPM").
> 
> cpu_up/down seem to be more of a internal implementation detail for the cpu
> subsystem to use to boot up cpus, perform suspend/resume and low level hotplug
> operations. Users outside of the cpu subsystem would be better using the device
> core API to bring a cpu online/offline which is the interface used to hotplug
> memory and other system devices.
> 
> Several users have already migrated to use the device core API, this series
> converts the remaining users and hides cpu_up/down from internal users at the
> end.
> 
> I still need to update the documentation to remove references to cpu_up/down
> and advocate for device_online/offline instead if this series will make its way
> through.
> 
> I noticed this problem while working on a hack to disable offlining
> a particular CPU but noticed that setting the offline_disabled attribute in the
> device struct isn't enough because users can easily bypass the device core.
> While my hack isn't a valid use case but it did highlight the inconsistency in
> the way cpus are being onlined/offlined and this attempt hopefully improves on
> this.
> 
> The first 6 patches fixes arch users.
> 
> The next 5 patches fixes generic code users. Particularly creating a new
> special exported API for the device core to use instead of cpu_up/down.
> Maybe we can do something more restrictive than that.
> 
> The last patch removes cpu_up/down from cpu.h and unexport the functions.
> 
> In some cases where the use of cpu_up/down seemed legitimate, I encapsulated
> the logic in a higher level - special purposed function; and converted the code
> to use that instead.
> 
> I did run the rcu torture, lock torture and psci checker tests and no problem
> was noticed. I did perform build tests on all arch affected except for parisc.
> 
> Hopefully I got the CC list right for all the patches. Apologies in advance if
> some people were omitted from some patches but they should have been CCed.

I had to make an educated guess that you're probably the 'maintainer' of cpu
hotplug - but there's no explicit entry that says that. Please let me know if
I need to bring the attention of others too.

The series do have few rough edges to address, but it's relatively
straightforward and I think does offer a nice improvement in the form of
consolidating the API for bringing up/down cpus from external
subsystems/drivers. Beside fix the inconsistency of device's core view of the
state of the cpu which can happen when cpu_{up/down} are called directly.

The downside I see is that the external API to bring cpus up/down for
suspend/resume and at boot seem to have grown a bit organically (I've added
a couple in this series to address 2 direct users of cpu_{up,down}). We might
need to rethink this API, but I think this is outside the scope of this series.

Any thoughts/feedback would be appreciated.

Thanks

--
Qais Yousef

Re: [Xen-devel] [PATCH 00/12] Convert cpu_up/down to device_online/offline

[Qais Yousef]

Hi Thomas

On 10/30/19 15:38, Qais Yousef wrote:
> Using cpu_up/down directly to bring cpus online/offline loses synchronization
> with sysfs and could suffer from a race similar to what is described in
> commit a6717c01ddc2 ("powerpc/rtas: use device model APIs and serialization
> during LPM").
> 
> cpu_up/down seem to be more of a internal implementation detail for the cpu
> subsystem to use to boot up cpus, perform suspend/resume and low level hotplug
> operations. Users outside of the cpu subsystem would be better using the device
> core API to bring a cpu online/offline which is the interface used to hotplug
> memory and other system devices.
> 
> Several users have already migrated to use the device core API, this series
> converts the remaining users and hides cpu_up/down from internal users at the
> end.
> 
> I still need to update the documentation to remove references to cpu_up/down
> and advocate for device_online/offline instead if this series will make its way
> through.
> 
> I noticed this problem while working on a hack to disable offlining
> a particular CPU but noticed that setting the offline_disabled attribute in the
> device struct isn't enough because users can easily bypass the device core.
> While my hack isn't a valid use case but it did highlight the inconsistency in
> the way cpus are being onlined/offlined and this attempt hopefully improves on
> this.
> 
> The first 6 patches fixes arch users.
> 
> The next 5 patches fixes generic code users. Particularly creating a new
> special exported API for the device core to use instead of cpu_up/down.
> Maybe we can do something more restrictive than that.
> 
> The last patch removes cpu_up/down from cpu.h and unexport the functions.
> 
> In some cases where the use of cpu_up/down seemed legitimate, I encapsulated
> the logic in a higher level - special purposed function; and converted the code
> to use that instead.
> 
> I did run the rcu torture, lock torture and psci checker tests and no problem
> was noticed. I did perform build tests on all arch affected except for parisc.
> 
> Hopefully I got the CC list right for all the patches. Apologies in advance if
> some people were omitted from some patches but they should have been CCed.

I had to make an educated guess that you're probably the 'maintainer' of cpu
hotplug - but there's no explicit entry that says that. Please let me know if
I need to bring the attention of others too.

The series do have few rough edges to address, but it's relatively
straightforward and I think does offer a nice improvement in the form of
consolidating the API for bringing up/down cpus from external
subsystems/drivers. Beside fix the inconsistency of device's core view of the
state of the cpu which can happen when cpu_{up/down} are called directly.

The downside I see is that the external API to bring cpus up/down for
suspend/resume and at boot seem to have grown a bit organically (I've added
a couple in this series to address 2 direct users of cpu_{up,down}). We might
need to rethink this API, but I think this is outside the scope of this series.

Any thoughts/feedback would be appreciated.

Thanks

--
Qais Yousef

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] Convert cpu_up/down to device_online/offline

[Thomas Gleixner]

On Mon, 18 Nov 2019, Qais Yousef wrote:
> I had to make an educated guess that you're probably the 'maintainer' of cpu
> hotplug - but there's no explicit entry that says that. Please let me know if
> I need to bring the attention of others too.

:)
 
> The series do have few rough edges to address, but it's relatively
> straightforward and I think does offer a nice improvement in the form of
> consolidating the API for bringing up/down cpus from external
> subsystems/drivers. Beside fix the inconsistency of device's core view of the
> state of the cpu which can happen when cpu_{up/down} are called directly.
> 
> The downside I see is that the external API to bring cpus up/down for
> suspend/resume and at boot seem to have grown a bit organically (I've added
> a couple in this series to address 2 direct users of cpu_{up,down}). We might
> need to rethink this API, but I think this is outside the scope of this series.
> 
> Any thoughts/feedback would be appreciated.

Let me have a look.

Re: [Xen-devel] [PATCH 00/12] Convert cpu_up/down to device_online/offline

[Thomas Gleixner]

On Mon, 18 Nov 2019, Qais Yousef wrote:
> I had to make an educated guess that you're probably the 'maintainer' of cpu
> hotplug - but there's no explicit entry that says that. Please let me know if
> I need to bring the attention of others too.

:)
 
> The series do have few rough edges to address, but it's relatively
> straightforward and I think does offer a nice improvement in the form of
> consolidating the API for bringing up/down cpus from external
> subsystems/drivers. Beside fix the inconsistency of device's core view of the
> state of the cpu which can happen when cpu_{up/down} are called directly.
> 
> The downside I see is that the external API to bring cpus up/down for
> suspend/resume and at boot seem to have grown a bit organically (I've added
> a couple in this series to address 2 direct users of cpu_{up,down}). We might
> need to rethink this API, but I think this is outside the scope of this series.
> 
> Any thoughts/feedback would be appreciated.

Let me have a look.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel