From fa2a5dfe2ddd0e7c77e5f608e1fa374192e5be97 Mon Sep 17 00:00:00 2001

From: Nathan Lynch <nathanl@linux.ibm.com>

Date: Mon, 27 Sep 2021 15:19:32 -0500

Subject: [PATCH] powerpc/pseries/cpuhp: delete add/remove_by_count code

References: bsc#1065729

Patch-mainline: v5.16-rc1

Git-commit: fa2a5dfe2ddd0e7c77e5f608e1fa374192e5be97

The core DLPAR code supports two actions (add and remove) and three

subtypes of action:

* By DRC index: the action is attempted on a single specified resource.

  This is the usual case for processors.

* By indexed count: the action is attempted on a range of resources

  beginning at the specified index. This is implemented only by the memory

  DLPAR code.

* By count: the lower layer (CPU or memory) is responsible for locating the

  specified number of resources to which the action can be applied.

I cannot find any evidence of the "by count" subtype being used by drmgr or

qemu for processors. And when I try to exercise this code, the add case

does not work:

  $ ppc64_cpu --smt ; nproc

  SMT=8

  24

  $ printf "cpu remove count 2" > /sys/kernel/dlpar

  $ nproc

  8

  $ printf "cpu add count 2" > /sys/kernel/dlpar

  -bash: printf: write error: Invalid argument

  $ dmesg | tail -2

  pseries-hotplug-cpu: Failed to find enough CPUs (1 of 2) to add

  dlpar: Could not handle DLPAR request "cpu add count 2"

  $ nproc

  8

  $ drmgr -c cpu -a -q 2         # this uses the by-index method

  Validating CPU DLPAR capability...yes.

  CPU 1

  CPU 17

  $ nproc

  24

This is because find_drc_info_cpus_to_add() does not increment drc_index

appropriately during its search.

This is not hard to fix. But the _by_count() functions also have the

property that they attempt to roll back all prior operations if the entire

request cannot be satisfied, even though the rollback itself can encounter

errors. It's not possible to provide transaction-like behavior at this

level, and it's undesirable to have code that can only pretend to do that.

Any users of these functions cannot know what the state of the system is in

the error case. And the error paths are, to my knowledge, impossible to

test without adding custom error injection code.

Summary:

* This code has not worked reliably since its introduction.

* There is no evidence that it is used.

* It contains questionable rollback behaviors in error paths which are

  difficult to test.

So let's remove it.

Fixes: ac71380071d1 ("powerpc/pseries: Add CPU dlpar remove functionality")

Fixes: 90edf184b9b7 ("powerpc/pseries: Add CPU dlpar add functionality")

Fixes: b015f6bc9547 ("powerpc/pseries: Add cpu DLPAR support for drc-info property")

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

Link: https://lore.kernel.org/r/20210927201933.76786-4-nathanl@linux.ibm.com

Acked-by: Michal Suchanek <msuchanek@suse.de>

---

 arch/powerpc/platforms/pseries/hotplug-cpu.c | 218 +------------------

 1 file changed, 2 insertions(+), 216 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c

--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c

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

@@ -741,216 +741,6 @@ static int dlpar_cpu_remove_by_index(u32 drc_index)

 	return rc;

 }

-static int find_dlpar_cpus_to_remove(u32 *cpu_drcs, int cpus_to_remove)

-{

-	struct device_node *dn;

-	int cpus_found = 0;

-	int rc;

-

-	/* We want to find cpus_to_remove + 1 CPUs to ensure we do not

-	 * remove the last CPU.

-	 */

-	for_each_node_by_type(dn, "cpu") {

-		cpus_found++;

-

-		if (cpus_found > cpus_to_remove) {

-			of_node_put(dn);

-			break;

-		}

-

-		/* Note that cpus_found is always 1 ahead of the index

-		 * into the cpu_drcs array, so we use cpus_found - 1

-		 */

-		rc = of_property_read_u32(dn, "ibm,my-drc-index",

-					  &cpu_drcs[cpus_found - 1]);

-		if (rc) {

-			pr_warn("Error occurred getting drc-index for %pOFn\n",

-				dn);

-			of_node_put(dn);

-			return -1;

-		}

-	}

-

-	if (cpus_found < cpus_to_remove) {

-		pr_warn("Failed to find enough CPUs (%d of %d) to remove\n",

-			cpus_found, cpus_to_remove);

-	} else if (cpus_found == cpus_to_remove) {

-		pr_warn("Cannot remove all CPUs\n");

-	}

-

-	return cpus_found;

-}

-

-static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)

