[PATCH v3 1/3] powerpc/pseries: Use correct data types from pseries_hp_errorlog struct

[PATCH v3 1/3] powerpc/pseries: Use correct data types from pseries_hp_errorlog struct

[Haren Myneni]

_be32 type is defined for some elements in pseries_hp_errorlog
struct but also used them u32 after be32_to_cpu() conversion.

Example: In handle_dlpar_errorlog()
hp_elog->_drc_u.drc_index = be32_to_cpu(hp_elog->_drc_u.drc_index);

And later assigned to u32 type
dlpar_cpu() - u32 drc_index = hp_elog->_drc_u.drc_index;

This incorrect usage is giving the following warnings and the
patch resolve these warnings with the correct assignment.

arch/powerpc/platforms/pseries/dlpar.c:398:53: sparse: sparse:
incorrect type in argument 1 (different base types) @@
expected unsigned int [usertype] drc_index @@
got restricted __be32 [usertype] drc_index @@
...
arch/powerpc/platforms/pseries/dlpar.c:418:43: sparse: sparse:
incorrect type in assignment (different base types) @@
expected restricted __be32 [usertype] drc_count @@
got unsigned int [usertype] @@

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202408182142.wuIKqYae-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202408182302.o7QRO45S-lkp@intel.com/
Signed-off-by: Haren Myneni <haren@linux.ibm.com>

v3:
- Fix warnings from using incorrect data types in pseries_hp_errorlog
  struct
