[RFC 0/6] powerpc/pseries: Refactor code to centralize drcinfo parsing

[RFC 0/6] powerpc/pseries: Refactor code to centralize drcinfo parsing

[Michael Bringmann]

The implementation of the pseries-specific drc info information feature
is currently implemented within and outside of the powerpc pseries
code.  This patch set moves the parsing code for the pseries-specific
device-tree properties ibm,drc-indexes, ibm,drc-names, ibm,drc-types,
ibm,drc-power-domains, and the compressed ibm,drc-info into the
platform-specific code for the Pseries features.

Signed-off-by: Michael W. Bringmann <mwb@linux.vnet.ibm.com>

Michael Bringmann (6):
  powerpc:/drc Define interface to acquire arch-specific drc info
  pseries/drcinfo: Fix bug parsing ibm,drc-info
  pseries/drcinfo: Pseries impl of arch_find_drc_info
  powerpc/pseries: Use common drcinfo parsing
  powerpc/pci/hotplug: Use common drcinfo parsing
  powerpc: Enable support for ibm,drc-info devtree property

[RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info

[Michael Bringmann]

Define interface to acquire arch-specific drc info to match against
hotpluggable devices.  The current implementation exposes several
pseries-specific dynamic memory properties in generic kernel code.
This patch set provides an interface to pull that code out of the
generic kernel.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
---
 include/linux/topology.h |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/topology.h b/include/linux/topology.h
index cb0775e..df97f5f 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -44,6 +44,15 @@
 
 int arch_update_cpu_topology(void);
 
+int arch_find_drc_match(struct device_node *dn,
+			bool (*usercb)(struct device_node *dn,
+				u32 drc_index, char *drc_name,
+				char *drc_type, u32 drc_power_domain,
+				void *data),
+			char *opt_drc_type, char *opt_drc_name,
+			bool match_drc_index, bool ck_php_type,
+			void *data);
+
 /* Conform to ACPI 2.0 SLIT distance definitions */
 #define LOCAL_DISTANCE		10
 #define REMOTE_DISTANCE		20

[RFC 2/6] pseries/drcinfo: Fix bug parsing ibm,drc-info

[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)
---
 arch/powerpc/platforms/pseries/of_helpers.c |   24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
index 6df192f..0185e50 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
+
 #include <linux/string.h>
 #include <linux/err.h>
 #include <linux/slab.h>
@@ -6,6 +7,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)
@@ -65,29 +69,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;

[RFC 3/6] pseries/drcinfo: Pseries impl of arch_find_drc_info

[Michael Bringmann]

This patch provides a common interface to parse ibm,drc-indexes,
ibm,drc-names, ibm,drc-types, ibm,drc-power-domains, or ibm,drc-info.
The generic interface arch_find_drc_match is provided which accepts
callback functions that may be applied to examine the data for each
entry.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/prom.h             |    3 
 arch/powerpc/platforms/pseries/of_helpers.c |  299 +++++++++++++++++++++++++++
 include/linux/topology.h                    |    2 
 3 files changed, 298 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
index b04c5ce..910d1dc 100644
--- a/arch/powerpc/include/asm/prom.h
+++ b/arch/powerpc/include/asm/prom.h
@@ -91,9 +91,6 @@ struct of_drc_info {
 	u32 last_drc_index;
 };
 
-extern int of_read_drc_info_cell(struct property **prop,
-			const __be32 **curval, struct of_drc_info *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 0185e50..11c90cd 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#define pr_fmt(fmt) "drc: " fmt
+
 #include <linux/string.h>
 #include <linux/err.h>
 #include <linux/slab.h>
@@ -11,6 +13,12 @@
 
 #define	MAX_DRC_NAME_LEN 64
 
+static int drc_debug;
+#define dbg(args...) if (drc_debug) { printk(KERN_DEBUG args); }
+#define err(arg...) printk(KERN_ERR args);
+#define info(arg...) printk(KERN_INFO args);
+#define warn(arg...) printk(KERN_WARNING args);
+
 /**
  * pseries_of_derive_parent - basically like dirname(1)
  * @path:  the full_name of a node to be added to the tree
@@ -46,7 +54,8 @@ struct device_node *pseries_of_derive_parent(const char *path)
 
 /* Helper Routines to convert between drc_index to cpu numbers */
 
-int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
+static int of_read_drc_info_cell(struct property **prop,
+			const __be32 **curval,
 			struct of_drc_info *data)
 {
 	const char *p;
@@ -90,4 +99,290 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
 
 	return 0;
 }
-EXPORT_SYMBOL(of_read_drc_info_cell);
+
+static int walk_drc_info(struct device_node *dn,
+		bool (*usercb)(struct of_drc_info *drc,
+				void *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, ret_code = -EINVAL;
+	bool done = false;
+
+	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, &ret_code);
+	}
+
+	return ret_code;
+}
+
+static int get_children_props(struct device_node *dn, const int **drc_indexes,
+		const int **drc_names, const int **drc_types,
+		const int **drc_power_domains)
+{
+	const int *indexes, *names, *types, *domains;
+
+	indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
+	names = of_get_property(dn, "ibm,drc-names", NULL);
+	types = of_get_property(dn, "ibm,drc-types", NULL);
+	domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
+
+	if (!indexes || !names || !types || !domains) {
+		/* Slot does not have dynamically-removable children */
+		return -EINVAL;
+	}
+	if (drc_indexes)
+		*drc_indexes = indexes;
+	if (drc_names)
+		/* &drc_names[1] contains NULL terminated slot names */
+		*drc_names = names;
+	if (drc_types)
+		/* &drc_types[1] contains NULL terminated slot types */
+		*drc_types = types;
+	if (drc_power_domains)
+		*drc_power_domains = domains;
+
+	return 0;
+}
+
+static int is_php_type(char *drc_type)
+{
+	unsigned long value;
+	char *endptr;
+
+	/* PCI Hotplug nodes have an integer for drc_type */
+	value = simple_strtoul(drc_type, &endptr, 10);
+	if (endptr == drc_type)
+		return 0;
+
+	return 1;
+}
+
+/**
+ * is_php_dn() - return 1 if this is a hotpluggable pci slot, else 0
+ * @dn: target &device_node
+ * @indexes: passed to get_children_props()
+ * @names: passed to get_children_props()
+ * @types: returned from get_children_props()
+ * @power_domains:
+ *
+ * This routine will return true only if the device node is
+ * a hotpluggable slot. This routine will return false
+ * for built-in pci slots (even when the built-in slots are
+ * dlparable.)
+ */
+static int is_php_dn(struct device_node *dn, const int **indexes,
+		const int **names, const int **types,
+		const int **power_domains)
+{
+	const int *drc_types;
+	int rc;
+
+	rc = get_children_props(dn, indexes, names, &drc_types,
+				power_domains);
+	if (rc < 0)
+		return 0;
+
+	if (!is_php_type((char *) &drc_types[1]))
+		return 0;
+
+	*types = drc_types;
+	return 1;
+}
+
+struct find_drc_match_cb_struct {
+	struct device_node *dn;
+	bool (*usercb)(struct device_node *dn,
+			u32 drc_index, char *drc_name,
+			char *drc_type, u32 drc_power_domain,
+			void *data);
+	char *drc_type;
+	char *drc_name;
+	u32 drc_index;
+	bool match_drc_index;
+	bool add_slot;
+	void *data;
+};
+
+static int find_drc_match_v1(struct device_node *dn, void *data)
+{
+	struct find_drc_match_cb_struct *cdata = data;
+	int i, retval = 0;
+	const int *indexes, *names, *types, *domains;
+	char *name, *type;
+	struct device_node *root = dn;
+
+	if (cdata->match_drc_index)
+		root = dn->parent;
+
+	if (cdata->add_slot) {
+		/* If this is not a hotplug slot, return without doing
+		 * anything.
+		 */
+		if (!is_php_dn(root, &indexes, &names, &types, &domains))
+			return 0;
+	} else {
+		if (get_children_props(root, &indexes, &names, &types,
+			&domains) < 0)
+			return 0;
+	}
+
+	dbg("Entry %s: dn=%pOF\n", __func__, dn);
+
+	name = (char *) &names[1];
+	type = (char *) &types[1];
+	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
+
+		if (cdata->match_drc_index &&
+			((unsigned int) indexes[i + 1] != cdata->drc_index)) {
+			name += strlen(name) + 1;
+			type += strlen(type) + 1;
+			continue;
+		}
+
+		if (((cdata->drc_name == NULL) ||
+		     (cdata->drc_name && !strcmp(cdata->drc_name, name))) &&
+		    ((cdata->drc_type == NULL) ||
+		     (cdata->drc_type && !strcmp(cdata->drc_type, type)))) {
+
+			if (cdata->usercb) {
+				retval = cdata->usercb(dn,
+					be32_to_cpu(indexes[i + 1]),
+					name, type,
+					be32_to_cpu(domains[i + 1]),
+					cdata->data);
+				if (!retval)
+					return retval;
+			} else {
+				return 0;
+			}
+		}
+
+		name += strlen(name) + 1;
+		type += strlen(type) + 1;
+	}
+
+	dbg("%s - Exit: rc[%d]\n", __func__, retval);
+
+	/* XXX FIXME: reports a failure only if last entry in loop failed */
+	return retval;
+}
+
+static bool find_drc_match_v2_cb(struct of_drc_info *drc, void *data,
+				int *ret_code)
+{
+	struct find_drc_match_cb_struct *cdata = data;
+	u32 drc_index;
+	char drc_name[MAX_DRC_NAME_LEN];
+	int i, retval;
+
+	(*ret_code) = -EINVAL;
+
+	/* This set not a PHP type? */
+	if (cdata->add_slot) {
+		if (!is_php_type((char *) drc->drc_type)) {
+			return false;
+		}
+	}
+
+	/* Anything to use from this set? */
+	if (cdata->match_drc_index && (cdata->drc_index > drc->last_drc_index))
+		return false;
+	if ((cdata->drc_type && strcmp(cdata->drc_type, drc->drc_type))
+		return false;
+
+	/* Check the drc-index entries of this set */
+	for (i = 0, drc_index = drc->drc_index_start;
+		i < drc->num_sequential_elems; i++, drc_index++) {
+
+		if (cdata->match_drc_index && (cdata->drc_index != drc_index))
+			continue;
+
+		sprintf(drc_name, "%s%d", drc->drc_name_prefix,
+			drc_index - drc->drc_index_start +
+			drc->drc_name_suffix_start);
+
+		if ((cdata->drc_name == NULL) ||
+		    (cdata->drc_name && !strcmp(cdata->drc_name, drc_name))) {
+
+			if (cdata->usercb) {
+				retval = cdata->usercb(cdata->dn,
+						drc_index, drc_name,
+						drc->drc_type,
+						drc->drc_power_domain,
+						cdata->data);
+				if (!retval) {
+					(*ret_code) = retval;
+					return true;
+				}
+			} else {
+				(*ret_code) = 0;
+				return true;
+			}
+		}
+	}
+
+	(*ret_code) = retval;
+	return false;
+}
+
+static int find_drc_match_v2(struct device_node *dn, void *data)
+{
+	struct find_drc_match_cb_struct *cdata = data;
+	struct device_node *root = cdata->dn;
+
+	if (!cdata->add_slot) {
+		if (!cdata->drc_type ||
+			(cdata->drc_type && strcmp(cdata->drc_type, "SLOT")))
+			root = dn->parent;
+	}
+
+	return walk_drc_info(root, find_drc_match_v2_cb, NULL, data);
+}
+
+int arch_find_drc_match(struct device_node *dn,
+			bool (*usercb)(struct device_node *dn,
+				u32 drc_index, char *drc_name,
+				char *drc_type, u32 drc_power_domain,
+				void *data),
+			char *opt_drc_type, char *opt_drc_name,
+			bool match_drc_index, bool add_slot,
+			void *data)
+{
+	struct find_drc_match_cb_struct cdata = { dn, usercb,
+			opt_drc_type, opt_drc_name, 0,
+			match_drc_index, add_slot, data };
+
+	if (match_drc_index) {
+		const int *my_index =
+			of_get_property(dn, "ibm,my-drc-index", NULL);
+		if (!my_index) {
+			/* Node isn't DLPAR/hotplug capable */
+			return -EINVAL;
+		}
+		cdata.drc_index = *my_index;
+	}
+
+	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
+		return find_drc_match_v2(dn, &cdata);
+	else
+		return find_drc_match_v1(dn, &cdata);
+}
+EXPORT_SYMBOL(arch_find_drc_match);
diff --git a/include/linux/topology.h b/include/linux/topology.h
index df97f5f..c3dfa53 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -50,7 +50,7 @@ int arch_find_drc_match(struct device_node *dn,
 				char *drc_type, u32 drc_power_domain,
 				void *data),
 			char *opt_drc_type, char *opt_drc_name,
-			bool match_drc_index, bool ck_php_type,
+			bool match_drc_index, bool add_slot,
 			void *data);
 
 /* Conform to ACPI 2.0 SLIT distance definitions */

