pseries: Move CPU dlpar into the kernel

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.

arch/powerpc/platforms/pseries/hotplug-cpu.c +7 -1

 #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;
 	}

arch/powerpc/platforms/pseries/hotplug-cpu.c +2 -1

 		return -EINVAL;
+	}
 
 	rc = dlpar_release_drc(drc_index);
-	if (rc)
+	if (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

arch/powerpc/platforms/pseries/hotplug-cpu.c +0 -0

index 7890b2f..49b7196 100644
@@ -506,6 +507,207 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
 	return rc;
 }
 

arch/powerpc/platforms/pseries/hotplug-cpu.c +54 -0

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

arch/powerpc/platforms/pseries/hotplug-cpu.c +9 -0

+
+		index++;
+	}
+
+	return dr_cpus;
+}
+
+static int dlpar_cpu_remove_by_count(struct device_node *parent,
+				     u32 cpus_to_remove)

arch/powerpc/platforms/pseries/hotplug-cpu.c +7 -0

+	dr_cpus_possible = dlpar_cpus_possible(parent);
+
+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus[i].present)
+			dr_cpus_present++;
+	}
+

arch/powerpc/platforms/pseries/hotplug-cpu.c +14 -0

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

arch/powerpc/platforms/pseries/hotplug-cpu.c +26 -0

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

arch/powerpc/platforms/pseries/pseries.h +0 -0

index 8411c27..58892f1 100644

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

arch/powerpc/platforms/pseries/hotplug-cpu.c +0 -0

index 7890b2f..49b7196 100644
@@ -506,6 +507,207 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
 	return rc;
 }
 

arch/powerpc/platforms/pseries/hotplug-cpu.c +54 -0

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

arch/powerpc/platforms/pseries/hotplug-cpu.c +9 -0

+
+		index++;
+	}
+
+	return dr_cpus;
+}
+
+static int dlpar_cpu_remove_by_count(struct device_node *parent,
+				     u32 cpus_to_remove)

arch/powerpc/platforms/pseries/hotplug-cpu.c +7 -0

+	dr_cpus_possible = dlpar_cpus_possible(parent);
+
+	for (i = 0; i < dr_cpus_possible; i++) {
+		if (dr_cpus[i].present)
+			dr_cpus_present++;
+	}
+

arch/powerpc/platforms/pseries/hotplug-cpu.c +14 -0

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

arch/powerpc/platforms/pseries/hotplug-cpu.c +26 -0

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

arch/powerpc/platforms/pseries/pseries.h +0 -0

index 8411c27..58892f1 100644

Quoted 3 lines · Nathan Fontenot

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

arch/powerpc/platforms/pseries/hotplug-cpu.c line 21

Quoted 9 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +1 -0

index f58d902..7890b2f 100644
@@ -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 28 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +7 -1

 #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;
 	}

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_add(struct device_node *parent, u32 drc_index) · line 420

Quoted 10 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +7 -0

 
 	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.

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 487

Quoted 11 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +4 -1

@@ -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 6 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +2 -1

 		return -EINVAL;
+	}
 
 	rc = dlpar_release_drc(drc_index);
-	if (rc)
+	if (rc) {

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 501

Quoted 10 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +5 -1

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

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 502

Quoted 1 line

arch/powerpc/platforms/pseries/hotplug-cpu.c +1 -0

+		dlpar_online_cpu(dn);

And if it did this would presumably not be safe?

Quoted 5 lines · Nathan Fontenot

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.

Quoted 8 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +0 -0

index 7890b2f..49b7196 100644
@@ -506,6 +507,207 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index)
 	return rc;
 }
 

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 519

Quoted 10 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +10 -0

+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" ?

Quoted 55 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +54 -0

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

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 584

Quoted 10 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +10 -0

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

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 594

Quoted 10 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +10 -0

+			/* 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.

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 598

Quoted 4 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +4 -0

+		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 9 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +9 -0

+
+		index++;
+	}
+
+	return dr_cpus;
+}
+
+static int dlpar_cpu_remove_by_count(struct device_node *parent,
+				     u32 cpus_to_remove)

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 617

Quoted 10 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +10 -0

+{
+	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 1 line

arch/powerpc/platforms/pseries/hotplug-cpu.c +1 -0

+	dr_cpus = get_dlpar_cpus(parent, cpus_to_remove);

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 621

Quoted 4 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +4 -0

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

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

Quoted 12 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +12 -0

+	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",

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 639

Quoted 5 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +5 -0

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

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 651

Quoted 13 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +12 -0

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

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 659

Quoted 9 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +8 -0

+	}
+
+	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.

Quoted 15 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +14 -0

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

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 683

Quoted 10 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +10 -0

+}
+
+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.

Quoted 35 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +26 -0

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

arch/powerpc/platforms/pseries/pseries.h +0 -0

index 8411c27..58892f1 100644
@@ -66,11 +66,16 @@ extern int dlpar_release_drc(u32 drc_index);
 

arch/powerpc/platforms/pseries/pseries.h extern int dlpar_release_drc(u32 drc_index); · line 69

Quoted 3 lines

arch/powerpc/platforms/pseries/pseries.h +1 -0

 #ifdef CONFIG_MEMORY_HOTPLUG
 int dlpar_memory(struct pseries_hp_errorlog *hp_elog);
+int dlpar_cpu(struct pseries_hp_errorlog *hp_elog);

I don't think this should be under CONFIG_MEMORY_HOTPLUG ?

Quoted 18 lines · Michael Ellerman

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 617

Quoted 12 lines · Nathan Fontenot

arch/powerpc/platforms/pseries/hotplug-cpu.c +12 -0

+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 1 line

(quoted diff) +1 -0

+	dr_cpus = get_dlpar_cpus(parent, cpus_to_remove);

Quoted 7 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 621

Quoted 4 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +4 -0

+	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 5 lines

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.

That patch is coming. For hotplug in KVM guests the pseries_hp_errorlog is received when we call rtas_check_exception(). Currently these are sent up to rtas_errd in userspace, When this partchset goes in I planned on sending a patch to have cpu and memory hotplug requests handled entirely in the kernel instead of going to userspace.

Quoted 6 lines · Michael Ellerman

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 27 lines · Nathan Fontenot

Quoted 18 lines · Michael Ellerman

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 617

Quoted 12 lines · Nathan Fontenot

arch/powerpc/platforms/pseries/hotplug-cpu.c +12 -0

+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 1 line

(quoted diff) +1 -0

+	dr_cpus = get_dlpar_cpus(parent, cpus_to_remove);

Quoted 7 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) · line 621

Quoted 4 lines

arch/powerpc/platforms/pseries/hotplug-cpu.c +4 -0

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

Quoted 8 lines

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?

I think so. But you can convince me otherwise if you like :)

It seems to me that we don't expect failures in the general case, so a failure genuinely indicates something has gone wrong. In which case it's best to stop and back out the request, rather than trying to continue and possibly making things worse.

There's also the dilemma that if you get an error offlining one cpu, but then continue and manage to offline enough cpus, should you report an error to the caller (userspace)?

So I think it's better to bail on the first error, undo what's been done, and then report the error to the caller.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 5 lines

That patch is coming. For hotplug in KVM guests the pseries_hp_errorlog is received when we call rtas_check_exception(). Currently these are sent up to rtas_errd in userspace, When this partchset goes in I planned on sending a patch to have cpu and memory hotplug requests handled entirely in the kernel instead of going to userspace.

OK that makes sense.

I'll wait to see that patch, but when it comes I'll probably tell you to do the endian swaps once when we receive the error log, rather than at each usage.