[PATCH 0/6] pseries: Move CPU dlpar into the kernel

To better support CPU hotplug in PowerKVM and PowerVM environments, the
handling of CPU dlpar should be done entirely in the kernel. This will allow
a common entry point for PowerVM and PowerKVM CPU dlpar requests. For both
environments, the entry point is the same one introduced in a previous
patch set that moved memory hotplug into the kernel. The entry point accepts
a rtas hotplug event which is either constructed when using the 
/sys/kernel/dlpar interface or is passed to the kernel when handling a
ras epow interrupt.

-Nathan

Patch 1/6:
- Consolidate cpu hotplug code from pseries/dlpar.c to pseries/hotplug-cpu.c

Patch 2/6:
- Factor out common code pieces for both environments

Patch 3/6:
- Update cpu dlpar error recovery

Patch 4/6:
- Add cpu hotplug remove capability

Patch 5/6:
- Add cpu hotplug add capability

Patch 6/6:
- Enable sysfs interface for cpu hotplug

 dlpar.c       |  195 -------------------
 hotplug-cpu.c |  578 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 pseries.h     |    5 
 3 files changed, 549 insertions(+), 229 deletions(-)

No functional changes, this patch is simply a move of the cpu hotplug
code from pseries/dlpar.c to pseries/hotplug-cpu.c. This is in an effort
to consolidate all of the cpu hotplug code in a common place.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/platforms/pseries/dlpar.c       |  189 --------------------------
 arch/powerpc/platforms/pseries/hotplug-cpu.c |  183 +++++++++++++++++++++++++
 2 files changed, 184 insertions(+), 188 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 47d9cebe..4463e9a 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c

@@ -18,7 +18,6 @@
 #include <linux/cpu.h>
 #include <linux/slab.h>
 #include <linux/of.h>
-#include "offline_states.h"
 #include "pseries.h"
 
 #include <asm/prom.h>

@@ -359,183 +358,6 @@ int dlpar_release_drc(u32 drc_index)
 	return 0;
 }
 
-#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
-
-static int dlpar_online_cpu(struct device_node *dn)
-{
-	int rc = 0;
-	unsigned int cpu;
-	int len, nthreads, i;
-	const __be32 *intserv;
-	u32 thread;
-
-	intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
-	if (!intserv)
-		return -EINVAL;
-
-	nthreads = len / sizeof(u32);
-
-	cpu_maps_update_begin();
-	for (i = 0; i < nthreads; i++) {
-		thread = be32_to_cpu(intserv[i]);
-		for_each_present_cpu(cpu) {
-			if (get_hard_smp_processor_id(cpu) != thread)
-				continue;
-			BUG_ON(get_cpu_current_state(cpu)
-					!= CPU_STATE_OFFLINE);
-			cpu_maps_update_done();
-			rc = device_online(get_cpu_device(cpu));
-			if (rc)
-				goto out;
-			cpu_maps_update_begin();
-
-			break;
-		}
-		if (cpu == num_possible_cpus())
-			printk(KERN_WARNING "Could not find cpu to online "
-			       "with physical id 0x%x\n", thread);
-	}
-	cpu_maps_update_done();
-
-out:
-	return rc;
-
-}
-
-static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
-{
-	struct device_node *dn, *parent;
-	u32 drc_index;
-	int rc;
-
-	rc = kstrtou32(buf, 0, &drc_index);
-	if (rc)
-		return -EINVAL;
-
-	rc = dlpar_acquire_drc(drc_index);
-	if (rc)
-		return -EINVAL;
-
-	parent = of_find_node_by_path("/cpus");
-	if (!parent)
-		return -ENODEV;
-
-	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
-	of_node_put(parent);
-	if (!dn)
-		return -EINVAL;
-
-	rc = dlpar_attach_node(dn);
-	if (rc) {
-		dlpar_release_drc(drc_index);
-		dlpar_free_cc_nodes(dn);
-		return rc;
-	}
-
-	rc = dlpar_online_cpu(dn);
-	if (rc)
-		return rc;
-
-	return count;
-}
-
-static int dlpar_offline_cpu(struct device_node *dn)
-{
-	int rc = 0;
-	unsigned int cpu;
-	int len, nthreads, i;
-	const __be32 *intserv;
-	u32 thread;
-
-	intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
-	if (!intserv)
-		return -EINVAL;
-
-	nthreads = len / sizeof(u32);
-
-	cpu_maps_update_begin();
-	for (i = 0; i < nthreads; i++) {
-		thread = be32_to_cpu(intserv[i]);
-		for_each_present_cpu(cpu) {
-			if (get_hard_smp_processor_id(cpu) != thread)
-				continue;
-
-			if (get_cpu_current_state(cpu) == CPU_STATE_OFFLINE)
-				break;
-
-			if (get_cpu_current_state(cpu) == CPU_STATE_ONLINE) {
-				set_preferred_offline_state(cpu, CPU_STATE_OFFLINE);
-				cpu_maps_update_done();
-				rc = device_offline(get_cpu_device(cpu));
-				if (rc)
-					goto out;
-				cpu_maps_update_begin();
-				break;
-
-			}
-
-			/*
-			 * The cpu is in CPU_STATE_INACTIVE.
-			 * Upgrade it's state to CPU_STATE_OFFLINE.
-			 */
-			set_preferred_offline_state(cpu, CPU_STATE_OFFLINE);
-			BUG_ON(plpar_hcall_norets(H_PROD, thread)
-								!= H_SUCCESS);
-			__cpu_die(cpu);
-			break;
-		}
-		if (cpu == num_possible_cpus())
-			printk(KERN_WARNING "Could not find cpu to offline "
-			       "with physical id 0x%x\n", thread);
-	}
-	cpu_maps_update_done();
-
-out:
-	return rc;
-
-}
-
-static ssize_t dlpar_cpu_release(const char *buf, size_t count)
-{
-	struct device_node *dn;
-	u32 drc_index;
-	int rc;
-
-	dn = of_find_node_by_path(buf);
-	if (!dn)
-		return -EINVAL;
-
-	rc = of_property_read_u32(dn, "ibm,my-drc-index", &drc_index);
-	if (rc) {
-		of_node_put(dn);
-		return -EINVAL;
-	}
-
-	rc = dlpar_offline_cpu(dn);
-	if (rc) {
-		of_node_put(dn);
-		return -EINVAL;
-	}
-
-	rc = dlpar_release_drc(drc_index);
-	if (rc) {
-		of_node_put(dn);
-		return rc;
-	}
-
-	rc = dlpar_detach_node(dn);
-	if (rc) {
-		dlpar_acquire_drc(drc_index);
-		return rc;
-	}
-
-	of_node_put(dn);
-
-	return count;
-}
-
-#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
-
 static int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
 {
 	int rc;

@@ -643,16 +465,7 @@ static CLASS_ATTR(dlpar, S_IWUSR, NULL, dlpar_store);
 
 static int __init pseries_dlpar_init(void)
 {
-	int rc;
-
-#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
-	ppc_md.cpu_probe = dlpar_cpu_probe;
-	ppc_md.cpu_release = dlpar_cpu_release;
-#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
-
-	rc = sysfs_create_file(kernel_kobj, &class_attr_dlpar.attr);
-
-	return rc;
+	return sysfs_create_file(kernel_kobj, &class_attr_dlpar.attr);
 }
 machine_device_initcall(pseries, pseries_dlpar_init);
 

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 6247544..c55cdbc 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c

@@ -32,6 +32,7 @@
 #include <asm/xics.h>
 #include <asm/plpar_wrappers.h>
 
+#include "pseries.h"
 #include "offline_states.h"
 
 /* This version can't take the spinlock, because it never returns */

@@ -339,6 +340,183 @@ static void pseries_remove_processor(struct device_node *np)
 	cpu_maps_update_done();
 }
 