[RFC 4/6] powerpc/pseries: Use common drcinfo parsing

[Michael Bringmann]

The implementation of the pseries-specific drc info properties
is currently implemented in pseries-specific and non-pseries-specific
files.  This patch set uses a new implementation of the device-tree
parsing code for the properties.

This patch refactors parsing of the drc-info properties out of
pseries_energy.c and hotplug-cpu.c to use the common parser.
Changes include creating appropriate callback functions and
passing callback-specific data blocks into arch_find_drc_match.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c    |   83 +++++++-----
 arch/powerpc/platforms/pseries/pseries_energy.c |  157 ++++++++---------------
 2 files changed, 107 insertions(+), 133 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 2f8e621..ee3028c 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -411,23 +411,29 @@ 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)
+struct cpu_drc_index_struct {
+	u32 drc_index;
+};
+
+bool cpu_drc_index_cb(struct device_node *dn,
+			u32 drc_index, char *drc_name,
+			char *drc_type, u32 drc_power_domain,
+			void *data)
 {
-	bool found = false;
-	int rc, index;
+	struct cpu_drc_index_struct *cdata = data;
 
-	index = 0;
-	while (!found) {
-		u32 drc;
+	if (drc_index == cdata->drc_index)
+		return true;
+	return false;
+}
 
-		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
-						index++, &drc);
-		if (rc)
-			break;
+static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
+{
+	struct cpu_drc_index_struct cdata = { drc_index };
+	bool found = false;
 
-		if (drc == drc_index)
-			found = true;
-	}
+	found = arch_find_drc_match(parent, cpu_drc_index_cb,
+			"CPU", NULL, false, false, &cdata);
 
 	return found;
 }
@@ -721,11 +727,34 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
 	return rc;
 }
 
+struct cpus_to_add_cb_struct {
+	struct device_node *parent;
+	u32 *cpu_drcs;
+	u32 cpus_to_add;
+	u32 cpus_found;
+};
+
+static bool cpus_to_add_cb(struct device_node *dn,
+			u32 drc_index, char *drc_name,
+			char *drc_type, u32 drc_power_domain,
+			void *data)
+{
+	struct cpus_to_add_cb_struct *cdata = data;
+
+	if (cdata->cpus_found < cdata->cpus_to_add) {
+		if (!dlpar_cpu_exists(cdata->parent, drc_index))
+			cdata->cpu_drcs[cdata->cpus_found++] = drc_index;
+	}
+
+	return !(cdata->cpus_found < cdata->cpus_to_add);
+}
+
 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_cb_struct cdata = {
+		NULL, cpu_drcs, cpus_to_add, 0 };
+	int cpus_found;
 
 	parent = of_find_node_by_path("/cpus");
 	if (!parent) {
@@ -734,25 +763,13 @@ 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;
+	arch_find_drc_match(parent, cpus_to_add_cb, "CPU",
+			NULL, false, false, &cdata);
+	cpus_found = cdata.cpus_found;
 
 	of_node_put(parent);
 	return cpus_found;
diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c
index 6ed2212..f7b9d86 100644
--- a/arch/powerpc/platforms/pseries/pseries_energy.c
+++ b/arch/powerpc/platforms/pseries/pseries_energy.c
@@ -23,6 +23,7 @@
 #include <asm/hvcall.h>
 #include <asm/firmware.h>
 #include <asm/prom.h>
+#include <asm/topology.h>
 
 
 #define MODULE_VERS "1.0"
@@ -36,60 +37,43 @@
 
 /* Helper Routines to convert between drc_index to cpu numbers */
 
+struct cpu_to_drc_index_struct {
+	u32	thread_index;
+	u32	drc_index;
+	int	counter;
+};
+
+static bool cpu_to_drc_index_cb(struct device_node *dn,
+				u32 drc_index, char *drc_name,
+				char *drc_type, u32 drc_power_domain,
+				void *data)
+{
+	struct cpu_to_drc_index_struct *cdata = data;
+
+	if (cdata->thread_index == cdata->counter++) {
+		cdata->drc_index = drc_index;
+		return true;
+	}
+	return false;
+}
+
 static u32 cpu_to_drc_index(int cpu)
 {
 	struct device_node *dn = NULL;
-	int thread_index;
+	struct cpu_to_drc_index_struct cdata = { 0, 0, 0 };
 	int rc = 1;
-	u32 ret = 0;
 
 	dn = of_find_node_by_path("/cpus");
 	if (dn == NULL)
 		goto err;
 
 	/* Convert logical cpu number to core number */
-	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;
-
-		value = of_prop_next_u32(info, NULL, &num_set_entries);
-		if (!value)
-			goto err_of_node_put;
-
-		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);
-	} else {
-		const __be32 *indexes;
-
-		indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
-		if (indexes == NULL)
-			goto err_of_node_put;
-
-		/*
-		 * The first element indexes[0] is the number of drc_indexes
-		 * returned in the list.  Hence thread_index+1 will get the
-		 * drc_index corresponding to core number thread_index.
-		 */
-		ret = indexes[thread_index + 1];
-	}
+	cdata.thread_index = cpu_core_index_of_thread(cpu);
+
+	rc = arch_find_drc_match(dn, cpu_to_drc_index_cb,
+			"CPU", NULL, false, false, &cdata);
+	if (rc < 0)
+		goto err_of_node_put;
 
 	rc = 0;
 
@@ -98,78 +82,51 @@ static u32 cpu_to_drc_index(int cpu)
 err:
 	if (rc)
 		printk(KERN_WARNING "cpu_to_drc_index(%d) failed", cpu);
-	return ret;
+	return cdata.drc_index;
+}
+
+struct drc_index_to_cpu_struct {
+	u32	drc_index;
+	u32	thread_index;
+	int	counter;
+};
+
+static bool drc_index_to_cpu_cb(struct device_node *dn,
+				u32 drc_index, char *drc_name,
+				char *drc_type, u32 drc_power_domain,
+				void *data)
+{
+	struct drc_index_to_cpu_struct *cdata = data;
+
+	if (cdata->drc_index == drc_index) {
+		cdata->thread_index = cpu_first_thread_of_core(cdata->counter);
+		return true;
+	}
+	cdata->counter++;
+
+	return false;
 }
 
 static int drc_index_to_cpu(u32 drc_index)
 {
 	struct device_node *dn = NULL;
-	const int *indexes;
-	int thread_index = 0, cpu = 0;
+	struct drc_index_to_cpu_struct cdata = {
+			drc_index, 0, 0 };
 	int rc = 1;
 
 	dn = of_find_node_by_path("/cpus");
 	if (dn == NULL)
 		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 = of_prop_next_u32(info, NULL, &num_set_entries);
-		if (!value)
-			goto err_of_node_put;
-
-		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;
+	rc = arch_find_drc_match(dn, drc_index_to_cpu_cb,
+		   "CPU", NULL, false, false, &cdata);
 
-			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;
-
-		indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
-		if (indexes == NULL)
-			goto err_of_node_put;
-		/*
-		 * First element in the array is the number of drc_indexes
-		 * returned.  Search through the list to find the matching
-		 * drc_index and get the core number
-		 */
-		for (i = 0; i < indexes[0]; i++) {
-			if (indexes[i + 1] == drc_index)
-				break;
-		}
-		/* Convert core number to logical cpu number */
-		thread_index = cpu_first_thread_of_core(i);
-		rc = 0;
-	}
-
-err_of_node_put:
 	of_node_put(dn);
+
 err:
 	if (rc)
 		printk(KERN_WARNING "drc_index_to_cpu(%d) failed", drc_index);
-	return thread_index;
+	return cdata.thread_index;
 }
 
 /*

[RFC 5/6] powerpc/pci/hotplug: Use common drcinfo parsing

[Michael Bringmann]

The implementation of the pseries-specific drc info properties
is currently implemented in pseries-specific and non-pseries-specific
files.  This patch set uses a new implementation of the device-tree
parsing code for the properties.

This patch refactors parsing of the pseries-specific drc-info properties
out of rpaphp_core.c to use the common parser.  In the case where an
architecture does not use these properties, an __weak copy of the
function is provided with dummy return values.  Changes include creating
appropriate callback functions and passing callback-specific data
blocks into arch_find_drc_match.  All functions that were used just
to support the previous parsing implementation have been moved.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
---
 drivers/pci/hotplug/rpaphp_core.c |  232 ++++---------------------------------
 1 file changed, 28 insertions(+), 204 deletions(-)

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index bcd5d35..9ad7384 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -154,182 +154,18 @@ static enum pci_bus_speed get_max_bus_speed(struct slot *slot)
 	return speed;
 }
 
-static int get_children_props(struct device_node *dn, const int **drc_indexes,
-		const int **drc_names, const int **drc_types,
-		const int **drc_power_domains)
-{
-	const int *indexes, *names, *types, *domains;
-
-	indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
-	names = of_get_property(dn, "ibm,drc-names", NULL);
-	types = of_get_property(dn, "ibm,drc-types", NULL);
-	domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
-
-	if (!indexes || !names || !types || !domains) {
-		/* Slot does not have dynamically-removable children */
-		return -EINVAL;
-	}
-	if (drc_indexes)
-		*drc_indexes = indexes;
-	if (drc_names)
-		/* &drc_names[1] contains NULL terminated slot names */
-		*drc_names = names;
-	if (drc_types)
-		/* &drc_types[1] contains NULL terminated slot types */
-		*drc_types = types;
-	if (drc_power_domains)
-		*drc_power_domains = domains;
-
-	return 0;
-}
-
-
 /* Verify the existence of 'drc_name' and/or 'drc_type' within the
- * current node.  First obtain it's my-drc-index property.  Next,
- * obtain the DRC info from it's parent.  Use the my-drc-index for
- * correlation, and obtain/validate the requested properties.
+ * current node.
  */
 
-static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
-				char *drc_type, unsigned int my_index)
-{
-	char *name_tmp, *type_tmp;
-	const int *indexes, *names;
-	const int *types, *domains;
-	int i, rc;
-
-	rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
-	if (rc < 0) {
-		return -EINVAL;
-	}
-
-	name_tmp = (char *) &names[1];
-	type_tmp = (char *) &types[1];
-
-	/* Iterate through parent properties, looking for my-drc-index */
-	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
-		if ((unsigned int) indexes[i + 1] == my_index)
-			break;
-
-		name_tmp += (strlen(name_tmp) + 1);
-		type_tmp += (strlen(type_tmp) + 1);
-	}
-
-	if (((drc_name == NULL) || (drc_name && !strcmp(drc_name, name_tmp))) &&
-	    ((drc_type == NULL) || (drc_type && !strcmp(drc_type, type_tmp))))
-		return 0;
-
-	return -EINVAL;
-}
-
-static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
-				char *drc_type, unsigned int my_index)
-{
-	struct property *info;
-	unsigned int entries;
-	struct of_drc_info drc;
-	const __be32 *value;
-	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;
-
-	value = of_prop_next_u32(info, NULL, &entries);
-	if (!value)
-		return -EINVAL;
-
-	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;
-
-		fndit = 1;
-		break;
-	}
-	/* Found it */
-
-	if (fndit)
-		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
-			my_index);
-
-	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;
-
-	return -EINVAL;
-}
-
 int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
 			char *drc_type)
 {
-	const unsigned int *my_index;
-
-	my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
-	if (!my_index) {
-		/* Node isn't DLPAR/hotplug capable */
-		return -EINVAL;
-	}
-
-	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
-		return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
-						*my_index);
-	else
-		return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
-						*my_index);
+	return arch_find_drc_match(dn, NULL, drc_type, drc_name,
+				true, false, NULL);
 }
 EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
 
-
-static int is_php_type(char *drc_type)
-{
-	unsigned long value;
-	char *endptr;
-
-	/* PCI Hotplug nodes have an integer for drc_type */
-	value = simple_strtoul(drc_type, &endptr, 10);
-	if (endptr == drc_type)
-		return 0;
-
-	return 1;
-}
-
-/**
- * is_php_dn() - return 1 if this is a hotpluggable pci slot, else 0
- * @dn: target &device_node
- * @indexes: passed to get_children_props()
- * @names: passed to get_children_props()
- * @types: returned from get_children_props()
- * @power_domains:
- *
- * This routine will return true only if the device node is
- * a hotpluggable slot. This routine will return false
- * for built-in pci slots (even when the built-in slots are
- * dlparable.)
- */
-static int is_php_dn(struct device_node *dn, const int **indexes,
-		const int **names, const int **types, const int **power_domains)
-{
-	const int *drc_types;
-	int rc;
-
-	rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
-	if (rc < 0)
-		return 0;
-
-	if (!is_php_type((char *) &drc_types[1]))
-		return 0;
-
-	*types = drc_types;
-	return 1;
-}
-
 /**
  * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
  * @dn: device node of slot
@@ -346,54 +182,42 @@ 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_cb(struct device_node *dn,
+			u32 drc_index, char *drc_name, char *drc_type,
+			u32 drc_power_domain, void *data)
 {
 	struct slot *slot;
 	int retval = 0;
-	int i;
-	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;
-
-	dbg("Entry %s: dn=%pOF\n", __func__, dn);
 
-	/* register PCI devices */
-	name = (char *) &names[1];
-	type = (char *) &types[1];
-	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
-		int index;
+	slot = alloc_slot_struct(dn, drc_index, drc_name, drc_power_domain);
+	if (!slot)
+		return -ENOMEM;
 
