Hi Grant,
On Nov 14, 2013, at 2:44 AM, Grant Likely wrote:
> On Wed, 13 Nov 2013 10:03:37 +0100, Pantelis Antoniou
> <pa...@antoniou-consulting.com> wrote:
>> On Nov 13, 2013, at 2:34 AM, Grant Likely wrote:
>>> On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou
>>> <pa...@antoniou-consulting.com> wrote:
>>>>> On Tue, 5 Nov 2013 19:50:16 +0200, Pantelis Antoniou
>>>>> <pa...@antoniou-consulting.com> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
