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