[RFC v4 0/4] powerpc/drcinfo: Fix bugs 'ibm,drc-info' property

[RFC v4 0/4] powerpc/drcinfo: Fix bugs 'ibm,drc-info' property

[Michael Bringmann]

This patch set corrects some errors and omissions in the previous
set of patches adding support for the "ibm,drc-info" property to
powerpc systems.

Unfortunately, some errors in the previous patch set break things
in some of the DLPAR operations.  In particular when attempting to
hot-add a new CPU or set of CPUs, the original patch failed to
properly calculate the available resources, and aborted the operation.
In addition, the original set missed several opportunities to compress
and reuse common code, especially, in the area of device processing.

Signed-off-by: Michael W. Bringmann <mwb@linux.vnet.ibm.com>
---
Changes in V4:
  -- Update code for latest kernel checkins.
  -- Fix bug with virtual devices.
  -- Rebase on top of 4.17-rc5 kernel
  -- Simplify parser code for "ibm,drc-info" property

[RFC v4 1/4] hotplug/drcinfo: Simplify parse ibm,drc-info structs

[Michael Bringmann]

Replace use of of_prop_next_u32() in when parsing 'ibm,drc-info'
structure to simplify and reduce parsing code.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmware feature" -- end of patch series applied to powerpc next)
---
Changes in V4:
  -- Rebased patch to 4.17-rc5 kernel
  -- Replace of_prop_next_u32() by of_read_number()
---
 arch/powerpc/platforms/pseries/of_helpers.c     |   20 +++++---------------
 arch/powerpc/platforms/pseries/pseries_energy.c |   10 ++++------
 drivers/pci/hotplug/rpaphp_core.c               |    5 ++---
 3 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
index 6df192f..11b2ef1 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -65,29 +65,19 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
 
 	/* Get drc-index-start:encode-int */
 	p2 = (const __be32 *)p;
-	p2 = of_prop_next_u32(*prop, p2, &data->drc_index_start);
-	if (!p2)
-		return -EINVAL;
+	data->drc_index_start = of_read_number(p2++, 1);
 
 	/* Get drc-name-suffix-start:encode-int */
-	p2 = of_prop_next_u32(*prop, p2, &data->drc_name_suffix_start);
-	if (!p2)
-		return -EINVAL;
+	data->drc_name_suffix_start = of_read_number(p2++, 1);
 
 	/* Get number-sequential-elements:encode-int */
-	p2 = of_prop_next_u32(*prop, p2, &data->num_sequential_elems);
-	if (!p2)
-		return -EINVAL;
+	data->num_sequential_elems = of_read_number(p2++, 1);
 
 	/* Get sequential-increment:encode-int */
-	p2 = of_prop_next_u32(*prop, p2, &data->sequential_inc);
-	if (!p2)
-		return -EINVAL;
+	data->sequential_inc = of_read_number(p2++, 1);
 
 	/* Get drc-power-domain:encode-int */
-	p2 = of_prop_next_u32(*prop, p2, &data->drc_power_domain);
-	if (!p2)
-		return -EINVAL;
+	data->drc_power_domain = of_read_number(p2++, 1);
 
 	/* Should now know end of current entry */
 	(*curval) = (void *)p2;
diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c
index 6ed2212..5261975 100644
--- a/arch/powerpc/platforms/pseries/pseries_energy.c
+++ b/arch/powerpc/platforms/pseries/pseries_energy.c
@@ -61,9 +61,8 @@ static u32 cpu_to_drc_index(int cpu)
 		if (info == NULL)
 			goto err_of_node_put;
 