-		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(drc_type, NULL, 10);
+	if (retval)
+		return -EINVAL;
 
-		slot->type = simple_strtoul(type, NULL, 10);
+	dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
+		drc_index, drc_name, drc_type);
 
-		dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
-				index, name, type);
+	retval = rpaphp_enable_slot(slot);
+	if (!retval)
+		retval = rpaphp_register_slot(slot);
 
-		retval = rpaphp_enable_slot(slot);
-		if (!retval)
-			retval = rpaphp_register_slot(slot);
+	if (retval)
+		dealloc_slot_struct(slot);
 
-		if (retval)
-			dealloc_slot_struct(slot);
+	return retval;
+}
 
-		name += strlen(name) + 1;
-		type += strlen(type) + 1;
-	}
-	dbg("%s - Exit: rc[%d]\n", __func__, retval);
+int rpaphp_add_slot(struct device_node *dn)
+{
+	if (!dn->name || strcmp(dn->name, "pci"))
+		return 0;
 
-	/* XXX FIXME: reports a failure only if last entry in loop failed */
-	return retval;
+	return arch_find_drc_match(dn, rpaphp_add_slot_cb,
+			NULL, NULL, false, true, NULL);
 }
 EXPORT_SYMBOL_GPL(rpaphp_add_slot);
 

Re: [RFC 5/6] powerpc/pci/hotplug: Use common drcinfo parsing

[Bjorn Helgaas]

On Fri, Dec 14, 2018 at 02:51:31PM -0600, Michael Bringmann wrote:
> The implementation of the pseries-specific drc info properties
> is currently implemented in pseries-specific and non-pseries-specific
> files.  This patch set uses a new implementation of the device-tree
> parsing code for the properties.
> 
> This patch refactors parsing of the pseries-specific drc-info properties
> out of rpaphp_core.c to use the common parser.  In the case where an
> architecture does not use these properties, an __weak copy of the
> function is provided with dummy return values.  Changes include creating
> appropriate callback functions and passing callback-specific data
> blocks into arch_find_drc_match.  All functions that were used just
> to support the previous parsing implementation have been moved.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>

This is fine with me.  Any rpaphp_core.c maintainers want to comment?
Tyrel?

$ ./scripts/get_maintainer.pl -f drivers/pci/hotplug/rpaphp_core.c
Tyrel Datwyler <tyreld@linux.vnet.ibm.com> (supporter:IBM Power PCI Hotplug Driver for RPA-compliant...)
Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
Michael Ellerman <mpe@ellerman.id.au> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))

> ---
>  drivers/pci/hotplug/rpaphp_core.c |  232 ++++---------------------------------
>  1 file changed, 28 insertions(+), 204 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> index bcd5d35..9ad7384 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -154,182 +154,18 @@ static enum pci_bus_speed get_max_bus_speed(struct slot *slot)
>  	return speed;
>  }
>  
> -static int get_children_props(struct device_node *dn, const int **drc_indexes,
> -		const int **drc_names, const int **drc_types,
> -		const int **drc_power_domains)
> -{
> -	const int *indexes, *names, *types, *domains;
> -
> -	indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> -	names = of_get_property(dn, "ibm,drc-names", NULL);
> -	types = of_get_property(dn, "ibm,drc-types", NULL);
> -	domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
> -
> -	if (!indexes || !names || !types || !domains) {
> -		/* Slot does not have dynamically-removable children */
> -		return -EINVAL;
> -	}
> -	if (drc_indexes)
> -		*drc_indexes = indexes;
> -	if (drc_names)
> -		/* &drc_names[1] contains NULL terminated slot names */
> -		*drc_names = names;
> -	if (drc_types)
> -		/* &drc_types[1] contains NULL terminated slot types */
> -		*drc_types = types;
> -	if (drc_power_domains)
> -		*drc_power_domains = domains;
> -
> -	return 0;
> -}
> -
> -
>  /* Verify the existence of 'drc_name' and/or 'drc_type' within the
> - * current node.  First obtain it's my-drc-index property.  Next,
> - * obtain the DRC info from it's parent.  Use the my-drc-index for
> - * correlation, and obtain/validate the requested properties.
> + * current node.
>   */
>  
> -static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
> -				char *drc_type, unsigned int my_index)
> -{
> -	char *name_tmp, *type_tmp;
> -	const int *indexes, *names;
> -	const int *types, *domains;
> -	int i, rc;
> -
> -	rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
> -	if (rc < 0) {
> -		return -EINVAL;
> -	}
> -
> -	name_tmp = (char *) &names[1];
> -	type_tmp = (char *) &types[1];
> -
> -	/* Iterate through parent properties, looking for my-drc-index */
> -	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
> -		if ((unsigned int) indexes[i + 1] == my_index)
> -			break;
> -
> -		name_tmp += (strlen(name_tmp) + 1);
> -		type_tmp += (strlen(type_tmp) + 1);
> -	}
> -
> -	if (((drc_name == NULL) || (drc_name && !strcmp(drc_name, name_tmp))) &&
> -	    ((drc_type == NULL) || (drc_type && !strcmp(drc_type, type_tmp))))
> -		return 0;
> -
> -	return -EINVAL;
> -}
> -
> -static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
> -				char *drc_type, unsigned int my_index)
> -{
> -	struct property *info;
> -	unsigned int entries;
> -	struct of_drc_info drc;
> -	const __be32 *value;
> -	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;
> -
> -	value = of_prop_next_u32(info, NULL, &entries);
> -	if (!value)
> -		return -EINVAL;
> -
> -	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;
> -
> -		fndit = 1;
> -		break;
> -	}
> -	/* Found it */
> -
> -	if (fndit)
> -		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
> -			my_index);
> -
> -	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;
> -
> -	return -EINVAL;
> -}
> -
>  int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
>  			char *drc_type)
>  {
> -	const unsigned int *my_index;
> -
> -	my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
> -	if (!my_index) {
> -		/* Node isn't DLPAR/hotplug capable */
> -		return -EINVAL;
> -	}
> -
> -	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
> -		return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
> -						*my_index);
> -	else
> -		return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
> -						*my_index);
> +	return arch_find_drc_match(dn, NULL, drc_type, drc_name,
> +				true, false, NULL);
>  }
>  EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
>  
> -
> -static int is_php_type(char *drc_type)
> -{
> -	unsigned long value;
> -	char *endptr;
> -
> -	/* PCI Hotplug nodes have an integer for drc_type */
> -	value = simple_strtoul(drc_type, &endptr, 10);
> -	if (endptr == drc_type)
> -		return 0;
> -
> -	return 1;
> -}
> -
> -/**
> - * is_php_dn() - return 1 if this is a hotpluggable pci slot, else 0
> - * @dn: target &device_node
> - * @indexes: passed to get_children_props()
> - * @names: passed to get_children_props()
> - * @types: returned from get_children_props()
> - * @power_domains:
> - *
> - * This routine will return true only if the device node is
> - * a hotpluggable slot. This routine will return false
> - * for built-in pci slots (even when the built-in slots are
> - * dlparable.)
> - */
> -static int is_php_dn(struct device_node *dn, const int **indexes,
> -		const int **names, const int **types, const int **power_domains)
> -{
> -	const int *drc_types;
> -	int rc;
> -
> -	rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
> -	if (rc < 0)
> -		return 0;
> -
> -	if (!is_php_type((char *) &drc_types[1]))
> -		return 0;
> -
> -	*types = drc_types;
> -	return 1;
> -}
> -
>  /**
>   * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
>   * @dn: device node of slot
> @@ -346,54 +182,42 @@ 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_cb(struct device_node *dn,
> +			u32 drc_index, char *drc_name, char *drc_type,
> +			u32 drc_power_domain, void *data)
>  {
>  	struct slot *slot;
>  	int retval = 0;
> -	int i;
> -	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;
> -
> -	dbg("Entry %s: dn=%pOF\n", __func__, dn);
>  
> -	/* register PCI devices */
> -	name = (char *) &names[1];
> -	type = (char *) &types[1];
> -	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
> -		int index;
> +	slot = alloc_slot_struct(dn, drc_index, drc_name, drc_power_domain);
> +	if (!slot)
> +		return -ENOMEM;
>  
> -		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(drc_type, NULL, 10);
> +	if (retval)
> +		return -EINVAL;
>  
> -		slot->type = simple_strtoul(type, NULL, 10);
> +	dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
> +		drc_index, drc_name, drc_type);
>  
> -		dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
> -				index, name, type);
> +	retval = rpaphp_enable_slot(slot);
> +	if (!retval)
> +		retval = rpaphp_register_slot(slot);
>  
> -		retval = rpaphp_enable_slot(slot);
> -		if (!retval)
> -			retval = rpaphp_register_slot(slot);
> +	if (retval)
> +		dealloc_slot_struct(slot);
>  
> -		if (retval)
> -			dealloc_slot_struct(slot);
> +	return retval;
> +}
>  
> -		name += strlen(name) + 1;
> -		type += strlen(type) + 1;
> -	}
> -	dbg("%s - Exit: rc[%d]\n", __func__, retval);
> +int rpaphp_add_slot(struct device_node *dn)
> +{
> +	if (!dn->name || strcmp(dn->name, "pci"))
> +		return 0;
>  
> -	/* XXX FIXME: reports a failure only if last entry in loop failed */
> -	return retval;
> +	return arch_find_drc_match(dn, rpaphp_add_slot_cb,
> +			NULL, NULL, false, true, NULL);
>  }
>  EXPORT_SYMBOL_GPL(rpaphp_add_slot);
>  
> 

Re: [RFC 5/6] powerpc/pci/hotplug: Use common drcinfo parsing

[Bjorn Helgaas]

On Mon, Jan 14, 2019 at 06:28:57PM -0600, Bjorn Helgaas wrote:
> On Fri, Dec 14, 2018 at 02:51:31PM -0600, Michael Bringmann wrote:
> > The implementation of the pseries-specific drc info properties
> > is currently implemented in pseries-specific and non-pseries-specific
> > files.  This patch set uses a new implementation of the device-tree
> > parsing code for the properties.
> > 
> > This patch refactors parsing of the pseries-specific drc-info properties
> > out of rpaphp_core.c to use the common parser.  In the case where an
> > architecture does not use these properties, an __weak copy of the
> > function is provided with dummy return values.  Changes include creating
> > appropriate callback functions and passing callback-specific data
> > blocks into arch_find_drc_match.  All functions that were used just
> > to support the previous parsing implementation have been moved.
> > 
> > Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> 
> This is fine with me.  Any rpaphp_core.c maintainers want to comment?
> Tyrel?
> 
> $ ./scripts/get_maintainer.pl -f drivers/pci/hotplug/rpaphp_core.c
> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> (supporter:IBM Power PCI Hotplug Driver for RPA-compliant...)
> Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> Michael Ellerman <mpe@ellerman.id.au> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))

