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

[Frank Rowand <frowand.list@gmail.com>]

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