-{

-	u32 *cpu_drcs;

-	int cpus_found;

-	int cpus_removed = 0;

-	int i, rc;

-

-	pr_debug("Attempting to hot-remove %d CPUs\n", cpus_to_remove);

-

-	cpu_drcs = kcalloc(cpus_to_remove, sizeof(*cpu_drcs), GFP_KERNEL);

-	if (!cpu_drcs)

-		return -EINVAL;

-

-	cpus_found = find_dlpar_cpus_to_remove(cpu_drcs, cpus_to_remove);

-	if (cpus_found <= cpus_to_remove) {

-		kfree(cpu_drcs);

-		return -EINVAL;

-	}

-

-	for (i = 0; i < cpus_to_remove; i++) {

-		rc = dlpar_cpu_remove_by_index(cpu_drcs[i]);

-		if (rc)

-			break;

-

-		cpus_removed++;

-	}

-

-	if (cpus_removed != cpus_to_remove) {

-		pr_warn("CPU hot-remove failed, adding back removed CPUs\n");

-

-		for (i = 0; i < cpus_removed; i++)

-			dlpar_cpu_add(cpu_drcs[i]);

-

-		rc = -EINVAL;

-	} else {

-		rc = 0;

-	}

-

-	kfree(cpu_drcs);

-	return rc;

-}

-

-static int find_drc_info_cpus_to_add(struct device_node *cpus,

-				     struct property *info,

-				     u32 *cpu_drcs, u32 cpus_to_add)

-{

-	struct of_drc_info drc;

-	const __be32 *value;

-	u32 count, drc_index;

-	int cpus_found = 0;

-	int i, j;

-

-	if (!info)

-		return -1;

-

-	value = of_prop_next_u32(info, NULL, &count);

-	if (value)

-		value++;

-

-	for (i = 0; i < count; i++) {

-		of_read_drc_info_cell(&info, &value, &drc);

-		if (strncmp(drc.drc_type, "CPU", 3))

-			break;

-

-		drc_index = drc.drc_index_start;

-		for (j = 0; j < drc.num_sequential_elems; j++) {

-			if (dlpar_cpu_exists(cpus, drc_index))

-				continue;

-

-			cpu_drcs[cpus_found++] = drc_index;

-

-			if (cpus_found == cpus_to_add)

-				return cpus_found;

-

-			drc_index += drc.sequential_inc;

-		}

-	}

-

-	return cpus_found;

-}

-

-static int find_drc_index_cpus_to_add(struct device_node *cpus,

-				      u32 *cpu_drcs, u32 cpus_to_add)

-{

-	int cpus_found = 0;

-	int index, rc;

-	u32 drc_index;

-

-	/* Search the ibm,drc-indexes array for possible CPU drcs to

-	 * add. Note that the format of the ibm,drc-indexes array is

-	 * the number of entries in the array followed by the array

-	 * of drc values so we start looking at index = 1.

-	 */

-	index = 1;

-	while (cpus_found < cpus_to_add) {

-		rc = of_property_read_u32_index(cpus, "ibm,drc-indexes",

-						index++, &drc_index);

-

-		if (rc)

-			break;

-

-		if (dlpar_cpu_exists(cpus, drc_index))

-			continue;

-

-		cpu_drcs[cpus_found++] = drc_index;

-	}

-

-	return cpus_found;

-}

-

-static int dlpar_cpu_add_by_count(u32 cpus_to_add)

-{

-	struct device_node *parent;

-	struct property *info;

-	u32 *cpu_drcs;

-	int cpus_added = 0;

-	int cpus_found;

-	int i, rc;

-

-	pr_debug("Attempting to hot-add %d CPUs\n", cpus_to_add);

-

-	cpu_drcs = kcalloc(cpus_to_add, sizeof(*cpu_drcs), GFP_KERNEL);

-	if (!cpu_drcs)

-		return -EINVAL;

-

-	parent = of_find_node_by_path("/cpus");

-	if (!parent) {

-		pr_warn("Could not find CPU root node in device tree\n");

-		kfree(cpu_drcs);

-		return -1;

-	}

-

-	info = of_find_property(parent, "ibm,drc-info", NULL);

-	if (info)

-		cpus_found = find_drc_info_cpus_to_add(parent, info, cpu_drcs, cpus_to_add);

-	else

-		cpus_found = find_drc_index_cpus_to_add(parent, cpu_drcs, cpus_to_add);

-

-	of_node_put(parent);

-

-	if (cpus_found < cpus_to_add) {

-		pr_warn("Failed to find enough CPUs (%d of %d) to add\n",

-			cpus_found, cpus_to_add);

-		kfree(cpu_drcs);

-		return -EINVAL;

-	}

-

-	for (i = 0; i < cpus_to_add; i++) {

-		rc = dlpar_cpu_add(cpu_drcs[i]);

-		if (rc)

-			break;

-

-		cpus_added++;

-	}

-

-	if (cpus_added < cpus_to_add) {

-		pr_warn("CPU hot-add failed, removing any added CPUs\n");

-

-		for (i = 0; i < cpus_added; i++)

-			dlpar_cpu_remove_by_index(cpu_drcs[i]);

-

-		rc = -EINVAL;

-	} else {

-		rc = 0;

-	}

-

-	kfree(cpu_drcs);

-	return rc;

-}

-

 int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)

 {

 	u32 drc_index;

@@ -962,9 +752,7 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)

 	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(count);

-		else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)

+		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)

 			rc = dlpar_cpu_remove_by_index(drc_index);

 		else

 			rc = -EINVAL;

@@ -978,9 +766,7 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)

 			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(count);

-		else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)

+		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)

 			rc = dlpar_cpu_add(drc_index);

 		else

 			rc = -EINVAL;

-- 

2.31.1