This looks like part of a larger series, but I only got this patch.  So I
assume you'll route this elsewhere along with the rest of the series.
Here's my ack if it's useful:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> > ---
> >  drivers/pci/hotplug/rpaphp_core.c |  232 ++++---------------------------------
> >  1 file changed, 28 insertions(+), 204 deletions(-)
> > 
> > diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> > index bcd5d35..9ad7384 100644
> > --- a/drivers/pci/hotplug/rpaphp_core.c
> > +++ b/drivers/pci/hotplug/rpaphp_core.c
> > @@ -154,182 +154,18 @@ static enum pci_bus_speed get_max_bus_speed(struct slot *slot)
> >  	return speed;
> >  }
> >  
> > -static int get_children_props(struct device_node *dn, const int **drc_indexes,
> > -		const int **drc_names, const int **drc_types,
> > -		const int **drc_power_domains)
> > -{
> > -	const int *indexes, *names, *types, *domains;
> > -
> > -	indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> > -	names = of_get_property(dn, "ibm,drc-names", NULL);
> > -	types = of_get_property(dn, "ibm,drc-types", NULL);
> > -	domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
> > -
> > -	if (!indexes || !names || !types || !domains) {
> > -		/* Slot does not have dynamically-removable children */
> > -		return -EINVAL;
> > -	}
> > -	if (drc_indexes)
> > -		*drc_indexes = indexes;
> > -	if (drc_names)
> > -		/* &drc_names[1] contains NULL terminated slot names */
> > -		*drc_names = names;
> > -	if (drc_types)
> > -		/* &drc_types[1] contains NULL terminated slot types */
> > -		*drc_types = types;
> > -	if (drc_power_domains)
> > -		*drc_power_domains = domains;
> > -
> > -	return 0;
> > -}
> > -
> > -
> >  /* Verify the existence of 'drc_name' and/or 'drc_type' within the
> > - * current node.  First obtain it's my-drc-index property.  Next,
> > - * obtain the DRC info from it's parent.  Use the my-drc-index for
> > - * correlation, and obtain/validate the requested properties.
> > + * current node.
> >   */
> >  
> > -static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
> > -				char *drc_type, unsigned int my_index)
> > -{
> > -	char *name_tmp, *type_tmp;
> > -	const int *indexes, *names;
> > -	const int *types, *domains;
> > -	int i, rc;
> > -
> > -	rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
> > -	if (rc < 0) {
> > -		return -EINVAL;
> > -	}
> > -
> > -	name_tmp = (char *) &names[1];
> > -	type_tmp = (char *) &types[1];
> > -
> > -	/* Iterate through parent properties, looking for my-drc-index */
> > -	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
> > -		if ((unsigned int) indexes[i + 1] == my_index)
> > -			break;
> > -
> > -		name_tmp += (strlen(name_tmp) + 1);
> > -		type_tmp += (strlen(type_tmp) + 1);
> > -	}
> > -
> > -	if (((drc_name == NULL) || (drc_name && !strcmp(drc_name, name_tmp))) &&
> > -	    ((drc_type == NULL) || (drc_type && !strcmp(drc_type, type_tmp))))
> > -		return 0;
> > -
> > -	return -EINVAL;
> > -}
> > -
> > -static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
> > -				char *drc_type, unsigned int my_index)
> > -{
> > -	struct property *info;
> > -	unsigned int entries;
> > -	struct of_drc_info drc;
> > -	const __be32 *value;
> > -	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;
> > -
> > -	value = of_prop_next_u32(info, NULL, &entries);
> > -	if (!value)
> > -		return -EINVAL;
> > -
> > -	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;
> > -
> > -		fndit = 1;
> > -		break;
> > -	}
> > -	/* Found it */
> > -
> > -	if (fndit)
> > -		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
> > -			my_index);
> > -
> > -	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;
> > -
> > -	return -EINVAL;
> > -}
> > -
> >  int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
> >  			char *drc_type)
> >  {
> > -	const unsigned int *my_index;
> > -
> > -	my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
> > -	if (!my_index) {
> > -		/* Node isn't DLPAR/hotplug capable */
> > -		return -EINVAL;
> > -	}
> > -
> > -	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
> > -		return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
> > -						*my_index);
> > -	else
> > -		return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
> > -						*my_index);
> > +	return arch_find_drc_match(dn, NULL, drc_type, drc_name,
> > +				true, false, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
> >  
> > -
> > -static int is_php_type(char *drc_type)
> > -{
> > -	unsigned long value;
> > -	char *endptr;
> > -
> > -	/* PCI Hotplug nodes have an integer for drc_type */
> > -	value = simple_strtoul(drc_type, &endptr, 10);
> > -	if (endptr == drc_type)
> > -		return 0;
> > -
> > -	return 1;
> > -}
> > -
> > -/**
> > - * is_php_dn() - return 1 if this is a hotpluggable pci slot, else 0
> > - * @dn: target &device_node
> > - * @indexes: passed to get_children_props()
> > - * @names: passed to get_children_props()
> > - * @types: returned from get_children_props()
> > - * @power_domains:
> > - *
> > - * This routine will return true only if the device node is
> > - * a hotpluggable slot. This routine will return false
> > - * for built-in pci slots (even when the built-in slots are
> > - * dlparable.)
> > - */
> > -static int is_php_dn(struct device_node *dn, const int **indexes,
> > -		const int **names, const int **types, const int **power_domains)
> > -{
> > -	const int *drc_types;
> > -	int rc;
> > -
> > -	rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
> > -	if (rc < 0)
> > -		return 0;
> > -
> > -	if (!is_php_type((char *) &drc_types[1]))
> > -		return 0;
> > -
> > -	*types = drc_types;
> > -	return 1;
> > -}
> > -
> >  /**
> >   * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
> >   * @dn: device node of slot
> > @@ -346,54 +182,42 @@ 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_cb(struct device_node *dn,
> > +			u32 drc_index, char *drc_name, char *drc_type,
> > +			u32 drc_power_domain, void *data)
> >  {
> >  	struct slot *slot;
> >  	int retval = 0;
> > -	int i;
> > -	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;
> > -
> > -	dbg("Entry %s: dn=%pOF\n", __func__, dn);
> >  
> > -	/* register PCI devices */
> > -	name = (char *) &names[1];
> > -	type = (char *) &types[1];
> > -	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
> > -		int index;
> > +	slot = alloc_slot_struct(dn, drc_index, drc_name, drc_power_domain);
> > +	if (!slot)
> > +		return -ENOMEM;
> >  
> > -		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(drc_type, NULL, 10);
> > +	if (retval)
> > +		return -EINVAL;
> >  
> > -		slot->type = simple_strtoul(type, NULL, 10);
> > +	dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
> > +		drc_index, drc_name, drc_type);
> >  
> > -		dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
> > -				index, name, type);
> > +	retval = rpaphp_enable_slot(slot);
> > +	if (!retval)
> > +		retval = rpaphp_register_slot(slot);
> >  
> > -		retval = rpaphp_enable_slot(slot);
> > -		if (!retval)
> > -			retval = rpaphp_register_slot(slot);
> > +	if (retval)
> > +		dealloc_slot_struct(slot);
> >  
> > -		if (retval)
> > -			dealloc_slot_struct(slot);
> > +	return retval;
> > +}
> >  
> > -		name += strlen(name) + 1;
> > -		type += strlen(type) + 1;
> > -	}
> > -	dbg("%s - Exit: rc[%d]\n", __func__, retval);
> > +int rpaphp_add_slot(struct device_node *dn)
> > +{
> > +	if (!dn->name || strcmp(dn->name, "pci"))
> > +		return 0;
> >  
> > -	/* XXX FIXME: reports a failure only if last entry in loop failed */
> > -	return retval;
> > +	return arch_find_drc_match(dn, rpaphp_add_slot_cb,
> > +			NULL, NULL, false, true, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(rpaphp_add_slot);
> >  
> > 

Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info

[Tyrel Datwyler]

On 12/14/2018 12:50 PM, Michael Bringmann wrote:
> Define interface to acquire arch-specific drc info to match against
> hotpluggable devices.  The current implementation exposes several
> pseries-specific dynamic memory properties in generic kernel code.
> This patch set provides an interface to pull that code out of the
> generic kernel.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
>  include/linux/topology.h |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index cb0775e..df97f5f 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -44,6 +44,15 @@

As far as I know pseries is the only platform that uses DR connectors, and I
highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
that this is really generic enough to belong in topology.h. If anything I would
suggest putting this in an include in arch/powerpc/include/ named something like
drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
that want/need to use this functionality.

-Tyrel

>  
>  int arch_update_cpu_topology(void);
>  
> +int arch_find_drc_match(struct device_node *dn,
> +			bool (*usercb)(struct device_node *dn,
> +				u32 drc_index, char *drc_name,
> +				char *drc_type, u32 drc_power_domain,
> +				void *data),
> +			char *opt_drc_type, char *opt_drc_name,
> +			bool match_drc_index, bool ck_php_type,
> +			void *data);
> +
>  /* Conform to ACPI 2.0 SLIT distance definitions */
>  #define LOCAL_DISTANCE		10
>  #define REMOTE_DISTANCE		20
> 

Re: [RFC 3/6] pseries/drcinfo: Pseries impl of arch_find_drc_info

[Tyrel Datwyler]

On 12/14/2018 12:51 PM, Michael Bringmann wrote:
> This patch provides a common interface to parse ibm,drc-indexes,
> ibm,drc-names, ibm,drc-types, ibm,drc-power-domains, or ibm,drc-info.
> The generic interface arch_find_drc_match is provided which accepts
> callback functions that may be applied to examine the data for each
> entry.
> 

The title of your patch is "pseries/drcinfo: Pseries impl of arch_find_drc_info"
but the name of the function you are ultimately implementing is
arch_find_drc_match if I'm not mistaken.

> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/prom.h             |    3 
>  arch/powerpc/platforms/pseries/of_helpers.c |  299 +++++++++++++++++++++++++++
>  include/linux/topology.h                    |    2 
>  3 files changed, 298 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index b04c5ce..910d1dc 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -91,9 +91,6 @@ struct of_drc_info {
>  	u32 last_drc_index;
>  };
>  
> -extern int of_read_drc_info_cell(struct property **prop,
> -			const __be32 **curval, struct of_drc_info *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 0185e50..11c90cd 100644
> --- a/arch/powerpc/platforms/pseries/of_helpers.c
> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
> @@ -1,5 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +#define pr_fmt(fmt) "drc: " fmt
> +
>  #include <linux/string.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> @@ -11,6 +13,12 @@
>  
>  #define	MAX_DRC_NAME_LEN 64
>  
> +static int drc_debug;
> +#define dbg(args...) if (drc_debug) { printk(KERN_DEBUG args); }
> +#define err(arg...) printk(KERN_ERR args);
> +#define info(arg...) printk(KERN_INFO args);
> +#define warn(arg...) printk(KERN_WARNING args);

Its pretty standard these days to use the pr_debug, pr_err, pr_info, pr_warn
variations over printk(LEVEL args).

> +
>  /**
>   * pseries_of_derive_parent - basically like dirname(1)
>   * @path:  the full_name of a node to be added to the tree
> @@ -46,7 +54,8 @@ struct device_node *pseries_of_derive_parent(const char *path)
>  
>  /* Helper Routines to convert between drc_index to cpu numbers */
>  
> -int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
> +static int of_read_drc_info_cell(struct property **prop,
> +			const __be32 **curval,
>  			struct of_drc_info *data)
>  {
>  	const char *p;
> @@ -90,4 +99,290 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL(of_read_drc_info_cell);
> +
> +static int walk_drc_info(struct device_node *dn,
> +		bool (*usercb)(struct of_drc_info *drc,
> +				void *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, ret_code = -EINVAL;
> +	bool done = false;
> +
> +	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, &ret_code);
> +	}
> +
> +	return ret_code;
> +}
> +
> +static int get_children_props(struct device_node *dn, const int **drc_indexes,
> +		const int **drc_names, const int **drc_types,
> +		const int **drc_power_domains)
> +{
> +	const int *indexes, *names, *types, *domains;
> +
> +	indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> +	names = of_get_property(dn, "ibm,drc-names", NULL);
> +	types = of_get_property(dn, "ibm,drc-types", NULL);
> +	domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
> +
> +	if (!indexes || !names || !types || !domains) {
> +		/* Slot does not have dynamically-removable children */
> +		return -EINVAL;
> +	}
> +	if (drc_indexes)
> +		*drc_indexes = indexes;
> +	if (drc_names)
> +		/* &drc_names[1] contains NULL terminated slot names */
> +		*drc_names = names;
> +	if (drc_types)
> +		/* &drc_types[1] contains NULL terminated slot types */
> +		*drc_types = types;
> +	if (drc_power_domains)
> +		*drc_power_domains = domains;
> +
> +	return 0;
> +}
> +
> +static int is_php_type(char *drc_type)
> +{
> +	unsigned long value;
> +	char *endptr;
> +
> +	/* PCI Hotplug nodes have an integer for drc_type */
> +	value = simple_strtoul(drc_type, &endptr, 10);
> +	if (endptr == drc_type)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +/**
> + * is_php_dn() - return 1 if this is a hotpluggable pci slot, else 0
> + * @dn: target &device_node
> + * @indexes: passed to get_children_props()
> + * @names: passed to get_children_props()
> + * @types: returned from get_children_props()
> + * @power_domains:
> + *
> + * This routine will return true only if the device node is
> + * a hotpluggable slot. This routine will return false
> + * for built-in pci slots (even when the built-in slots are
> + * dlparable.)
> + */
> +static int is_php_dn(struct device_node *dn, const int **indexes,
> +		const int **names, const int **types,
> +		const int **power_domains)
> +{
> +	const int *drc_types;
> +	int rc;
> +
> +	rc = get_children_props(dn, indexes, names, &drc_types,
> +				power_domains);
> +	if (rc < 0)
> +		return 0;
> +
> +	if (!is_php_type((char *) &drc_types[1]))
> +		return 0;
> +
> +	*types = drc_types;
> +	return 1;
> +}
> +
> +struct find_drc_match_cb_struct {
> +	struct device_node *dn;
> +	bool (*usercb)(struct device_node *dn,
> +			u32 drc_index, char *drc_name,
> +			char *drc_type, u32 drc_power_domain,
> +			void *data);
> +	char *drc_type;
> +	char *drc_name;
> +	u32 drc_index;
> +	bool match_drc_index;
> +	bool add_slot;
> +	void *data;
> +};
> +
> +static int find_drc_match_v1(struct device_node *dn, void *data)
> +{
> +	struct find_drc_match_cb_struct *cdata = data;
> +	int i, retval = 0;
> +	const int *indexes, *names, *types, *domains;
> +	char *name, *type;
> +	struct device_node *root = dn;
> +
> +	if (cdata->match_drc_index)
> +		root = dn->parent;
> +
> +	if (cdata->add_slot) {
> +		/* If this is not a hotplug slot, return without doing
> +		 * anything.
> +		 */
> +		if (!is_php_dn(root, &indexes, &names, &types, &domains))
> +			return 0;
> +	} else {
> +		if (get_children_props(root, &indexes, &names, &types,
> +			&domains) < 0)
> +			return 0;
> +	}
> +
> +	dbg("Entry %s: dn=%pOF\n", __func__, dn);
> +
> +	name = (char *) &names[1];
> +	type = (char *) &types[1];
> +	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
> +
> +		if (cdata->match_drc_index &&
> +			((unsigned int) indexes[i + 1] != cdata->drc_index)) {
> +			name += strlen(name) + 1;
> +			type += strlen(type) + 1;
> +			continue;
> +		}
> +
> +		if (((cdata->drc_name == NULL) ||
> +		     (cdata->drc_name && !strcmp(cdata->drc_name, name))) &&
> +		    ((cdata->drc_type == NULL) ||
> +		     (cdata->drc_type && !strcmp(cdata->drc_type, type)))) {
> +
> +			if (cdata->usercb) {
> +				retval = cdata->usercb(dn,
> +					be32_to_cpu(indexes[i + 1]),
> +					name, type,
> +					be32_to_cpu(domains[i + 1]),
> +					cdata->data);
> +				if (!retval)
> +					return retval;
> +			} else {
> +				return 0;
> +			}
> +		}
> +
> +		name += strlen(name) + 1;
> +		type += strlen(type) + 1;
> +	}
> +
> +	dbg("%s - Exit: rc[%d]\n", __func__, retval);
> +
> +	/* XXX FIXME: reports a failure only if last entry in loop failed */
> +	return retval;
> +}
> +
> +static bool find_drc_match_v2_cb(struct of_drc_info *drc, void *data,
> +				int *ret_code)
> +{
> +	struct find_drc_match_cb_struct *cdata = data;
> +	u32 drc_index;
> +	char drc_name[MAX_DRC_NAME_LEN];
> +	int i, retval;
> +
> +	(*ret_code) = -EINVAL;
> +
> +	/* This set not a PHP type? */
> +	if (cdata->add_slot) {
> +		if (!is_php_type((char *) drc->drc_type)) {
> +			return false;
> +		}
> +	}
> +
> +	/* Anything to use from this set? */
> +	if (cdata->match_drc_index && (cdata->drc_index > drc->last_drc_index))
> +		return false;
> +	if ((cdata->drc_type && strcmp(cdata->drc_type, drc->drc_type))
> +		return false;
> +
> +	/* Check the drc-index entries of this set */
> +	for (i = 0, drc_index = drc->drc_index_start;
> +		i < drc->num_sequential_elems; i++, drc_index++) {
> +
> +		if (cdata->match_drc_index && (cdata->drc_index != drc_index))
> +			continue;
> +
> +		sprintf(drc_name, "%s%d", drc->drc_name_prefix,
> +			drc_index - drc->drc_index_start +
> +			drc->drc_name_suffix_start);
> +
> +		if ((cdata->drc_name == NULL) ||
> +		    (cdata->drc_name && !strcmp(cdata->drc_name, drc_name))) {
> +
> +			if (cdata->usercb) {
> +				retval = cdata->usercb(cdata->dn,
> +						drc_index, drc_name,
> +						drc->drc_type,
> +						drc->drc_power_domain,
> +						cdata->data);
> +				if (!retval) {
> +					(*ret_code) = retval;
> +					return true;
> +				}
> +			} else {
> +				(*ret_code) = 0;
> +				return true;
> +			}
> +		}
> +	}
> +
> +	(*ret_code) = retval;
> +	return false;
> +}
> +
> +static int find_drc_match_v2(struct device_node *dn, void *data)
> +{
> +	struct find_drc_match_cb_struct *cdata = data;
> +	struct device_node *root = cdata->dn;
> +
> +	if (!cdata->add_slot) {
> +		if (!cdata->drc_type ||
> +			(cdata->drc_type && strcmp(cdata->drc_type, "SLOT")))
> +			root = dn->parent;
> +	}
> +
> +	return walk_drc_info(root, find_drc_match_v2_cb, NULL, data);
> +}
> +
> +int arch_find_drc_match(struct device_node *dn,
> +			bool (*usercb)(struct device_node *dn,
> +				u32 drc_index, char *drc_name,
> +				char *drc_type, u32 drc_power_domain,
> +				void *data),
> +			char *opt_drc_type, char *opt_drc_name,
> +			bool match_drc_index, bool add_slot,
> +			void *data)
> +{
> +	struct find_drc_match_cb_struct cdata = { dn, usercb,
> +			opt_drc_type, opt_drc_name, 0,
> +			match_drc_index, add_slot, data };
> +
> +	if (match_drc_index) {
> +		const int *my_index =
> +			of_get_property(dn, "ibm,my-drc-index", NULL);
> +		if (!my_index) {
> +			/* Node isn't DLPAR/hotplug capable */
> +			return -EINVAL;
> +		}
> +		cdata.drc_index = *my_index;
> +	}
> +
> +	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
> +		return find_drc_match_v2(dn, &cdata);
> +	else
> +		return find_drc_match_v1(dn, &cdata);
> +}
> +EXPORT_SYMBOL(arch_find_drc_match);
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index df97f5f..c3dfa53 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -50,7 +50,7 @@ int arch_find_drc_match(struct device_node *dn,
>  				char *drc_type, u32 drc_power_domain,
>  				void *data),
>  			char *opt_drc_type, char *opt_drc_name,
> -			bool match_drc_index, bool ck_php_type,
> +			bool match_drc_index, bool add_slot,
>  			void *data);

Why are you making a change to the prototype here? You should just define it in
your first patch instead of touching it again here.

-Tyrel

>  
>  /* Conform to ACPI 2.0 SLIT distance definitions */
> 

Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info

[Tyrel Datwyler]

On 12/14/2018 12:50 PM, Michael Bringmann wrote:
> Define interface to acquire arch-specific drc info to match against
> hotpluggable devices.  The current implementation exposes several
> pseries-specific dynamic memory properties in generic kernel code.
> This patch set provides an interface to pull that code out of the
> generic kernel.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
>  include/linux/topology.h |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index cb0775e..df97f5f 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -44,6 +44,15 @@
>  
>  int arch_update_cpu_topology(void);

On another note a kern doc comment for this function would also be nice.

-Tyrel

>  
> +int arch_find_drc_match(struct device_node *dn,
> +			bool (*usercb)(struct device_node *dn,
> +				u32 drc_index, char *drc_name,
> +				char *drc_type, u32 drc_power_domain,
> +				void *data),
> +			char *opt_drc_type, char *opt_drc_name,
> +			bool match_drc_index, bool ck_php_type,
> +			void *data);
> +
>  /* Conform to ACPI 2.0 SLIT distance definitions */
>  #define LOCAL_DISTANCE		10
>  #define REMOTE_DISTANCE		20
> 

Re: [RFC 5/6] powerpc/pci/hotplug: Use common drcinfo parsing

[Tyrel Datwyler]

On 01/14/2019 04:28 PM, Bjorn Helgaas wrote:
> On Fri, Dec 14, 2018 at 02:51:31PM -0600, Michael Bringmann wrote:
>> The implementation of the pseries-specific drc info properties
>> is currently implemented in pseries-specific and non-pseries-specific
>> files.  This patch set uses a new implementation of the device-tree
>> parsing code for the properties.
>>
>> This patch refactors parsing of the pseries-specific drc-info properties
>> out of rpaphp_core.c to use the common parser.  In the case where an
>> architecture does not use these properties, an __weak copy of the
>> function is provided with dummy return values.  Changes include creating
>> appropriate callback functions and passing callback-specific data
>> blocks into arch_find_drc_match.  All functions that were used just
>> to support the previous parsing implementation have been moved.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> 
> This is fine with me.  Any rpaphp_core.c maintainers want to comment?
> Tyrel?

It greatly simplifies the code in rpaphp_core.c, and as far as I can tell the
refactoring maintains the existing functionality.

Acked-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

> 
> $ ./scripts/get_maintainer.pl -f drivers/pci/hotplug/rpaphp_core.c
> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> (supporter:IBM Power PCI Hotplug Driver for RPA-compliant...)
> Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> Michael Ellerman <mpe@ellerman.id.au> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> 
>> ---
>>  drivers/pci/hotplug/rpaphp_core.c |  232 ++++---------------------------------
>>  1 file changed, 28 insertions(+), 204 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
>> index bcd5d35..9ad7384 100644
>> --- a/drivers/pci/hotplug/rpaphp_core.c
>> +++ b/drivers/pci/hotplug/rpaphp_core.c
>> @@ -154,182 +154,18 @@ static enum pci_bus_speed get_max_bus_speed(struct slot *slot)
>>  	return speed;
>>  }
>>  
>> -static int get_children_props(struct device_node *dn, const int **drc_indexes,
>> -		const int **drc_names, const int **drc_types,
>> -		const int **drc_power_domains)
>> -{
>> -	const int *indexes, *names, *types, *domains;
>> -
>> -	indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
>> -	names = of_get_property(dn, "ibm,drc-names", NULL);
>> -	types = of_get_property(dn, "ibm,drc-types", NULL);
>> -	domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
>> -
>> -	if (!indexes || !names || !types || !domains) {
>> -		/* Slot does not have dynamically-removable children */
>> -		return -EINVAL;
>> -	}
>> -	if (drc_indexes)
>> -		*drc_indexes = indexes;
>> -	if (drc_names)
>> -		/* &drc_names[1] contains NULL terminated slot names */
>> -		*drc_names = names;
>> -	if (drc_types)
>> -		/* &drc_types[1] contains NULL terminated slot types */
>> -		*drc_types = types;
>> -	if (drc_power_domains)
>> -		*drc_power_domains = domains;
>> -
>> -	return 0;
>> -}
>> -
>> -
>>  /* Verify the existence of 'drc_name' and/or 'drc_type' within the
>> - * current node.  First obtain it's my-drc-index property.  Next,
>> - * obtain the DRC info from it's parent.  Use the my-drc-index for
>> - * correlation, and obtain/validate the requested properties.
>> + * current node.
>>   */
>>  
>> -static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
>> -				char *drc_type, unsigned int my_index)
>> -{
>> -	char *name_tmp, *type_tmp;
>> -	const int *indexes, *names;
>> -	const int *types, *domains;
>> -	int i, rc;
>> -
>> -	rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
>> -	if (rc < 0) {
>> -		return -EINVAL;
>> -	}
>> -
>> -	name_tmp = (char *) &names[1];
>> -	type_tmp = (char *) &types[1];
>> -
>> -	/* Iterate through parent properties, looking for my-drc-index */
>> -	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
>> -		if ((unsigned int) indexes[i + 1] == my_index)
>> -			break;
>> -
>> -		name_tmp += (strlen(name_tmp) + 1);
>> -		type_tmp += (strlen(type_tmp) + 1);
>> -	}
>> -
>> -	if (((drc_name == NULL) || (drc_name && !strcmp(drc_name, name_tmp))) &&
>> -	    ((drc_type == NULL) || (drc_type && !strcmp(drc_type, type_tmp))))
>> -		return 0;
>> -
>> -	return -EINVAL;
>> -}
>> -
>> -static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
>> -				char *drc_type, unsigned int my_index)
>> -{
>> -	struct property *info;
>> -	unsigned int entries;
>> -	struct of_drc_info drc;
>> -	const __be32 *value;
>> -	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;
>> -
>> -	value = of_prop_next_u32(info, NULL, &entries);
>> -	if (!value)
>> -		return -EINVAL;
>> -
>> -	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;
>> -
>> -		fndit = 1;
>> -		break;
>> -	}
>> -	/* Found it */
>> -
>> -	if (fndit)
>> -		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
>> -			my_index);
>> -
>> -	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;
>> -
>> -	return -EINVAL;
>> -}
>> -
>>  int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
>>  			char *drc_type)
>>  {
>> -	const unsigned int *my_index;
>> -
>> -	my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
>> -	if (!my_index) {
>> -		/* Node isn't DLPAR/hotplug capable */
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
>> -		return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
>> -						*my_index);
>> -	else
>> -		return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
>> -						*my_index);
>> +	return arch_find_drc_match(dn, NULL, drc_type, drc_name,
>> +				true, false, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
>>  
>> -
>> -static int is_php_type(char *drc_type)
>> -{
>> -	unsigned long value;
>> -	char *endptr;
>> -
>> -	/* PCI Hotplug nodes have an integer for drc_type */
>> -	value = simple_strtoul(drc_type, &endptr, 10);
>> -	if (endptr == drc_type)
>> -		return 0;
>> -
>> -	return 1;
>> -}
>> -
>> -/**
>> - * is_php_dn() - return 1 if this is a hotpluggable pci slot, else 0
>> - * @dn: target &device_node
>> - * @indexes: passed to get_children_props()
>> - * @names: passed to get_children_props()
>> - * @types: returned from get_children_props()
>> - * @power_domains:
>> - *
>> - * This routine will return true only if the device node is
>> - * a hotpluggable slot. This routine will return false
>> - * for built-in pci slots (even when the built-in slots are
>> - * dlparable.)
>> - */
>> -static int is_php_dn(struct device_node *dn, const int **indexes,
>> -		const int **names, const int **types, const int **power_domains)
>> -{
>> -	const int *drc_types;
>> -	int rc;
>> -
>> -	rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
>> -	if (rc < 0)
>> -		return 0;
>> -
>> -	if (!is_php_type((char *) &drc_types[1]))
>> -		return 0;
>> -
>> -	*types = drc_types;
>> -	return 1;
>> -}
>> -
>>  /**
>>   * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
>>   * @dn: device node of slot
>> @@ -346,54 +182,42 @@ 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_cb(struct device_node *dn,
>> +			u32 drc_index, char *drc_name, char *drc_type,
>> +			u32 drc_power_domain, void *data)
>>  {
>>  	struct slot *slot;
>>  	int retval = 0;
>> -	int i;
>> -	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;
>> -
>> -	dbg("Entry %s: dn=%pOF\n", __func__, dn);
>>  
>> -	/* register PCI devices */
>> -	name = (char *) &names[1];
>> -	type = (char *) &types[1];
>> -	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
>> -		int index;
>> +	slot = alloc_slot_struct(dn, drc_index, drc_name, drc_power_domain);
>> +	if (!slot)
>> +		return -ENOMEM;
>>  
>> -		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(drc_type, NULL, 10);
>> +	if (retval)
>> +		return -EINVAL;
>>  
>> -		slot->type = simple_strtoul(type, NULL, 10);
>> +	dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
>> +		drc_index, drc_name, drc_type);
>>  
>> -		dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
>> -				index, name, type);
>> +	retval = rpaphp_enable_slot(slot);
>> +	if (!retval)
>> +		retval = rpaphp_register_slot(slot);
>>  
>> -		retval = rpaphp_enable_slot(slot);
>> -		if (!retval)
>> -			retval = rpaphp_register_slot(slot);
>> +	if (retval)
>> +		dealloc_slot_struct(slot);
>>  
>> -		if (retval)
>> -			dealloc_slot_struct(slot);
>> +	return retval;
>> +}
>>  
>> -		name += strlen(name) + 1;
>> -		type += strlen(type) + 1;
>> -	}
>> -	dbg("%s - Exit: rc[%d]\n", __func__, retval);
>> +int rpaphp_add_slot(struct device_node *dn)
>> +{
>> +	if (!dn->name || strcmp(dn->name, "pci"))
>> +		return 0;
>>  
>> -	/* XXX FIXME: reports a failure only if last entry in loop failed */
>> -	return retval;
>> +	return arch_find_drc_match(dn, rpaphp_add_slot_cb,
>> +			NULL, NULL, false, true, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(rpaphp_add_slot);
>>  
>>

Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info

[Michael Bringmann]

Adding Nathan Lynch

On 1/24/19 6:04 PM, Tyrel Datwyler wrote:
> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>> Define interface to acquire arch-specific drc info to match against
>> hotpluggable devices.  The current implementation exposes several
>> pseries-specific dynamic memory properties in generic kernel code.
>> This patch set provides an interface to pull that code out of the
>> generic kernel.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>>  include/linux/topology.h |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index cb0775e..df97f5f 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -44,6 +44,15 @@
> 
> As far as I know pseries is the only platform that uses DR connectors, and I
> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
> that this is really generic enough to belong in topology.h. If anything I would
> suggest putting this in an include in arch/powerpc/include/ named something like
> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
> that want/need to use this functionality.
> 
> -Tyrel
> 
>>  
>>  int arch_update_cpu_topology(void);
>>  
>> +int arch_find_drc_match(struct device_node *dn,
>> +			bool (*usercb)(struct device_node *dn,
>> +				u32 drc_index, char *drc_name,
>> +				char *drc_type, u32 drc_power_domain,
>> +				void *data),
>> +			char *opt_drc_type, char *opt_drc_name,
>> +			bool match_drc_index, bool ck_php_type,
>> +			void *data);
>> +
>>  /* Conform to ACPI 2.0 SLIT distance definitions */
>>  #define LOCAL_DISTANCE		10
>>  #define REMOTE_DISTANCE		20
>>
> 
> 

-- 
Michael W. Bringmann
Linux I/O, Networking and Security Development
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

Re: [RFC 3/6] pseries/drcinfo: Pseries impl of arch_find_drc_info

[Michael Bringmann]

Adding Nathan Lynch.

Yes.  We can amend the title.

On 1/24/19 6:04 PM, Tyrel Datwyler wrote:
> On 12/14/2018 12:51 PM, Michael Bringmann wrote:
>> This patch provides a common interface to parse ibm,drc-indexes,
>> ibm,drc-names, ibm,drc-types, ibm,drc-power-domains, or ibm,drc-info.
>> The generic interface arch_find_drc_match is provided which accepts
>> callback functions that may be applied to examine the data for each
>> entry.
>>
> 
> The title of your patch is "pseries/drcinfo: Pseries impl of arch_find_drc_info"
> but the name of the function you are ultimately implementing is
> arch_find_drc_match if I'm not mistaken.
> 
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/include/asm/prom.h             |    3 
>>  arch/powerpc/platforms/pseries/of_helpers.c |  299 +++++++++++++++++++++++++++
>>  include/linux/topology.h                    |    2 
>>  3 files changed, 298 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
>> index b04c5ce..910d1dc 100644
>> --- a/arch/powerpc/include/asm/prom.h
>> +++ b/arch/powerpc/include/asm/prom.h
>> @@ -91,9 +91,6 @@ struct of_drc_info {
>>  	u32 last_drc_index;
>>  };
>>  
>> -extern int of_read_drc_info_cell(struct property **prop,
>> -			const __be32 **curval, struct of_drc_info *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 0185e50..11c90cd 100644
>> --- a/arch/powerpc/platforms/pseries/of_helpers.c
>> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
>> @@ -1,5 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  
>> +#define pr_fmt(fmt) "drc: " fmt
>> +
>>  #include <linux/string.h>
>>  #include <linux/err.h>
>>  #include <linux/slab.h>
>> @@ -11,6 +13,12 @@
>>  
>>  #define	MAX_DRC_NAME_LEN 64
>>  
>> +static int drc_debug;
>> +#define dbg(args...) if (drc_debug) { printk(KERN_DEBUG args); }
>> +#define err(arg...) printk(KERN_ERR args);
>> +#define info(arg...) printk(KERN_INFO args);
>> +#define warn(arg...) printk(KERN_WARNING args);
> 
> Its pretty standard these days to use the pr_debug, pr_err, pr_info, pr_warn
> variations over printk(LEVEL args).
> 
>> +
>>  /**
>>   * pseries_of_derive_parent - basically like dirname(1)
>>   * @path:  the full_name of a node to be added to the tree
>> @@ -46,7 +54,8 @@ struct device_node *pseries_of_derive_parent(const char *path)
>>  
>>  /* Helper Routines to convert between drc_index to cpu numbers */
>>  
>> -int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
>> +static int of_read_drc_info_cell(struct property **prop,
>> +			const __be32 **curval,
>>  			struct of_drc_info *data)
>>  {
>>  	const char *p;
>> @@ -90,4 +99,290 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
>>  
>>  	return 0;
>>  }
>> -EXPORT_SYMBOL(of_read_drc_info_cell);
>> +
>> +static int walk_drc_info(struct device_node *dn,
>> +		bool (*usercb)(struct of_drc_info *drc,
>> +				void *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, ret_code = -EINVAL;
>> +	bool done = false;
>> +
>> +	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, &ret_code);
>> +	}
>> +
>> +	return ret_code;
>> +}
>> +
>> +static int get_children_props(struct device_node *dn, const int **drc_indexes,
>> +		const int **drc_names, const int **drc_types,
>> +		const int **drc_power_domains)
>> +{
>> +	const int *indexes, *names, *types, *domains;
>> +
>> +	indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
>> +	names = of_get_property(dn, "ibm,drc-names", NULL);
>> +	types = of_get_property(dn, "ibm,drc-types", NULL);
>> +	domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
>> +
>> +	if (!indexes || !names || !types || !domains) {
>> +		/* Slot does not have dynamically-removable children */
>> +		return -EINVAL;
>> +	}
>> +	if (drc_indexes)
>> +		*drc_indexes = indexes;
>> +	if (drc_names)
>> +		/* &drc_names[1] contains NULL terminated slot names */
>> +		*drc_names = names;
>> +	if (drc_types)
>> +		/* &drc_types[1] contains NULL terminated slot types */
>> +		*drc_types = types;
>> +	if (drc_power_domains)
>> +		*drc_power_domains = domains;
>> +
>> +	return 0;
>> +}
>> +
>> +static int is_php_type(char *drc_type)
>> +{
>> +	unsigned long value;
>> +	char *endptr;
>> +
>> +	/* PCI Hotplug nodes have an integer for drc_type */
>> +	value = simple_strtoul(drc_type, &endptr, 10);
>> +	if (endptr == drc_type)
>> +		return 0;
>> +
>> +	return 1;
>> +}
>> +
>> +/**
>> + * is_php_dn() - return 1 if this is a hotpluggable pci slot, else 0
>> + * @dn: target &device_node
>> + * @indexes: passed to get_children_props()
>> + * @names: passed to get_children_props()
>> + * @types: returned from get_children_props()
>> + * @power_domains:
>> + *
>> + * This routine will return true only if the device node is
>> + * a hotpluggable slot. This routine will return false
>> + * for built-in pci slots (even when the built-in slots are
>> + * dlparable.)
>> + */
>> +static int is_php_dn(struct device_node *dn, const int **indexes,
>> +		const int **names, const int **types,
>> +		const int **power_domains)
>> +{
>> +	const int *drc_types;
>> +	int rc;
>> +
>> +	rc = get_children_props(dn, indexes, names, &drc_types,
>> +				power_domains);
>> +	if (rc < 0)
>> +		return 0;
>> +
>> +	if (!is_php_type((char *) &drc_types[1]))
>> +		return 0;
>> +
>> +	*types = drc_types;
>> +	return 1;
>> +}
>> +
>> +struct find_drc_match_cb_struct {
>> +	struct device_node *dn;
>> +	bool (*usercb)(struct device_node *dn,
>> +			u32 drc_index, char *drc_name,
>> +			char *drc_type, u32 drc_power_domain,
>> +			void *data);
>> +	char *drc_type;
>> +	char *drc_name;
>> +	u32 drc_index;
>> +	bool match_drc_index;
>> +	bool add_slot;
>> +	void *data;
>> +};
>> +
>> +static int find_drc_match_v1(struct device_node *dn, void *data)
>> +{
>> +	struct find_drc_match_cb_struct *cdata = data;
>> +	int i, retval = 0;
>> +	const int *indexes, *names, *types, *domains;
>> +	char *name, *type;
>> +	struct device_node *root = dn;
>> +
>> +	if (cdata->match_drc_index)
>> +		root = dn->parent;
>> +
>> +	if (cdata->add_slot) {
>> +		/* If this is not a hotplug slot, return without doing
>> +		 * anything.
>> +		 */
>> +		if (!is_php_dn(root, &indexes, &names, &types, &domains))
>> +			return 0;
>> +	} else {
>> +		if (get_children_props(root, &indexes, &names, &types,
>> +			&domains) < 0)
>> +			return 0;
>> +	}
>> +
>> +	dbg("Entry %s: dn=%pOF\n", __func__, dn);
>> +
>> +	name = (char *) &names[1];
>> +	type = (char *) &types[1];
>> +	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
>> +
>> +		if (cdata->match_drc_index &&
>> +			((unsigned int) indexes[i + 1] != cdata->drc_index)) {
>> +			name += strlen(name) + 1;
>> +			type += strlen(type) + 1;
>> +			continue;
>> +		}
>> +
>> +		if (((cdata->drc_name == NULL) ||
>> +		     (cdata->drc_name && !strcmp(cdata->drc_name, name))) &&
>> +		    ((cdata->drc_type == NULL) ||
>> +		     (cdata->drc_type && !strcmp(cdata->drc_type, type)))) {
>> +
>> +			if (cdata->usercb) {
>> +				retval = cdata->usercb(dn,
>> +					be32_to_cpu(indexes[i + 1]),
>> +					name, type,
>> +					be32_to_cpu(domains[i + 1]),
>> +					cdata->data);
>> +				if (!retval)
>> +					return retval;
>> +			} else {
>> +				return 0;
>> +			}
>> +		}
>> +
>> +		name += strlen(name) + 1;
>> +		type += strlen(type) + 1;
>> +	}
>> +
>> +	dbg("%s - Exit: rc[%d]\n", __func__, retval);
>> +
>> +	/* XXX FIXME: reports a failure only if last entry in loop failed */
>> +	return retval;
>> +}
>> +
>> +static bool find_drc_match_v2_cb(struct of_drc_info *drc, void *data,
>> +				int *ret_code)
>> +{
>> +	struct find_drc_match_cb_struct *cdata = data;
>> +	u32 drc_index;
>> +	char drc_name[MAX_DRC_NAME_LEN];
>> +	int i, retval;
>> +
>> +	(*ret_code) = -EINVAL;
>> +
>> +	/* This set not a PHP type? */
>> +	if (cdata->add_slot) {
>> +		if (!is_php_type((char *) drc->drc_type)) {
>> +			return false;
>> +		}
>> +	}
>> +
>> +	/* Anything to use from this set? */
>> +	if (cdata->match_drc_index && (cdata->drc_index > drc->last_drc_index))
>> +		return false;
>> +	if ((cdata->drc_type && strcmp(cdata->drc_type, drc->drc_type))
>> +		return false;
>> +
>> +	/* Check the drc-index entries of this set */
>> +	for (i = 0, drc_index = drc->drc_index_start;
>> +		i < drc->num_sequential_elems; i++, drc_index++) {
>> +
>> +		if (cdata->match_drc_index && (cdata->drc_index != drc_index))
>> +			continue;
>> +
>> +		sprintf(drc_name, "%s%d", drc->drc_name_prefix,
>> +			drc_index - drc->drc_index_start +
>> +			drc->drc_name_suffix_start);
>> +
>> +		if ((cdata->drc_name == NULL) ||
>> +		    (cdata->drc_name && !strcmp(cdata->drc_name, drc_name))) {
>> +
>> +			if (cdata->usercb) {
>> +				retval = cdata->usercb(cdata->dn,
>> +						drc_index, drc_name,
>> +						drc->drc_type,
>> +						drc->drc_power_domain,
>> +						cdata->data);
>> +				if (!retval) {
>> +					(*ret_code) = retval;
>> +					return true;
>> +				}
>> +			} else {
>> +				(*ret_code) = 0;
>> +				return true;
>> +			}
>> +		}
>> +	}
>> +
>> +	(*ret_code) = retval;
>> +	return false;
>> +}
>> +
>> +static int find_drc_match_v2(struct device_node *dn, void *data)
>> +{
>> +	struct find_drc_match_cb_struct *cdata = data;
>> +	struct device_node *root = cdata->dn;
>> +
>> +	if (!cdata->add_slot) {
>> +		if (!cdata->drc_type ||
>> +			(cdata->drc_type && strcmp(cdata->drc_type, "SLOT")))
>> +			root = dn->parent;
>> +	}
>> +
>> +	return walk_drc_info(root, find_drc_match_v2_cb, NULL, data);
>> +}
>> +
>> +int arch_find_drc_match(struct device_node *dn,
>> +			bool (*usercb)(struct device_node *dn,
>> +				u32 drc_index, char *drc_name,
>> +				char *drc_type, u32 drc_power_domain,
>> +				void *data),
>> +			char *opt_drc_type, char *opt_drc_name,
>> +			bool match_drc_index, bool add_slot,
>> +			void *data)
>> +{
>> +	struct find_drc_match_cb_struct cdata = { dn, usercb,
>> +			opt_drc_type, opt_drc_name, 0,
>> +			match_drc_index, add_slot, data };
>> +
>> +	if (match_drc_index) {
>> +		const int *my_index =
>> +			of_get_property(dn, "ibm,my-drc-index", NULL);
>> +		if (!my_index) {
>> +			/* Node isn't DLPAR/hotplug capable */
>> +			return -EINVAL;
>> +		}
>> +		cdata.drc_index = *my_index;
>> +	}
>> +
>> +	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
>> +		return find_drc_match_v2(dn, &cdata);
>> +	else
>> +		return find_drc_match_v1(dn, &cdata);
>> +}
>> +EXPORT_SYMBOL(arch_find_drc_match);
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index df97f5f..c3dfa53 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -50,7 +50,7 @@ int arch_find_drc_match(struct device_node *dn,
>>  				char *drc_type, u32 drc_power_domain,
>>  				void *data),
>>  			char *opt_drc_type, char *opt_drc_name,
>> -			bool match_drc_index, bool ck_php_type,
>> +			bool match_drc_index, bool add_slot,
>>  			void *data);
> 
> Why are you making a change to the prototype here? You should just define it in
> your first patch instead of touching it again here.
> 
> -Tyrel
> 
>>  
>>  /* Conform to ACPI 2.0 SLIT distance definitions */
>>
> 
> 

-- 
Michael W. Bringmann
Linux I/O, Networking and Security Development
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info

[Michael Bringmann]

Adding Nathan Lynch.

On 1/24/19 6:10 PM, Tyrel Datwyler wrote:
> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>> Define interface to acquire arch-specific drc info to match against
>> hotpluggable devices.  The current implementation exposes several
>> pseries-specific dynamic memory properties in generic kernel code.
>> This patch set provides an interface to pull that code out of the
>> generic kernel.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>>  include/linux/topology.h |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index cb0775e..df97f5f 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -44,6 +44,15 @@
>>  
>>  int arch_update_cpu_topology(void);
> 
> On another note a kern doc comment for this function would also be nice.
> 
> -Tyrel
> 
>>  
>> +int arch_find_drc_match(struct device_node *dn,
>> +			bool (*usercb)(struct device_node *dn,
>> +				u32 drc_index, char *drc_name,
>> +				char *drc_type, u32 drc_power_domain,
>> +				void *data),
>> +			char *opt_drc_type, char *opt_drc_name,
>> +			bool match_drc_index, bool ck_php_type,
>> +			void *data);
>> +
>>  /* Conform to ACPI 2.0 SLIT distance definitions */
>>  #define LOCAL_DISTANCE		10
>>  #define REMOTE_DISTANCE		20
>>
> 
> 

-- 
Michael W. Bringmann
Linux I/O, Networking and Security Development
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

Re: [RFC 5/6] powerpc/pci/hotplug: Use common drcinfo parsing

[Michael Bringmann]

Adding Nathan Lynch.

On 1/24/19 6:29 PM, Tyrel Datwyler wrote:
> On 01/14/2019 04:28 PM, Bjorn Helgaas wrote:
>> On Fri, Dec 14, 2018 at 02:51:31PM -0600, Michael Bringmann wrote:
>>> The implementation of the pseries-specific drc info properties
>>> is currently implemented in pseries-specific and non-pseries-specific
>>> files.  This patch set uses a new implementation of the device-tree
>>> parsing code for the properties.
>>>
>>> This patch refactors parsing of the pseries-specific drc-info properties
>>> out of rpaphp_core.c to use the common parser.  In the case where an
>>> architecture does not use these properties, an __weak copy of the
>>> function is provided with dummy return values.  Changes include creating
>>> appropriate callback functions and passing callback-specific data
>>> blocks into arch_find_drc_match.  All functions that were used just
>>> to support the previous parsing implementation have been moved.
>>>
>>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>>
>> This is fine with me.  Any rpaphp_core.c maintainers want to comment?
>> Tyrel?
> 
> It greatly simplifies the code in rpaphp_core.c, and as far as I can tell the
> refactoring maintains the existing functionality.
> 
> Acked-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
> 
>>
>> $ ./scripts/get_maintainer.pl -f drivers/pci/hotplug/rpaphp_core.c
>> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> (supporter:IBM Power PCI Hotplug Driver for RPA-compliant...)
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
>> Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
>> Michael Ellerman <mpe@ellerman.id.au> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
>>
>>> ---
>>>  drivers/pci/hotplug/rpaphp_core.c |  232 ++++---------------------------------
>>>  1 file changed, 28 insertions(+), 204 deletions(-)
>>>
>>> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
>>> index bcd5d35..9ad7384 100644
>>> --- a/drivers/pci/hotplug/rpaphp_core.c
>>> +++ b/drivers/pci/hotplug/rpaphp_core.c
>>> @@ -154,182 +154,18 @@ static enum pci_bus_speed get_max_bus_speed(struct slot *slot)
>>>  	return speed;
>>>  }
>>>  
>>> -static int get_children_props(struct device_node *dn, const int **drc_indexes,
>>> -		const int **drc_names, const int **drc_types,
>>> -		const int **drc_power_domains)
>>> -{
>>> -	const int *indexes, *names, *types, *domains;
>>> -
>>> -	indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
>>> -	names = of_get_property(dn, "ibm,drc-names", NULL);
>>> -	types = of_get_property(dn, "ibm,drc-types", NULL);
>>> -	domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
>>> -
>>> -	if (!indexes || !names || !types || !domains) {
>>> -		/* Slot does not have dynamically-removable children */
>>> -		return -EINVAL;
>>> -	}
>>> -	if (drc_indexes)
>>> -		*drc_indexes = indexes;
>>> -	if (drc_names)
>>> -		/* &drc_names[1] contains NULL terminated slot names */
>>> -		*drc_names = names;
>>> -	if (drc_types)
>>> -		/* &drc_types[1] contains NULL terminated slot types */
>>> -		*drc_types = types;
>>> -	if (drc_power_domains)
>>> -		*drc_power_domains = domains;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -
>>>  /* Verify the existence of 'drc_name' and/or 'drc_type' within the
>>> - * current node.  First obtain it's my-drc-index property.  Next,
>>> - * obtain the DRC info from it's parent.  Use the my-drc-index for
>>> - * correlation, and obtain/validate the requested properties.
>>> + * current node.
>>>   */
>>>  
>>> -static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
>>> -				char *drc_type, unsigned int my_index)
>>> -{
>>> -	char *name_tmp, *type_tmp;
>>> -	const int *indexes, *names;
>>> -	const int *types, *domains;
>>> -	int i, rc;
>>> -
>>> -	rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
>>> -	if (rc < 0) {
>>> -		return -EINVAL;
>>> -	}
>>> -
>>> -	name_tmp = (char *) &names[1];
>>> -	type_tmp = (char *) &types[1];
>>> -
>>> -	/* Iterate through parent properties, looking for my-drc-index */
>>> -	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
>>> -		if ((unsigned int) indexes[i + 1] == my_index)
>>> -			break;
>>> -
>>> -		name_tmp += (strlen(name_tmp) + 1);
>>> -		type_tmp += (strlen(type_tmp) + 1);
>>> -	}
>>> -
>>> -	if (((drc_name == NULL) || (drc_name && !strcmp(drc_name, name_tmp))) &&
>>> -	    ((drc_type == NULL) || (drc_type && !strcmp(drc_type, type_tmp))))
>>> -		return 0;
>>> -
>>> -	return -EINVAL;
>>> -}
>>> -
>>> -static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
>>> -				char *drc_type, unsigned int my_index)
>>> -{
>>> -	struct property *info;
>>> -	unsigned int entries;
>>> -	struct of_drc_info drc;
>>> -	const __be32 *value;
>>> -	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;
>>> -
>>> -	value = of_prop_next_u32(info, NULL, &entries);
>>> -	if (!value)
>>> -		return -EINVAL;
>>> -
>>> -	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;
>>> -
>>> -		fndit = 1;
>>> -		break;
>>> -	}
>>> -	/* Found it */
>>> -
>>> -	if (fndit)
>>> -		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
>>> -			my_index);
>>> -
>>> -	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;
>>> -
>>> -	return -EINVAL;
>>> -}
>>> -
>>>  int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
>>>  			char *drc_type)
>>>  {
>>> -	const unsigned int *my_index;
>>> -
>>> -	my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
>>> -	if (!my_index) {
>>> -		/* Node isn't DLPAR/hotplug capable */
>>> -		return -EINVAL;
>>> -	}
>>> -
>>> -	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
>>> -		return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
>>> -						*my_index);
>>> -	else
>>> -		return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
>>> -						*my_index);
>>> +	return arch_find_drc_match(dn, NULL, drc_type, drc_name,
>>> +				true, false, NULL);
>>>  }
>>>  EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
>>>  
>>> -
>>> -static int is_php_type(char *drc_type)
>>> -{
>>> -	unsigned long value;
>>> -	char *endptr;
>>> -
>>> -	/* PCI Hotplug nodes have an integer for drc_type */
>>> -	value = simple_strtoul(drc_type, &endptr, 10);
>>> -	if (endptr == drc_type)
>>> -		return 0;
>>> -
>>> -	return 1;
>>> -}
>>> -
>>> -/**
>>> - * is_php_dn() - return 1 if this is a hotpluggable pci slot, else 0
>>> - * @dn: target &device_node
>>> - * @indexes: passed to get_children_props()
>>> - * @names: passed to get_children_props()
>>> - * @types: returned from get_children_props()
>>> - * @power_domains:
>>> - *
>>> - * This routine will return true only if the device node is
>>> - * a hotpluggable slot. This routine will return false
>>> - * for built-in pci slots (even when the built-in slots are
>>> - * dlparable.)
>>> - */
>>> -static int is_php_dn(struct device_node *dn, const int **indexes,
>>> -		const int **names, const int **types, const int **power_domains)
>>> -{
>>> -	const int *drc_types;
>>> -	int rc;
>>> -
>>> -	rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
>>> -	if (rc < 0)
>>> -		return 0;
>>> -
>>> -	if (!is_php_type((char *) &drc_types[1]))
>>> -		return 0;
>>> -
>>> -	*types = drc_types;
>>> -	return 1;
>>> -}
>>> -
>>>  /**
>>>   * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
>>>   * @dn: device node of slot
>>> @@ -346,54 +182,42 @@ 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_cb(struct device_node *dn,
>>> +			u32 drc_index, char *drc_name, char *drc_type,
>>> +			u32 drc_power_domain, void *data)
>>>  {
>>>  	struct slot *slot;
>>>  	int retval = 0;
>>> -	int i;
>>> -	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;
>>> -
>>> -	dbg("Entry %s: dn=%pOF\n", __func__, dn);
>>>  
>>> -	/* register PCI devices */
>>> -	name = (char *) &names[1];
>>> -	type = (char *) &types[1];
>>> -	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
>>> -		int index;
>>> +	slot = alloc_slot_struct(dn, drc_index, drc_name, drc_power_domain);
>>> +	if (!slot)
>>> +		return -ENOMEM;
>>>  
>>> -		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(drc_type, NULL, 10);
>>> +	if (retval)
>>> +		return -EINVAL;
>>>  
>>> -		slot->type = simple_strtoul(type, NULL, 10);
>>> +	dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
>>> +		drc_index, drc_name, drc_type);
>>>  
>>> -		dbg("Found drc-index:0x%x drc-name:%s drc-type:%s\n",
>>> -				index, name, type);
>>> +	retval = rpaphp_enable_slot(slot);
>>> +	if (!retval)
>>> +		retval = rpaphp_register_slot(slot);
>>>  
>>> -		retval = rpaphp_enable_slot(slot);
>>> -		if (!retval)
>>> -			retval = rpaphp_register_slot(slot);
>>> +	if (retval)
>>> +		dealloc_slot_struct(slot);
>>>  
>>> -		if (retval)
>>> -			dealloc_slot_struct(slot);
>>> +	return retval;
>>> +}
>>>  
>>> -		name += strlen(name) + 1;
>>> -		type += strlen(type) + 1;
>>> -	}
>>> -	dbg("%s - Exit: rc[%d]\n", __func__, retval);
>>> +int rpaphp_add_slot(struct device_node *dn)
>>> +{
>>> +	if (!dn->name || strcmp(dn->name, "pci"))
>>> +		return 0;
>>>  
>>> -	/* XXX FIXME: reports a failure only if last entry in loop failed */
>>> -	return retval;
>>> +	return arch_find_drc_match(dn, rpaphp_add_slot_cb,
>>> +			NULL, NULL, false, true, NULL);
>>>  }
>>>  EXPORT_SYMBOL_GPL(rpaphp_add_slot);
>>>  
>>>
> 
> 