+#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
+
+static int dlpar_online_cpu(struct device_node *dn)
+{
+	int rc = 0;
+	unsigned int cpu;
+	int len, nthreads, i;
+	const __be32 *intserv;
+	u32 thread;
+
+	intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
+	if (!intserv)
+		return -EINVAL;
+
+	nthreads = len / sizeof(u32);
+
+	cpu_maps_update_begin();
+	for (i = 0; i < nthreads; i++) {
+		thread = be32_to_cpu(intserv[i]);
+		for_each_present_cpu(cpu) {
+			if (get_hard_smp_processor_id(cpu) != thread)
+				continue;
+			BUG_ON(get_cpu_current_state(cpu)
+					!= CPU_STATE_OFFLINE);
+			cpu_maps_update_done();
+			rc = device_online(get_cpu_device(cpu));
+			if (rc)
+				goto out;
+			cpu_maps_update_begin();
+
+			break;
+		}
+		if (cpu == num_possible_cpus())
+			printk(KERN_WARNING "Could not find cpu to online "
+			       "with physical id 0x%x\n", thread);
+	}
+	cpu_maps_update_done();
+
+out:
+	return rc;
+
+}
+
+static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
+{
+	struct device_node *dn, *parent;
+	u32 drc_index;
+	int rc;
+
+	rc = kstrtou32(buf, 0, &drc_index);
+	if (rc)
+		return -EINVAL;
+
+	rc = dlpar_acquire_drc(drc_index);
+	if (rc)
+		return -EINVAL;
+
+	parent = of_find_node_by_path("/cpus");
+	if (!parent)
+		return -ENODEV;
+
+	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
+	of_node_put(parent);
+	if (!dn)
+		return -EINVAL;
+
+	rc = dlpar_attach_node(dn);
+	if (rc) {
+		dlpar_release_drc(drc_index);
+		dlpar_free_cc_nodes(dn);
+		return rc;
+	}
+
+	rc = dlpar_online_cpu(dn);
+	if (rc)
+		return rc;
+
+	return count;
+}
+
+static int dlpar_offline_cpu(struct device_node *dn)
+{
+	int rc = 0;
+	unsigned int cpu;
+	int len, nthreads, i;
+	const __be32 *intserv;
+	u32 thread;
+
+	intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
+	if (!intserv)
+		return -EINVAL;
+
+	nthreads = len / sizeof(u32);
+
+	cpu_maps_update_begin();
+	for (i = 0; i < nthreads; i++) {
+		thread = be32_to_cpu(intserv[i]);
+		for_each_present_cpu(cpu) {
+			if (get_hard_smp_processor_id(cpu) != thread)
+				continue;
+
+			if (get_cpu_current_state(cpu) == CPU_STATE_OFFLINE)
+				break;
+
+			if (get_cpu_current_state(cpu) == CPU_STATE_ONLINE) {
+				set_preferred_offline_state(cpu,
+							    CPU_STATE_OFFLINE);
+				cpu_maps_update_done();
+				rc = device_offline(get_cpu_device(cpu));
+				if (rc)
+					goto out;
+				cpu_maps_update_begin();
+				break;
+
+			}
+
+			/*
+			 * The cpu is in CPU_STATE_INACTIVE.
+			 * Upgrade it's state to CPU_STATE_OFFLINE.
+			 */
+			set_preferred_offline_state(cpu, CPU_STATE_OFFLINE);
+			BUG_ON(plpar_hcall_norets(H_PROD, thread)
+								!= H_SUCCESS);
+			__cpu_die(cpu);
+			break;
+		}
+		if (cpu == num_possible_cpus())
+			printk(KERN_WARNING "Could not find cpu to offline with physical id 0x%x\n", thread);
+	}
+	cpu_maps_update_done();
+
+out:
+	return rc;
+
+}
+
+static ssize_t dlpar_cpu_release(const char *buf, size_t count)
+{
+	struct device_node *dn;
+	u32 drc_index;
+	int rc;
+
+	dn = of_find_node_by_path(buf);
+	if (!dn)
+		return -EINVAL;
+
+	rc = of_property_read_u32(dn, "ibm,my-drc-index", &drc_index);
+	if (rc) {
+		of_node_put(dn);
+		return -EINVAL;
+	}
+
+	rc = dlpar_offline_cpu(dn);
+	if (rc) {
+		of_node_put(dn);
+		return -EINVAL;
+	}
+
+	rc = dlpar_release_drc(drc_index);
+	if (rc) {
+		of_node_put(dn);
+		return rc;
+	}
+
+	rc = dlpar_detach_node(dn);
+	if (rc) {
+		dlpar_acquire_drc(drc_index);
+		return rc;
+	}
+
+	of_node_put(dn);
+
+	return count;
+}
+
+#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
+
 static int pseries_smp_notifier(struct notifier_block *nb,
 				unsigned long action, void *data)
 {

@@ -385,6 +563,11 @@ static int __init pseries_cpu_hotplug_init(void)
 	int cpu;
 	int qcss_tok;
 
+#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
+	ppc_md.cpu_probe = dlpar_cpu_probe;
+	ppc_md.cpu_release = dlpar_cpu_release;
+#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
+
 	for_each_node_by_name(np, "interrupt-controller") {
 		typep = of_get_property(np, "compatible", NULL);
 		if (strstr(typep, "open-pic")) {

Re-factor the cpu hotplug code to support cpu hotplug completely in
the kernel and using the existing sysfs probe/release interface. Move
pieces of the existing cpu hotplug code that will be common to both
interfaces into common routines. This patch does not introduce any
functional changes.
    
Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c |   83 ++++++++++++++------------
 1 file changed, 46 insertions(+), 37 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index c55cdbc..f58d902 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c

@@ -340,8 +340,6 @@ static void pseries_remove_processor(struct device_node *np)
 	cpu_maps_update_done();
 }
 
-#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
-
 static int dlpar_online_cpu(struct device_node *dn)
 {
 	int rc = 0;

@@ -383,26 +381,16 @@ out:
 
 }
 
-static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
+static ssize_t dlpar_cpu_add(struct device_node *parent, u32 drc_index)
 {
-	struct device_node *dn, *parent;
-	u32 drc_index;
+	struct device_node *dn;
 	int rc;
 
-	rc = kstrtou32(buf, 0, &drc_index);
-	if (rc)
-		return -EINVAL;
-
 	rc = dlpar_acquire_drc(drc_index);
 	if (rc)
 		return -EINVAL;
 
-	parent = of_find_node_by_path("/cpus");
-	if (!parent)
-		return -ENODEV;
-
 	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
-	of_node_put(parent);
 	if (!dn)
 		return -EINVAL;
 

@@ -414,10 +402,7 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
 	}
 
 	rc = dlpar_online_cpu(dn);
-	if (rc)
-		return rc;
-
-	return count;
+	return rc;
 }
 
 static int dlpar_offline_cpu(struct device_node *dn)

@@ -476,6 +461,47 @@ out:
 
 }
 
