On 1/29/2016 9:33 AM, Pantelis Antoniou wrote:
> Hi Rob,
>
>> On Jan 29, 2016, at 18:45 , Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxx> 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@xxxxxxxxx>
>>>>> ---
>>>>> 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