v2:
- Remove pr_info() and TODO comments
- Update more information in the commit logs
---
 arch/powerpc/platforms/pseries/dlpar.c          | 17 -----------------
 arch/powerpc/platforms/pseries/hotplug-cpu.c    |  2 +-
 arch/powerpc/platforms/pseries/hotplug-memory.c | 16 ++++++++--------
 arch/powerpc/platforms/pseries/pmem.c           |  2 +-
 4 files changed, 10 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 47f8eabd1bee..9873b916b237 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -334,23 +334,6 @@ int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
 {
 	int rc;
 
-	/* pseries error logs are in BE format, convert to cpu type */
-	switch (hp_elog->id_type) {
-	case PSERIES_HP_ELOG_ID_DRC_COUNT:
-		hp_elog->_drc_u.drc_count =
-				be32_to_cpu(hp_elog->_drc_u.drc_count);
-		break;
-	case PSERIES_HP_ELOG_ID_DRC_INDEX:
-		hp_elog->_drc_u.drc_index =
-				be32_to_cpu(hp_elog->_drc_u.drc_index);
-		break;
-	case PSERIES_HP_ELOG_ID_DRC_IC:
-		hp_elog->_drc_u.ic.count =
-				be32_to_cpu(hp_elog->_drc_u.ic.count);
-		hp_elog->_drc_u.ic.index =
-				be32_to_cpu(hp_elog->_drc_u.ic.index);
-	}
-
 	switch (hp_elog->resource) {
 	case PSERIES_HP_ELOG_RESOURCE_MEM:
 		rc = dlpar_memory(hp_elog);
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index e62835a12d73..6838a0fcda29 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -757,7 +757,7 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
 	u32 drc_index;
 	int rc;
 
-	drc_index = hp_elog->_drc_u.drc_index;
+	drc_index = be32_to_cpu(hp_elog->_drc_u.drc_index);
 
 	lock_device_hotplug();
 
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 3fe3ddb30c04..38dc4f7c9296 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -817,16 +817,16 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 	case PSERIES_HP_ELOG_ACTION_ADD:
 		switch (hp_elog->id_type) {
 		case PSERIES_HP_ELOG_ID_DRC_COUNT:
-			count = hp_elog->_drc_u.drc_count;
+			count = be32_to_cpu(hp_elog->_drc_u.drc_count);
 			rc = dlpar_memory_add_by_count(count);
 			break;
 		case PSERIES_HP_ELOG_ID_DRC_INDEX:
-			drc_index = hp_elog->_drc_u.drc_index;
+			drc_index = be32_to_cpu(hp_elog->_drc_u.drc_index);
 			rc = dlpar_memory_add_by_index(drc_index);
 			break;
 		case PSERIES_HP_ELOG_ID_DRC_IC:
-			count = hp_elog->_drc_u.ic.count;
-			drc_index = hp_elog->_drc_u.ic.index;
+			count = be32_to_cpu(hp_elog->_drc_u.ic.count);
+			drc_index = be32_to_cpu(hp_elog->_drc_u.ic.index);
 			rc = dlpar_memory_add_by_ic(count, drc_index);
 			break;
 		default:
@@ -838,16 +838,16 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 	case PSERIES_HP_ELOG_ACTION_REMOVE:
 		switch (hp_elog->id_type) {
 		case PSERIES_HP_ELOG_ID_DRC_COUNT:
-			count = hp_elog->_drc_u.drc_count;
+			count = be32_to_cpu(hp_elog->_drc_u.drc_count);
 			rc = dlpar_memory_remove_by_count(count);
 			break;
 		case PSERIES_HP_ELOG_ID_DRC_INDEX:
-			drc_index = hp_elog->_drc_u.drc_index;
+			drc_index = be32_to_cpu(hp_elog->_drc_u.drc_index);
 			rc = dlpar_memory_remove_by_index(drc_index);
 			break;
 		case PSERIES_HP_ELOG_ID_DRC_IC:
-			count = hp_elog->_drc_u.ic.count;
-			drc_index = hp_elog->_drc_u.ic.index;
+			count = be32_to_cpu(hp_elog->_drc_u.ic.count);
+			drc_index = be32_to_cpu(hp_elog->_drc_u.ic.index);
 			rc = dlpar_memory_remove_by_ic(count, drc_index);
 			break;
 		default:
diff --git a/arch/powerpc/platforms/pseries/pmem.c b/arch/powerpc/platforms/pseries/pmem.c
index 3c290b9ed01b..0f1d45f32e4a 100644
--- a/arch/powerpc/platforms/pseries/pmem.c
+++ b/arch/powerpc/platforms/pseries/pmem.c
@@ -121,7 +121,7 @@ int dlpar_hp_pmem(struct pseries_hp_errorlog *hp_elog)
 		return -EINVAL;
 	}
 
-	drc_index = hp_elog->_drc_u.drc_index;
+	drc_index = be32_to_cpu(hp_elog->_drc_u.drc_index);
 
 	lock_device_hotplug();
 
-- 
2.43.5

[PATCH v3 2/3] powerpc/pseries/dlpar: Remove device tree node for DLPAR IO remove

[Haren Myneni]

In the powerpc-pseries specific implementation, the IO hotplug
event is handled in the user space (drmgr tool). But update the
device tree and /dev/mem access to allocate buffers for some
RTAS calls are restricted when the kernel lockdown feature is
enabled. For the DLPAR IO REMOVE, the corresponding device tree
nodes and properties have to be removed from the device tree
after the device disable. The user space removes the device tree
nodes by updating /proc/ppc64/ofdt which is not allowed  under
system lockdown is enabled. This restriction can be resolved
by moving the complete IO hotplug handling in the kernel. But
the pseries implementation need user interaction to power off
and to remove device from the slot during hotplug event handling.

To overcome the /proc/ppc64/ofdt restriction, this patch extends
the /sys/kernel/dlpar interface and provides
‘dt remove index <drc_index>’ to the user space so that drmgr
tool can remove the corresponding device tree nodes based on DRC
index from the device tree.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/include/asm/rtas.h        |  1 +
 arch/powerpc/platforms/pseries/dlpar.c | 88 +++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 065ffd1b2f8a..04406162fc5a 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -397,6 +397,7 @@ inline uint16_t pseries_errorlog_length(struct pseries_errorlog *sect)
 #define PSERIES_HP_ELOG_RESOURCE_SLOT	3
 #define PSERIES_HP_ELOG_RESOURCE_PHB	4
 #define PSERIES_HP_ELOG_RESOURCE_PMEM   6
+#define PSERIES_HP_ELOG_RESOURCE_DT	7
 
 #define PSERIES_HP_ELOG_ACTION_ADD	1
 #define PSERIES_HP_ELOG_ACTION_REMOVE	2
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 9873b916b237..1b49b47c4a4f 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -330,6 +330,87 @@ int dlpar_unisolate_drc(u32 drc_index)
 	return 0;
 }
 
+static int changeset_detach_node_recursive(struct of_changeset *ocs,
+					struct device_node *node)
+{
+	struct device_node *child;
+	int rc;
+
+	for_each_child_of_node(node, child) {
+		rc = changeset_detach_node_recursive(ocs, child);
+		if (rc) {
+			of_node_put(child);
+			return rc;
+		}
+	}
+
+	return of_changeset_detach_node(ocs, node);
+}
+
+static int dlpar_hp_dt_remove(u32 drc_index)
+{
+	struct device_node *np;
+	struct of_changeset ocs;
+	u32 index;
+	int rc = 0;
+
+	/*
+	 * Prune all nodes with a matching index.
+	 */
+	of_changeset_init(&ocs);
+
+	for_each_node_with_property(np, "ibm,my-drc-index") {
+		rc = of_property_read_u32(np, "ibm,my-drc-index", &index);
+		if (rc) {
+			pr_err("%s: %pOF: of_property_read_u32 %s: %d\n",
+				__func__, np, "ibm,my-drc-index", rc);
+			of_node_put(np);
+			goto out;
+		}
+
+		if (index == drc_index) {
+			rc = changeset_detach_node_recursive(&ocs, np);
+			if (rc) {
+				of_node_put(np);
+				goto out;
+			}
+		}
+	}
+
+	rc = of_changeset_apply(&ocs);
+
+out:
+	of_changeset_destroy(&ocs);
+	return rc;
+}
+
+static int dlpar_hp_dt(struct pseries_hp_errorlog *phpe)
+{
+	u32 drc_index;
+	int rc;
+
+	if (phpe->id_type != PSERIES_HP_ELOG_ID_DRC_INDEX)
+		return -EINVAL;
+
+	drc_index = be32_to_cpu(phpe->_drc_u.drc_index);
+
+	lock_device_hotplug();
+
+	switch (phpe->action) {
+	case PSERIES_HP_ELOG_ACTION_REMOVE:
+		rc = dlpar_hp_dt_remove(drc_index);
+		break;
+	default:
+		pr_err("Invalid action (%d) specified\n", phpe->action);
+		rc = -EINVAL;
+		break;
+	}
+
+	unlock_device_hotplug();
+
+	return rc;
+}
+
 int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
 {
 	int rc;
@@ -344,6 +425,9 @@ int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
 	case PSERIES_HP_ELOG_RESOURCE_PMEM:
 		rc = dlpar_hp_pmem(hp_elog);
 		break;
+	case PSERIES_HP_ELOG_RESOURCE_DT:
+		rc = dlpar_hp_dt(hp_elog);
+		break;
 
 	default:
 		pr_warn_ratelimited("Invalid resource (%d) specified\n",
@@ -396,6 +480,8 @@ static int dlpar_parse_resource(char **cmd, struct pseries_hp_errorlog *hp_elog)
 		hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_MEM;
 	} else if (sysfs_streq(arg, "cpu")) {
 		hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_CPU;
+	} else if (sysfs_streq(arg, "dt")) {
+		hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_DT;
 	} else {
 		pr_err("Invalid resource specified.\n");
 		return -EINVAL;
@@ -537,7 +623,7 @@ static ssize_t dlpar_store(const struct class *class, const struct class_attribu
 static ssize_t dlpar_show(const struct class *class, const struct class_attribute *attr,
 			  char *buf)
 {
-	return sprintf(buf, "%s\n", "memory,cpu");
+	return sprintf(buf, "%s\n", "memory,cpu,dt");
 }
 
 static CLASS_ATTR_RW(dlpar);
-- 
2.43.5

[PATCH v3 3/3] powerpc/pseries/dlpar: Add device tree nodes for DLPAR IO add

[Haren Myneni]

In the powerpc-pseries specific implementation, the IO hotplug
event is handled in the user space (drmgr tool). For the DLPAR
IO ADD, the corresponding device tree nodes and properties will
be added to the device tree after the device enable. The user
space (drmgr tool) uses configure_connector RTAS call with the
DRC index to retrieve the device nodes and updates the device
tree by writing to /proc/ppc64/ofdt. Under system lockdown,
/dev/mem access to allocate buffers for configure_connector RTAS
call is restricted which means the user space can not issue this
RTAS call and also can not access to /proc/ppc64/ofdt. The
pseries implementation need user interaction to power-on and add
device to the slot during the ADD event handling. So adds
complexity if the complete hotplug ADD event handling moved to
the kernel.

To overcome /dev/mem access restriction, this patch extends the
/sys/kernel/dlpar interface and provides ‘dt add index <drc_index>’
to the user space. The drmgr tool uses this interface to update
the device tree whenever the device is added. This interface
retrieves device tree nodes for the corresponding DRC index using
the configure_connector RTAS call and adds new device nodes /
properties to the device tree.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/dlpar.c | 130 +++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 1b49b47c4a4f..6f0bc3ddbf85 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -23,6 +23,7 @@
 #include <linux/uaccess.h>
 #include <asm/rtas.h>
 #include <asm/rtas-work-area.h>
+#include <asm/prom.h>
 
 static struct workqueue_struct *pseries_hp_wq;
 
@@ -264,6 +265,20 @@ int dlpar_detach_node(struct device_node *dn)
 
 	return 0;
 }
+static int dlpar_changeset_attach_cc_nodes(struct of_changeset *ocs,
+					struct device_node *dn)
+{
+	int rc;
+
+	rc = of_changeset_attach_node(ocs, dn);
+
+	if (!rc && dn->child)
+		rc = dlpar_changeset_attach_cc_nodes(ocs, dn->child);
+	if (!rc && dn->sibling)
+		rc = dlpar_changeset_attach_cc_nodes(ocs, dn->sibling);
+
+	return rc;
+}
 
 #define DR_ENTITY_SENSE		9003
 #define DR_ENTITY_PRESENT	1
@@ -330,6 +345,118 @@ int dlpar_unisolate_drc(u32 drc_index)
 	return 0;
 }
 
+static struct device_node *
+get_device_node_with_drc_index(u32 index)
+{
+	struct device_node *np = NULL;
+	u32 node_index;
+	int rc;
+
+	for_each_node_with_property(np, "ibm,my-drc-index") {
+		rc = of_property_read_u32(np, "ibm,my-drc-index",
+					     &node_index);
+		if (rc) {
+			pr_err("%s: %pOF: of_property_read_u32 %s: %d\n",
+			       __func__, np, "ibm,my-drc-index", rc);
+			of_node_put(np);
+			return NULL;
+		}
+
+		if (index == node_index)
+			break;
+	}
+
+	return np;
+}
+
+static struct device_node *
+get_device_node_with_drc_info(u32 index)
+{
+	struct device_node *np = NULL;
+	struct of_drc_info drc;
+	struct property *info;
+	const __be32 *value;
+	u32 node_index;
+	int i, j, count;
+
+	for_each_node_with_property(np, "ibm,drc-info") {
+		info = of_find_property(np, "ibm,drc-info", NULL);
+		if (info == NULL) {
+			/* XXX can this happen? */
+			of_node_put(np);
+			return NULL;
+		}
+		value = of_prop_next_u32(info, NULL, &count);
+		if (value == NULL)
+			continue;
+		value++;
+		for (i = 0; i < count; i++) {
+			if (of_read_drc_info_cell(&info, &value, &drc))
+				break;
+			if (index > drc.last_drc_index)
+				continue;
+			node_index = drc.drc_index_start;
+			for (j = 0; j < drc.num_sequential_elems; j++) {
+				if (index == node_index)
+					return np;
+				node_index += drc.sequential_inc;
+			}
+		}
+	}
+
+	return NULL;
+}
+
+static int dlpar_hp_dt_add(u32 index)
+{
+	struct device_node *np, *nodes;
+	struct of_changeset ocs;
+	int rc;
+
+	/*
+	 * Do not add device node(s) if already exists in the
+	 * device tree.
+	 */
+	np = get_device_node_with_drc_index(index);
+	if (np) {
+		pr_err("%s: Adding device node for index (%d), but "
+				"already exists in the device tree\n",
+				__func__, index);
+		rc = -EINVAL;
+		goto out;
+	}
+
+	np = get_device_node_with_drc_info(index);
+
+	if (!np)
+		return -EIO;
+
+	/* Next, configure the connector. */
+	nodes = dlpar_configure_connector(cpu_to_be32(index), np);
+	if (!nodes) {
+		rc = -EIO;
+		goto out;
+	}
+
+	/*
+	 * Add the new nodes from dlpar_configure_connector() onto
+	 * the device-tree.
+	 */
+	of_changeset_init(&ocs);
+	rc = dlpar_changeset_attach_cc_nodes(&ocs, nodes);
+
+	if (!rc)
+		rc = of_changeset_apply(&ocs);
+	else
+		dlpar_free_cc_nodes(nodes);
+
+	of_changeset_destroy(&ocs);
+
+out:
+	of_node_put(np);
+	return rc;
+}
+
 static int changeset_detach_node_recursive(struct of_changeset *ocs,
 					struct device_node *node)
 {
@@ -397,6 +524,9 @@ static int dlpar_hp_dt(struct pseries_hp_errorlog *phpe)
 	lock_device_hotplug();
 
 	switch (phpe->action) {
+	case PSERIES_HP_ELOG_ACTION_ADD:
+		rc = dlpar_hp_dt_add(drc_index);
+		break;
 	case PSERIES_HP_ELOG_ACTION_REMOVE:
 		rc = dlpar_hp_dt_remove(drc_index);
 		break;
-- 
2.43.5

Re: [PATCH v3 3/3] powerpc/pseries/dlpar: Add device tree nodes for DLPAR IO add

[Michael Ellerman]

Hi Haren,

One query below about the of_node refcounting.

Haren Myneni <haren@linux.ibm.com> writes:
> In the powerpc-pseries specific implementation, the IO hotplug
> event is handled in the user space (drmgr tool). For the DLPAR
> IO ADD, the corresponding device tree nodes and properties will
> be added to the device tree after the device enable. The user
> space (drmgr tool) uses configure_connector RTAS call with the
> DRC index to retrieve the device nodes and updates the device
> tree by writing to /proc/ppc64/ofdt. Under system lockdown,
> /dev/mem access to allocate buffers for configure_connector RTAS
> call is restricted which means the user space can not issue this
> RTAS call and also can not access to /proc/ppc64/ofdt. The
> pseries implementation need user interaction to power-on and add
> device to the slot during the ADD event handling. So adds
> complexity if the complete hotplug ADD event handling moved to
> the kernel.
>
> To overcome /dev/mem access restriction, this patch extends the
> /sys/kernel/dlpar interface and provides ‘dt add index <drc_index>’
> to the user space. The drmgr tool uses this interface to update
> the device tree whenever the device is added. This interface
> retrieves device tree nodes for the corresponding DRC index using
> the configure_connector RTAS call and adds new device nodes /
> properties to the device tree.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/dlpar.c | 130 +++++++++++++++++++++++++
>  1 file changed, 130 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
> index 1b49b47c4a4f..6f0bc3ddbf85 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
...
> @@ -330,6 +345,118 @@ int dlpar_unisolate_drc(u32 drc_index)
>  	return 0;
>  }
>  
> +static struct device_node *
> +get_device_node_with_drc_index(u32 index)
> +{
> +	struct device_node *np = NULL;
> +	u32 node_index;
> +	int rc;
> +
> +	for_each_node_with_property(np, "ibm,my-drc-index") {
> +		rc = of_property_read_u32(np, "ibm,my-drc-index",
> +					     &node_index);
> +		if (rc) {
> +			pr_err("%s: %pOF: of_property_read_u32 %s: %d\n",
> +			       __func__, np, "ibm,my-drc-index", rc);
> +			of_node_put(np);
> +			return NULL;
> +		}
> +
> +		if (index == node_index)
> +			break;

Here we return with np's refcount elevated.

> +	}
> +
> +	return np;
> +}
...
> +
> +static int dlpar_hp_dt_add(u32 index)
> +{
> +	struct device_node *np, *nodes;
> +	struct of_changeset ocs;
> +	int rc;
> +
> +	/*
> +	 * Do not add device node(s) if already exists in the
> +	 * device tree.
> +	 */
> +	np = get_device_node_with_drc_index(index);
> +	if (np) {
> +		pr_err("%s: Adding device node for index (%d), but "
> +				"already exists in the device tree\n",
> +				__func__, index);
> +		rc = -EINVAL;
> +		goto out;
 
In the error case you drop the reference on np (at out).

> +	}

> +	np = get_device_node_with_drc_info(index);
> 
But in the success case np is reassigned, so the refcount is leaked.
I think that's unintentional, but I'm not 100% sure.

> +	if (!np)
> +		return -EIO;
> +
> +	/* Next, configure the connector. */
> +	nodes = dlpar_configure_connector(cpu_to_be32(index), np);
> +	if (!nodes) {
> +		rc = -EIO;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Add the new nodes from dlpar_configure_connector() onto
> +	 * the device-tree.
> +	 */
> +	of_changeset_init(&ocs);
> +	rc = dlpar_changeset_attach_cc_nodes(&ocs, nodes);
> +
> +	if (!rc)
> +		rc = of_changeset_apply(&ocs);
> +	else
> +		dlpar_free_cc_nodes(nodes);
> +
> +	of_changeset_destroy(&ocs);
> +
> +out:
> +	of_node_put(np);
> +	return rc;
> +}
> +
>  static int changeset_detach_node_recursive(struct of_changeset *ocs,
>  					struct device_node *node)
>  {

cheers

Re: [PATCH v3 3/3] powerpc/pseries/dlpar: Add device tree nodes for DLPAR IO add

[Haren Myneni]

On Wed, 2024-08-28 at 18:12 +1000, Michael Ellerman wrote:
> Hi Haren,
> 
> One query below about the of_node refcounting.
> 
> Haren Myneni <haren@linux.ibm.com> writes:
> > In the powerpc-pseries specific implementation, the IO hotplug
> > event is handled in the user space (drmgr tool). For the DLPAR
> > IO ADD, the corresponding device tree nodes and properties will
> > be added to the device tree after the device enable. The user
> > space (drmgr tool) uses configure_connector RTAS call with the
> > DRC index to retrieve the device nodes and updates the device
> > tree by writing to /proc/ppc64/ofdt. Under system lockdown,
> > /dev/mem access to allocate buffers for configure_connector RTAS
> > call is restricted which means the user space can not issue this
> > RTAS call and also can not access to /proc/ppc64/ofdt. The
> > pseries implementation need user interaction to power-on and add
> > device to the slot during the ADD event handling. So adds
> > complexity if the complete hotplug ADD event handling moved to
> > the kernel.
> > 
> > To overcome /dev/mem access restriction, this patch extends the
> > /sys/kernel/dlpar interface and provides ‘dt add index <drc_index>’
> > to the user space. The drmgr tool uses this interface to update
> > the device tree whenever the device is added. This interface
> > retrieves device tree nodes for the corresponding DRC index using
> > the configure_connector RTAS call and adds new device nodes /
> > properties to the device tree.
> > 
> > Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> >  arch/powerpc/platforms/pseries/dlpar.c | 130
> > +++++++++++++++++++++++++
> >  1 file changed, 130 insertions(+)
> > 
> > diff --git a/arch/powerpc/platforms/pseries/dlpar.c
> > b/arch/powerpc/platforms/pseries/dlpar.c
> > index 1b49b47c4a4f..6f0bc3ddbf85 100644
> > --- a/arch/powerpc/platforms/pseries/dlpar.c
> > +++ b/arch/powerpc/platforms/pseries/dlpar.c
> ...
> > @@ -330,6 +345,118 @@ int dlpar_unisolate_drc(u32 drc_index)
> >  	return 0;
> >  }
> >  
> > +static struct device_node *
> > +get_device_node_with_drc_index(u32 index)
> > +{
> > +	struct device_node *np = NULL;
> > +	u32 node_index;
> > +	int rc;
> > +
> > +	for_each_node_with_property(np, "ibm,my-drc-index") {
> > +		rc = of_property_read_u32(np, "ibm,my-drc-index",
> > +					     &node_index);
> > +		if (rc) {
> > +			pr_err("%s: %pOF: of_property_read_u32 %s:
> > %d\n",
> > +			       __func__, np, "ibm,my-drc-index", rc);
> > +			of_node_put(np);
> > +			return NULL;
> > +		}
> > +
> > +		if (index == node_index)
> > +			break;
> 
> Here we return with np's refcount elevated.
> 
> > +	}
> > +
> > +	return np;
> > +}
> ...
> > +
> > +static int dlpar_hp_dt_add(u32 index)
> > +{
> > +	struct device_node *np, *nodes;
> > +	struct of_changeset ocs;
> > +	int rc;
> > +
> > +	/*
> > +	 * Do not add device node(s) if already exists in the
> > +	 * device tree.
> > +	 */
> > +	np = get_device_node_with_drc_index(index);
> > +	if (np) {
> > +		pr_err("%s: Adding device node for index (%d), but "
> > +				"already exists in the device tree\n",
> > +				__func__, index);
> > +		rc = -EINVAL;
> > +		goto out;
>  
> In the error case you drop the reference on np (at out).
> 
> > +	}
> > +	np = get_device_node_with_drc_info(index);
> > 
> But in the success case np is reassigned, so the refcount is leaked.
> I think that's unintentional, but I'm not 100% sure.

Michael, 

get_device_node_with_drc_index() holds the refcount only if the node is
already exists. In this case this refcount is dropped. 

if (np) {
                pr_err("%s: Adding device node for index (%d), but "
                                "already exists in the device tree\n",
                                __func__, index);
                rc = -EINVAL;
                goto out; --> drop refcount 
        }

Whereas failure from the get_device_node_with_drc_index() - can not
find the match node. means we do not hold the refcount and need to add
the node from get_device_node_with_drc_info()

I should add a comment regarding refcount to make it clear. will post
V4 patch with this comment.


Thanks
Haren

> cheers

Re: [PATCH v3 3/3] powerpc/pseries/dlpar: Add device tree nodes for DLPAR IO add

[Michael Ellerman]

Haren Myneni <haren@linux.ibm.com> writes:
> On Wed, 2024-08-28 at 18:12 +1000, Michael Ellerman wrote:
>> Hi Haren,
>> 
>> One query below about the of_node refcounting.
>> 
>> Haren Myneni <haren@linux.ibm.com> writes:
>> > In the powerpc-pseries specific implementation, the IO hotplug
>> > event is handled in the user space (drmgr tool). For the DLPAR
>> > IO ADD, the corresponding device tree nodes and properties will
>> > be added to the device tree after the device enable. The user
>> > space (drmgr tool) uses configure_connector RTAS call with the
>> > DRC index to retrieve the device nodes and updates the device
>> > tree by writing to /proc/ppc64/ofdt. Under system lockdown,
>> > /dev/mem access to allocate buffers for configure_connector RTAS
>> > call is restricted which means the user space can not issue this
>> > RTAS call and also can not access to /proc/ppc64/ofdt. The
>> > pseries implementation need user interaction to power-on and add
>> > device to the slot during the ADD event handling. So adds
>> > complexity if the complete hotplug ADD event handling moved to
>> > the kernel.
>> > 
>> > To overcome /dev/mem access restriction, this patch extends the
>> > /sys/kernel/dlpar interface and provides ‘dt add index <drc_index>’
>> > to the user space. The drmgr tool uses this interface to update
>> > the device tree whenever the device is added. This interface
>> > retrieves device tree nodes for the corresponding DRC index using
>> > the configure_connector RTAS call and adds new device nodes /
>> > properties to the device tree.
>> > 
>> > Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
>> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
>> > ---
>> >  arch/powerpc/platforms/pseries/dlpar.c | 130
>> > +++++++++++++++++++++++++
>> >  1 file changed, 130 insertions(+)
>> > 
>> > diff --git a/arch/powerpc/platforms/pseries/dlpar.c
>> > b/arch/powerpc/platforms/pseries/dlpar.c
>> > index 1b49b47c4a4f..6f0bc3ddbf85 100644
>> > --- a/arch/powerpc/platforms/pseries/dlpar.c
>> > +++ b/arch/powerpc/platforms/pseries/dlpar.c
>> ...
>> > @@ -330,6 +345,118 @@ int dlpar_unisolate_drc(u32 drc_index)
>> >  	return 0;
>> >  }
>> >  
>> > +static struct device_node *
>> > +get_device_node_with_drc_index(u32 index)
>> > +{
>> > +	struct device_node *np = NULL;
>> > +	u32 node_index;
>> > +	int rc;
>> > +
>> > +	for_each_node_with_property(np, "ibm,my-drc-index") {
>> > +		rc = of_property_read_u32(np, "ibm,my-drc-index",
>> > +					     &node_index);
>> > +		if (rc) {
>> > +			pr_err("%s: %pOF: of_property_read_u32 %s:
>> > %d\n",
>> > +			       __func__, np, "ibm,my-drc-index", rc);
>> > +			of_node_put(np);
>> > +			return NULL;
>> > +		}
>> > +
>> > +		if (index == node_index)
>> > +			break;
>> 
>> Here we return with np's refcount elevated.
>> 
>> > +	}
>> > +
>> > +	return np;
>> > +}
>> ...
>> > +
>> > +static int dlpar_hp_dt_add(u32 index)
>> > +{
>> > +	struct device_node *np, *nodes;
>> > +	struct of_changeset ocs;
>> > +	int rc;
>> > +
>> > +	/*
>> > +	 * Do not add device node(s) if already exists in the
>> > +	 * device tree.
>> > +	 */
>> > +	np = get_device_node_with_drc_index(index);
>> > +	if (np) {
>> > +		pr_err("%s: Adding device node for index (%d), but "
>> > +				"already exists in the device tree\n",
>> > +				__func__, index);
>> > +		rc = -EINVAL;
>> > +		goto out;
>>  
>> In the error case you drop the reference on np (at out).
>> 
>> > +	}
>> > +	np = get_device_node_with_drc_info(index);
>> > 
>> But in the success case np is reassigned, so the refcount is leaked.
>> I think that's unintentional, but I'm not 100% sure.
>
> Michael, 
>
> get_device_node_with_drc_index() holds the refcount only if the node is
> already exists. In this case this refcount is dropped. 
>
> if (np) {
>                 pr_err("%s: Adding device node for index (%d), but "
>                                 "already exists in the device tree\n",
>                                 __func__, index);
>                 rc = -EINVAL;
>                 goto out; --> drop refcount 
>         }

Oh yep. I misread that as if (!np).

> Whereas failure from the get_device_node_with_drc_index() - can not
> find the match node. means we do not hold the refcount and need to add
> the node from get_device_node_with_drc_info()

Right.

> I should add a comment regarding refcount to make it clear. will post
> V4 patch with this comment.

It's probably fine as-is, I just needed to read it properly :)

cheers

Re: [PATCH v3 1/3] powerpc/pseries: Use correct data types from pseries_hp_errorlog struct

[Michael Ellerman]

On Wed, 21 Aug 2024 19:50:26 -0700, Haren Myneni wrote:
> _be32 type is defined for some elements in pseries_hp_errorlog
> struct but also used them u32 after be32_to_cpu() conversion.
> 
> Example: In handle_dlpar_errorlog()
> hp_elog->_drc_u.drc_index = be32_to_cpu(hp_elog->_drc_u.drc_index);
> 
> And later assigned to u32 type
> dlpar_cpu() - u32 drc_index = hp_elog->_drc_u.drc_index;
> 
> [...]

Applied to powerpc/next.

[1/3] powerpc/pseries: Use correct data types from pseries_hp_errorlog struct
      https://git.kernel.org/powerpc/c/b76e0d4215b6b622127ebcceaa7f603313ceaec4
[2/3] powerpc/pseries/dlpar: Remove device tree node for DLPAR IO remove
      https://git.kernel.org/powerpc/c/17a51171c20d590d3d3c632bcdd946f5fc3c0061
[3/3] powerpc/pseries/dlpar: Add device tree nodes for DLPAR IO add
      https://git.kernel.org/powerpc/c/02b98ff44a57c1376c5a92a8518fda5c82bb5a91

cheers