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

[Sakari Ailus]

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 

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 
---
 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 @@ 

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

[Niklas Söderlund]

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()")

> 
> 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?

> 
> > 
> > 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 
> > > > ---
> > > >  drivers/of/property.c | 6 ++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/of/property.c 

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

[Sakari Ailus]

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.


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


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.




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 
---
 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.ai...@linux.intel.com





--
Sakari Ailus
sakari.ai...@linux.intel.com

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

[Geert Uytterhoeven]

Hi Niklas,

On Mon, Aug 21, 2017 at 2:51 PM, 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().
>
> Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware 
> specific locations")
> Signed-off-by: Niklas Söderlund 
> ---
>  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)

FWIW, I'd use "np" to store the intermediate value:

struct device_node *np = to_of_node(fwnode);

 /*
  * Preserve usecount for passed in node as of_get_next_parent()
  * will do of_node_put() on it.
  */
of_node_get(np);

/* Get the parent of the port */
np = of_get_next_parent(np);

Alternatively, perhaps to_of_node() should increment the refcount and
call of_node_get()? Oh, there's (static) of_fwnode_get(), too.

Is drivers/iommu/iommu.c:iommu_fwspec_init() really the only place outside
drivers/of/property.c that calls of_node_get() on a node obtained by
to_of_node()?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

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

[Niklas Söderlund]

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

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 
> > ---
> >  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.ai...@linux.intel.com

-- 
Regards,
Niklas Söderlund

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

[Sakari Ailus]

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 wonder if I miss something.



Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware 
specific locations")
Signed-off-by: Niklas Söderlund 
---
 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.ai...@linux.intel.com