+static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
+{
+	int rc;
+
+	rc = dlpar_offline_cpu(dn);
+	if (rc)
+		return -EINVAL;
+
+	rc = dlpar_release_drc(drc_index);
+	if (rc)
+		return rc;
+
+	rc = dlpar_detach_node(dn);
+	if (rc)
+		dlpar_acquire_drc(drc_index);
+
+	return rc;
+}
+
+#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
+
+static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
+{
+	struct device_node *parent;
+	u32 drc_index;
+	int rc;
+
+	rc = kstrtou32(buf, 0, &drc_index);
+	if (rc)
+		return -EINVAL;
+
+	parent = of_find_node_by_path("/cpus");
+	if (!parent)
+		return -ENODEV;
+
+	rc = dlpar_cpu_add(parent, drc_index);
+	of_node_put(parent);
+
+	return rc ? rc : count;
+}
+
 static ssize_t dlpar_cpu_release(const char *buf, size_t count)
 {
 	struct device_node *dn;

@@ -492,27 +518,10 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
 		return -EINVAL;
 	}
 
-	rc = dlpar_offline_cpu(dn);
-	if (rc) {
-		of_node_put(dn);
-		return -EINVAL;
-	}
-
-	rc = dlpar_release_drc(drc_index);
-	if (rc) {
-		of_node_put(dn);
-		return rc;
-	}
-
-	rc = dlpar_detach_node(dn);
-	if (rc) {
-		dlpar_acquire_drc(drc_index);
-		return rc;
-	}
-
+	rc = dlpar_cpu_remove(dn, drc_index);
 	of_node_put(dn);
 
-	return count;
+	return rc ? rc : count;
 }
 
 #endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */

Update the cpu dlpar add/remove paths to do better error recovery when 
a failure occurs during the add/remove operation. This includes adding
some pr_info and pr_debug statements.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c |   34 +++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index f58d902..7890b2f 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c

@@ -18,6 +18,8 @@
  *      2 of the License, or (at your option) any later version.
  */
 
+#define pr_fmt(fmt)     "pseries-hotplug-cpu: " fmt
+
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>

@@ -386,22 +388,35 @@ static ssize_t dlpar_cpu_add(struct device_node *parent, u32 drc_index)
 	struct device_node *dn;
 	int rc;
 
+	pr_info("Attempting to add CPU, drc index %x\n", drc_index);
+
 	rc = dlpar_acquire_drc(drc_index);
 	if (rc)
 		return -EINVAL;
 
 	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
-	if (!dn)
+	if (!dn) {
+		pr_debug("Failed call to configure-connector\n");
+		dlpar_release_drc(drc_index);
 		return -EINVAL;
+	}
 
 	rc = dlpar_attach_node(dn);
 	if (rc) {
+		pr_debug("Failed to attach node (%d)\n", rc);
 		dlpar_release_drc(drc_index);
 		dlpar_free_cc_nodes(dn);
 		return rc;
 	}
 
 	rc = dlpar_online_cpu(dn);
+	if (rc) {
+		pr_debug("Failed to online cpu (%d)\n", rc);
+		dlpar_detach_node(dn);
+		dlpar_release_drc(drc_index);
+	}
+
+	pr_info("Successfully added CPU, drc index %x\n", drc_index);
 	return rc;
 }
 

@@ -465,18 +480,29 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
 {
 	int rc;
 
+	pr_info("Attemping to remove CPU, drc index %x\n", drc_index);
+
 	rc = dlpar_offline_cpu(dn);
-	if (rc)
+	if (rc) {
+		pr_debug("Failed top offline cpu (%d)\n", rc);
 		return -EINVAL;
+	}
 
 	rc = dlpar_release_drc(drc_index);
-	if (rc)
+	if (rc) {
+		pr_debug("Failed to release drc (%d)\n", rc);
+		dlpar_online_cpu(dn);
 		return rc;
+	}
 
 	rc = dlpar_detach_node(dn);
-	if (rc)
+	if (rc) {
+		pr_debug("Failed to detach cpu node (%d)\n", rc);
 		dlpar_acquire_drc(drc_index);
+		dlpar_online_cpu(dn);
+	}
 
+	pr_info("Successfully removed CPU, drc index %x\n", drc_index);
 	return rc;
 }
 

Add the ability to dlpar remove CPUs via hotplug rtas events, either by
specifying the drc-index of the CPU to remove or providing a count of cpus 
to remove.

To accomplish we create a list of possible dr cpus and their drc indexes
so we can easily traverse the list looking for candidates to remove and
easily clean up in any cases of failure.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c |  202 ++++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/pseries.h     |    5 +
 2 files changed, 207 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 7890b2f..49b7196 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c

@@ -26,6 +26,7 @@
 #include <linux/sched.h>	/* for idle_task_exit */
 #include <linux/cpu.h>
 #include <linux/of.h>
+#include <linux/slab.h>
 #include <asm/prom.h>
 #include <asm/rtas.h>
 #include <asm/firmware.h>

@@ -506,6 +507,207 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
 	return rc;
 }
 