-- 
Michael W. Bringmann
Linux I/O, Networking and Security Development
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info

[Michael Bringmann]

On 1/25/19 10:09 AM, Michael Bringmann wrote:
> Adding Nathan Lynch
> 
> On 1/24/19 6:04 PM, Tyrel Datwyler wrote:
>> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>>> Define interface to acquire arch-specific drc info to match against
>>> hotpluggable devices.  The current implementation exposes several
>>> pseries-specific dynamic memory properties in generic kernel code.
>>> This patch set provides an interface to pull that code out of the
>>> generic kernel.
>>>
>>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>>> ---
>>>  include/linux/topology.h |    9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>>> index cb0775e..df97f5f 100644
>>> --- a/include/linux/topology.h
>>> +++ b/include/linux/topology.h
>>> @@ -44,6 +44,15 @@
>>
>> As far as I know pseries is the only platform that uses DR connectors, and I
>> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
>> that this is really generic enough to belong in topology.h. If anything I would
>> suggest putting this in an include in arch/powerpc/include/ named something like
>> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
>> that want/need to use this functionality.

It looks like the 'rpaphp' and 'rpadlpar_io' modules are also dependent upon the
powerpc platform.  Shouldn't the relevant source files be moved completely to the
powerpc-specific directories out of drivers/pci/hotplug as well?

