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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