+static struct device_node *cpu_drc_index_to_dn(struct device_node *parent,
+					       u32 drc_index)
+{
+	struct device_node *dn;
+	u32 my_index;
+	int rc;
+
+	for_each_child_of_node(parent, dn) {
+		if (of_node_cmp(dn->type, "cpu") != 0)
+			continue;
+
+		rc = of_property_read_u32(dn, "ibm,my-drc-index", &my_index);
+		if (rc)
+			continue;
+
+		if (my_index == drc_index)
+			break;
+	}
+
+	return dn;
+}
+
+static int dlpar_cpu_remove_by_index(struct device_node *parent,
+				     u32 drc_index)
+{
+	struct device_node *dn;
+	int rc;
+
+	dn = cpu_drc_index_to_dn(parent, drc_index);
+	if (!dn)
+		return -ENODEV;
+
+	rc = dlpar_cpu_remove(dn, drc_index);
+	of_node_put(dn);
+	return rc;
+}
+
+static int dlpar_cpus_possible(struct device_node *parent)
+{
+	int dr_cpus_possible;
+
+	/* The first u32 in the ibm,drc-indexes property is the numnber
+	 * of drc entries in the property, which is the possible number
+	 * number of dr capable cpus.
+	 */
+	of_property_read_u32(parent, "ibm,drc-indexes", &dr_cpus_possible);
+	return dr_cpus_possible;
+}
+
+struct dr_cpu {
+	u32     drc_index;
+	bool    present;
+	bool    modified;
+};
+
+static struct dr_cpu *get_dlpar_cpus(struct device_node *parent)
+{
+	struct device_node *dn;
+	struct property *prop;
+	struct dr_cpu *dr_cpus;
+	const __be32 *p;
+	u32 drc_index;
+	int dr_cpus_possible, index;
+	bool first;
+
+	dr_cpus_possible = dlpar_cpus_possible(parent);
+	dr_cpus = kcalloc(dr_cpus_possible, sizeof(*dr_cpus), GFP_KERNEL);
+	if (!dr_cpus)
+		return NULL;
+
+	first = true;
+	index = 0;
+	of_property_for_each_u32(parent, "ibm,drc-indexes", prop, p,
+				 drc_index) {
+		if (first) {
+			/* The first entry is the number of drc indexes in
+			 * the property, skip it.
+			 */
+			first = false;
+			continue;
+		}
+
+		dr_cpus[index].drc_index = drc_index;
+
+		dn = cpu_drc_index_to_dn(parent, drc_index);
+		if (dn) {
+			dr_cpus[index].present = true;
+			of_node_put(dn);
+		}
+
+		index++;
+	}
+
+	return dr_cpus;
+}
+
+static int dlpar_cpu_remove_by_count(struct device_node *parent,
+				     u32 cpus_to_remove)
+{
+	struct dr_cpu *dr_cpus;
+	int dr_cpus_removed = 0;
+	int dr_cpus_present = 0;
+	int dr_cpus_possible;
+	int i, rc;
+
+	pr_info("Attempting to hot-remove %d CPUs\n", cpus_to_remove);
+
+	dr_cpus = get_dlpar_cpus(parent);
+	if (!dr_cpus) {
+		pr_info("Could not gather dr CPU info\n");
+		return -EINVAL;
+	}
+
+	dr_cpus_possible = dlpar_cpus_possible(parent);
+
+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus[i].present)
+			dr_cpus_present++;
+	}
+
+	/* Validate the available CPUs to remove.
+	 * NOTE: we can't remove the last CPU.
+	 */
+	if (cpus_to_remove >= dr_cpus_present) {
+		pr_err("Insufficient CPUs (%d) to satisfy remove request\n",
+		       dr_cpus_present);
+		kfree(dr_cpus);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus_removed == cpus_to_remove)
+			break;
+
+		if (!dr_cpus[i].present)
+			continue;
+
+		rc = dlpar_cpu_remove_by_index(parent, dr_cpus[i].drc_index);
+		if (!rc) {
+			dr_cpus_removed++;
+			dr_cpus[i].modified = true;
+		}
+	}
+
+	if (dr_cpus_removed != cpus_to_remove) {
+		pr_info("CPU hot-remove failed, adding any removed CPUs\n");
+
+		for (i = 0; i < dr_cpus_possible; i++) {
+			if (!dr_cpus[i].modified)
+				continue;
+
+			rc = dlpar_cpu_add(parent, dr_cpus[i].drc_index);
+			if (rc)
+				pr_info("Failed to re-add CPU (%x)\n",
+					dr_cpus[i].drc_index);
+		}
+
+		rc = -EINVAL;
+	} else {
+		rc = 0;
+	}
+
+	kfree(dr_cpus);
+	return rc;
+}
+
+int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
+{
+	struct device_node *parent;
+	u32 count, drc_index;
+	int rc;
+
+	count = hp_elog->_drc_u.drc_count;
+	drc_index = hp_elog->_drc_u.drc_index;
+
+	parent = of_find_node_by_path("/cpus");
+	if (!parent)
+		return -ENODEV;
+
+	lock_device_hotplug();
+
+	switch (hp_elog->action) {
+	case PSERIES_HP_ELOG_ACTION_REMOVE:
+		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT)
+			rc = dlpar_cpu_remove_by_count(parent, count);
+		else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)
+			rc = dlpar_cpu_remove_by_index(parent, drc_index);
+		else
+			rc = -EINVAL;
+		break;
+	default:
+		pr_err("Invalid action (%d) specified\n", hp_elog->action);
+		rc = -EINVAL;
+		break;
+	}
+
+	unlock_device_hotplug();
+	of_node_put(parent);
+	return rc;
+}
+
 #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
 
 static ssize_t dlpar_cpu_probe(const char *buf, size_t count)

diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index 8411c27..58892f1 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h

@@ -66,11 +66,16 @@ extern int dlpar_release_drc(u32 drc_index);
 
 #ifdef CONFIG_MEMORY_HOTPLUG
 int dlpar_memory(struct pseries_hp_errorlog *hp_elog);
+int dlpar_cpu(struct pseries_hp_errorlog *hp_elog);
 #else
 static inline int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 {
 	return -EOPNOTSUPP;
 }
+static inline int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 /* PCI root bridge prepare function override for pseries */

Add the ability to hotplug add cpus via rtas hotplug events by either
specifying the drc index of the CPU to add, or providing a count of the
number of CPUs to add.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c |   76 ++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 49b7196..a5d4c89 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c

@@ -673,6 +673,74 @@ static int dlpar_cpu_remove_by_count(struct device_node *parent,
 	return rc;
 }
 
+static int dlpar_cpu_add_by_count(struct device_node *parent, u32 cpus_to_add)
+{
+	struct dr_cpu *dr_cpus;
+	int dr_cpus_added = 0;
+	int dr_cpus_available = 0;
+	int dr_cpus_possible;
+	int i, rc;
+
+	pr_info("Attempting to hot-add %d CPUs\n", cpus_to_add);
+
+	dr_cpus = get_dlpar_cpus(parent);
+	if (!dr_cpus) {
+		pr_info("Could not gather dr CPU info\n");
+		return -EINVAL;
+	}
+
+	dr_cpus_possible = dlpar_cpus_possible(parent);
+
+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (!dr_cpus[i].present)
+			dr_cpus_available++;
+	}
+
+	/* Validate the available CPUs to add. */
+	if (cpus_to_add > dr_cpus_available) {
+		pr_err("Insufficient CPUs (%d) to satisfy add request\n",
+		       dr_cpus_available);
+		kfree(dr_cpus);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus_added == cpus_to_add)
+			break;
+
+		if (dr_cpus[i].present)
+			continue;
+
+		rc = dlpar_cpu_add(parent, dr_cpus[i].drc_index);
+		if (!rc) {
+			dr_cpus_added++;
+			dr_cpus[i].modified = true;
+		}
+	}
+
+	if (dr_cpus_added < cpus_to_add) {
+		pr_err("CPU hot-add failed, removing any added CPUs\n");
+
+		for (i = 0; i < dr_cpus_possible; i++) {
+			if (!dr_cpus[i].modified)
+				continue;
+
+			rc = dlpar_cpu_remove_by_index(parent,
+						       dr_cpus[i].drc_index);
+			if (rc)
+				pr_info("Failed to remove added CPU (%x)\n",
+					dr_cpus[i].drc_index);
+		}
+
+		rc = -EINVAL;
+	} else {
+		rc = 0;
+	}
+
+	kfree(dr_cpus);
+	return rc;
+}
+
 int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
 {
 	struct device_node *parent;

@@ -697,6 +765,14 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
 		else
 			rc = -EINVAL;
 		break;
+	case PSERIES_HP_ELOG_ACTION_ADD:
+		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT)
+			rc = dlpar_cpu_add_by_count(parent, count);
+		else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)
+			rc = dlpar_cpu_add(parent, drc_index);
+		else
+			rc = -EINVAL;
+		break;
 	default:
 		pr_err("Invalid action (%d) specified\n", hp_elog->action);
 		rc = -EINVAL;

Enable new kernel cpu hotplug functionality by allowing cpu dlpar requests
to be initiated from sysfs.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/platforms/pseries/dlpar.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 4463e9a..85b8d9e 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c

@@ -377,6 +377,9 @@ static int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
 	case PSERIES_HP_ELOG_RESOURCE_MEM:
 		rc = dlpar_memory(hp_elog);
 		break;
+	case PSERIES_HP_ELOG_RESOURCE_CPU:
+		rc = dlpar_cpu(hp_elog);
+		break;
 	default:
 		pr_warn_ratelimited("Invalid resource (%d) specified\n",
 				    hp_elog->resource);

