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

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

[Niklas Söderlund]

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 <niklas.soderlund+renesas@ragnatech.se>
---
 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)
-- 
2.14.0

For posterity here is the console log:

OF: ERROR: Bad of_node_put() on /soc/i2c@e66d8000/gmsl-deserializer@1/ports/port@4
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.13.0-rc4-00418-g32df6aeea9a6f626 #14
Hardware name: Renesas Salvator-X board based on r8a7795 ES1.x (DT)
Call trace:
[<ffff000008088f58>] dump_backtrace+0x0/0x238
[<ffff00000808929c>] show_stack+0x14/0x20
[<ffff000008ed6c50>] dump_stack+0x9c/0xbc
[<ffff000008c58450>] of_node_release+0xb8/0xc0
[<ffff000008edb644>] kobject_put+0x84/0xf0
[<ffff000008c57c04>] of_node_put+0x14/0x28
[<ffff000008c5677c>] of_fwnode_put+0x24/0x40
[<ffff0000087be488>] fwnode_graph_get_port_parent+0x60/0xb0
[<ffff000008b85e2c>] match_fwnode+0x2c/0x88
[<ffff000008b85f98>] v4l2_async_belongs+0x78/0x120
[<ffff000008b8615c>] v4l2_async_notifier_register+0x11c/0x1d8
[<ffff000008b86270>] v4l2_async_test_notify+0x58/0x160
[<ffff000008b86130>] v4l2_async_notifier_register+0xf0/0x1d8
[<ffff000008bcd39c>] rcar_vin_probe+0x65c/0x718
[<ffff0000087b9848>] platform_drv_probe+0x58/0xb8
[<ffff0000087b8014>] driver_probe_device+0x22c/0x2d8
[<ffff0000087b817c>] __driver_attach+0xbc/0xc0
[<ffff0000087b622c>] bus_for_each_dev+0x4c/0x98
[<ffff0000087b82b8>] driver_attach+0x20/0x28
[<ffff0000087b6c98>] bus_add_driver+0x1b8/0x228
[<ffff0000087b8b80>] driver_register+0x60/0xf8
[<ffff0000087ba550>] __platform_driver_register+0x40/0x48
[<ffff000009482830>] rcar_vin_driver_init+0x18/0x20
[<ffff000009440c5c>] do_one_initcall+0x80/0x110
[<ffff000009440e74>] kernel_init_freeable+0x188/0x224
[<ffff000008ee8ab8>] kernel_init+0x10/0x100
[<ffff0000080836c0>] ret_from_fork+0x10/0x50

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 <niklas.soderlund+renesas@ragnatech.se>
> ---
>  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@linux.intel.com

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 <niklas.soderlund+renesas-1zkq55x86MTxsAP9Fp7wbw@public.gmane.org>
> > ---
> >  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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org

-- 
Regards,
Niklas Söderlund
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
<niklas.soderlund+renesas@ragnatech.se> 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 <niklas.soderlund+renesas@ragnatech.se>
> ---
>  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 -- geert@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()

[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 <niklas.soderlund+renesas@ragnatech.se>
>>> ---
>>>  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@linux.intel.com
>


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

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 <niklas.soderlund+renesas@ragnatech.se>
> > > > ---
> > > >  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@linux.intel.com
> > 
> 
> 
> -- 
> Sakari Ailus
> sakari.ailus@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]

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

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@ragnatech.se>
>>>>> ---
>>>>>  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@linux.intel.com
>>>
>>
>>
>> --
>> Sakari Ailus
>> sakari.ailus@linux.intel.com
>


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