drivers/pci/hotplug/Kconfig has:

config HOTPLUG_PCI_RPA
        tristate "RPA PCI Hotplug driver"
        depends on PPC_PSERIES && EEH
        help
          Say Y here if you have a RPA system that supports PCI Hotplug.

          To compile this driver as a module, choose M here: the
          module will be called rpaphp.

          When in doubt, say N.

config HOTPLUG_PCI_RPA_DLPAR
        tristate "RPA Dynamic Logical Partitioning for I/O slots"
        depends on HOTPLUG_PCI_RPA
        help
          Say Y here if your system supports Dynamic Logical Partitioning
          for I/O slots.

          To compile this driver as a module, choose M here: the
          module will be called rpadlpar_io.

          When in doubt, say N.

Michael

>>
>> -Tyrel
>>
>>>  
>>>  int arch_update_cpu_topology(void);
>>>  
>>> +int arch_find_drc_match(struct device_node *dn,
>>> +			bool (*usercb)(struct device_node *dn,
>>> +				u32 drc_index, char *drc_name,
>>> +				char *drc_type, u32 drc_power_domain,
>>> +				void *data),
>>> +			char *opt_drc_type, char *opt_drc_name,
>>> +			bool match_drc_index, bool ck_php_type,
>>> +			void *data);
>>> +
>>>  /* Conform to ACPI 2.0 SLIT distance definitions */
>>>  #define LOCAL_DISTANCE		10
>>>  #define REMOTE_DISTANCE		20
>>>
>>
>>
> 

