On Fri, Jun 22, 2012 at 8:01 AM, Benjamin Herrenschmidt
[off-list ref] wrote:

On Thu, 2012-06-21 at 17:37 +0800, Dong Aisheng wrote:

quoted

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.

 .../...

quoted

quoted

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

quoted

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.

quoted

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?

quoted

quoted

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.

quoted

quoted

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