| Message ID | 20240822025028.938332-3-haren@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 02b98ff44a57c1376c5a92a8518fda5c82bb5a91 |
| Headers | show |
| Series | [v3,1/3] powerpc/pseries: Use correct data types from pseries_hp_errorlog struct | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 21 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 5 jobs. |
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. > + 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
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 } 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
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
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 @@ -23,6 +23,7 @@ #include <linux/uaccess.h> #include <asm/rtas.h> #include <asm/rtas-work-area.h> +#include <asm/prom.h> static struct workqueue_struct *pseries_hp_wq; @@ -264,6 +265,20 @@ int dlpar_detach_node(struct device_node *dn) return 0; } +static int dlpar_changeset_attach_cc_nodes(struct of_changeset *ocs, + struct device_node *dn) +{ + int rc; + + rc = of_changeset_attach_node(ocs, dn); + + if (!rc && dn->child) + rc = dlpar_changeset_attach_cc_nodes(ocs, dn->child); + if (!rc && dn->sibling) + rc = dlpar_changeset_attach_cc_nodes(ocs, dn->sibling); + + return rc; +} #define DR_ENTITY_SENSE 9003 #define DR_ENTITY_PRESENT 1 @@ -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; + } + + return np; +} + +static struct device_node * +get_device_node_with_drc_info(u32 index) +{ + struct device_node *np = NULL; + struct of_drc_info drc; + struct property *info; + const __be32 *value; + u32 node_index; + int i, j, count; + + for_each_node_with_property(np, "ibm,drc-info") { + info = of_find_property(np, "ibm,drc-info", NULL); + if (info == NULL) { + /* XXX can this happen? */ + of_node_put(np); + return NULL; + } + value = of_prop_next_u32(info, NULL, &count); + if (value == NULL) + continue; + value++; + for (i = 0; i < count; i++) { + if (of_read_drc_info_cell(&info, &value, &drc)) + break; + if (index > drc.last_drc_index) + continue; + node_index = drc.drc_index_start; + for (j = 0; j < drc.num_sequential_elems; j++) { + if (index == node_index) + return np; + node_index += drc.sequential_inc; + } + } + } + + return NULL; +} + +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; + } + + np = get_device_node_with_drc_info(index); + + 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) { @@ -397,6 +524,9 @@ static int dlpar_hp_dt(struct pseries_hp_errorlog *phpe) lock_device_hotplug(); switch (phpe->action) { + case PSERIES_HP_ELOG_ACTION_ADD: + rc = dlpar_hp_dt_add(drc_index); + break; case PSERIES_HP_ELOG_ACTION_REMOVE: rc = dlpar_hp_dt_remove(drc_index); break;