-		value = of_prop_next_u32(info, NULL, &num_set_entries);
-		if (!value)
-			goto err_of_node_put;
+		value = info->value;
+		num_set_entries = of_read_number(value++, 1);
 
 		for (j = 0; j < num_set_entries; j++) {
 
@@ -123,9 +122,8 @@ static int drc_index_to_cpu(u32 drc_index)
 		if (info == NULL)
 			goto err_of_node_put;
 
-		value = of_prop_next_u32(info, NULL, &num_set_entries);
-		if (!value)
-			goto err_of_node_put;
+		value = info->value;
+		num_set_entries = of_read_number(value++, 1);
 
 		for (j = 0; j < num_set_entries; j++) {
 
diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index fb5e084..435c1a0 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -236,9 +236,8 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
 	if (info == NULL)
 		return -EINVAL;
 
-	value = of_prop_next_u32(info, NULL, &entries);
-	if (!value)
-		return -EINVAL;
+	value = info->value;
+	entries = of_read_number(value++, 1);
 
 	for (j = 0; j < entries; j++) {
 		of_read_drc_info_cell(&info, &value, &drc);

[RFC v4 2/4] hotplug/drcinfo: Provide parser with callback

[Michael Bringmann]

This patch provides a common parse function for the ibm,drc-info
property that can be modified by a callback function.  The caller
provides a pointer to the function and a pointer to their unique
data, and the parser provides the current lmb set from the struct.
The callback function may return codes indicating that the parsing
is complete, or should continue, along with an error code that may
be returned to the caller.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
e feature" -- end of patch series applied to powerpc next)
---
Changes in V4:
  -- Update code to account for latest kernel checkins.
  -- Rebased to 4.17-rc5 kernel
  -- Some patch cleanup including file combination
---
 arch/powerpc/include/asm/prom.h             |    7 +++++
 arch/powerpc/platforms/pseries/of_helpers.c |   37 +++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
index b04c5ce..2e947b3 100644
--- a/arch/powerpc/include/asm/prom.h
+++ b/arch/powerpc/include/asm/prom.h
@@ -94,6 +94,13 @@ struct of_drc_info {
 extern int of_read_drc_info_cell(struct property **prop,
 			const __be32 **curval, struct of_drc_info *data);
 
+extern int drc_info_parser(struct device_node *dn,
+			int (*usercb)(struct of_drc_info *drc,
+					void *data,
+					void *optional_data,
+					int *ret_code),
+			char *opt_drc_type,
+			void *data);
 
 /*
  * There are two methods for telling firmware what our capabilities are.
diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
index 11b2ef1..a588ee6 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -6,6 +6,9 @@
 #include <asm/prom.h>
 
 #include "of_helpers.h"
+#include "pseries.h"
+
+#define	MAX_DRC_NAME_LEN 64
 
 /**
  * pseries_of_derive_parent - basically like dirname(1)
@@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
 	return 0;
 }
 EXPORT_SYMBOL(of_read_drc_info_cell);
+
+int drc_info_parser(struct device_node *dn,
+		int (*usercb)(struct of_drc_info *drc,
+				void *data,
+				void *optional_data,
+				int *ret_code),
+		char *opt_drc_type,
+		void *data)
+{
+	struct property *info;
+	unsigned int entries;
+	struct of_drc_info drc;
+	const __be32 *value;
+	int j, done = 0, ret_code = -EINVAL;
+
+	info = of_find_property(dn, "ibm,drc-info", NULL);
+	if (info == NULL)
+		return -EINVAL;
+
+	value = info->value;
+	entries = of_read_number(value++, 1);
+
+	for (j = 0, done = 0; (j < entries) && (!done); j++) {
+		of_read_drc_info_cell(&info, &value, &drc);
+
+		if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type))
+			continue;
+
+		done = usercb(&drc, data, NULL, &ret_code);
+	}
+
+	return ret_code;
+}
+EXPORT_SYMBOL(drc_info_parser);

[RFC v4 3/4] hotplug/drcinfo: Fix hot-add CPU issues

[Michael Bringmann]

This patch applies a common parse function for the ibm,drc-info
property that can be modified by a callback function to the
hot-add CPU code.  Candidate code is replaced by a call to the
parser including a pointer to a local context-specific functions,
and local data.

In addition, a bug in the release of the previous patch set may
break things in some of the CPU DLPAR operations.  For instance,
when attempting to hot-add a new CPU or set of CPUs, the original
patch failed to always properly calculate the available resources,
and aborted the operation.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
e feature" -- end of patch series applied to powerpc next)
---
Changes in V4:
  -- Update code to account for latest kernel checkins.
  -- Rebased to 4.17-rc5 kernel
  -- Compress some more code
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c    |  118 +++++++++++++++++------
 arch/powerpc/platforms/pseries/pseries_energy.c |  107 +++++++++++----------
 2 files changed, 141 insertions(+), 84 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 6ef77ca..ceacad9 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -411,27 +411,63 @@ static bool dlpar_cpu_exists(struct device_node *parent, u32 drc_index)
 	return found;
 }
 
-static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
+static bool check_cpu_drc_index(struct device_node *parent,
+				int (*cb)(struct of_drc_info *drc,
+					void *data, void *not_used,
+					int *ret_code),
+				void *cdata)
 {
 	bool found = false;
-	int rc, index;
 
-	index = 0;
-	while (!found) {
-		u32 drc;
+	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
+		if (drc_info_parser(parent, cb, "CPU", cdata))
+			found = true;
+	} else {
+		int index = 0;
 
-		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
-						index++, &drc);
-		if (rc)
-			break;
+		while (!found) {
+			u32 drc;
 
-		if (drc == drc_index)
-			found = true;
+			if (of_property_read_u32_index(parent,
+						"ibm,drc-indexes",
+						index++, &drc))
+				break;
+			if (cb(NULL, cdata, &drc, NULL))
+				found = true;
+		}
 	}
 
 	return found;
 }
 
+struct valid_drc_index_struct {
+	u32 targ_drc_index;
+};
+
+static int valid_drc_index_cb(struct of_drc_info *drc, void *idata,
+				void *drc_index, int *ret_code)
+{
+	struct valid_drc_index_struct *cdata = idata;
+
+	if (drc) {
+		if (!((drc->drc_index_start <= cdata->targ_drc_index) &&
+			(cdata->targ_drc_index <= drc->last_drc_index)))
+			return 0;
+	} else {
+		if (*((u32 *)drc_index) != cdata->targ_drc_index)
+			return 0;
+	}
+	(*ret_code) = 1;
+	return 1;
+}
+
+static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
+{
+	struct valid_drc_index_struct cdata = { drc_index };
+
+	return check_cpu_drc_index(parent, valid_drc_index_cb, &cdata);
+}
+
 static ssize_t dlpar_cpu_add(u32 drc_index)
 {
 	struct device_node *dn, *parent;
@@ -721,11 +757,43 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
 	return rc;
 }
 
+struct cpus_to_add_struct {
+	struct device_node *parent;
+	u32 *cpu_drcs;
+	u32 cpus_to_add;
+	u32 cpus_found;
+};
+
+static int cpus_to_add_cb(struct of_drc_info *drc, void *idata,
+			void *drc_index, int *ret_code)
+{
+	struct cpus_to_add_struct *cdata = idata;
+
+	if (drc) {
+		int k;
+
+		for (k = 0; (k < drc->num_sequential_elems) &&
+			(cdata->cpus_found < cdata->cpus_to_add); k++) {
+			u32 idrc = drc->drc_index_start +
+				(k * drc->sequential_inc);
+
+			if (dlpar_cpu_exists(cdata->parent, idrc))
+				continue;
+			cdata->cpu_drcs[cdata->cpus_found++] = idrc;
+		}
+	} else {
+		if (!dlpar_cpu_exists(cdata->parent, *((u32 *)drc_index)))
+			cdata->cpu_drcs[cdata->cpus_found++] =
+				*((u32 *)drc_index);
+	}
+	return 0;
+}
+
 static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
 {
 	struct device_node *parent;
-	int cpus_found = 0;
-	int index, rc;
+	struct cpus_to_add_struct cdata = {
+		NULL, cpu_drcs, cpus_to_add, 0 };
 
 	parent = of_find_node_by_path("/cpus");
 	if (!parent) {
@@ -734,28 +802,14 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
 		return -1;
 	}
 
-	/* 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.
+	/* Search the appropriate property for possible CPU drcs to
+	 * add.
 	 */
-	index = 1;
-	while (cpus_found < cpus_to_add) {
-		u32 drc;
-
-		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
-						index++, &drc);
-		if (rc)
-			break;
-
-		if (dlpar_cpu_exists(parent, drc))
-			continue;
-
-		cpu_drcs[cpus_found++] = drc;
-	}
+	cdata.parent = parent;
+	check_cpu_drc_index(parent, cpus_to_add_cb, &cdata);
 
 	of_node_put(parent);
-	return cpus_found;
+	return cdata.cpus_found;
 }
 
 static int dlpar_cpu_add_by_count(u32 cpus_to_add)
diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c
index 5261975..d8d7750 100644
--- a/arch/powerpc/platforms/pseries/pseries_energy.c
+++ b/arch/powerpc/platforms/pseries/pseries_energy.c
@@ -36,6 +36,26 @@
 
 /* Helper Routines to convert between drc_index to cpu numbers */
 
+struct cpu_to_drc_index_struct {
+	u32	thread_index;
+	u32	ret;
+};
+
+static int cpu_to_drc_index_cb(struct of_drc_info *drc, void *idata,
+				void *not_used, int *ret_code)
+{
+	struct cpu_to_drc_index_struct *cdata = idata;
+	int ret = 0;
+
+	if (cdata->thread_index < drc->last_drc_index) {
+		cdata->ret = drc->drc_index_start +
+			(cdata->thread_index * drc->sequential_inc);
+		ret = 1;
+	}
+	(*ret_code) = ret;
+	return ret;
+}
+
 static u32 cpu_to_drc_index(int cpu)
 {
 	struct device_node *dn = NULL;
@@ -51,30 +71,14 @@ static u32 cpu_to_drc_index(int cpu)
 	thread_index = cpu_core_index_of_thread(cpu);
 
 	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
-		struct property *info = NULL;
-		struct of_drc_info drc;
-		int j;
-		u32 num_set_entries;
-		const __be32 *value;
-
-		info = of_find_property(dn, "ibm,drc-info", NULL);
-		if (info == NULL)
-			goto err_of_node_put;
+		struct cpu_to_drc_index_struct cdata = {
+			thread_index, 0 };
 
-		value = info->value;
-		num_set_entries = of_read_number(value++, 1);
-
-		for (j = 0; j < num_set_entries; j++) {
-
-			of_read_drc_info_cell(&info, &value, &drc);
-			if (strncmp(drc.drc_type, "CPU", 3))
-				goto err;
-
-			if (thread_index < drc.last_drc_index)
-				break;
-		}
-
-		ret = drc.drc_index_start + (thread_index * drc.sequential_inc);
+		rc = drc_info_parser(dn, &cpu_to_drc_index_cb,
+					"CPU", &cdata);
+		if (rc < 0)
+			goto err_of_node_put;
+		ret = cdata.ret;
 	} else {
 		const __be32 *indexes;
 
@@ -100,11 +104,33 @@ static u32 cpu_to_drc_index(int cpu)
 	return ret;
 }
 
+struct drc_index_to_cpu_struct {
+	u32	drc_index;
+	u32	thread_index;
+	u32	cpu;
+};
+
+static int drc_index_to_cpu_cb(struct of_drc_info *drc,
+				void *idata, void *not_used, int *ret_code)
+{
+	struct drc_index_to_cpu_struct *cdata = idata;
+
+	if (cdata->drc_index > drc->last_drc_index) {
+		cdata->cpu += drc->num_sequential_elems;
+	} else {
+		cdata->cpu += ((cdata->drc_index - drc->drc_index_start) /
+				drc->sequential_inc);
+		cdata->thread_index = cpu_first_thread_of_core(cdata->cpu);
+	}
+	(*ret_code) = 0;
+	return 0;
+}
+
 static int drc_index_to_cpu(u32 drc_index)
 {
 	struct device_node *dn = NULL;
 	const int *indexes;
-	int thread_index = 0, cpu = 0;
+	int thread_index = 0;
 	int rc = 1;
 
 	dn = of_find_node_by_path("/cpus");
@@ -112,36 +138,13 @@ static int drc_index_to_cpu(u32 drc_index)
 		goto err;
 
 	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
-		struct property *info = NULL;
-		struct of_drc_info drc;
-		int j;
-		u32 num_set_entries;
-		const __be32 *value;
-
-		info = of_find_property(dn, "ibm,drc-info", NULL);
-		if (info == NULL)
-			goto err_of_node_put;
-
-		value = info->value;
-		num_set_entries = of_read_number(value++, 1);
-
-		for (j = 0; j < num_set_entries; j++) {
+		struct drc_index_to_cpu_struct cdata = {
+			drc_index, 0, 0 };
 
-			of_read_drc_info_cell(&info, &value, &drc);
-			if (strncmp(drc.drc_type, "CPU", 3))
-				goto err;
+		rc = drc_info_parser(dn, &drc_index_to_cpu_cb,
+					"CPU", &cdata);
+		thread_index = cdata.thread_index;
 
-			if (drc_index > drc.last_drc_index) {
-				cpu += drc.num_sequential_elems;
-				continue;
-			}
-			cpu += ((drc_index - drc.drc_index_start) /
-				drc.sequential_inc);
-
-			thread_index = cpu_first_thread_of_core(cpu);
-			rc = 0;
-			break;
-		}
 	} else {
 		unsigned long int i;
 

[RFC v4 4/4] hotplug/drcinfo: Code cleanup for devices

[Michael Bringmann]

This patch extends the use of a common parse function for the
ibm,drc-info property that can be modified by a callback function
to the hotplug device processing.  Candidate code is replaced by
a call to the parser including a pointer to a local context-specific
functions, and local data.

In addition, several more opportunities to compress and reuse
common code between the old and new property parsers were applied.

Finally, a bug with the registration of slots was observed on some
systems, and the code was rewritten to prevent its reoccurrence.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
e feature" -- end of patch series applied to powerpc next)
---
Changes in V4:
  -- Update code to account for latest kernel checkins.
  -- Fix bug searching for virtual device slots.
  -- Rebased to 4.17-rc5 kernel
  -- Patch cleanup
---
 drivers/pci/hotplug/rpaphp_core.c |  181 ++++++++++++++++++++++++++-----------
 1 file changed, 126 insertions(+), 55 deletions(-)

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index 435c1a0..dc4ec68 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -222,47 +222,51 @@ static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
 	return -EINVAL;
 }
 
-static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
-				char *drc_type, unsigned int my_index)
+struct check_drc_props_v2_struct {
+	char *drc_name;
+	char *drc_type;
+	unsigned int my_index;
+};
+
+static int check_drc_props_v2_cb(struct of_drc_info *drc, void *idata,
+				void *not_used, int *ret_code)
 {
-	struct property *info;
-	unsigned int entries;
-	struct of_drc_info drc;
-	const __be32 *value;
+	struct check_drc_props_v2_struct *cdata = idata;
 	char cell_drc_name[MAX_DRC_NAME_LEN];
-	int j, fndit;
 
-	info = of_find_property(dn->parent, "ibm,drc-info", NULL);
-	if (info == NULL)
-		return -EINVAL;
+	(*ret_code) = -EINVAL;
 
-	value = info->value;
-	entries = of_read_number(value++, 1);
-
-	for (j = 0; j < entries; j++) {
-		of_read_drc_info_cell(&info, &value, &drc);
-
-		/* Should now know end of current entry */
-
-		if (my_index > drc.last_drc_index)
-			continue;
+	if (cdata->my_index > drc->last_drc_index)
+		return 0;
 
-		fndit = 1;
-		break;
+	/* Found drc_index.  Now match the rest. */
+	sprintf(cell_drc_name, "%s%d", drc->drc_name_prefix,
+		cdata->my_index - drc->drc_index_start +
+		drc->drc_name_suffix_start);
+
+	if (((cdata->drc_name == NULL) ||
+	     (cdata->drc_name && !strcmp(cdata->drc_name, cell_drc_name))) &&
+	    ((cdata->drc_type == NULL) ||
+	     (cdata->drc_type && !strcmp(cdata->drc_type, drc->drc_type)))) {
+		(*ret_code) = 0;
+		return 1;
 	}
-	/* Found it */
 
-	if (fndit)
-		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
-			my_index);
+	return 0;
+}
 
-	if (((drc_name == NULL) ||
-	     (drc_name && !strcmp(drc_name, cell_drc_name))) &&
-	    ((drc_type == NULL) ||
-	     (drc_type && !strcmp(drc_type, drc.drc_type))))
-		return 0;
+static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
+				char *drc_type, unsigned int my_index)
+{
+	struct device_node *root = dn;
+	struct check_drc_props_v2_struct cdata = {
+		drc_name, drc_type, be32_to_cpu(my_index) };
 
-	return -EINVAL;
+	if (!drc_type || (drc_type && strcmp(drc_type, "SLOT")))
+		root = dn->parent;
+
+	return drc_info_parser(root, check_drc_props_v2_cb,
+				drc_type, &cdata);
 }
 
 int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
@@ -285,7 +289,6 @@ int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
 }
 EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
 
-
 static int is_php_type(char *drc_type)
 {
 	unsigned long value;
@@ -345,17 +348,41 @@ static int is_php_dn(struct device_node *dn, const int **indexes,
  *
  * To remove a slot, it suffices to call rpaphp_deregister_slot().
  */
-int rpaphp_add_slot(struct device_node *dn)
+
+static int rpaphp_add_slot_common(struct device_node *dn,
+			u32 drc_index, char *drc_name, char *drc_type,
+			u32 drc_power_domain)
 {
 	struct slot *slot;
 	int retval = 0;
-	int i;
+
+	slot = alloc_slot_struct(dn, drc_index, drc_name, drc_power_domain);
+	if (!slot)
+		return -ENOMEM;
+
+	slot->type = simple_strtoul(drc_type, NULL, 10);
+	if (retval)
+		return -EINVAL;
+
+	dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
+		drc_index, drc_name, drc_type);
+
+	retval = rpaphp_enable_slot(slot);
+	if (!retval)
+		retval = rpaphp_register_slot(slot);
+
+	if (retval)
+		dealloc_slot_struct(slot);
+
+	return retval;
+}
+
+static int rpaphp_add_slot_v1(struct device_node *dn)
+{
+	int i, retval = 0;
 	const int *indexes, *names, *types, *power_domains;
 	char *name, *type;
 
-	if (!dn->name || strcmp(dn->name, "pci"))
-		return 0;
-
 	/* If this is not a hotplug slot, return without doing anything. */
 	if (!is_php_dn(dn, &indexes, &names, &types, &power_domains))
 		return 0;
@@ -366,25 +393,12 @@ int rpaphp_add_slot(struct device_node *dn)
 	name = (char *) &names[1];
 	type = (char *) &types[1];
 	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
-		int index;
-
-		index = be32_to_cpu(indexes[i + 1]);
-		slot = alloc_slot_struct(dn, index, name,
-					 be32_to_cpu(power_domains[i + 1]));
-		if (!slot)
-			return -ENOMEM;
-
-		slot->type = simple_strtoul(type, NULL, 10);
 
-		dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
-				index, name, type);
-
-		retval = rpaphp_enable_slot(slot);
+		retval = rpaphp_add_slot_common(dn,
+				be32_to_cpu(indexes[i + 1]), name, type,
+				be32_to_cpu(power_domains[i + 1]));
 		if (!retval)
-			retval = rpaphp_register_slot(slot);
-
-		if (retval)
-			dealloc_slot_struct(slot);
+			return retval;
 
 		name += strlen(name) + 1;
 		type += strlen(type) + 1;
@@ -394,6 +408,63 @@ int rpaphp_add_slot(struct device_node *dn)
 	/* XXX FIXME: reports a failure only if last entry in loop failed */
 	return retval;
 }
+
+struct rpaphp_add_slot_v2_struct {
+	struct device_node *dn;
+};
+
+static int rpaphp_add_slot_v2_cb(struct of_drc_info *drc, void *idata,
+				void *not_used, int *ret_code)
+{
+	struct rpaphp_add_slot_v2_struct *cdata = idata;
+	u32 drc_index;
+	char drc_name[MAX_DRC_NAME_LEN];
+	int i, retval;
+
+	(*ret_code) = -EINVAL;
+
+	if (!is_php_type((char *) drc->drc_type)) {
+		(*ret_code) = 0;
+		return 1;
+	}
+
+	for (i = 0, drc_index = drc->drc_index_start;
+		i < drc->num_sequential_elems; i++, drc_index++) {
+
+		sprintf(drc_name, "%s%d", drc->drc_name_prefix,
+			drc_index - drc->drc_index_start +
+			drc->drc_name_suffix_start);
+
+		retval = rpaphp_add_slot_common(cdata->dn,
+				drc_index, drc_name, drc->drc_type,
+				drc->drc_power_domain);
+		if (!retval) {
+			(*ret_code) = retval;
+			return 1;
+		}
+	}
+
+	(*ret_code) = retval;
+	return 0;
+}
+
+static int rpaphp_add_slot_v2(struct device_node *dn)
+{
+	struct rpaphp_add_slot_v2_struct cdata = { dn };
+
+	return drc_info_parser(dn, rpaphp_add_slot_v2_cb, NULL, &cdata);
+}
+
+int rpaphp_add_slot(struct device_node *dn)
+{
+	if (!dn->name || strcmp(dn->name, "pci"))
+		return 0;
+
+	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
+		return rpaphp_add_slot_v2(dn);
+	else
+		return rpaphp_add_slot_v1(dn);
+}
 EXPORT_SYMBOL_GPL(rpaphp_add_slot);
 
 static void __exit cleanup_slots(void)

Re: [RFC v4 1/4] hotplug/drcinfo: Simplify parse ibm, drc-info structs

[Nathan Fontenot]

On 05/22/2018 11:37 AM, Michael Bringmann wrote:
> Replace use of of_prop_next_u32() in when parsing 'ibm,drc-info'
> structure to simplify and reduce parsing code.
>

You mention that this patch is to fix the parsing of the drc-info struct, but you end up
making changes to the parsing code in pseries_energy.c and rpaphp_core.c. If there is a
bug in the parsing code in those files that should be submitted as a separate patch
outside of the drc-info fixups.

-Nathan
 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmware feature" -- end of patch series applied to powerpc next)