-- 
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 1/6] powerpc:/drc Define interface to acquire arch-specific drc info

[Michael Ellerman]

Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
> On 1/25/19 10:09 AM, Michael Bringmann wrote:
>> Adding Nathan Lynch
>> 
>> On 1/24/19 6:04 PM, Tyrel Datwyler wrote:
>>> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>>>> Define interface to acquire arch-specific drc info to match against
>>>> hotpluggable devices.  The current implementation exposes several
>>>> pseries-specific dynamic memory properties in generic kernel code.
>>>> This patch set provides an interface to pull that code out of the
>>>> generic kernel.
>>>>
>>>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>>>> ---
>>>>  include/linux/topology.h |    9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>>>> index cb0775e..df97f5f 100644
>>>> --- a/include/linux/topology.h
>>>> +++ b/include/linux/topology.h
>>>> @@ -44,6 +44,15 @@
>>>
>>> As far as I know pseries is the only platform that uses DR connectors, and I
>>> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
>>> that this is really generic enough to belong in topology.h. If anything I would
>>> suggest putting this in an include in arch/powerpc/include/ named something like
>>> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
>>> that want/need to use this functionality.
>
> It looks like the 'rpaphp' and 'rpadlpar_io' modules are also dependent upon the
> powerpc platform.

Yes that's right.

