[PATCH] drmgr: Free nodes returned from configure_connector

[Haren Myneni]

[Haren Myneni]

of_nodes returned from configure_connector should be freed after
updating the device tree and is missing in acquire_hp_resource()
and add_work(). This patch calls free_of_node() in these functions.

Signed-off-by: Haren Myneni <ha...@linux.ibm.com>
---
src/drmgr/common_pci.c | 1 +
src/drmgr/drslot_chrp_pci.c | 1 +
2 files changed, 2 insertions(+)

diff --git a/src/drmgr/common_pci.c b/src/drmgr/common_pci.c
index 2411641..759589a 100644
--- a/src/drmgr/common_pci.c
+++ b/src/drmgr/common_pci.c
@@ -1434,6 +1434,7 @@ acquire_hp_resource(struct dr_connector *drc, char *of_path)
return -1;

rc = add_device_tree_nodes(of_path, new_nodes);
+ free_of_node(new_nodes);
if (rc) {
say(ERROR, "add nodes failed for 0x%x\n", drc->index);
return rc;
diff --git a/src/drmgr/drslot_chrp_pci.c b/src/drmgr/drslot_chrp_pci.c
index ac078db..ec3c77c 100644
--- a/src/drmgr/drslot_chrp_pci.c
+++ b/src/drmgr/drslot_chrp_pci.c
@@ -454,6 +454,7 @@ static int add_work(struct dr_node *node, bool partner_device)

say(DEBUG, "Adding %s to %s\n", new_nodes->name, node->ofdt_path);
rc = add_device_tree_nodes(node->ofdt_path, new_nodes);
+ free_of_node(new_nodes);
if (rc) {
say(DEBUG, "add_device_tree_nodes failed at %s\n",
node->ofdt_path);
--
2.31.1

[Nathan Lynch]

Haren Myneni <ha...@linux.ibm.com> writes:
> of_nodes returned from configure_connector should be freed after
> updating the device tree and is missing in acquire_hp_resource()
> and add_work(). This patch calls free_of_node() in these functions.
>
> Signed-off-by: Haren Myneni <ha...@linux.ibm.com>

This change looks correct to me. Other code in drmgr follows this
pattern (configure_connector() followed by add_device_tree_nodes()
followed by free_of_node()), and it doesn't look like any references to
the temporary node tree are retained after the calls to
add_device_tree_nodes().

The commit message would be improved by explaining how the problem was
found. I usually say something like "found by inspection" if it's just
something I noticed while reading the code, but if it was reported by a
tool like a sanitizer or Valgrind then it's worth including that detail.

Reviewed-by: Nathan Lynch <nat...@linux.ibm.com>

[Haren Myneni]

On Tue, 2024-07-09 at 11:19 -0500, Nathan Lynch wrote:
> Haren Myneni <ha...@linux.ibm.com> writes:
> > of_nodes returned from configure_connector should be freed after
> > updating the device tree and is missing in acquire_hp_resource()
> > and add_work(). This patch calls free_of_node() in these functions.
> >
> > Signed-off-by: Haren Myneni <ha...@linux.ibm.com>
>
> This change looks correct to me. Other code in drmgr follows this
> pattern (configure_connector() followed by add_device_tree_nodes()
> followed by free_of_node()), and it doesn't look like any references
> to
> the temporary node tree are retained after the calls to
> add_device_tree_nodes().
>
> The commit message would be improved by explaining how the problem
> was
> found. I usually say something like "found by inspection" if it's
> just
> something I noticed while reading the code, but if it was reported by
> a
> tool like a sanitizer or Valgrind then it's worth including that
> detail.

Thanks for the review. yes found this missing free nodes while looking
at the code. Update the commit description and will repost the patch.

[Tyrel Datwyler]

On 6/29/24 14:14, Haren Myneni wrote:
>
> of_nodes returned from configure_connector should be freed after
> updating the device tree and is missing in acquire_hp_resource()
> and add_work(). This patch calls free_of_node() in these functions.
>
> Signed-off-by: Haren Myneni <ha...@linux.ibm.com>
> ---

Patch applied to powerpc-utils/next branch.

https://github.com/ibm-power-utilities/powerpc-utils/commit/5db2df531f9c242b13ef6520814c99685144c6d4

Thanks,
-Tyrel