Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1321887 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] of: resolver: Add missing of_node_put Rob Herring <robh@kernel.org> - 2016-01-29 17:50 +0100
    Re: [PATCH] of: resolver: Add missing of_node_put Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-01-29 18:40 +0100
      Re: [PATCH] of: resolver: Add missing of_node_put Frank Rowand <frowand.list@gmail.com> - 2016-01-30 00:50 +0100

#1321887 — Re: [PATCH] of: resolver: Add missing of_node_put

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

[toc] | [next] | [standalone]

#1321907

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

[toc] | [prev] | [next] | [standalone]

#1322184

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web