Re: [PATCH 1/6] of: Do not free memory at of_node_release

[Pantelis Antoniou <pantelis.antoniou@konsulko.com>]

Hi Guenter,

On Jun 24, 2014, at 11:33 PM, Guenter Roeck wrote:

> On Tue, Jun 24, 2014 at 11:23:35PM +0300, Pantelis Antoniou wrote:
>> Hi Grant,
>> 
>> On Jun 24, 2014, at 11:21 PM, Grant Likely wrote:
>> 
>>> On Tue, 24 Jun 2014 17:23:35 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>>>> Hi Grant,
>>>> 
>>>> On Jun 24, 2014, at 5:10 PM, Grant Likely wrote:
>>>> 
>>>>> On Sun, 22 Jun 2014 12:40:34 +0300, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>>>>>> The life-cycle of nodes and properties does not allow us to release
>>>>>> the memory taken by a device_node. Pointer to properties and nodes
>>>>>> might still be in use in drivers, so any memory free'ing is dangerous.
>>>>>> 
>>>>>> Simply move all the properties to the deadprops list, and the node
>>>>>> itself to of_alldeadnodes until the life-cycles issues are resolved.
>>>>> 
>>>>> Ummm. this looks wrong. The release function is supposed to be the place
>>>>> to do the freeing, and with our discussion the other day about moving to
>>>>> rcu, but keeping of_node_get/put() for anything that needs to hold a
>>>>> long term reference, that means the lifecycle issues are pretty much
>>>>> resolved. I don't think this patch is necessary.
>>>>> 
>>>> 
>>>> Well, I thought about it too. This is the culmination of that process :)
>>>> 
>>>> The problem is not the node life-cycle, it's the properties. We can't
>>>> tell for sure who and where has a pointer to the properties of the node,
>>>> and when we free the node, the current code iterates over the properties
>>>> list and frees them. This works only because in my tests, of_node_release
>>>> only gets called when using overlays created nodes.
>>> 
>>> If a caller want's to use the value of a property, then it needs to
>>> of_node_get() the node. Property lifecycle is strictly attached to the
>>> lifecycle of it's node.
>>> 
>> 
>> Taking bets if that's always the case?
>> 
> For my part I prefer to take the bet (and handle the consequences) over
> a potential memory leak.
> 

It's your call really. But let me just demonstrate why this is harder than it appears,
with a small example.

The of_node_get() method is not invoked for instance when reading string properties.

The of_property_read_string() method will return a pointer to the string property value.
That means that potentially all drivers that call of_property_read_string() will have to
be converted to taking a reference of the node using of_node_get(), or the of_property_read_string()
must do so in the background.

Also note that the release method is never called on non-dynamic node since that memory is
never released.

FWIW, the overlay code until now operated just fine releasing the nodes, but I can't 
guarantee you won't be screwed sometime by an errand driver :)

> Guenter

Regards

-- Pantelis