Re: [PATCH 5/5] OF: Introduce utility helper functions

[Pantelis Antoniou]

Hi Grant,

On Nov 14, 2013, at 2:44 AM, Grant Likely wrote:

> On Wed, 13 Nov 2013 10:03:37 +0100, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>> On Nov 13, 2013, at 2:34 AM, Grant Likely wrote:
>>> On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>>>>> On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>>>>>> +	} else {
>>>>>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
>>>>>> +				__func__, node);
>>>>>> +	}
>>>>>> +}
>>>>> 
>>>>> All of the above is potentially dangerous. There is no way to determine
>>>>> if anything still holds a reference to a node. The proper way to handle
>>>>> removal of properties is to have a release method when the last
>>>>> of_node_put is called.
>>>>> 
>>>> 
>>>> This is safe, and expected to be called only on a dynamically created tree,
>>>> that's what all the checks against OF_DYNAMIC guard against.
>>>> 
>>>> It is not ever meant to be called on an arbitrary tree, created by unflattening
>>>> a blob.
>>> 
>>> I am talking about when being used on a dynamic tree. The problem is
>>> when a driver or other code holds a reference to a dynamic nodes, but
>>> doesn't release it correctly. The memory must not be freed until all of
>>> the references are relased. OF_DYNAMIC doesn't actually help in that
>>> case, and it is the reason for of_node_get()/of_node_put()
>>> 
>> 
>> I know, but even that is not enough. of_node_get()/of_node_put() handles the
>> case of references to the nodes, but not what happens with references to
>> properties. deadprops is mitigating the problem somewhat, but if we're going
>> to go to all the trouble of kobjectification let's do the props as well.
>> 
>> of_get_property could be modified to return a devm_kmalloced copy of the real
>> property and that would deal with most of the callers. Of course for
>> the small sized scalar data we can avoid the copy.
>> 
>> By using the devm_* interface we also avoid having to mess too much with the callers.
>> 
>> I.e. what about something like devm_of_get_property()?
> 
> Reference counting is already a horrible pain to keep correct. I don't
> see a better way to handle it in the dynamic case, so we're stuck with
> it, but I don't want to make it any harder. Adding ref counting to
> properties will make it harder than it already is to get the code right.
> I'm absolutely fine with a little bit of wasted memory in the form of
> deadprops when the alternative is so horrible. References at the node
> level is enough granularity.
> 
> I don't think kduping the property is the solution either. I strongly
> suspect that will be far more expensive than the deadprop solution.
> 

As long as we can live with deadprops all is fine. Perhaps a devm_of_get_property()
makes sense for new drivers though? What do you think? Perhaps copying to a
user supplied buffer as well?

>>>>>> +{
>>>>>> +	struct device_node *node;
>>>>>> +
>>>>>> +	node = kzalloc(sizeof(*node), flags);
>>>>>> +	if (node == NULL)
>>>>>> +		return NULL;
>>>>>> +
>>>>>> +	node->name = kstrdup(name, flags);
>>>>>> +	if (node->name == NULL)
>>>>>> +		goto err_return;
>>>>>> +
>>>>>> +	node->type = kstrdup(type, flags);
>>>>>> +	if (node->type == NULL)
>>>>>> +		goto err_return;
>>>>>> +
>>>>>> +	node->full_name = kstrdup(full_name, flags);
>>>>>> +	if (node->type == NULL)
>>>>>> +		goto err_return;
>>>>> 
>>>>> Again, who do you expect the user of this function to be? If it is part
>>>>> of unflattening an overlay tree, is there a reason that the passed in
>>>>> names cannot be used directly instead of kmallocing them?
>>>>> 
>>>> 
>>>> I want to be able to get rid of the blob eventually; I don't need to keep
>>>> dragging it around.
>>> 
>>> Why? It really doesn't hurt and it means data does not need to be
>>> copied.
>> 
>> Copying data lead to less problems that having to drag that blob around. 
>> That's just preference, so not a big issue.
> 
> Can you elaborate? What problems do you foresee being created by keeping
> the blob?

It's a kind of drag. That means you get handed a device_node pointer you are not
able to free it without having the blob along with the container/accessor of it.
I.e. For the normal case where the blob comes from a request_firmware() call
You have to keep the firmware structure around.

Depending on what other method you're going to use tends to make the code a little
bit messier. 

> 
> g.

Regards

-- Pantelis

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/