> ---
> Changes in V4:
>   -- Rebased patch to 4.17-rc5 kernel
>   -- Replace of_prop_next_u32() by of_read_number()
> ---
>  arch/powerpc/platforms/pseries/of_helpers.c     |   20 +++++---------------
>  arch/powerpc/platforms/pseries/pseries_energy.c |   10 ++++------
>  drivers/pci/hotplug/rpaphp_core.c               |    5 ++---
>  3 files changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
> index 6df192f..11b2ef1 100644
> --- a/arch/powerpc/platforms/pseries/of_helpers.c
> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
> @@ -65,29 +65,19 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
> 
>  	/* Get drc-index-start:encode-int */
>  	p2 = (const __be32 *)p;
> -	p2 = of_prop_next_u32(*prop, p2, &data->drc_index_start);
> -	if (!p2)
> -		return -EINVAL;
> +	data->drc_index_start = of_read_number(p2++, 1);
> 
>  	/* Get drc-name-suffix-start:encode-int */
> -	p2 = of_prop_next_u32(*prop, p2, &data->drc_name_suffix_start);
> -	if (!p2)
> -		return -EINVAL;
> +	data->drc_name_suffix_start = of_read_number(p2++, 1);
> 
>  	/* Get number-sequential-elements:encode-int */
> -	p2 = of_prop_next_u32(*prop, p2, &data->num_sequential_elems);
> -	if (!p2)
> -		return -EINVAL;
> +	data->num_sequential_elems = of_read_number(p2++, 1);
> 
>  	/* Get sequential-increment:encode-int */
> -	p2 = of_prop_next_u32(*prop, p2, &data->sequential_inc);
> -	if (!p2)
> -		return -EINVAL;
> +	data->sequential_inc = of_read_number(p2++, 1);
> 
>  	/* Get drc-power-domain:encode-int */
> -	p2 = of_prop_next_u32(*prop, p2, &data->drc_power_domain);
> -	if (!p2)
> -		return -EINVAL;
> +	data->drc_power_domain = of_read_number(p2++, 1);
> 
>  	/* Should now know end of current entry */
>  	(*curval) = (void *)p2;
> diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c
> index 6ed2212..5261975 100644
> --- a/arch/powerpc/platforms/pseries/pseries_energy.c
> +++ b/arch/powerpc/platforms/pseries/pseries_energy.c
> @@ -61,9 +61,8 @@ static u32 cpu_to_drc_index(int cpu)
>  		if (info == NULL)
>  			goto err_of_node_put;
> 
> -		value = of_prop_next_u32(info, NULL, &num_set_entries);
> -		if (!value)
> -			goto err_of_node_put;
> +		value = info->value;
> +		num_set_entries = of_read_number(value++, 1);
> 
>  		for (j = 0; j < num_set_entries; j++) {
> 
> @@ -123,9 +122,8 @@ static int drc_index_to_cpu(u32 drc_index)
>  		if (info == NULL)
>  			goto err_of_node_put;
> 
> -		value = of_prop_next_u32(info, NULL, &num_set_entries);
> -		if (!value)
> -			goto err_of_node_put;
> +		value = info->value;
> +		num_set_entries = of_read_number(value++, 1);
> 
>  		for (j = 0; j < num_set_entries; j++) {
> 
> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> index fb5e084..435c1a0 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -236,9 +236,8 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
>  	if (info == NULL)
>  		return -EINVAL;
> 
> -	value = of_prop_next_u32(info, NULL, &entries);
> -	if (!value)
> -		return -EINVAL;
> +	value = info->value;
> +	entries = of_read_number(value++, 1);
> 
>  	for (j = 0; j < entries; j++) {
>  		of_read_drc_info_cell(&info, &value, &drc);
> 

Re: [RFC v4 2/4] hotplug/drcinfo: Provide parser with callback

[Nathan Fontenot]

On 05/22/2018 11:37 AM, Michael Bringmann wrote:
> This patch provides a common parse function for the ibm,drc-info
> property that can be modified by a callback function.  The caller
> provides a pointer to the function and a pointer to their unique
> data, and the parser provides the current lmb set from the struct.
> The callback function may return codes indicating that the parsing
> is complete, or should continue, along with an error code that may
> be returned to the caller.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
> e feature" -- end of patch series applied to powerpc next)
> ---
> Changes in V4:
>   -- Update code to account for latest kernel checkins.
>   -- Rebased to 4.17-rc5 kernel
>   -- Some patch cleanup including file combination
> ---
>  arch/powerpc/include/asm/prom.h             |    7 +++++
>  arch/powerpc/platforms/pseries/of_helpers.c |   37 +++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index b04c5ce..2e947b3 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -94,6 +94,13 @@ struct of_drc_info {
>  extern int of_read_drc_info_cell(struct property **prop,
>  			const __be32 **curval, struct of_drc_info *data);
> 
> +extern int drc_info_parser(struct device_node *dn,
> +			int (*usercb)(struct of_drc_info *drc,
> +					void *data,
> +					void *optional_data,

The optional_data parameter to the callback routine doesn't seem to be used.

-Nathan

> +					int *ret_code),
> +			char *opt_drc_type,
> +			void *data);
> 
>  /*
>   * There are two methods for telling firmware what our capabilities are.
> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
> index 11b2ef1..a588ee6 100644
> --- a/arch/powerpc/platforms/pseries/of_helpers.c
> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
> @@ -6,6 +6,9 @@
>  #include <asm/prom.h>
> 
>  #include "of_helpers.h"
> +#include "pseries.h"
> +
> +#define	MAX_DRC_NAME_LEN 64
> 
>  /**
>   * pseries_of_derive_parent - basically like dirname(1)
> @@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
>  	return 0;
>  }
>  EXPORT_SYMBOL(of_read_drc_info_cell);
> +
> +int drc_info_parser(struct device_node *dn,
> +		int (*usercb)(struct of_drc_info *drc,
> +				void *data,
> +				void *optional_data,
> +				int *ret_code),
> +		char *opt_drc_type,
> +		void *data)
> +{
> +	struct property *info;
> +	unsigned int entries;
> +	struct of_drc_info drc;
> +	const __be32 *value;
> +	int j, done = 0, ret_code = -EINVAL;
> +
> +	info = of_find_property(dn, "ibm,drc-info", NULL);
> +	if (info == NULL)
> +		return -EINVAL;
> +
> +	value = info->value;
> +	entries = of_read_number(value++, 1);
> +
> +	for (j = 0, done = 0; (j < entries) && (!done); j++) {
> +		of_read_drc_info_cell(&info, &value, &drc);
> +
> +		if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type))
> +			continue;
> +
> +		done = usercb(&drc, data, NULL, &ret_code);
> +	}
> +
> +	return ret_code;
> +}
> +EXPORT_SYMBOL(drc_info_parser);
> 

Re: [RFC v4 2/4] hotplug/drcinfo: Provide parser with callback

[Nathan Fontenot]

On 05/22/2018 11:37 AM, Michael Bringmann wrote:
> This patch provides a common parse function for the ibm,drc-info
> property that can be modified by a callback function.  The caller
> provides a pointer to the function and a pointer to their unique
> data, and the parser provides the current lmb set from the struct.
> The callback function may return codes indicating that the parsing
> is complete, or should continue, along with an error code that may
> be returned to the caller.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
> e feature" -- end of patch series applied to powerpc next)
> ---
> Changes in V4:
>   -- Update code to account for latest kernel checkins.
>   -- Rebased to 4.17-rc5 kernel
>   -- Some patch cleanup including file combination
> ---
>  arch/powerpc/include/asm/prom.h             |    7 +++++
>  arch/powerpc/platforms/pseries/of_helpers.c |   37 +++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index b04c5ce..2e947b3 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -94,6 +94,13 @@ struct of_drc_info {
>  extern int of_read_drc_info_cell(struct property **prop,
>  			const __be32 **curval, struct of_drc_info *data);
> 
> +extern int drc_info_parser(struct device_node *dn,
> +			int (*usercb)(struct of_drc_info *drc,
> +					void *data,
> +					void *optional_data,
> +					int *ret_code),
> +			char *opt_drc_type,
> +			void *data);

After looking at the patch 3 in this series, I think a couple of comments and
a small change may help. It was not clear at first what the call back function
was supposed to return. After reading users of this routine it appears that the
callback function is returning a bool value indicating whether or not the parser
should continue. I documenting this and having the callback routine return a bool
may make this clearer.

Also, I see other places in the kernel name these types of routines as walk_*,
perhaps a slight name change to walk_drc_info_entries() may also make it clearer
what the code is doing.

-Nathan

> 
>  /*
>   * There are two methods for telling firmware what our capabilities are.
> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
> index 11b2ef1..a588ee6 100644
> --- a/arch/powerpc/platforms/pseries/of_helpers.c
> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
> @@ -6,6 +6,9 @@
>  #include <asm/prom.h>
> 
>  #include "of_helpers.h"
> +#include "pseries.h"
> +
> +#define	MAX_DRC_NAME_LEN 64
> 
>  /**
>   * pseries_of_derive_parent - basically like dirname(1)
> @@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
>  	return 0;
>  }
>  EXPORT_SYMBOL(of_read_drc_info_cell);
> +
> +int drc_info_parser(struct device_node *dn,
> +		int (*usercb)(struct of_drc_info *drc,
> +				void *data,
> +				void *optional_data,
> +				int *ret_code),
> +		char *opt_drc_type,
> +		void *data)
> +{
> +	struct property *info;
> +	unsigned int entries;
> +	struct of_drc_info drc;
> +	const __be32 *value;
> +	int j, done = 0, ret_code = -EINVAL;
> +
> +	info = of_find_property(dn, "ibm,drc-info", NULL);
> +	if (info == NULL)
> +		return -EINVAL;
> +
> +	value = info->value;
> +	entries = of_read_number(value++, 1);
> +
> +	for (j = 0, done = 0; (j < entries) && (!done); j++) {
> +		of_read_drc_info_cell(&info, &value, &drc);
> +
> +		if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type))
> +			continue;
> +
> +		done = usercb(&drc, data, NULL, &ret_code);
> +	}
> +
> +	return ret_code;
> +}
> +EXPORT_SYMBOL(drc_info_parser);
> 

Re: [RFC v4 3/4] hotplug/drcinfo: Fix hot-add CPU issues

[Nathan Fontenot]

On 05/22/2018 11:37 AM, Michael Bringmann wrote:
> This patch applies a common parse function for the ibm,drc-info
> property that can be modified by a callback function to the
> hot-add CPU code.  Candidate code is replaced by a call to the
> parser including a pointer to a local context-specific functions,
> and local data.
> 
> In addition, a bug in the release of the previous patch set may
> break things in some of the CPU DLPAR operations.  For instance,
> when attempting to hot-add a new CPU or set of CPUs, the original
> patch failed to always properly calculate the available resources,
> and aborted the operation.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
> e feature" -- end of patch series applied to powerpc next)
> ---
> Changes in V4:
>   -- Update code to account for latest kernel checkins.
>   -- Rebased to 4.17-rc5 kernel
>   -- Compress some more code
> ---
>  arch/powerpc/platforms/pseries/hotplug-cpu.c    |  118 +++++++++++++++++------
>  arch/powerpc/platforms/pseries/pseries_energy.c |  107 +++++++++++----------
>  2 files changed, 141 insertions(+), 84 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 6ef77ca..ceacad9 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -411,27 +411,63 @@ static bool dlpar_cpu_exists(struct device_node *parent, u32 drc_index)
>  	return found;
>  }
> 
> -static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
> +static bool check_cpu_drc_index(struct device_node *parent,
> +				int (*cb)(struct of_drc_info *drc,
> +					void *data, void *not_used,
> +					int *ret_code),
> +				void *cdata)
>  {
>  	bool found = false;
> -	int rc, index;
> 
> -	index = 0;
> -	while (!found) {
> -		u32 drc;
> +	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
> +		if (drc_info_parser(parent, cb, "CPU", cdata))
> +			found = true;
> +	} else {
> +		int index = 0;
> 
> -		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
> -						index++, &drc);
> -		if (rc)
> -			break;
> +		while (!found) {
> +			u32 drc;
> 
> -		if (drc == drc_index)
> -			found = true;
> +			if (of_property_read_u32_index(parent,
> +						"ibm,drc-indexes",
> +						index++, &drc))
> +				break;
> +			if (cb(NULL, cdata, &drc, NULL))
> +				found = true;
> +		}
>  	}
> 
>  	return found;
>  }
> 
> +struct valid_drc_index_struct {
> +	u32 targ_drc_index;
> +};

Can you help me understand the need to encapsulate the drc_index as a struct.

> +
> +static int valid_drc_index_cb(struct of_drc_info *drc, void *idata,
> +				void *drc_index, int *ret_code)
> +{
> +	struct valid_drc_index_struct *cdata = idata;
> +
> +	if (drc) {
> +		if (!((drc->drc_index_start <= cdata->targ_drc_index) &&
> +			(cdata->targ_drc_index <= drc->last_drc_index)))
> +			return 0;
> +	} else {
> +		if (*((u32 *)drc_index) != cdata->targ_drc_index)
> +			return 0;
> +	}
> +	(*ret_code) = 1;
> +	return 1;
> +}
> +
> +static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
> +{
> +	struct valid_drc_index_struct cdata = { drc_index };
> +
> +	return check_cpu_drc_index(parent, valid_drc_index_cb, &cdata);
> +}
> +
>  static ssize_t dlpar_cpu_add(u32 drc_index)
>  {
>  	struct device_node *dn, *parent;
> @@ -721,11 +757,43 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
>  	return rc;
>  }
> 
> +struct cpus_to_add_struct {
> +	struct device_node *parent;
> +	u32 *cpu_drcs;
> +	u32 cpus_to_add;
> +	u32 cpus_found;
> +};
> +
> +static int cpus_to_add_cb(struct of_drc_info *drc, void *idata,
> +			void *drc_index, int *ret_code)
> +{
> +	struct cpus_to_add_struct *cdata = idata;
> +
> +	if (drc) {
> +		int k;
> +
> +		for (k = 0; (k < drc->num_sequential_elems) &&
> +			(cdata->cpus_found < cdata->cpus_to_add); k++) {
> +			u32 idrc = drc->drc_index_start +
> +				(k * drc->sequential_inc);
> +
> +			if (dlpar_cpu_exists(cdata->parent, idrc))
> +				continue;
> +			cdata->cpu_drcs[cdata->cpus_found++] = idrc;
> +		}
> +	} else {
> +		if (!dlpar_cpu_exists(cdata->parent, *((u32 *)drc_index)))
> +			cdata->cpu_drcs[cdata->cpus_found++] =
> +				*((u32 *)drc_index);
> +	}
> +	return 0;
> +}
> +
>  static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>  {
>  	struct device_node *parent;
> -	int cpus_found = 0;
> -	int index, rc;
> +	struct cpus_to_add_struct cdata = {
> +		NULL, cpu_drcs, cpus_to_add, 0 };
> 
>  	parent = of_find_node_by_path("/cpus");
>  	if (!parent) {
> @@ -734,28 +802,14 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>  		return -1;
>  	}
> 
> -	/* 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.
> +	/* Search the appropriate property for possible CPU drcs to
> +	 * add.
>  	 */
> -	index = 1;
> -	while (cpus_found < cpus_to_add) {
> -		u32 drc;
> -
> -		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
> -						index++, &drc);
> -		if (rc)
> -			break;
> -
> -		if (dlpar_cpu_exists(parent, drc))
> -			continue;
> -
> -		cpu_drcs[cpus_found++] = drc;
> -	}
> +	cdata.parent = parent;
> +	check_cpu_drc_index(parent, cpus_to_add_cb, &cdata);
> 
>  	of_node_put(parent);
> -	return cpus_found;
> +	return cdata.cpus_found;
>  }
> 
>  static int dlpar_cpu_add_by_count(u32 cpus_to_add)
> diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c
> index 5261975..d8d7750 100644
> --- a/arch/powerpc/platforms/pseries/pseries_energy.c
> +++ b/arch/powerpc/platforms/pseries/pseries_energy.c
> @@ -36,6 +36,26 @@
> 
>  /* Helper Routines to convert between drc_index to cpu numbers */
> 
> +struct cpu_to_drc_index_struct {
> +	u32	thread_index;
> +	u32	ret;
> +};
> +
> +static int cpu_to_drc_index_cb(struct of_drc_info *drc, void *idata,
> +				void *not_used, int *ret_code)
> +{
> +	struct cpu_to_drc_index_struct *cdata = idata;
> +	int ret = 0;
> +
> +	if (cdata->thread_index < drc->last_drc_index) {

Is this correct? You're comparing a thread index to a drc index.

> +		cdata->ret = drc->drc_index_start +
> +			(cdata->thread_index * drc->sequential_inc);
> +		ret = 1;
> +	}
> +	(*ret_code) = ret;
> +	return ret;
> +}
> +
>  static u32 cpu_to_drc_index(int cpu)
>  {
>  	struct device_node *dn = NULL;
> @@ -51,30 +71,14 @@ static u32 cpu_to_drc_index(int cpu)
>  	thread_index = cpu_core_index_of_thread(cpu);
> 
>  	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
> -		struct property *info = NULL;
> -		struct of_drc_info drc;
> -		int j;
> -		u32 num_set_entries;
> -		const __be32 *value;
> -
> -		info = of_find_property(dn, "ibm,drc-info", NULL);
> -		if (info == NULL)
> -			goto err_of_node_put;
> +		struct cpu_to_drc_index_struct cdata = {
> +			thread_index, 0 };
> 
> -		value = info->value;
> -		num_set_entries = of_read_number(value++, 1);
> -
> -		for (j = 0; j < num_set_entries; j++) {
> -
> -			of_read_drc_info_cell(&info, &value, &drc);
> -			if (strncmp(drc.drc_type, "CPU", 3))
> -				goto err;
> -
> -			if (thread_index < drc.last_drc_index)
> -				break;
> -		}
> -
> -		ret = drc.drc_index_start + (thread_index * drc.sequential_inc);
> +		rc = drc_info_parser(dn, &cpu_to_drc_index_cb,
> +					"CPU", &cdata);
> +		if (rc < 0)
> +			goto err_of_node_put;
> +		ret = cdata.ret;
>  	} else {
>  		const __be32 *indexes;
> 
> @@ -100,11 +104,33 @@ static u32 cpu_to_drc_index(int cpu)
>  	return ret;
>  }
> 
> +struct drc_index_to_cpu_struct {
> +	u32	drc_index;
> +	u32	thread_index;
> +	u32	cpu;
> +};
> +
> +static int drc_index_to_cpu_cb(struct of_drc_info *drc,
> +				void *idata, void *not_used, int *ret_code)
> +{
> +	struct drc_index_to_cpu_struct *cdata = idata;
> +
> +	if (cdata->drc_index > drc->last_drc_index) {
> +		cdata->cpu += drc->num_sequential_elems;
> +	} else {
> +		cdata->cpu += ((cdata->drc_index - drc->drc_index_start) /
> +				drc->sequential_inc);
> +		cdata->thread_index = cpu_first_thread_of_core(cdata->cpu);

Should this return 1 here to avoid continuing to walk the drc_info entries?

-Nathan

> +	}
> +	(*ret_code) = 0;
> +	return 0;
> +}
> +
>  static int drc_index_to_cpu(u32 drc_index)
>  {
>  	struct device_node *dn = NULL;
>  	const int *indexes;
> -	int thread_index = 0, cpu = 0;
> +	int thread_index = 0;
>  	int rc = 1;
> 
>  	dn = of_find_node_by_path("/cpus");
> @@ -112,36 +138,13 @@ static int drc_index_to_cpu(u32 drc_index)
>  		goto err;
> 
>  	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
> -		struct property *info = NULL;
> -		struct of_drc_info drc;
> -		int j;
> -		u32 num_set_entries;
> -		const __be32 *value;
> -
> -		info = of_find_property(dn, "ibm,drc-info", NULL);
> -		if (info == NULL)
> -			goto err_of_node_put;
> -
> -		value = info->value;
> -		num_set_entries = of_read_number(value++, 1);
> -
> -		for (j = 0; j < num_set_entries; j++) {
> +		struct drc_index_to_cpu_struct cdata = {
> +			drc_index, 0, 0 };
> 
> -			of_read_drc_info_cell(&info, &value, &drc);
> -			if (strncmp(drc.drc_type, "CPU", 3))
> -				goto err;
> +		rc = drc_info_parser(dn, &drc_index_to_cpu_cb,
> +					"CPU", &cdata);
> +		thread_index = cdata.thread_index;
> 
> -			if (drc_index > drc.last_drc_index) {
> -				cpu += drc.num_sequential_elems;
> -				continue;
> -			}
> -			cpu += ((drc_index - drc.drc_index_start) /
> -				drc.sequential_inc);
> -
> -			thread_index = cpu_first_thread_of_core(cpu);
> -			rc = 0;
> -			break;
> -		}
>  	} else {
>  		unsigned long int i;
> 

Re: [RFC v4 4/4] hotplug/drcinfo: Code cleanup for devices

[Nathan Fontenot]

On 05/22/2018 11:37 AM, Michael Bringmann wrote:
> This patch extends the use of a common parse function for the
> ibm,drc-info property that can be modified by a callback function
> to the hotplug device processing.  Candidate code is replaced by
> a call to the parser including a pointer to a local context-specific
> functions, and local data.
> 
> In addition, several more opportunities to compress and reuse
> common code between the old and new property parsers were applied.
> 
> Finally, a bug with the registration of slots was observed on some
> systems, and the code was rewritten to prevent its reoccurrence.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
> e feature" -- end of patch series applied to powerpc next)
> ---
> Changes in V4:
>   -- Update code to account for latest kernel checkins.
>   -- Fix bug searching for virtual device slots.
>   -- Rebased to 4.17-rc5 kernel
>   -- Patch cleanup
> ---
>  drivers/pci/hotplug/rpaphp_core.c |  181 ++++++++++++++++++++++++++-----------
>  1 file changed, 126 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> index 435c1a0..dc4ec68 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -222,47 +222,51 @@ static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
>  	return -EINVAL;
>  }
> 
> -static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
> -				char *drc_type, unsigned int my_index)
> +struct check_drc_props_v2_struct {
> +	char *drc_name;
> +	char *drc_type;
> +	unsigned int my_index;
> +};
> +
> +static int check_drc_props_v2_cb(struct of_drc_info *drc, void *idata,
> +				void *not_used, int *ret_code)
>  {
> -	struct property *info;
> -	unsigned int entries;
> -	struct of_drc_info drc;
> -	const __be32 *value;
> +	struct check_drc_props_v2_struct *cdata = idata;
>  	char cell_drc_name[MAX_DRC_NAME_LEN];
> -	int j, fndit;
> 
> -	info = of_find_property(dn->parent, "ibm,drc-info", NULL);
> -	if (info == NULL)
> -		return -EINVAL;
> +	(*ret_code) = -EINVAL;
> 
> -	value = info->value;
> -	entries = of_read_number(value++, 1);
> -
> -	for (j = 0; j < entries; j++) {
> -		of_read_drc_info_cell(&info, &value, &drc);
> -
> -		/* Should now know end of current entry */
> -
> -		if (my_index > drc.last_drc_index)
> -			continue;
> +	if (cdata->my_index > drc->last_drc_index)
> +		return 0;
> 
> -		fndit = 1;
> -		break;
> +	/* Found drc_index.  Now match the rest. */
> +	sprintf(cell_drc_name, "%s%d", drc->drc_name_prefix,
> +		cdata->my_index - drc->drc_index_start +
> +		drc->drc_name_suffix_start);
> +
> +	if (((cdata->drc_name == NULL) ||
> +	     (cdata->drc_name && !strcmp(cdata->drc_name, cell_drc_name))) &&
> +	    ((cdata->drc_type == NULL) ||
> +	     (cdata->drc_type && !strcmp(cdata->drc_type, drc->drc_type)))) {
> +		(*ret_code) = 0;
> +		return 1;
>  	}
> -	/* Found it */
> 
> -	if (fndit)
> -		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
> -			my_index);
> +	return 0;
> +}
> 
> -	if (((drc_name == NULL) ||
> -	     (drc_name && !strcmp(drc_name, cell_drc_name))) &&
> -	    ((drc_type == NULL) ||
> -	     (drc_type && !strcmp(drc_type, drc.drc_type))))
> -		return 0;
> +static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
> +				char *drc_type, unsigned int my_index)
> +{
> +	struct device_node *root = dn;
> +	struct check_drc_props_v2_struct cdata = {
> +		drc_name, drc_type, be32_to_cpu(my_index) };
> 
> -	return -EINVAL;
> +	if (!drc_type || (drc_type && strcmp(drc_type, "SLOT")))
> +		root = dn->parent;
> +
> +	return drc_info_parser(root, check_drc_props_v2_cb,
> +				drc_type, &cdata);
>  }
> 
>  int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
> @@ -285,7 +289,6 @@ int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
>  }
>  EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
> 
> -
>  static int is_php_type(char *drc_type)
>  {
>  	unsigned long value;
> @@ -345,17 +348,41 @@ static int is_php_dn(struct device_node *dn, const int **indexes,
>   *
>   * To remove a slot, it suffices to call rpaphp_deregister_slot().
>   */
> -int rpaphp_add_slot(struct device_node *dn)
> +
> +static int rpaphp_add_slot_common(struct device_node *dn,
> +			u32 drc_index, char *drc_name, char *drc_type,
> +			u32 drc_power_domain)
>  {
>  	struct slot *slot;
>  	int retval = 0;
> -	int i;
> +
> +	slot = alloc_slot_struct(dn, drc_index, drc_name, drc_power_domain);
> +	if (!slot)
> +		return -ENOMEM;
> +
> +	slot->type = simple_strtoul(drc_type, NULL, 10);
> +	if (retval)
> +		return -EINVAL;
> +
> +	dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
> +		drc_index, drc_name, drc_type);
> +
> +	retval = rpaphp_enable_slot(slot);
> +	if (!retval)
> +		retval = rpaphp_register_slot(slot);
> +
> +	if (retval)
> +		dealloc_slot_struct(slot);
> +
> +	return retval;
> +}
> +
> +static int rpaphp_add_slot_v1(struct device_node *dn)
> +{
> +	int i, retval = 0;
>  	const int *indexes, *names, *types, *power_domains;
>  	char *name, *type;
> 
> -	if (!dn->name || strcmp(dn->name, "pci"))
> -		return 0;
> -
>  	/* If this is not a hotplug slot, return without doing anything. */
>  	if (!is_php_dn(dn, &indexes, &names, &types, &power_domains))
>  		return 0;
> @@ -366,25 +393,12 @@ int rpaphp_add_slot(struct device_node *dn)
>  	name = (char *) &names[1];
>  	type = (char *) &types[1];
>  	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
> -		int index;
> -
> -		index = be32_to_cpu(indexes[i + 1]);
> -		slot = alloc_slot_struct(dn, index, name,
> -					 be32_to_cpu(power_domains[i + 1]));
> -		if (!slot)
> -			return -ENOMEM;
> -
> -		slot->type = simple_strtoul(type, NULL, 10);
> 
> -		dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
> -				index, name, type);
> -
> -		retval = rpaphp_enable_slot(slot);
> +		retval = rpaphp_add_slot_common(dn,
> +				be32_to_cpu(indexes[i + 1]), name, type,
> +				be32_to_cpu(power_domains[i + 1]));
>  		if (!retval)
> -			retval = rpaphp_register_slot(slot);
> -
> -		if (retval)
> -			dealloc_slot_struct(slot);
> +			return retval;
> 
>  		name += strlen(name) + 1;
>  		type += strlen(type) + 1;
> @@ -394,6 +408,63 @@ int rpaphp_add_slot(struct device_node *dn)
>  	/* XXX FIXME: reports a failure only if last entry in loop failed */
>  	return retval;
>  }
> +
> +struct rpaphp_add_slot_v2_struct {
> +	struct device_node *dn;
> +};
> +
> +static int rpaphp_add_slot_v2_cb(struct of_drc_info *drc, void *idata,
> +				void *not_used, int *ret_code)
> +{
> +	struct rpaphp_add_slot_v2_struct *cdata = idata;
> +	u32 drc_index;
> +	char drc_name[MAX_DRC_NAME_LEN];
> +	int i, retval;
> +
> +	(*ret_code) = -EINVAL;
> +
> +	if (!is_php_type((char *) drc->drc_type)) {
> +		(*ret_code) = 0;
> +		return 1;
> +	}
> +
> +	for (i = 0, drc_index = drc->drc_index_start;
> +		i < drc->num_sequential_elems; i++, drc_index++) {
> +
> +		sprintf(drc_name, "%s%d", drc->drc_name_prefix,
> +			drc_index - drc->drc_index_start +
> +			drc->drc_name_suffix_start);
> +
> +		retval = rpaphp_add_slot_common(cdata->dn,
> +				drc_index, drc_name, drc->drc_type,
> +				drc->drc_power_domain);
> +		if (!retval) {
> +			(*ret_code) = retval;
> +			return 1;
> +		}
> +	}

I may be reading this loop wrong, but it appears that it will add all drc indexes
for all drc_info entries to the specified device node, minus drc-types that don't match.

-Nathan

> +
> +	(*ret_code) = retval;
> +	return 0;
> +}
> +
> +static int rpaphp_add_slot_v2(struct device_node *dn)
> +{
> +	struct rpaphp_add_slot_v2_struct cdata = { dn };
> +
> +	return drc_info_parser(dn, rpaphp_add_slot_v2_cb, NULL, &cdata);
> +}
> +
> +int rpaphp_add_slot(struct device_node *dn)
> +{
> +	if (!dn->name || strcmp(dn->name, "pci"))
> +		return 0;
> +
> +	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
> +		return rpaphp_add_slot_v2(dn);
> +	else
> +		return rpaphp_add_slot_v1(dn);
> +}
>  EXPORT_SYMBOL_GPL(rpaphp_add_slot);
> 
>  static void __exit cleanup_slots(void)
> 