@@ -406,6 +409,9 @@ static ssize_t dlpar_store(struct class *class, struct class_attribute *attr,
 	if (!strncmp(arg, "memory", 6)) {
 		hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_MEM;
 		arg += strlen("memory ");
+	} else if (!strncmp(arg, "cpu", 3)) {
+		hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_CPU;
+		arg += strlen("cpu ");
 	} else {
 		pr_err("Invalid resource specified: \"%s\"\n", buf);
 		rc = -EINVAL;

On Mon, 2015-22-06 at 20:59:20 UTC, Nathan Fontenot wrote:

Update the cpu dlpar add/remove paths to do better error recovery when 
a failure occurs during the add/remove operation. This includes adding
some pr_info and pr_debug statements.

So I'm happy with the idea there, but ..

This is good.

But this is the opposite of what we want.

By default this will print two info lines for each successful cpu hotplug, but
when we get an error nothing will be printed - because pr_debug() is off by
default. What's worse, if dlpar_online_cpu() fails, the pr_debug() will produce
nothing but we will *still* print "Successfully added CPU".

So the pr_info()s should go entirely and the pr_debugs() should become
pr_warns(). The warning messages should become more verbose so they stand on
their own, ie. include the drc_index.

When everything goes perfectly there should be no output.

                                   ^
				   should be to

 		return -EINVAL;
+	}
 
 	rc = dlpar_release_drc(drc_index);
-	if (rc)
+	if (rc) {
+		pr_debug("Failed to release drc (%d)\n", rc);
+		dlpar_online_cpu(dn);
 		return rc;
+	}
 
 	rc = dlpar_detach_node(dn);
-	if (rc)
+	if (rc) {
+		pr_debug("Failed to detach cpu node (%d)\n", rc);
 		dlpar_acquire_drc(drc_index);

But that can fail?

+		dlpar_online_cpu(dn);

And if it did this would presumably not be safe?


cheers

On Mon, 2015-22-06 at 21:00:49 UTC, Nathan Fontenot wrote:

Add the ability to dlpar remove CPUs via hotplug rtas events, either by
specifying the drc-index of the CPU to remove or providing a count of cpus 
to remove.

To accomplish we create a list of possible dr cpus and their drc indexes

What does "dr" mean? I know but not everyone does. If it's an acronymn it
should be uppercase and spelt out at its first usage.

parent is always "/cpus", so I wonder if this should just be:

	for_each_node_by_type(dn, "cpu") {

And then you don't need parent in here at all.

Or do we realistically expect to be looking for cpus under a node other than "/cpus" ?

+		rc = of_property_read_u32(dn, "ibm,my-drc-index", &my_index);
+		if (rc)
+			continue;
+
+		if (my_index == drc_index)
+			break;
+	}
+
+	return dn;
+}
+
+static int dlpar_cpu_remove_by_index(struct device_node *parent,
+				     u32 drc_index)
+{
+	struct device_node *dn;
+	int rc;
+
+	dn = cpu_drc_index_to_dn(parent, drc_index);
+	if (!dn)
+		return -ENODEV;
+
+	rc = dlpar_cpu_remove(dn, drc_index);
+	of_node_put(dn);
+	return rc;
+}
+
+static int dlpar_cpus_possible(struct device_node *parent)
+{
+	int dr_cpus_possible;
+
+	/* The first u32 in the ibm,drc-indexes property is the numnber
+	 * of drc entries in the property, which is the possible number
+	 * number of dr capable cpus.
+	 */
+	of_property_read_u32(parent, "ibm,drc-indexes", &dr_cpus_possible);
+	return dr_cpus_possible;
+}
+
+struct dr_cpu {
+	u32     drc_index;
+	bool    present;
+	bool    modified;
+};
+
+static struct dr_cpu *get_dlpar_cpus(struct device_node *parent)
+{
+	struct device_node *dn;
+	struct property *prop;
+	struct dr_cpu *dr_cpus;
+	const __be32 *p;
+	u32 drc_index;
+	int dr_cpus_possible, index;
+	bool first;
+
+	dr_cpus_possible = dlpar_cpus_possible(parent);
+	dr_cpus = kcalloc(dr_cpus_possible, sizeof(*dr_cpus), GFP_KERNEL);
+	if (!dr_cpus)
+		return NULL;
+
+	first = true;
+	index = 0;
+	of_property_for_each_u32(parent, "ibm,drc-indexes", prop, p,
+				 drc_index) {
+		if (first) {

What about:

		if (index == 0) {

Then you don't need first at all?

+			/* The first entry is the number of drc indexes in
+			 * the property, skip it.
+			 */
+			first = false;
+			continue;
+		}
+
+		dr_cpus[index].drc_index = drc_index;
+
+		dn = cpu_drc_index_to_dn(parent, drc_index);

The outer loop is iterating over all drc indexes (ie. one per cpu usually?),
and then cpu_drc_index_to_dn() will iterate over all cpus.

So that's n^2 for n = nr_cpus which is not ideal.

I realise we can't do much better, but what we can do is limit the number of
iterations of the outer loop. Because usually we will be here because we want
to offline less than nr_cpus cpus.

ie. if I ask to offline 1 cpu on a 4096 cpu system we will still do up to 16M
iterations in here, when all we needed to do was one iteration of the outer
loop.

So I think this routine should take the number of cpus we're trying to remove
and only iterate until it's found that many.

+		if (dn) {
+			dr_cpus[index].present = true;
+			of_node_put(dn);
+		}

Why do we put non present ones in the dr_cpus array at all? It just means you
have to skip them later? (in two separate places)

+
+		index++;
+	}
+
+	return dr_cpus;
+}
+
+static int dlpar_cpu_remove_by_count(struct device_node *parent,
+				     u32 cpus_to_remove)
+{
+	struct dr_cpu *dr_cpus;
+	int dr_cpus_removed = 0;
+	int dr_cpus_present = 0;
+	int dr_cpus_possible;
+	int i, rc;
+
+	pr_info("Attempting to hot-remove %d CPUs\n", cpus_to_remove);
+
+	dr_cpus = get_dlpar_cpus(parent);

So I think this should be:

+	dr_cpus = get_dlpar_cpus(parent, cpus_to_remove);

+	if (!dr_cpus) {
+		pr_info("Could not gather dr CPU info\n");
+		return -EINVAL;
+	}

And get_dlpar_cpus() should return cpus_to_remove worth of present cpus, or it
should error, meaning the below then won't be needed:

+	dr_cpus_possible = dlpar_cpus_possible(parent);
+
+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus[i].present)
+			dr_cpus_present++;
+	}
+
+	/* Validate the available CPUs to remove.
+	 * NOTE: we can't remove the last CPU.
+	 */
+	if (cpus_to_remove >= dr_cpus_present) {
+		pr_err("Insufficient CPUs (%d) to satisfy remove request\n",
+		       dr_cpus_present);
+		kfree(dr_cpus);
+		return -EINVAL;
+	}
+

Having only present cpus in dr_cpus should also simplify this loop because you
don't need to skip non-present ones:

+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus_removed == cpus_to_remove)
+			break;
+
+		if (!dr_cpus[i].present)
+			continue;
+
+		rc = dlpar_cpu_remove_by_index(parent, dr_cpus[i].drc_index);
+		if (!rc) {
+			dr_cpus_removed++;
+			dr_cpus[i].modified = true;
+		}

else .. ?

