Re: [PATCH] device property: preserve usecount for node passed to of_fwnode_graph_get_port_parent()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hejssan Niklas,

Niklas Söderlund wrote:
Hi Sakari,

On 2017-08-21 22:03:02 +0300, Sakari Ailus wrote:
Hi Niklas,

Niklas Söderlund wrote:
Hi Sakari,

On 2017-08-21 16:30:17 +0300, Sakari Ailus wrote:
Hi Niklas,

Niklas Söderlund wrote:
Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the
node being passed to of_fwnode_graph_get_port_parent(). Preserve the
usecount just like it is done in of_graph_get_port_parent().

The of_fwnode_graph_get_port_parent() is called by
fwnode_graph_get_port_parent() which obtains the port node through
fwnode_get_parent(). If you take a reference here, calling
fwnode_graph_get_port_parent() will end up incrementing the port node's use
count. In other words, my understanding is that dropping the reference to
the port node isn't a problem but intended behaviour here.

I'm not sure but I don't think the usecount will be incremented, without
this patch I think it's decremented by one instead. Lets look at the
code starting with fwnode_graph_get_port_parent().

struct fwnode_handle *
fwnode_graph_get_port_parent(struct fwnode_handle *endpoint)
{
        struct fwnode_handle *port, *parent;

Increment usecount by 1

        port = fwnode_get_parent(endpoint);
        parent = fwnode_call_ptr_op(port, graph_get_port_parent);

Decrement usecount by 1

        fwnode_handle_put(port); << Usecount -1

Here it is; this is the one I missed.

I spotted something else, too. Look at of_graph_get_port_parent(); it
appears to decrement the use count of the node passed to it, too:

struct device_node *of_graph_get_port_parent(struct device_node *node)
{
        unsigned int depth;

        /* Walk 3 levels up only if there is 'ports' node. */
        for (depth = 3; depth && node; depth--) {
                node = of_get_next_parent(node);
                if (depth == 2 && of_node_cmp(node->name, "ports"))
                        break;
        }
        return node;
}
EXPORT_SYMBOL(of_graph_get_port_parent);

I think you'd need to of_node_get(node) first. I think it'd be good to
address this at the same time.

Your tree is old :-)

I did check of_graph_get_port_parent() when looking for how this was
handled elsewhere in the kernel. But I did not realise that the fix was
accepted after 4.13-rc1 so I did not mention that this was just a copy
of that fix in the patch description. For reference see

  c0a480d1acf7dc18 ("device property: Fix usecount for of_graph_get_port_parent()")

Ack, good. I didn't check new developments there, I have to admit.



One could claim the original design principle has truly been adopted in the
fwnode variant of the function. X-)

Yes and I adopted the same fix for the original :-)


On your original patch --- could you replace of_get_next_parent() by
of_get_parent()? In that case it won't drop the reference to the parent,
i.e. does what's required.

I do however think this is a much nicer solution. So I would still be
inclined to send a v2 whit this change instead. Which solution would you
prefer?

of_get_parent() is my preference; you can add to v2:

Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>

of_get_next_parent() is intended for cases where you expressly want to drop the reference AFAIK.

Thanks!




        return parent;
}

Here it looks like the counting is correct and balanced. But without
this patch it's in this function 'fwnode_handle_put(port)' which
triggers the error which this patch aims to fix. Lets look at
of_fwnode_graph_get_port_parent() which in my case is what is called by
the fwnode_call_ptr_op().

static struct fwnode_handle *
of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode)
{
        struct device_node *np;

Here in of_get_next_parent() the usecount is decremented by 1 and the
parents usecount is incremented by 1. So for our node node which passed
in from fwnode_graph_get_port_parent() (where it's named 'port') will be
decremented by 1.

        /* Get the parent of the port */
        np = of_get_next_parent(to_of_node(fwnode));
        if (!np)
                return NULL;

        /* Is this the "ports" node? If not, it's the port parent. */
        if (of_node_cmp(np->name, "ports"))
                return of_fwnode_handle(np);

        return of_fwnode_handle(of_get_next_parent(np));
}

So unless I miss something I do think this patch is needed to restore
balance to the usecount of the node being passed to
of_fwnode_graph_get_port_parent(). Or maybe I have misunderstood
something?


I wonder if I miss something.

I also wonder what I missed :-)



Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware specific locations")
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
---
 drivers/of/property.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 067f9fab7b77c794..637dcb4833e2af60 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -922,6 +922,12 @@ of_fwnode_graph_get_port_parent(struct fwnode_handle *fwnode)
 {
 	struct device_node *np;

+	/*
+	 * Preserve usecount for passed in node as of_get_next_parent()
+	 * will do of_node_put() on it.
+	 */
+	of_node_get(to_of_node(fwnode));
+
 	/* Get the parent of the port */
 	np = of_get_next_parent(to_of_node(fwnode));
 	if (!np)



--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx



--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx



--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux