[PATCH] of: resolver: Add missing of_node_put

[PATCH] of: resolver: Add missing of_node_put

[Amitoj Kaur Chawla]

for_each_child_of_node performs an of_node_get on each iteration, so
to break out of the loop an of_node_put is required.

Found using Coccinelle. The semantic patch used for this is as follows:

// <smpl>
@@
expression e;
local idexpression n;
@@

 for_each_child_of_node(..., n) {
   ... when != of_node_put(n)
       when != e = n
(
   return n;
|
+  of_node_put(n);
?  return ...;
)
   ...
 }
// </smpl

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/of/resolver.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 640eb4c..e2a0143 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
 
 	for_each_child_of_node(node, child) {
 		found = __of_find_node_by_full_name(child, full_name);
-		if (found != NULL)
+		if (found != NULL) {
+			of_node_put(child);
 			return found;
+		}
 	}
 
 	return NULL;
-- 
1.9.1

Re: [PATCH] of: resolver: Add missing of_node_put

[Mark Rutland]

On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> for_each_child_of_node performs an of_node_get on each iteration, so
> to break out of the loop an of_node_put is required.
> 
> Found using Coccinelle. The semantic patch used for this is as follows:
> 
> // <smpl>
> @@
> expression e;
> local idexpression n;
> @@
> 
>  for_each_child_of_node(..., n) {
>    ... when != of_node_put(n)
>        when != e = n
> (
>    return n;
> |
> +  of_node_put(n);
> ?  return ...;
> )
>    ...
>  }
> // </smpl
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> ---
>  drivers/of/resolver.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 640eb4c..e2a0143 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c
> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
>  
>  	for_each_child_of_node(node, child) {
>  		found = __of_find_node_by_full_name(child, full_name);
> -		if (found != NULL)
> +		if (found != NULL) {
> +			of_node_put(child);
>  			return found;
> +		}
>  	}
>  
>  	return NULL;

I don't think this is quite right. When child == found, this change will
leave it decremented.

Thanks,
Mark.

Re: [PATCH] of: resolver: Add missing of_node_put

[Pantelis Antoniou]

Hi Mark,

> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
>> for_each_child_of_node performs an of_node_get on each iteration, so
>> to break out of the loop an of_node_put is required.
>> 
>> Found using Coccinelle. The semantic patch used for this is as follows:
>> 
>> // <smpl>
>> @@
>> expression e;
>> local idexpression n;
>> @@
>> 
>> for_each_child_of_node(..., n) {
>>   ... when != of_node_put(n)
>>       when != e = n
>> (
>>   return n;
>> |
>> +  of_node_put(n);
>> ?  return ...;
>> )
>>   ...
>> }
>> // </smpl
>> 
>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> ---
>> drivers/of/resolver.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> index 640eb4c..e2a0143 100644
>> --- a/drivers/of/resolver.c
>> +++ b/drivers/of/resolver.c
>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
>> 
>> 	for_each_child_of_node(node, child) {
>> 		found = __of_find_node_by_full_name(child, full_name);
>> -		if (found != NULL)
>> +		if (found != NULL) {
>> +			of_node_put(child);
>> 			return found;
>> +		}
>> 	}
>> 
>> 	return NULL;
> 
> I don't think this is quite right. When child == found, this change will
> leave it decremented.
> 


This patch is bogus. 

__of_find_node_by_full_name() is not taking a reference on the node if found. 
This method relies on keeping the reference taken by the loop.

Taking this into account all of these conccinelle tests are bogus.

The DT internal method are not using the object model in an obvious manner
and applying these patches without vetting each and everyone is bound to
break things.

Regards

— Pantelis


> Thanks,
> Mark.

Re: [PATCH] of: resolver: Add missing of_node_put

[Mark Rutland]

On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
> Hi Mark,
> 
> > On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> >> for_each_child_of_node performs an of_node_get on each iteration, so
> >> to break out of the loop an of_node_put is required.
> >> 
> >> Found using Coccinelle. The semantic patch used for this is as follows:
> >> 
> >> // <smpl>
> >> @@
> >> expression e;
> >> local idexpression n;
> >> @@
> >> 
> >> for_each_child_of_node(..., n) {
> >>   ... when != of_node_put(n)
> >>       when != e = n
> >> (
> >>   return n;
> >> |
> >> +  of_node_put(n);
> >> ?  return ...;
> >> )
> >>   ...
> >> }
> >> // </smpl
> >> 
> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >> ---
> >> drivers/of/resolver.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> >> index 640eb4c..e2a0143 100644
> >> --- a/drivers/of/resolver.c
> >> +++ b/drivers/of/resolver.c
> >> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
> >> 
> >> 	for_each_child_of_node(node, child) {
> >> 		found = __of_find_node_by_full_name(child, full_name);
> >> -		if (found != NULL)
> >> +		if (found != NULL) {
> >> +			of_node_put(child);
> >> 			return found;
> >> +		}
> >> 	}
> >> 
> >> 	return NULL;
> > 
> > I don't think this is quite right. When child == found, this change will
> > leave it decremented.
> > 
> 
> 
> This patch is bogus. 
> 
> __of_find_node_by_full_name() is not taking a reference on the node if found. 
> This method relies on keeping the reference taken by the loop.

Sure. For the found node, that makes sense.

However, it also increments the refcount of all the parents, which does
not seem correct to me, given they're not put on the return path, and a
put of the found node won't decrement its parents refcounts, unless I
have missed something.

So I believe we are missing some of_node_put logic here.

> Taking this into account all of these conccinelle tests are bogus.
> 
> The DT internal method are not using the object model in an obvious manner
> and applying these patches without vetting each and everyone is bound to
> break things.

Agreed.

Thanks,
Mark.

Re: [PATCH] of: resolver: Add missing of_node_put

[Pantelis Antoniou]

Hi Mark,

> On Jan 27, 2016, at 18:21 , Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
>> Hi Mark,
>> 
>>> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
>>> 
>>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
>>>> for_each_child_of_node performs an of_node_get on each iteration, so
>>>> to break out of the loop an of_node_put is required.
>>>> 
>>>> Found using Coccinelle. The semantic patch used for this is as follows:
>>>> 
>>>> // <smpl>
>>>> @@
>>>> expression e;
>>>> local idexpression n;
>>>> @@
>>>> 
>>>> for_each_child_of_node(..., n) {
>>>>  ... when != of_node_put(n)
>>>>      when != e = n
>>>> (
>>>>  return n;
>>>> |
>>>> +  of_node_put(n);
>>>> ?  return ...;
>>>> )
>>>>  ...
>>>> }
>>>> // </smpl
>>>> 
>>>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>>>> ---
>>>> drivers/of/resolver.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>>>> index 640eb4c..e2a0143 100644
>>>> --- a/drivers/of/resolver.c
>>>> +++ b/drivers/of/resolver.c
>>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
>>>> 
>>>> 	for_each_child_of_node(node, child) {
>>>> 		found = __of_find_node_by_full_name(child, full_name);
>>>> -		if (found != NULL)
>>>> +		if (found != NULL) {
>>>> +			of_node_put(child);
>>>> 			return found;
>>>> +		}
>>>> 	}
>>>> 
>>>> 	return NULL;
>>> 
>>> I don't think this is quite right. When child == found, this change will
>>> leave it decremented.
>>> 
>> 
>> 
>> This patch is bogus. 
>> 
>> __of_find_node_by_full_name() is not taking a reference on the node if found. 
>> This method relies on keeping the reference taken by the loop.
> 
> Sure. For the found node, that makes sense.
> 
> However, it also increments the refcount of all the parents, which does
> not seem correct to me, given they're not put on the return path, and a
> put of the found node won't decrement its parents refcounts, unless I
> have missed something.
> 

Hmm, yes. The parent refcounts must be decremented. 

> So I believe we are missing some of_node_put logic here.
> 
>> Taking this into account all of these conccinelle tests are bogus.
>> 
>> The DT internal method are not using the object model in an obvious manner
>> and applying these patches without vetting each and everyone is bound to
>> break things.
> 
> Agreed.
> 
> Thanks,
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: resolver: Add missing of_node_put

[Julia Lawall]

On Wed, 27 Jan 2016, Pantelis Antoniou wrote:

> Hi Mark,
> 
> > On Jan 27, 2016, at 18:21 , Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
> >> Hi Mark,
> >> 
> >>> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
> >>> 
> >>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> >>>> for_each_child_of_node performs an of_node_get on each iteration, so
> >>>> to break out of the loop an of_node_put is required.
> >>>> 
> >>>> Found using Coccinelle. The semantic patch used for this is as follows:
> >>>> 
> >>>> // <smpl>
> >>>> @@
> >>>> expression e;
> >>>> local idexpression n;
> >>>> @@
> >>>> 
> >>>> for_each_child_of_node(..., n) {
> >>>>  ... when != of_node_put(n)
> >>>>      when != e = n
> >>>> (
> >>>>  return n;
> >>>> |
> >>>> +  of_node_put(n);
> >>>> ?  return ...;
> >>>> )
> >>>>  ...
> >>>> }
> >>>> // </smpl
> >>>> 
> >>>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >>>> ---
> >>>> drivers/of/resolver.c | 4 +++-
> >>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>> 
> >>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> >>>> index 640eb4c..e2a0143 100644
> >>>> --- a/drivers/of/resolver.c
> >>>> +++ b/drivers/of/resolver.c
> >>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
> >>>> 
> >>>> 	for_each_child_of_node(node, child) {
> >>>> 		found = __of_find_node_by_full_name(child, full_name);
> >>>> -		if (found != NULL)
> >>>> +		if (found != NULL) {
> >>>> +			of_node_put(child);
> >>>> 			return found;
> >>>> +		}
> >>>> 	}
> >>>> 
> >>>> 	return NULL;
> >>> 
> >>> I don't think this is quite right. When child == found, this change will
> >>> leave it decremented.
> >>> 
> >> 
> >> 
> >> This patch is bogus. 
> >> 
> >> __of_find_node_by_full_name() is not taking a reference on the node if found. 
> >> This method relies on keeping the reference taken by the loop.
> > 
> > Sure. For the found node, that makes sense.
> > 
> > However, it also increments the refcount of all the parents, which does
> > not seem correct to me, given they're not put on the return path, and a
> > put of the found node won't decrement its parents refcounts, unless I
> > have missed something.
> > 
> 
> Hmm, yes. The parent refcounts must be decremented. 

So there should be if (found != child) of_node_put(child); ?  Another 
option would be to duplicate the test and avoid the recursive call.

julia

> > So I believe we are missing some of_node_put logic here.
> > 
> >> Taking this into account all of these conccinelle tests are bogus.
> >> 
> >> The DT internal method are not using the object model in an obvious manner
> >> and applying these patches without vetting each and everyone is bound to
> >> break things.
> > 
> > Agreed.
> > 
> > Thanks,
> > Mark.
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] of: resolver: Add missing of_node_put

[Mark Rutland]

On Wed, Jan 27, 2016 at 08:48:00PM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 27 Jan 2016, Pantelis Antoniou wrote:
> 
> > Hi Mark,
> > 
> > > On Jan 27, 2016, at 18:21 , Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > > On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
> > >> Hi Mark,
> > >> 
> > >>> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
> > >>> 
> > >>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> > >>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> > >>>> index 640eb4c..e2a0143 100644
> > >>>> --- a/drivers/of/resolver.c
> > >>>> +++ b/drivers/of/resolver.c
> > >>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
> > >>>> 
> > >>>> 	for_each_child_of_node(node, child) {
> > >>>> 		found = __of_find_node_by_full_name(child, full_name);
> > >>>> -		if (found != NULL)
> > >>>> +		if (found != NULL) {
> > >>>> +			of_node_put(child);
> > >>>> 			return found;
> > >>>> +		}
> > >>>> 	}
> > >>>> 
> > >>>> 	return NULL;
> > >>> 
> > >>> I don't think this is quite right. When child == found, this change will
> > >>> leave it decremented.
> > >>> 
> > >> 
> > >> 
> > >> This patch is bogus. 
> > >> 
> > >> __of_find_node_by_full_name() is not taking a reference on the node if found. 
> > >> This method relies on keeping the reference taken by the loop.
> > > 
> > > Sure. For the found node, that makes sense.
> > > 
> > > However, it also increments the refcount of all the parents, which does
> > > not seem correct to me, given they're not put on the return path, and a
> > > put of the found node won't decrement its parents refcounts, unless I
> > > have missed something.
> > > 
> > 
> > Hmm, yes. The parent refcounts must be decremented. 
> 
> So there should be if (found != child) of_node_put(child); ? 

That would match the intended semantics, yes.

Thanks,
Mark.

Re: [PATCH] of: resolver: Add missing of_node_put

[Julia Lawall]

On Thu, 28 Jan 2016, Mark Rutland wrote:

> On Wed, Jan 27, 2016 at 08:48:00PM +0100, Julia Lawall wrote:
> >
> >
> > On Wed, 27 Jan 2016, Pantelis Antoniou wrote:
> >
> > > Hi Mark,
> > >
> > > > On Jan 27, 2016, at 18:21 , Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
> > > >> Hi Mark,
> > > >>
> > > >>> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
> > > >>>
> > > >>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> > > >>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> > > >>>> index 640eb4c..e2a0143 100644
> > > >>>> --- a/drivers/of/resolver.c
> > > >>>> +++ b/drivers/of/resolver.c
> > > >>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
> > > >>>>
> > > >>>> 	for_each_child_of_node(node, child) {
> > > >>>> 		found = __of_find_node_by_full_name(child, full_name);
> > > >>>> -		if (found != NULL)
> > > >>>> +		if (found != NULL) {
> > > >>>> +			of_node_put(child);
> > > >>>> 			return found;
> > > >>>> +		}
> > > >>>> 	}
> > > >>>>
> > > >>>> 	return NULL;
> > > >>>
> > > >>> I don't think this is quite right. When child == found, this change will
> > > >>> leave it decremented.
> > > >>>
> > > >>
> > > >>
> > > >> This patch is bogus.
> > > >>
> > > >> __of_find_node_by_full_name() is not taking a reference on the node if found.
> > > >> This method relies on keeping the reference taken by the loop.
> > > >
> > > > Sure. For the found node, that makes sense.
> > > >
> > > > However, it also increments the refcount of all the parents, which does
> > > > not seem correct to me, given they're not put on the return path, and a
> > > > put of the found node won't decrement its parents refcounts, unless I
> > > > have missed something.
> > > >
> > >
> > > Hmm, yes. The parent refcounts must be decremented.
> >
> > So there should be if (found != child) of_node_put(child); ?
>
> That would match the intended semantics, yes.

I don't think so.  I sent another mail with what seems like a better
solution (upping the reference count of the child that is selected).

julia

Re: [PATCH] of: resolver: Add missing of_node_put

[Julia Lawall]

On Wed, 27 Jan 2016, Pantelis Antoniou wrote:

> Hi Mark,
> 
> > On Jan 27, 2016, at 18:21 , Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
> >> Hi Mark,
> >> 
> >>> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
> >>> 
> >>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> >>>> for_each_child_of_node performs an of_node_get on each iteration, so
> >>>> to break out of the loop an of_node_put is required.
> >>>> 
> >>>> Found using Coccinelle. The semantic patch used for this is as follows:
> >>>> 
> >>>> // <smpl>
> >>>> @@
> >>>> expression e;
> >>>> local idexpression n;
> >>>> @@
> >>>> 
> >>>> for_each_child_of_node(..., n) {
> >>>>  ... when != of_node_put(n)
> >>>>      when != e = n
> >>>> (
> >>>>  return n;
> >>>> |
> >>>> +  of_node_put(n);
> >>>> ?  return ...;
> >>>> )
> >>>>  ...
> >>>> }
> >>>> // </smpl
> >>>> 
> >>>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >>>> ---
> >>>> drivers/of/resolver.c | 4 +++-
> >>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>> 
> >>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> >>>> index 640eb4c..e2a0143 100644
> >>>> --- a/drivers/of/resolver.c
> >>>> +++ b/drivers/of/resolver.c
> >>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
> >>>> 
> >>>> 	for_each_child_of_node(node, child) {
> >>>> 		found = __of_find_node_by_full_name(child, full_name);
> >>>> -		if (found != NULL)
> >>>> +		if (found != NULL) {
> >>>> +			of_node_put(child);
> >>>> 			return found;
> >>>> +		}
> >>>> 	}
> >>>> 
> >>>> 	return NULL;
> >>> 
> >>> I don't think this is quite right. When child == found, this change will
> >>> leave it decremented.
> >>> 
> >> 
> >> 
> >> This patch is bogus. 
> >> 
> >> __of_find_node_by_full_name() is not taking a reference on the node if found. 
> >> This method relies on keeping the reference taken by the loop.
> > 
> > Sure. For the found node, that makes sense.
> > 
> > However, it also increments the refcount of all the parents, which does
> > not seem correct to me, given they're not put on the return path, and a
> > put of the found node won't decrement its parents refcounts, unless I
> > have missed something.
> > 
> 
> Hmm, yes. The parent refcounts must be decremented. 

The code is very strange.  The function is called at only one place:

                refnode = __of_find_node_by_full_name(node, nodestr);
                if (!refnode) {
                        pr_warn("%s: Could not find refnode '%s'\n",
                                __func__, (char *)rprop->value);
                        continue;
                }

                /* now find the property */
                for_each_property_of_node(refnode, sprop) {
                        if (of_prop_cmp(sprop->name, propstr) == 0)
                                break;
	        }

This is in the function __of_adjust_phandle_ref.  These are the only 
references to refnode.  That is, there is no of_node_put on refnode.  So 
if __of_find_node_by_full_name ups the reference count, there will be a 
memory leak.  Furthermore, the invariant on __of_find_node_by_full_name is 
quite strange, because if the of_node_cmp succeeds immediately, the ref 
count of the returned value will be what it was on the way into the 
function, while if it does not succeed the ref count of the returned value 
will be one greater than what it was on the way into the function.

Note also that there is similar refnode code in the function 
of_resolve_phandles:

                refnode = of_find_node_by_path(refpath);
                if (!refnode) {
	                pr_err("%s: Could not find node by path '%s'\n",
	                                __func__, refpath);
                        err = -ENOENT;
                        goto out;
		}

                phandle = refnode->phandle;
                of_node_put(refnode);


There there is a put of the obtained value.

So it seems that the patch as is is going in the right direction.  It is 
just necessary to also add an of_node_get in the following code:

        if (of_node_cmp(node->full_name, full_name) == 0)
                return node;

and to add an of_code_put after the above refnode code.

julia

Re: [PATCH] of: resolver: Add missing of_node_put

[Rob Herring]

On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
> Hi Mark,
> 
> > On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> >> for_each_child_of_node performs an of_node_get on each iteration, so
> >> to break out of the loop an of_node_put is required.
> >> 
> >> Found using Coccinelle. The semantic patch used for this is as follows:
> >> 
> >> // <smpl>
> >> @@
> >> expression e;
> >> local idexpression n;
> >> @@
> >> 
> >> for_each_child_of_node(..., n) {
> >>   ... when != of_node_put(n)
> >>       when != e = n
> >> (
> >>   return n;
> >> |
> >> +  of_node_put(n);
> >> ?  return ...;
> >> )
> >>   ...
> >> }
> >> // </smpl
> >> 
> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >> ---
> >> drivers/of/resolver.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> >> index 640eb4c..e2a0143 100644
> >> --- a/drivers/of/resolver.c
> >> +++ b/drivers/of/resolver.c
> >> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
> >> 
> >> 	for_each_child_of_node(node, child) {
> >> 		found = __of_find_node_by_full_name(child, full_name);
> >> -		if (found != NULL)
> >> +		if (found != NULL) {
> >> +			of_node_put(child);
> >> 			return found;
> >> +		}
> >> 	}
> >> 
> >> 	return NULL;
> > 
> > I don't think this is quite right. When child == found, this change will
> > leave it decremented.
> > 
> 
> 
> This patch is bogus. 
> 
> __of_find_node_by_full_name() is not taking a reference on the node if found. 
> This method relies on keeping the reference taken by the loop.
> 
> Taking this into account all of these conccinelle tests are bogus.
> 
> The DT internal method are not using the object model in an obvious manner
> and applying these patches without vetting each and everyone is bound to
> break things.

Things are already broken. But does it matter?

Our time would be better spent re-designing any refcounting around where 
we actually need it rather than trying to fix up the many locations 
which are wrong and don't matter. As long as it is callers' 
responsibility to get this right, it will never be right. Even the core 
code has a hard time getting it right.

Rob

Re: [PATCH] of: resolver: Add missing of_node_put

[Pantelis Antoniou]

Hi Rob,

> On Jan 29, 2016, at 18:45 , Rob Herring <robh@kernel.org> wrote:
> 
> On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
>> Hi Mark,
>> 
>>> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
>>> 
>>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
>>>> for_each_child_of_node performs an of_node_get on each iteration, so
>>>> to break out of the loop an of_node_put is required.
>>>> 
>>>> Found using Coccinelle. The semantic patch used for this is as follows:
>>>> 
>>>> // <smpl>
>>>> @@
>>>> expression e;
>>>> local idexpression n;
>>>> @@
>>>> 
>>>> for_each_child_of_node(..., n) {
>>>>  ... when != of_node_put(n)
>>>>      when != e = n
>>>> (
>>>>  return n;
>>>> |
>>>> +  of_node_put(n);
>>>> ?  return ...;
>>>> )
>>>>  ...
>>>> }
>>>> // </smpl
>>>> 
>>>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>>>> ---
>>>> drivers/of/resolver.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>>>> index 640eb4c..e2a0143 100644
>>>> --- a/drivers/of/resolver.c
>>>> +++ b/drivers/of/resolver.c
>>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
>>>> 
>>>> 	for_each_child_of_node(node, child) {
>>>> 		found = __of_find_node_by_full_name(child, full_name);
>>>> -		if (found != NULL)
>>>> +		if (found != NULL) {
>>>> +			of_node_put(child);
>>>> 			return found;
>>>> +		}
>>>> 	}
>>>> 
>>>> 	return NULL;
>>> 
>>> I don't think this is quite right. When child == found, this change will
>>> leave it decremented.
>>> 
>> 
>> 
>> This patch is bogus. 
>> 
>> __of_find_node_by_full_name() is not taking a reference on the node if found. 
>> This method relies on keeping the reference taken by the loop.
>> 
>> Taking this into account all of these conccinelle tests are bogus.
>> 
>> The DT internal method are not using the object model in an obvious manner
>> and applying these patches without vetting each and everyone is bound to
>> break things.
> 
> Things are already broken. But does it matter?
> 
> Our time would be better spent re-designing any refcounting around where 
> we actually need it rather than trying to fix up the many locations 
> which are wrong and don't matter. As long as it is callers' 
> responsibility to get this right, it will never be right. Even the core 
> code has a hard time getting it right.
> 