Surely you should bail on the first error?

Definitely you should, because you're going to undo everything below anyway:

+	}
+
+	if (dr_cpus_removed != cpus_to_remove) {
+		pr_info("CPU hot-remove failed, adding any removed CPUs\n");
+
+		for (i = 0; i < dr_cpus_possible; i++) {
+			if (!dr_cpus[i].modified)
+				continue;

And if you bail on the first error above you shouldn't need modified, instead
you just iterate in reverse from i using a new counter, eg. j.

+
+			rc = dlpar_cpu_add(parent, dr_cpus[i].drc_index);
+			if (rc)
+				pr_info("Failed to re-add CPU (%x)\n",
+					dr_cpus[i].drc_index);
+		}
+
+		rc = -EINVAL;
+	} else {
+		rc = 0;
+	}
+
+	kfree(dr_cpus);
+	return rc;
+}
+
+int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
+{
+	struct device_node *parent;
+	u32 count, drc_index;
+	int rc;
+
+	count = hp_elog->_drc_u.drc_count;
+	drc_index = hp_elog->_drc_u.drc_index;

Those both need be32_to_cpu().

   arch/powerpc/platforms/pseries/hotplug-cpu.c:750:15: warning: incorrect type in assignment (different base types)
   arch/powerpc/platforms/pseries/hotplug-cpu.c:750:15:    expected unsigned int [unsigned] [usertype] count
   arch/powerpc/platforms/pseries/hotplug-cpu.c:750:15:    got restricted __be32 [usertype] drc_count
   arch/powerpc/platforms/pseries/hotplug-cpu.c:751:19: warning: incorrect type in assignment (different base types)
   arch/powerpc/platforms/pseries/hotplug-cpu.c:751:19:    expected unsigned int [unsigned] [usertype] drc_index
   arch/powerpc/platforms/pseries/hotplug-cpu.c:751:19:    got restricted __be32 [usertype] drc_index


Though looking closer I don't see where we ever pass or receive a
pseries_hp_errorlog to or from firmware? So I'm a bit confused why we're
bothering with the __be32 shenanigans. Hopefully I've just missed a detail
somewhere.

I don't think this should be under CONFIG_MEMORY_HOTPLUG ?


cheers

On Mon, 2015-22-06 at 21:00:49 UTC, Nathan Fontenot wrote:

Add the ability to dlpar remove CPUs via hotplug rtas events, either by
specifying the drc-index of the CPU to remove or providing a count of cpus 
to remove.

To accomplish we create a list of possible dr cpus and their drc indexes

What does "dr" mean? I know but not everyone does. If it's an acronymn it
should be uppercase and spelt out at its first usage.

parent is always "/cpus", so I wonder if this should just be:

	for_each_node_by_type(dn, "cpu") {

And then you don't need parent in here at all.

Or do we realistically expect to be looking for cpus under a node other than "/cpus" ?

+		rc = of_property_read_u32(dn, "ibm,my-drc-index", &my_index);
+		if (rc)
+			continue;
+
+		if (my_index == drc_index)
+			break;
+	}
+
+	return dn;
+}
+
+static int dlpar_cpu_remove_by_index(struct device_node *parent,
+				     u32 drc_index)
+{
+	struct device_node *dn;
+	int rc;
+
+	dn = cpu_drc_index_to_dn(parent, drc_index);
+	if (!dn)
+		return -ENODEV;
+
+	rc = dlpar_cpu_remove(dn, drc_index);
+	of_node_put(dn);
+	return rc;
+}
+
+static int dlpar_cpus_possible(struct device_node *parent)
+{
+	int dr_cpus_possible;
+
+	/* The first u32 in the ibm,drc-indexes property is the numnber
+	 * of drc entries in the property, which is the possible number
+	 * number of dr capable cpus.
+	 */
+	of_property_read_u32(parent, "ibm,drc-indexes", &dr_cpus_possible);
+	return dr_cpus_possible;
+}
+
+struct dr_cpu {
+	u32     drc_index;
+	bool    present;
+	bool    modified;
+};
+
+static struct dr_cpu *get_dlpar_cpus(struct device_node *parent)
+{
+	struct device_node *dn;
+	struct property *prop;
+	struct dr_cpu *dr_cpus;
+	const __be32 *p;
+	u32 drc_index;
+	int dr_cpus_possible, index;
+	bool first;
+
+	dr_cpus_possible = dlpar_cpus_possible(parent);
+	dr_cpus = kcalloc(dr_cpus_possible, sizeof(*dr_cpus), GFP_KERNEL);
+	if (!dr_cpus)
+		return NULL;
+
+	first = true;
+	index = 0;
+	of_property_for_each_u32(parent, "ibm,drc-indexes", prop, p,
+				 drc_index) {
+		if (first) {

What about:

		if (index == 0) {

Then you don't need first at all?

+			/* The first entry is the number of drc indexes in
+			 * the property, skip it.
+			 */
+			first = false;
+			continue;
+		}
+
+		dr_cpus[index].drc_index = drc_index;
+
+		dn = cpu_drc_index_to_dn(parent, drc_index);

The outer loop is iterating over all drc indexes (ie. one per cpu usually?),
and then cpu_drc_index_to_dn() will iterate over all cpus.

So that's n^2 for n = nr_cpus which is not ideal.

I realise we can't do much better, but what we can do is limit the number of
iterations of the outer loop. Because usually we will be here because we want
to offline less than nr_cpus cpus.

ie. if I ask to offline 1 cpu on a 4096 cpu system we will still do up to 16M
iterations in here, when all we needed to do was one iteration of the outer
loop.

So I think this routine should take the number of cpus we're trying to remove
and only iterate until it's found that many.

+		if (dn) {
+			dr_cpus[index].present = true;
+			of_node_put(dn);
+		}

Why do we put non present ones in the dr_cpus array at all? It just means you
have to skip them later? (in two separate places)

+
+		index++;
+	}
+
+	return dr_cpus;
+}
+
+static int dlpar_cpu_remove_by_count(struct device_node *parent,
+				     u32 cpus_to_remove)
+{
+	struct dr_cpu *dr_cpus;
+	int dr_cpus_removed = 0;
+	int dr_cpus_present = 0;
+	int dr_cpus_possible;
+	int i, rc;
+
+	pr_info("Attempting to hot-remove %d CPUs\n", cpus_to_remove);
+
+	dr_cpus = get_dlpar_cpus(parent);

So I think this should be:

+	dr_cpus = get_dlpar_cpus(parent, cpus_to_remove);

+	if (!dr_cpus) {
+		pr_info("Could not gather dr CPU info\n");
+		return -EINVAL;
+	}

And get_dlpar_cpus() should return cpus_to_remove worth of present cpus, or it
should error, meaning the below then won't be needed:

+	dr_cpus_possible = dlpar_cpus_possible(parent);
+
+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus[i].present)
+			dr_cpus_present++;
+	}
+
+	/* Validate the available CPUs to remove.
+	 * NOTE: we can't remove the last CPU.
+	 */
+	if (cpus_to_remove >= dr_cpus_present) {
+		pr_err("Insufficient CPUs (%d) to satisfy remove request\n",
+		       dr_cpus_present);
+		kfree(dr_cpus);
+		return -EINVAL;
+	}
+

Having only present cpus in dr_cpus should also simplify this loop because you
don't need to skip non-present ones:

