Re: [PATCH] of: fix of_update_property()
From: Pantelis Antoniou
Date: Fri Jan 17 2014 - 11:42:57 EST
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.
>> @@ -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 */
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/