[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <56ABF9E8.2080609@gmail.com>
Date: Fri, 29 Jan 2016 15:46:48 -0800
From: Frank Rowand <frowand.list@...il.com>
To: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
CC: Rob Herring <robh@...nel.org>, Mark Rutland <mark.rutland@....com>,
Amitoj Kaur Chawla <amitoj1606@...il.com>,
Grant Likely <grant.likely@...aro.org>,
Devicetree List <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
julia.lawall@...6.fr
Subject: Re: [PATCH] of: resolver: Add missing of_node_put
On 1/29/2016 9:33 AM, Pantelis Antoniou wrote:
> Hi Rob,
>
>> On Jan 29, 2016, at 18:45 , Rob Herring <robh@...nel.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@....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@...il.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
>
>
Powered by blists - more mailing lists