Let me pile up. Refcounting for DT is broken. There’s no point trying to fix
it as it is. I have a big pile of TODO, one of these is fixing (as in severely
cutting down) the areas where refcounting is needed.

The idea would be to keep refcounting only in core and provide interfaces that
use different semantics for drivers and subsystems.

We can discuss things in ELC this April, perhaps on a BoF session again.


> Rob

Regards

— Pantelis

Re: [PATCH] of: resolver: Add missing of_node_put

[Frank Rowand]

On 1/29/2016 9:33 AM, Pantelis Antoniou wrote:
> Hi Rob,
> 
>> On Jan 29, 2016, at 18:45 , Rob Herring <robh@kernel.org> wrote:
>>
>> On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
>>> Hi Mark,
>>>
>>>> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
>>>>
>>>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
>>>>> for_each_child_of_node performs an of_node_get on each iteration, so
>>>>> to break out of the loop an of_node_put is required.
>>>>>
>>>>> Found using Coccinelle. The semantic patch used for this is as follows:
>>>>>
>>>>> // <smpl>
>>>>> @@
>>>>> expression e;
>>>>> local idexpression n;
>>>>> @@
>>>>>
>>>>> for_each_child_of_node(..., n) {
>>>>>  ... when != of_node_put(n)
>>>>>      when != e = n
>>>>> (
>>>>>  return n;
>>>>> |
>>>>> +  of_node_put(n);
>>>>> ?  return ...;
>>>>> )
>>>>>  ...
>>>>> }
>>>>> // </smpl
>>>>>
>>>>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>>>>> ---
>>>>> drivers/of/resolver.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>>>>> index 640eb4c..e2a0143 100644
>>>>> --- a/drivers/of/resolver.c
>>>>> +++ b/drivers/of/resolver.c
>>>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
>>>>>
>>>>> 	for_each_child_of_node(node, child) {
>>>>> 		found = __of_find_node_by_full_name(child, full_name);
>>>>> -		if (found != NULL)
>>>>> +		if (found != NULL) {
>>>>> +			of_node_put(child);
>>>>> 			return found;
>>>>> +		}
>>>>> 	}
>>>>>
>>>>> 	return NULL;
>>>>
>>>> I don't think this is quite right. When child == found, this change will
>>>> leave it decremented.
>>>>
>>>
>>>
>>> This patch is bogus. 
>>>
>>> __of_find_node_by_full_name() is not taking a reference on the node if found. 
>>> This method relies on keeping the reference taken by the loop.
>>>
>>> Taking this into account all of these conccinelle tests are bogus.
>>>
>>> The DT internal method are not using the object model in an obvious manner
>>> and applying these patches without vetting each and everyone is bound to
>>> break things.
>>
>> Things are already broken. But does it matter?
>>
>> Our time would be better spent re-designing any refcounting around where 
>> we actually need it rather than trying to fix up the many locations 
>> which are wrong and don't matter. As long as it is callers' 
>> responsibility to get this right, it will never be right. Even the core 
>> code has a hard time getting it right.
>>
> 
> Let me pile up. Refcounting for DT is broken. There’s no point trying to fix
> it as it is. I have a big pile of TODO, one of these is fixing (as in severely
> cutting down) the areas where refcounting is needed.

May as well violently agree.

An additional way that DT refcounting is architecturally broken is the concept
of using a held refcount as a lock substitute while traversing a linked list.
Fixing this is on my todo list, hopefully late this winter or early spring.

> 
> The idea would be to keep refcounting only in core and provide interfaces that
> use different semantics for drivers and subsystems.
> 
> We can discuss things in ELC this April, perhaps on a BoF session again.

Yes, all interested people please come discuss things with us.  I have
submitted a BoF proposal.

-Frank

> 
> 
>> Rob
> 
> Regards
> 
> — Pantelis
> 
>