> Shouldn't the relevant source files be moved completely to the
> powerpc-specific directories out of drivers/pci/hotplug as well?

I don't think so. They are PCI hotplug drivers, so they should sit with
the other PCI hotplug drivers.

It's true that PCI hotplug drivers are more platform specific than other
types of drivers, but still they have some things in common with other
PCI hotplug drivers.

cheers

Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info

[Michael Ellerman]

Tyrel Datwyler <turtle.in.the.kernel@gmail.com> writes:
> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>> Define interface to acquire arch-specific drc info to match against
>> hotpluggable devices.  The current implementation exposes several
>> pseries-specific dynamic memory properties in generic kernel code.
>> This patch set provides an interface to pull that code out of the
>> generic kernel.
>> 
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>>  include/linux/topology.h |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index cb0775e..df97f5f 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -44,6 +44,15 @@
>
> As far as I know pseries is the only platform that uses DR connectors, and I
> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
> that this is really generic enough to belong in topology.h.

Right. This does not belong in include/linux.

> If anything I would
> suggest putting this in an include in arch/powerpc/include/ named something like
> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
> that want/need to use this functionality.

Yeah that would make more sense.

Using "arch" in the name is wrong, it's pseries specific so
pseries_find_drc_match() would be more appropriate.

>> +int arch_find_drc_match(struct device_node *dn,
>> +			bool (*usercb)(struct device_node *dn,
>> +				u32 drc_index, char *drc_name,
>> +				char *drc_type, u32 drc_power_domain,
>> +				void *data),
>> +			char *opt_drc_type, char *opt_drc_name,
>> +			bool match_drc_index, bool ck_php_type,
>> +			void *data);

This function signature is kind of insane.

You end with calls like:

+	return arch_find_drc_match(dn, rpaphp_add_slot_cb,
+			NULL, NULL, false, true, NULL);

Which is impossible to parse.

I feel like maybe this isn't the right level of abstraction.

cheers

Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info

[Michael Bringmann]

On 1/29/19 3:31 AM, Michael Ellerman wrote:
> Tyrel Datwyler <turtle.in.the.kernel@gmail.com> writes:
>> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>>> Define interface to acquire arch-specific drc info to match against
>>> hotpluggable devices.  The current implementation exposes several
>>> pseries-specific dynamic memory properties in generic kernel code.
>>> This patch set provides an interface to pull that code out of the
>>> generic kernel.
>>>
>>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>>> ---
>>>  include/linux/topology.h |    9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>>> index cb0775e..df97f5f 100644
>>> --- a/include/linux/topology.h
>>> +++ b/include/linux/topology.h
>>> @@ -44,6 +44,15 @@
>>
>> As far as I know pseries is the only platform that uses DR connectors, and I
>> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
>> that this is really generic enough to belong in topology.h.
> 
> Right. This does not belong in include/linux.
> 
>> If anything I would
>> suggest putting this in an include in arch/powerpc/include/ named something like
>> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
>> that want/need to use this functionality.
> 
> Yeah that would make more sense.

If you see no objection to referencing a powerpc-specific function from
the code ...

> 
> Using "arch" in the name is wrong, it's pseries specific so
> pseries_find_drc_match() would be more appropriate.
> 
>>> +int arch_find_drc_match(struct device_node *dn,
>>> +			bool (*usercb)(struct device_node *dn,
>>> +				u32 drc_index, char *drc_name,
>>> +				char *drc_type, u32 drc_power_domain,
>>> +				void *data),
>>> +			char *opt_drc_type, char *opt_drc_name,
>>> +			bool match_drc_index, bool ck_php_type,
>>> +			void *data);
> 
> This function signature is kind of insane.
> 
> You end with calls like:
> 
> +	return arch_find_drc_match(dn, rpaphp_add_slot_cb,
> +			NULL, NULL, false, true, NULL);
> 
> Which is impossible to parse.
> 
> I feel like maybe this isn't the right level of abstraction.

...
I had already been considering simplifying the interface for these
calls to something like the following:

int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
			char *drc_type)
{
    return pseries_find_drc_match(dn, drc_type, drc_name);
}
...
int rpaphp_add_slot(struct device_node *dn)
{
       if (!dn->name || strcmp(dn->name, "pci"))
               return 0;

       return pseries_add_drc_slot(dn, rpaphp_add_slot_cb);
}
...

Further details would be hidden within the pseries code.


> 
> cheers

Regards

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