Re: [PATCH] of: fix of_update_property()

[Pantelis Antoniou]

Hi Rob,


On Jan 17, 2014, at 4:49 PM, Rob Herring wrote:

> On Thu, Jan 16, 2014 at 10:46 PM, Xiubo Li <Li.Xiubo@xxxxxxxxxxxxx> wrote:
>> The of_update_property() is intent to update a property in a node
> 
[ snip ]

>>                return of_add_property(np, newprop);
> 
> Isn't there also a race that if you do 2 updates for a non-existent
> property and both threads try to add the property, the first one will
> succeed and the 2nd will fail. The 2nd one needs to retry as well.
> 
> Also, couldn't the node itself be removed while trying to do the update?
> 
> There seem to be multiple problems with this code, but doing multiple
> simultaneous, conflicting updates seems like an unlikely case.
> 

There are multiple problems with this function.

First of all, the firing of the notifier at the beginning with OF_RECONFIG_UPDATE_PROPERTY
even though we have no idea if the property is found.

The locking is no good; the lock should be taken before the search by switching to using
__of_find_property. Threading is just not handled well at all at the moment. 
Retrying is just bogus.

The biggest problem is the semantics; IMHO we should just remove the option to create
a property if it doesn't exist. I don't think there are many callers that use update property
expecting to be created if it doesn't exist.

> Rob
> 
>> @@ -1593,7 +1594,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
>>        raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 
>>        if (!found)
>> -               return -ENODEV;
>> +               goto retry;
>> 
>> #ifdef CONFIG_PROC_DEVICETREE
>>        /* try to add to proc as well if it was initialized */
>> --
>> 1.8.4
>> 
>> 

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/