+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus_removed == cpus_to_remove)
+			break;
+
+		if (!dr_cpus[i].present)
+			continue;
+
+		rc = dlpar_cpu_remove_by_index(parent, dr_cpus[i].drc_index);
+		if (!rc) {
+			dr_cpus_removed++;
+			dr_cpus[i].modified = true;
+		}

else .. ?

Surely you should bail on the first error?

Definitely you should, because you're going to undo everything below anyway:

+	}
+
+	if (dr_cpus_removed != cpus_to_remove) {
+		pr_info("CPU hot-remove failed, adding any removed CPUs\n");
+
+		for (i = 0; i < dr_cpus_possible; i++) {
+			if (!dr_cpus[i].modified)
+				continue;

And if you bail on the first error above you shouldn't need modified, instead
you just iterate in reverse from i using a new counter, eg. j.

+
+			rc = dlpar_cpu_add(parent, dr_cpus[i].drc_index);
+			if (rc)
+				pr_info("Failed to re-add CPU (%x)\n",
+					dr_cpus[i].drc_index);
+		}
+
+		rc = -EINVAL;
+	} else {
+		rc = 0;
+	}
+
+	kfree(dr_cpus);
+	return rc;
+}
+
+int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
+{
+	struct device_node *parent;
+	u32 count, drc_index;
+	int rc;
+
+	count = hp_elog->_drc_u.drc_count;
+	drc_index = hp_elog->_drc_u.drc_index;

Those both need be32_to_cpu().

   arch/powerpc/platforms/pseries/hotplug-cpu.c:750:15: warning: incorrect type in assignment (different base types)
   arch/powerpc/platforms/pseries/hotplug-cpu.c:750:15:    expected unsigned int [unsigned] [usertype] count
   arch/powerpc/platforms/pseries/hotplug-cpu.c:750:15:    got restricted __be32 [usertype] drc_count
   arch/powerpc/platforms/pseries/hotplug-cpu.c:751:19: warning: incorrect type in assignment (different base types)
   arch/powerpc/platforms/pseries/hotplug-cpu.c:751:19:    expected unsigned int [unsigned] [usertype] drc_index
   arch/powerpc/platforms/pseries/hotplug-cpu.c:751:19:    got restricted __be32 [usertype] drc_index


Though looking closer I don't see where we ever pass or receive a
pseries_hp_errorlog to or from firmware? So I'm a bit confused why we're
bothering with the __be32 shenanigans. Hopefully I've just missed a detail
somewhere.

I don't think this should be under CONFIG_MEMORY_HOTPLUG ?


cheers

On 07/20/2015 11:46 PM, Michael Ellerman wrote:

On Mon, 2015-22-06 at 20:59:20 UTC, Nathan Fontenot wrote:

quoted

Update the cpu dlpar add/remove paths to do better error recovery when 
a failure occurs during the add/remove operation. This includes adding
some pr_info and pr_debug statements.

So I'm happy with the idea there, but ..

quoted

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index f58d902..7890b2f 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c

@@ -18,6 +18,8 @@
  *      2 of the License, or (at your option) any later version.
  */
 
+#define pr_fmt(fmt)     "pseries-hotplug-cpu: " fmt

This is good.

quoted

 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>

@@ -386,22 +388,35 @@ static ssize_t dlpar_cpu_add(struct device_node *parent, u32 drc_index)
 	struct device_node *dn;
 	int rc;
 
+	pr_info("Attempting to add CPU, drc index %x\n", drc_index);
+
 	rc = dlpar_acquire_drc(drc_index);
 	if (rc)
 		return -EINVAL;
 
 	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
-	if (!dn)
+	if (!dn) {
+		pr_debug("Failed call to configure-connector\n");
+		dlpar_release_drc(drc_index);
 		return -EINVAL;
+	}
 
 	rc = dlpar_attach_node(dn);
 	if (rc) {
+		pr_debug("Failed to attach node (%d)\n", rc);
 		dlpar_release_drc(drc_index);
 		dlpar_free_cc_nodes(dn);
 		return rc;
 	}
 
 	rc = dlpar_online_cpu(dn);
+	if (rc) {
+		pr_debug("Failed to online cpu (%d)\n", rc);
+		dlpar_detach_node(dn);
+		dlpar_release_drc(drc_index);
+	}
+
+	pr_info("Successfully added CPU, drc index %x\n", drc_index);
 	return rc;

But this is the opposite of what we want.

By default this will print two info lines for each successful cpu hotplug, but
when we get an error nothing will be printed - because pr_debug() is off by
default. What's worse, if dlpar_online_cpu() fails, the pr_debug() will produce
nothing but we will *still* print "Successfully added CPU".

So the pr_info()s should go entirely and the pr_debugs() should become
pr_warns(). The warning messages should become more verbose so they stand on
their own, ie. include the drc_index.

When everything goes perfectly there should be no output.

So... good idea, bad implementation :)

I have a feeling I may have messed this up somewhere else in the patch set too
so I'll take a look at all the pr_* calls.
 

quoted

