[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 at 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 at linux.ibm.com>
> > Signed-off-by: Haren Myneni <haren at 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