Re: [RFC v4 2/4] hotplug/drcinfo: Provide parser with callback

[Michael Bringmann]

See below.

On 05/22/2018 04:02 PM, Nathan Fontenot wrote:
> On 05/22/2018 11:37 AM, Michael Bringmann wrote:
>> This patch provides a common parse function for the ibm,drc-info
>> property that can be modified by a callback function.  The caller
>> provides a pointer to the function and a pointer to their unique
>> data, and the parser provides the current lmb set from the struct.
>> The callback function may return codes indicating that the parsing
>> is complete, or should continue, along with an error code that may
>> be returned to the caller.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
>> e feature" -- end of patch series applied to powerpc next)
>> ---
>> Changes in V4:
>>   -- Update code to account for latest kernel checkins.
>>   -- Rebased to 4.17-rc5 kernel
>>   -- Some patch cleanup including file combination
>> ---
>>  arch/powerpc/include/asm/prom.h             |    7 +++++
>>  arch/powerpc/platforms/pseries/of_helpers.c |   37 +++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
>> index b04c5ce..2e947b3 100644
>> --- a/arch/powerpc/include/asm/prom.h
>> +++ b/arch/powerpc/include/asm/prom.h
>> @@ -94,6 +94,13 @@ struct of_drc_info {
>>  extern int of_read_drc_info_cell(struct property **prop,
>>  			const __be32 **curval, struct of_drc_info *data);
>>
>> +extern int drc_info_parser(struct device_node *dn,
>> +			int (*usercb)(struct of_drc_info *drc,
>> +					void *data,
>> +					void *optional_data,
> 
> The optional_data parameter to the callback routine doesn't seem to be used.
> 
> -Nathan

There was one odd case when parsing the 'ibm,drc-info' code where
I remember needing extra data.  I think that I can compress it all
into the other pointer though.

Will remove the extra argument.

Michael
> 
>> +					int *ret_code),
>> +			char *opt_drc_type,
>> +			void *data);
>>
>>  /*
>>   * There are two methods for telling firmware what our capabilities are.
>> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
>> index 11b2ef1..a588ee6 100644
>> --- a/arch/powerpc/platforms/pseries/of_helpers.c
>> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
>> @@ -6,6 +6,9 @@
>>  #include <asm/prom.h>
>>
>>  #include "of_helpers.h"
>> +#include "pseries.h"
>> +
>> +#define	MAX_DRC_NAME_LEN 64
>>
>>  /**
>>   * pseries_of_derive_parent - basically like dirname(1)
>> @@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL(of_read_drc_info_cell);
>> +
>> +int drc_info_parser(struct device_node *dn,
>> +		int (*usercb)(struct of_drc_info *drc,
>> +				void *data,
>> +				void *optional_data,
>> +				int *ret_code),
>> +		char *opt_drc_type,
>> +		void *data)
>> +{
>> +	struct property *info;
>> +	unsigned int entries;
>> +	struct of_drc_info drc;
>> +	const __be32 *value;
>> +	int j, done = 0, ret_code = -EINVAL;
>> +
>> +	info = of_find_property(dn, "ibm,drc-info", NULL);
>> +	if (info == NULL)
>> +		return -EINVAL;
>> +
>> +	value = info->value;
>> +	entries = of_read_number(value++, 1);
>> +
>> +	for (j = 0, done = 0; (j < entries) && (!done); j++) {
>> +		of_read_drc_info_cell(&info, &value, &drc);
>> +
>> +		if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type))
>> +			continue;
>> +
>> +		done = usercb(&drc, data, NULL, &ret_code);
>> +	}
>> +
>> +	return ret_code;
>> +}
>> +EXPORT_SYMBOL(drc_info_parser);
>>
> 
> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

Re: [RFC v4 2/4] hotplug/drcinfo: Provide parser with callback

[Michael Bringmann]

See below.

On 05/22/2018 04:23 PM, Nathan Fontenot wrote:
> On 05/22/2018 11:37 AM, Michael Bringmann wrote:
>> This patch provides a common parse function for the ibm,drc-info
>> property that can be modified by a callback function.  The caller
>> provides a pointer to the function and a pointer to their unique
>> data, and the parser provides the current lmb set from the struct.
>> The callback function may return codes indicating that the parsing
>> is complete, or should continue, along with an error code that may
>> be returned to the caller.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
>> e feature" -- end of patch series applied to powerpc next)
>> ---
>> Changes in V4:
>>   -- Update code to account for latest kernel checkins.
>>   -- Rebased to 4.17-rc5 kernel
>>   -- Some patch cleanup including file combination
>> ---
>>  arch/powerpc/include/asm/prom.h             |    7 +++++
>>  arch/powerpc/platforms/pseries/of_helpers.c |   37 +++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
>> index b04c5ce..2e947b3 100644
>> --- a/arch/powerpc/include/asm/prom.h
>> +++ b/arch/powerpc/include/asm/prom.h
>> @@ -94,6 +94,13 @@ struct of_drc_info {
>>  extern int of_read_drc_info_cell(struct property **prop,
>>  			const __be32 **curval, struct of_drc_info *data);
>>
>> +extern int drc_info_parser(struct device_node *dn,
>> +			int (*usercb)(struct of_drc_info *drc,
>> +					void *data,
>> +					void *optional_data,
>> +					int *ret_code),
>> +			char *opt_drc_type,
>> +			void *data);
> 
> After looking at the patch 3 in this series, I think a couple of comments and
> a small change may help. It was not clear at first what the call back function
> was supposed to return. After reading users of this routine it appears that the
> callback function is returning a bool value indicating whether or not the parser
> should continue. I documenting this and having the callback routine return a bool
> may make this clearer.

Okay.
> 
> Also, I see other places in the kernel name these types of routines as walk_*,
> perhaps a slight name change to walk_drc_info_entries() may also make it clearer
> what the code is doing.

Okay.
> 
> -Nathan

Michael
> 
>>
>>  /*
>>   * There are two methods for telling firmware what our capabilities are.
>> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
>> index 11b2ef1..a588ee6 100644
>> --- a/arch/powerpc/platforms/pseries/of_helpers.c
>> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
>> @@ -6,6 +6,9 @@
>>  #include <asm/prom.h>
>>
>>  #include "of_helpers.h"
>> +#include "pseries.h"
>> +
>> +#define	MAX_DRC_NAME_LEN 64
>>
>>  /**
>>   * pseries_of_derive_parent - basically like dirname(1)
>> @@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL(of_read_drc_info_cell);
>> +
>> +int drc_info_parser(struct device_node *dn,
>> +		int (*usercb)(struct of_drc_info *drc,
>> +				void *data,
>> +				void *optional_data,
>> +				int *ret_code),
>> +		char *opt_drc_type,
>> +		void *data)
>> +{
>> +	struct property *info;
>> +	unsigned int entries;
>> +	struct of_drc_info drc;
>> +	const __be32 *value;
>> +	int j, done = 0, ret_code = -EINVAL;
>> +
>> +	info = of_find_property(dn, "ibm,drc-info", NULL);
>> +	if (info == NULL)
>> +		return -EINVAL;
>> +
>> +	value = info->value;
>> +	entries = of_read_number(value++, 1);
>> +
>> +	for (j = 0, done = 0; (j < entries) && (!done); j++) {
>> +		of_read_drc_info_cell(&info, &value, &drc);
>> +
>> +		if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type))
>> +			continue;
>> +
>> +		done = usercb(&drc, data, NULL, &ret_code);
>> +	}
>> +
>> +	return ret_code;
>> +}
>> +EXPORT_SYMBOL(drc_info_parser);
>>
> 
> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

Re: [RFC v4 3/4] hotplug/drcinfo: Fix hot-add CPU issues

[Michael Bringmann]

See below.

On 05/22/2018 04:31 PM, Nathan Fontenot wrote:
> On 05/22/2018 11:37 AM, Michael Bringmann wrote:
>> This patch applies a common parse function for the ibm,drc-info
>> property that can be modified by a callback function to the
>> hot-add CPU code.  Candidate code is replaced by a call to the
>> parser including a pointer to a local context-specific functions,
>> and local data.
>>
>> In addition, a bug in the release of the previous patch set may
>> break things in some of the CPU DLPAR operations.  For instance,
>> when attempting to hot-add a new CPU or set of CPUs, the original
>> patch failed to always properly calculate the available resources,
>> and aborted the operation.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
>> e feature" -- end of patch series applied to powerpc next)
>> ---
>> Changes in V4:
>>   -- Update code to account for latest kernel checkins.
>>   -- Rebased to 4.17-rc5 kernel
>>   -- Compress some more code
>> ---
>>  arch/powerpc/platforms/pseries/hotplug-cpu.c    |  118 +++++++++++++++++------
>>  arch/powerpc/platforms/pseries/pseries_energy.c |  107 +++++++++++----------
>>  2 files changed, 141 insertions(+), 84 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index 6ef77ca..ceacad9 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -411,27 +411,63 @@ static bool dlpar_cpu_exists(struct device_node *parent, u32 drc_index)
>>  	return found;
>>  }
>>
>> -static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
>> +static bool check_cpu_drc_index(struct device_node *parent,
>> +				int (*cb)(struct of_drc_info *drc,
>> +					void *data, void *not_used,
>> +					int *ret_code),
>> +				void *cdata)
>>  {
>>  	bool found = false;
>> -	int rc, index;
>>
>> -	index = 0;
>> -	while (!found) {
>> -		u32 drc;
>> +	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
>> +		if (drc_info_parser(parent, cb, "CPU", cdata))
>> +			found = true;
>> +	} else {
>> +		int index = 0;
>>
>> -		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
>> -						index++, &drc);
>> -		if (rc)
>> -			break;
>> +		while (!found) {
>> +			u32 drc;
>>
>> -		if (drc == drc_index)
>> -			found = true;
>> +			if (of_property_read_u32_index(parent,
>> +						"ibm,drc-indexes",
>> +						index++, &drc))
>> +				break;
>> +			if (cb(NULL, cdata, &drc, NULL))
>> +				found = true;
>> +		}
>>  	}
>>
>>  	return found;
>>  }
>>
>> +struct valid_drc_index_struct {
>> +	u32 targ_drc_index;
>> +};
> 
> Can you help me understand the need to encapsulate the drc_index as a struct.

At this point, it is consistency of use across all of the instances
of using 'walk_drc_info'.  I believe that there were more values in
the structure earlier on.

> 
>> +
>> +static int valid_drc_index_cb(struct of_drc_info *drc, void *idata,
>> +				void *drc_index, int *ret_code)
>> +{
>> +	struct valid_drc_index_struct *cdata = idata;
>> +
>> +	if (drc) {
>> +		if (!((drc->drc_index_start <= cdata->targ_drc_index) &&
>> +			(cdata->targ_drc_index <= drc->last_drc_index)))
>> +			return 0;
>> +	} else {
>> +		if (*((u32 *)drc_index) != cdata->targ_drc_index)
>> +			return 0;
>> +	}
>> +	(*ret_code) = 1;
>> +	return 1;
>> +}
>> +
>> +static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
>> +{
>> +	struct valid_drc_index_struct cdata = { drc_index };
>> +
>> +	return check_cpu_drc_index(parent, valid_drc_index_cb, &cdata);
>> +}
>> +
>>  static ssize_t dlpar_cpu_add(u32 drc_index)
>>  {
>>  	struct device_node *dn, *parent;
>> @@ -721,11 +757,43 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
>>  	return rc;
>>  }
>>
>> +struct cpus_to_add_struct {
>> +	struct device_node *parent;
>> +	u32 *cpu_drcs;
>> +	u32 cpus_to_add;
>> +	u32 cpus_found;
>> +};
>> +
>> +static int cpus_to_add_cb(struct of_drc_info *drc, void *idata,
>> +			void *drc_index, int *ret_code)
>> +{
>> +	struct cpus_to_add_struct *cdata = idata;
>> +
>> +	if (drc) {
>> +		int k;
>> +
>> +		for (k = 0; (k < drc->num_sequential_elems) &&
>> +			(cdata->cpus_found < cdata->cpus_to_add); k++) {
>> +			u32 idrc = drc->drc_index_start +
>> +				(k * drc->sequential_inc);
>> +
>> +			if (dlpar_cpu_exists(cdata->parent, idrc))
>> +				continue;
>> +			cdata->cpu_drcs[cdata->cpus_found++] = idrc;
>> +		}
>> +	} else {
>> +		if (!dlpar_cpu_exists(cdata->parent, *((u32 *)drc_index)))
>> +			cdata->cpu_drcs[cdata->cpus_found++] =
>> +				*((u32 *)drc_index);
>> +	}
>> +	return 0;
>> +}
>> +
>>  static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>>  {
>>  	struct device_node *parent;
>> -	int cpus_found = 0;
>> -	int index, rc;
>> +	struct cpus_to_add_struct cdata = {
>> +		NULL, cpu_drcs, cpus_to_add, 0 };
>>
>>  	parent = of_find_node_by_path("/cpus");
>>  	if (!parent) {
>> @@ -734,28 +802,14 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>>  		return -1;
>>  	}
>>
>> -	/* 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.
>> +	/* Search the appropriate property for possible CPU drcs to
>> +	 * add.
>>  	 */
>> -	index = 1;
>> -	while (cpus_found < cpus_to_add) {
>> -		u32 drc;
>> -
>> -		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
>> -						index++, &drc);
>> -		if (rc)
>> -			break;
>> -
>> -		if (dlpar_cpu_exists(parent, drc))
>> -			continue;
>> -
>> -		cpu_drcs[cpus_found++] = drc;
>> -	}
>> +	cdata.parent = parent;
>> +	check_cpu_drc_index(parent, cpus_to_add_cb, &cdata);
>>
>>  	of_node_put(parent);
>> -	return cpus_found;
>> +	return cdata.cpus_found;
>>  }
>>
>>  static int dlpar_cpu_add_by_count(u32 cpus_to_add)
>> diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c
>> index 5261975..d8d7750 100644
>> --- a/arch/powerpc/platforms/pseries/pseries_energy.c
>> +++ b/arch/powerpc/platforms/pseries/pseries_energy.c
>> @@ -36,6 +36,26 @@
>>
>>  /* Helper Routines to convert between drc_index to cpu numbers */
>>
>> +struct cpu_to_drc_index_struct {
>> +	u32	thread_index;
>> +	u32	ret;
>> +};
>> +
>> +static int cpu_to_drc_index_cb(struct of_drc_info *drc, void *idata,
>> +				void *not_used, int *ret_code)
>> +{
>> +	struct cpu_to_drc_index_struct *cdata = idata;
>> +	int ret = 0;
>> +
>> +	if (cdata->thread_index < drc->last_drc_index) {
> 
> Is this correct? You're comparing a thread index to a drc index.

I believe so.  I will check when I retest this week, hopefully.

> 
>> +		cdata->ret = drc->drc_index_start +
>> +			(cdata->thread_index * drc->sequential_inc);
>> +		ret = 1;
>> +	}
>> +	(*ret_code) = ret;
>> +	return ret;
>> +}
>> +
>>  static u32 cpu_to_drc_index(int cpu)
>>  {
>>  	struct device_node *dn = NULL;
>> @@ -51,30 +71,14 @@ static u32 cpu_to_drc_index(int cpu)
>>  	thread_index = cpu_core_index_of_thread(cpu);
>>
>>  	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
>> -		struct property *info = NULL;
>> -		struct of_drc_info drc;
>> -		int j;
>> -		u32 num_set_entries;
>> -		const __be32 *value;
>> -
>> -		info = of_find_property(dn, "ibm,drc-info", NULL);
>> -		if (info == NULL)
>> -			goto err_of_node_put;
>> +		struct cpu_to_drc_index_struct cdata = {
>> +			thread_index, 0 };
>>
>> -		value = info->value;
>> -		num_set_entries = of_read_number(value++, 1);
>> -
>> -		for (j = 0; j < num_set_entries; j++) {
>> -
>> -			of_read_drc_info_cell(&info, &value, &drc);
>> -			if (strncmp(drc.drc_type, "CPU", 3))
>> -				goto err;
>> -
>> -			if (thread_index < drc.last_drc_index)
>> -				break;
>> -		}
>> -
>> -		ret = drc.drc_index_start + (thread_index * drc.sequential_inc);
>> +		rc = drc_info_parser(dn, &cpu_to_drc_index_cb,
>> +					"CPU", &cdata);
>> +		if (rc < 0)
>> +			goto err_of_node_put;
>> +		ret = cdata.ret;
>>  	} else {
>>  		const __be32 *indexes;
>>
>> @@ -100,11 +104,33 @@ static u32 cpu_to_drc_index(int cpu)
>>  	return ret;
>>  }
>>
>> +struct drc_index_to_cpu_struct {
>> +	u32	drc_index;
>> +	u32	thread_index;
>> +	u32	cpu;
>> +};
>> +
>> +static int drc_index_to_cpu_cb(struct of_drc_info *drc,
>> +				void *idata, void *not_used, int *ret_code)
>> +{
>> +	struct drc_index_to_cpu_struct *cdata = idata;
>> +
>> +	if (cdata->drc_index > drc->last_drc_index) {
>> +		cdata->cpu += drc->num_sequential_elems;
>> +	} else {
>> +		cdata->cpu += ((cdata->drc_index - drc->drc_index_start) /
>> +				drc->sequential_inc);
>> +		cdata->thread_index = cpu_first_thread_of_core(cdata->cpu);
> 
> Should this return 1 here to avoid continuing to walk the drc_info entries?

Yes.

> 
> -Nathan

Michael

> 
>> +	}
>> +	(*ret_code) = 0;
>> +	return 0;
>> +}
>> +
>>  static int drc_index_to_cpu(u32 drc_index)
>>  {
>>  	struct device_node *dn = NULL;
>>  	const int *indexes;
>> -	int thread_index = 0, cpu = 0;
>> +	int thread_index = 0;
>>  	int rc = 1;
>>
>>  	dn = of_find_node_by_path("/cpus");
>> @@ -112,36 +138,13 @@ static int drc_index_to_cpu(u32 drc_index)
>>  		goto err;
>>
>>  	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
>> -		struct property *info = NULL;
>> -		struct of_drc_info drc;
>> -		int j;
>> -		u32 num_set_entries;
>> -		const __be32 *value;
>> -
>> -		info = of_find_property(dn, "ibm,drc-info", NULL);
>> -		if (info == NULL)
>> -			goto err_of_node_put;
>> -
>> -		value = info->value;
>> -		num_set_entries = of_read_number(value++, 1);
>> -
>> -		for (j = 0; j < num_set_entries; j++) {
>> +		struct drc_index_to_cpu_struct cdata = {
>> +			drc_index, 0, 0 };
>>
>> -			of_read_drc_info_cell(&info, &value, &drc);
>> -			if (strncmp(drc.drc_type, "CPU", 3))
>> -				goto err;
>> +		rc = drc_info_parser(dn, &drc_index_to_cpu_cb,
>> +					"CPU", &cdata);
>> +		thread_index = cdata.thread_index;
>>
>> -			if (drc_index > drc.last_drc_index) {
>> -				cpu += drc.num_sequential_elems;
>> -				continue;
>> -			}
>> -			cpu += ((drc_index - drc.drc_index_start) /
>> -				drc.sequential_inc);
>> -
>> -			thread_index = cpu_first_thread_of_core(cpu);
>> -			rc = 0;
>> -			break;
>> -		}
>>  	} else {
>>  		unsigned long int i;
>>
> 
> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

Re: [RFC v4 4/4] hotplug/drcinfo: Code cleanup for devices

[Michael Bringmann]

See below.

On 05/22/2018 04:39 PM, Nathan Fontenot wrote:
> On 05/22/2018 11:37 AM, Michael Bringmann wrote:
>> This patch extends the use of a common parse function for the
>> ibm,drc-info property that can be modified by a callback function
>> to the hotplug device processing.  Candidate code is replaced by
>> a call to the parser including a pointer to a local context-specific
>> functions, and local data.
>>
>> In addition, several more opportunities to compress and reuse
>> common code between the old and new property parsers were applied.
>>
>> Finally, a bug with the registration of slots was observed on some
>> systems, and the code was rewritten to prevent its reoccurrence.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar
>> e feature" -- end of patch series applied to powerpc next)
>> ---
>> Changes in V4:
>>   -- Update code to account for latest kernel checkins.
>>   -- Fix bug searching for virtual device slots.
>>   -- Rebased to 4.17-rc5 kernel
>>   -- Patch cleanup
>> ---
>>  drivers/pci/hotplug/rpaphp_core.c |  181 ++++++++++++++++++++++++++-----------
>>  1 file changed, 126 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
>> index 435c1a0..dc4ec68 100644
>> --- a/drivers/pci/hotplug/rpaphp_core.c
>> +++ b/drivers/pci/hotplug/rpaphp_core.c
>> @@ -222,47 +222,51 @@ static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
>>  	return -EINVAL;
>>  }
>>
>> -static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
>> -				char *drc_type, unsigned int my_index)
>> +struct check_drc_props_v2_struct {
>> +	char *drc_name;
>> +	char *drc_type;
>> +	unsigned int my_index;
>> +};
>> +
>> +static int check_drc_props_v2_cb(struct of_drc_info *drc, void *idata,
>> +				void *not_used, int *ret_code)
>>  {
>> -	struct property *info;
>> -	unsigned int entries;
>> -	struct of_drc_info drc;
>> -	const __be32 *value;
>> +	struct check_drc_props_v2_struct *cdata = idata;
>>  	char cell_drc_name[MAX_DRC_NAME_LEN];
>> -	int j, fndit;
>>
>> -	info = of_find_property(dn->parent, "ibm,drc-info", NULL);
>> -	if (info == NULL)
>> -		return -EINVAL;
>> +	(*ret_code) = -EINVAL;
>>
>> -	value = info->value;
>> -	entries = of_read_number(value++, 1);
>> -
>> -	for (j = 0; j < entries; j++) {
>> -		of_read_drc_info_cell(&info, &value, &drc);
>> -
>> -		/* Should now know end of current entry */
>> -
>> -		if (my_index > drc.last_drc_index)
>> -			continue;
>> +	if (cdata->my_index > drc->last_drc_index)
>> +		return 0;
>>
>> -		fndit = 1;
>> -		break;
>> +	/* Found drc_index.  Now match the rest. */
>> +	sprintf(cell_drc_name, "%s%d", drc->drc_name_prefix,
>> +		cdata->my_index - drc->drc_index_start +
>> +		drc->drc_name_suffix_start);
>> +
>> +	if (((cdata->drc_name == NULL) ||
>> +	     (cdata->drc_name && !strcmp(cdata->drc_name, cell_drc_name))) &&
>> +	    ((cdata->drc_type == NULL) ||
>> +	     (cdata->drc_type && !strcmp(cdata->drc_type, drc->drc_type)))) {
>> +		(*ret_code) = 0;
>> +		return 1;
>>  	}
>> -	/* Found it */
>>
>> -	if (fndit)
>> -		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
>> -			my_index);
>> +	return 0;
>> +}
>>
>> -	if (((drc_name == NULL) ||
>> -	     (drc_name && !strcmp(drc_name, cell_drc_name))) &&
>> -	    ((drc_type == NULL) ||
>> -	     (drc_type && !strcmp(drc_type, drc.drc_type))))
>> -		return 0;
>> +static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
>> +				char *drc_type, unsigned int my_index)
>> +{
>> +	struct device_node *root = dn;
>> +	struct check_drc_props_v2_struct cdata = {
>> +		drc_name, drc_type, be32_to_cpu(my_index) };
>>
>> -	return -EINVAL;
>> +	if (!drc_type || (drc_type && strcmp(drc_type, "SLOT")))
>> +		root = dn->parent;
>> +
>> +	return drc_info_parser(root, check_drc_props_v2_cb,
>> +				drc_type, &cdata);
>>  }
>>
>>  int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
>> @@ -285,7 +289,6 @@ int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
>>  }
>>  EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
>>
>> -
>>  static int is_php_type(char *drc_type)
>>  {
>>  	unsigned long value;
>> @@ -345,17 +348,41 @@ static int is_php_dn(struct device_node *dn, const int **indexes,
>>   *
>>   * To remove a slot, it suffices to call rpaphp_deregister_slot().
>>   */
>> -int rpaphp_add_slot(struct device_node *dn)
>> +
>> +static int rpaphp_add_slot_common(struct device_node *dn,
>> +			u32 drc_index, char *drc_name, char *drc_type,
>> +			u32 drc_power_domain)
>>  {
>>  	struct slot *slot;
>>  	int retval = 0;
>> -	int i;
>> +
>> +	slot = alloc_slot_struct(dn, drc_index, drc_name, drc_power_domain);
>> +	if (!slot)
>> +		return -ENOMEM;
>> +
>> +	slot->type = simple_strtoul(drc_type, NULL, 10);
>> +	if (retval)
>> +		return -EINVAL;
>> +
>> +	dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
>> +		drc_index, drc_name, drc_type);
>> +
>> +	retval = rpaphp_enable_slot(slot);
>> +	if (!retval)
>> +		retval = rpaphp_register_slot(slot);
>> +
>> +	if (retval)
>> +		dealloc_slot_struct(slot);
>> +
>> +	return retval;
>> +}
>> +
>> +static int rpaphp_add_slot_v1(struct device_node *dn)
>> +{
>> +	int i, retval = 0;
>>  	const int *indexes, *names, *types, *power_domains;
>>  	char *name, *type;
>>
>> -	if (!dn->name || strcmp(dn->name, "pci"))
>> -		return 0;
>> -
>>  	/* If this is not a hotplug slot, return without doing anything. */
>>  	if (!is_php_dn(dn, &indexes, &names, &types, &power_domains))
>>  		return 0;
>> @@ -366,25 +393,12 @@ int rpaphp_add_slot(struct device_node *dn)
>>  	name = (char *) &names[1];
>>  	type = (char *) &types[1];
>>  	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
>> -		int index;
>> -
>> -		index = be32_to_cpu(indexes[i + 1]);
>> -		slot = alloc_slot_struct(dn, index, name,
>> -					 be32_to_cpu(power_domains[i + 1]));
>> -		if (!slot)
>> -			return -ENOMEM;
>> -
>> -		slot->type = simple_strtoul(type, NULL, 10);
>>
>> -		dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
>> -				index, name, type);
>> -
>> -		retval = rpaphp_enable_slot(slot);
>> +		retval = rpaphp_add_slot_common(dn,
>> +				be32_to_cpu(indexes[i + 1]), name, type,
>> +				be32_to_cpu(power_domains[i + 1]));
>>  		if (!retval)
>> -			retval = rpaphp_register_slot(slot);
>> -
>> -		if (retval)
>> -			dealloc_slot_struct(slot);
>> +			return retval;
>>
>>  		name += strlen(name) + 1;
>>  		type += strlen(type) + 1;
>> @@ -394,6 +408,63 @@ int rpaphp_add_slot(struct device_node *dn)
>>  	/* XXX FIXME: reports a failure only if last entry in loop failed */
>>  	return retval;
>>  }
>> +
>> +struct rpaphp_add_slot_v2_struct {
>> +	struct device_node *dn;
>> +};
>> +
>> +static int rpaphp_add_slot_v2_cb(struct of_drc_info *drc, void *idata,
>> +				void *not_used, int *ret_code)
>> +{
>> +	struct rpaphp_add_slot_v2_struct *cdata = idata;
>> +	u32 drc_index;
>> +	char drc_name[MAX_DRC_NAME_LEN];
>> +	int i, retval;
>> +
>> +	(*ret_code) = -EINVAL;
>> +
>> +	if (!is_php_type((char *) drc->drc_type)) {
>> +		(*ret_code) = 0;
>> +		return 1;
>> +	}
>> +
>> +	for (i = 0, drc_index = drc->drc_index_start;
>> +		i < drc->num_sequential_elems; i++, drc_index++) {
>> +
>> +		sprintf(drc_name, "%s%d", drc->drc_name_prefix,
>> +			drc_index - drc->drc_index_start +
>> +			drc->drc_name_suffix_start);
>> +
>> +		retval = rpaphp_add_slot_common(cdata->dn,
>> +				drc_index, drc_name, drc->drc_type,
>> +				drc->drc_power_domain);
>> +		if (!retval) {
>> +			(*ret_code) = retval;
>> +			return 1;
>> +		}
>> +	}
> 
> I may be reading this loop wrong, but it appears that it will add all drc indexes
> for all drc_info entries to the specified device node, minus drc-types that don't match.

It is supposed to exit the function after the first successful call
to rpaphp_add_slot_common().  That function is supposed to try to
allocate/enable a slot, and return 0 when it succeeds.

Will reconfirm this week.

> 
> -Nathan

Michael

> 
>> +
>> +	(*ret_code) = retval;
>> +	return 0;
>> +}
>> +
>> +static int rpaphp_add_slot_v2(struct device_node *dn)
>> +{
>> +	struct rpaphp_add_slot_v2_struct cdata = { dn };
>> +
>> +	return drc_info_parser(dn, rpaphp_add_slot_v2_cb, NULL, &cdata);
>> +}
>> +
>> +int rpaphp_add_slot(struct device_node *dn)
>> +{
>> +	if (!dn->name || strcmp(dn->name, "pci"))
>> +		return 0;
>> +
>> +	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
>> +		return rpaphp_add_slot_v2(dn);
>> +	else
>> +		return rpaphp_add_slot_v1(dn);
>> +}
>>  EXPORT_SYMBOL_GPL(rpaphp_add_slot);
>>
>>  static void __exit cleanup_slots(void)
>>
> 
> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com