Re: [PATCH 1/1] of: reform prom_update_property function

[Dong Aisheng]

On Fri, Jun 22, 2012 at 8:01 AM, Benjamin Herrenschmidt
<benh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 2012-06-21 at 17:37 +0800, Dong Aisheng wrote:
>> Maybe we could change it as as follows.
>> It looks then the code follow is the same as before.
>> Do you think if it's ok?
>>
>> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
>> index 7b3bf76..4c237f4 100644
>> --- a/arch/powerpc/platforms/pseries/reconfig.c
>> +++ b/arch/powerpc/platforms/pseries/reconfig.c
>> @@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize)
>>         if (!next_prop)
>>                 return -EINVAL;
>>
>> +       if (!strlen(name)
>> +               return -ENODEV;
>> +
>>         newprop = new_property(name, length, value, NULL);
>>         if (!newprop)
>>                 return -ENOMEM;
>> @@ -450,13 +453,6 @@ static int do_update_property(char *buf, size_t bufsize)
>>         if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
>>                 slb_set_size(*(int *)value);
>>
>> -       oldprop = of_find_property(np, name,NULL);
>> -       if (!oldprop) {
>> -               if (strlen(name))
>> -                       return prom_add_property(np, newprop);
>> -               return -ENODEV;
>> -       }
>
> No:
>
> IE. Old code did:
>
>        if (property doesn't exist) {
>                if (has a name)
>                        create_it()
>                return -ENODEV;
>        }
>
What i saw is:
       if (property doesn't exist) {
               if (has a name)
                       return create_it()
               return -ENODEV;
       }
Which seems the same behavior as the new prop_update_property api.
The only different is if no name, return -EINVAL;
Am i wrong?

> What you propose is:
>
>        if (!has_a_name)
>                return -ENODEV;
>
> Not at all the same semantic.
>
>  .../...
>
>> > IE. The allocation of the "old" property isn't disposed of. It can't
>> > because today we don't know whether any of those pointers was
>> > dynamically allocated or not. IE they could point to the fdt
>
>> Hmm, i did not see static allocated property before.
>> Where can we see an exist case?
>
> Almost all your property names and values. They are pointers to the
> original fdt block, so you can't free them. But dynamically added
> propreties will have kmalloc'ed pointers which should be freed. We need
> to add flags to indicate that if we want to avoid leaking memory in very
> dynamic environments.
>
Okay, got it, thanks for clarify.

>> If we really have this issue, it seems of_node_release also has the same issue,
>> since it frees the property without distinguish whether the property is allocated
>> dynamically.
>
> Well, actually we do have a flag:
>
>        if (!of_node_check_flag(node, OF_DYNAMIC))
>                return;
>
Oh, i see.

> So we use that. Good. So if update property uses that flag it should be
> able to know when to free or not. I forgot we had that :-)
>
I'm still not sure whether we should free the property in update
property function.
Looking at the code, it seems prom_update_property actually does not remove it.
It only move the property to "dead properties" list.
See the function comment in kernel:
/**
 * prom_remove_property - Remove a property from a node.
 *
 * Note that we don't actually remove it, since we have given out
 * who-knows-how-many pointers to the data using get-property.
 * Instead we just move the property to the "dead properties"
 * list, so it won't be found any more.
 */

Finally the dead properties will be freed in of_release_node
if the node has no users after calling of_node_put.

static void of_node_release(struct kref *kref)
{
.....
        struct property *prop = node->properties;
.......
        if (!of_node_check_flag(node, OF_DYNAMIC))
                return;

        while (prop) {
                struct property *next = prop->next;
                kfree(prop->name);
                kfree(prop->value);
                kfree(prop);
                prop = next;

                if (!prop) {
                        prop = node->deadprops;
                        node->deadprops = NULL;
                }
        }
        kfree(node->full_name);
        kfree(node->data);
        kfree(node);
}

So it looks to me there's no memory leak,
did i understand wrong?

>> > string list, data block, or could be bootmem ... or could be
>> > actual kernel memory.
>> >
>> > We might want to extend the struct property to contain indications of
>> > the allocation type so we can kfree dynamic properties properly.
>> >
>> I wonder the simplest way may be not allow static allocated property, like dt
>> node does i guess.
>
> No way. We generate the device-tree way before we have an allocator
> available.
>
Oh, got it.

>> > But then there's the question of the lifetime of a property... since
>> > they aren't reference counted like nodes are.
>> >
>> Yes, that's a real exist problem.
>>
>> Anyway, i guess we could do that work of this problem in another patch
>> rather than have to in this patch series.
>
> Cheers,
> Ben.
>
>

Regards
Dong Aisheng
--
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/