Haren Myneni <ha...@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 <ha...@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 <chel...@linux.ibm.com>
>> > Signed-off-by: Haren Myneni <ha...@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
