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

[Michael Ellerman <mpe@ellerman.id.au>]

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