Haren Myneni [off-list ref] writes:

On Wed, 2024-08-28 at 18:12 +1000, Michael Ellerman wrote:

quoted

Hi Haren,

One query below about the of_node refcounting.

Haren Myneni [off-list ref] writes:

quoted

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

...

quoted

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

quoted

+	}
+
+	return np;
+}

...

quoted

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

quoted

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