> Subject: Re: [PATCH] of: fix of_update_property() > > 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 > > s/intent/indended/ > > > and if the property does not exist, will add it to the node. > > > > The second search of the property is possibly won't be found, that > > maybe removed by other thread just before the second search begain, > > if so just retry it. > > How did you find this problem? Actual use or some artificial stress test? > Some artificial stress test at home. > > Signed-off-by: Xiubo Li <Li.Xiubo@xxxxxxxxxxxxx> > > --- > > drivers/of/base.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/of/base.c b/drivers/of/base.c > > index f807d0e..d0c53bc 100644 > > --- a/drivers/of/base.c > > +++ b/drivers/of/base.c > > @@ -1572,6 +1572,7 @@ int of_update_property(struct device_node *np, struct > property *newprop) > > if (!newprop->name) > > return -EINVAL; > > > > +retry: > > oldprop = of_find_property(np, newprop->name, NULL); > > if (!oldprop) > > 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. > Well, yes, that will happen. Maybe we could add one __of_add_property() without any locks, like __of_find_property(). And then in of_update_prperty() move the searching and adding operations to between lock and unlock, like: raw_spin_lock_irqsave(); oldprop = __of_find_property(); if (!oldprop) { rc = __of_add_property(np, newprop); ... } ... replace the node... ... raw_spin_unlock_irqrestore(); > Also, couldn't the node itself be removed while trying to do the update? > For this is between the lock operations. I think this doesn't matter here. > There seem to be multiple problems with this code, but doing multiple > simultaneous, conflicting updates seems like an unlikely case. > Yes, but this will happen in theory. Thanks, Best Regards, Xiubo -- 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