@@ -465,18 +480,29 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
 {
 	int rc;
 
+	pr_info("Attemping to remove CPU, drc index %x\n", drc_index);
+
 	rc = dlpar_offline_cpu(dn);
-	if (rc)
+	if (rc) {
+		pr_debug("Failed top offline cpu (%d)\n", rc);

                                   ^
				   should be to

quoted

 		return -EINVAL;
+	}
 
 	rc = dlpar_release_drc(drc_index);
-	if (rc)
+	if (rc) {
+		pr_debug("Failed to release drc (%d)\n", rc);
+		dlpar_online_cpu(dn);
 		return rc;
+	}
 
 	rc = dlpar_detach_node(dn);
-	if (rc)
+	if (rc) {
+		pr_debug("Failed to detach cpu node (%d)\n", rc);
 		dlpar_acquire_drc(drc_index);

But that can fail?

quoted

+		dlpar_online_cpu(dn);

And if it did this would presumably not be safe?

Ahh, good catch. I'll fix in the next version of patches.

Thanks for the review.

-Nathan

On 07/21/2015 04:27 AM, Michael Ellerman wrote:

On Mon, 2015-22-06 at 21:00:49 UTC, Nathan Fontenot wrote:

quoted

Add the ability to dlpar remove CPUs via hotplug rtas events, either by
specifying the drc-index of the CPU to remove or providing a count of cpus 
to remove.

To accomplish we create a list of possible dr cpus and their drc indexes

What does "dr" mean? I know but not everyone does. If it's an acronymn it
should be uppercase and spelt out at its first usage.

Good point.

quoted

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 7890b2f..49b7196 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c

@@ -506,6 +507,207 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
 	return rc;
 }
 
+static struct device_node *cpu_drc_index_to_dn(struct device_node *parent,
+					       u32 drc_index)
+{
+	struct device_node *dn;
+	u32 my_index;
+	int rc;
+
+	for_each_child_of_node(parent, dn) {
+		if (of_node_cmp(dn->type, "cpu") != 0)
+			continue;

parent is always "/cpus", so I wonder if this should just be:

	for_each_node_by_type(dn, "cpu") {

And then you don't need parent in here at all.

Or do we realistically expect to be looking for cpus under a node other than "/cpus" ?

This, combined with the comments below about iterating over the list
of dr_cpus, is going away in the next version of the patch. The 'parent'
node pointer is not passed around anymore.

quoted

+		rc = of_property_read_u32(dn, "ibm,my-drc-index", &my_index);
+		if (rc)
+			continue;
+
+		if (my_index == drc_index)
+			break;
+	}
+
+	return dn;
+}
+
+static int dlpar_cpu_remove_by_index(struct device_node *parent,
+				     u32 drc_index)
+{
+	struct device_node *dn;
+	int rc;
+
+	dn = cpu_drc_index_to_dn(parent, drc_index);
+	if (!dn)
+		return -ENODEV;
+
+	rc = dlpar_cpu_remove(dn, drc_index);
+	of_node_put(dn);
+	return rc;
+}
+
+static int dlpar_cpus_possible(struct device_node *parent)
+{
+	int dr_cpus_possible;
+
+	/* The first u32 in the ibm,drc-indexes property is the numnber
+	 * of drc entries in the property, which is the possible number
+	 * number of dr capable cpus.
+	 */
+	of_property_read_u32(parent, "ibm,drc-indexes", &dr_cpus_possible);
+	return dr_cpus_possible;
+}
+
+struct dr_cpu {
+	u32     drc_index;
+	bool    present;
+	bool    modified;
+};
+
+static struct dr_cpu *get_dlpar_cpus(struct device_node *parent)
+{
+	struct device_node *dn;
+	struct property *prop;
+	struct dr_cpu *dr_cpus;
+	const __be32 *p;
+	u32 drc_index;
+	int dr_cpus_possible, index;
+	bool first;
+
+	dr_cpus_possible = dlpar_cpus_possible(parent);
+	dr_cpus = kcalloc(dr_cpus_possible, sizeof(*dr_cpus), GFP_KERNEL);
+	if (!dr_cpus)
+		return NULL;
+
+	first = true;
+	index = 0;
+	of_property_for_each_u32(parent, "ibm,drc-indexes", prop, p,
+				 drc_index) {
+		if (first) {

What about:

		if (index == 0) {

Then you don't need first at all?

This block of code is getting re-written, no need to look for the
first node in the new version.
 

quoted

+			/* The first entry is the number of drc indexes in
+			 * the property, skip it.
+			 */
+			first = false;
+			continue;
+		}
+
+		dr_cpus[index].drc_index = drc_index;
+
+		dn = cpu_drc_index_to_dn(parent, drc_index);

The outer loop is iterating over all drc indexes (ie. one per cpu usually?),
and then cpu_drc_index_to_dn() will iterate over all cpus.

So that's n^2 for n = nr_cpus which is not ideal.

I realise we can't do much better, but what we can do is limit the number of
iterations of the outer loop. Because usually we will be here because we want
to offline less than nr_cpus cpus.

ie. if I ask to offline 1 cpu on a 4096 cpu system we will still do up to 16M
iterations in here, when all we needed to do was one iteration of the outer
loop.

So I think this routine should take the number of cpus we're trying to remove
and only iterate until it's found that many.

Yeah, now that you point it out there is a lot of no good in that code.

Re-writing this with a new approach to only create lists of what is present.

quoted

+		if (dn) {
+			dr_cpus[index].present = true;
+			of_node_put(dn);
+		}

Why do we put non present ones in the dr_cpus array at all? It just means you
have to skip them later? (in two separate places)

quoted

+
+		index++;
+	}
+
+	return dr_cpus;
+}
+
+static int dlpar_cpu_remove_by_count(struct device_node *parent,
+				     u32 cpus_to_remove)
+{
+	struct dr_cpu *dr_cpus;
+	int dr_cpus_removed = 0;
+	int dr_cpus_present = 0;
+	int dr_cpus_possible;
+	int i, rc;
+
+	pr_info("Attempting to hot-remove %d CPUs\n", cpus_to_remove);
+
+	dr_cpus = get_dlpar_cpus(parent);

So I think this should be:

quoted

+	dr_cpus = get_dlpar_cpus(parent, cpus_to_remove);

quoted

+	if (!dr_cpus) {
+		pr_info("Could not gather dr CPU info\n");
+		return -EINVAL;
+	}

And get_dlpar_cpus() should return cpus_to_remove worth of present cpus, or it
should error, meaning the below then won't be needed:

When adding cpus by count we may need more than just cpus_to_remove worth
of present cpus. The goal was to provide all possibilities so we could
continue trying to satisfy the request even if one or more cpus fails
to remove.

From this comment and comments below I think your approach is that we

should bail if any error occurs during cpu remove. Is this what we
should be doing?
 

quoted

+	dr_cpus_possible = dlpar_cpus_possible(parent);
+
+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus[i].present)
+			dr_cpus_present++;
+	}
+
+	/* Validate the available CPUs to remove.
+	 * NOTE: we can't remove the last CPU.
+	 */
+	if (cpus_to_remove >= dr_cpus_present) {
+		pr_err("Insufficient CPUs (%d) to satisfy remove request\n",
+		       dr_cpus_present);
+		kfree(dr_cpus);
+		return -EINVAL;
+	}
+

Having only present cpus in dr_cpus should also simplify this loop because you
don't need to skip non-present ones:

Fixed in updated code.

quoted

+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus_removed == cpus_to_remove)
+			break;
+
+		if (!dr_cpus[i].present)
+			continue;
+
+		rc = dlpar_cpu_remove_by_index(parent, dr_cpus[i].drc_index);
+		if (!rc) {
+			dr_cpus_removed++;
+			dr_cpus[i].modified = true;
+		}

else .. ?

Surely you should bail on the first error?

Definitely you should, because you're going to undo everything below anyway:

See comment above, I kept going here in the hopes that we could satisfy the
request even if a failure occurred.

quoted

+	}
+
+	if (dr_cpus_removed != cpus_to_remove) {
+		pr_info("CPU hot-remove failed, adding any removed CPUs\n");
+
+		for (i = 0; i < dr_cpus_possible; i++) {
+			if (!dr_cpus[i].modified)
+				continue;

And if you bail on the first error above you shouldn't need modified, instead
you just iterate in reverse from i using a new counter, eg. j.

Yep, I'll work that into the new code.

quoted

+
+			rc = dlpar_cpu_add(parent, dr_cpus[i].drc_index);
+			if (rc)
+				pr_info("Failed to re-add CPU (%x)\n",
+					dr_cpus[i].drc_index);
+		}
+
+		rc = -EINVAL;
+	} else {
+		rc = 0;
+	}
+
+	kfree(dr_cpus);
+	return rc;
+}
+
+int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
+{
+	struct device_node *parent;
+	u32 count, drc_index;
+	int rc;
+
+	count = hp_elog->_drc_u.drc_count;
+	drc_index = hp_elog->_drc_u.drc_index;

Those both need be32_to_cpu().

   arch/powerpc/platforms/pseries/hotplug-cpu.c:750:15: warning: incorrect type in assignment (different base types)
   arch/powerpc/platforms/pseries/hotplug-cpu.c:750:15:    expected unsigned int [unsigned] [usertype] count
   arch/powerpc/platforms/pseries/hotplug-cpu.c:750:15:    got restricted __be32 [usertype] drc_count
   arch/powerpc/platforms/pseries/hotplug-cpu.c:751:19: warning: incorrect type in assignment (different base types)
   arch/powerpc/platforms/pseries/hotplug-cpu.c:751:19:    expected unsigned int [unsigned] [usertype] drc_index
   arch/powerpc/platforms/pseries/hotplug-cpu.c:751:19:    got restricted __be32 [usertype] drc_index