[PATCHv3] of: fix of_update_property again

[PATCHv3] of: fix of_update_property again

[Xiubo Li]

The of_update_property() is intented to update a property in a node
and if the property does not exist, will add it.

The second search of the property is possibly won't be found, that
maybe removed by other thread just before the second search begain.

Using the __of_find_property() and __of_add_property() instead and
move them into lock operations.

Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 drivers/of/base.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index be2861d..06623d0c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1767,7 +1767,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
 {
 	struct property **next, *oldprop;
 	unsigned long flags;
-	int rc, found = 0;
+	int rc;
 
 	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
 	if (rc)
@@ -1776,20 +1776,19 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	if (!newprop->name)
 		return -EINVAL;
 
-	oldprop = of_find_property(np, newprop->name, NULL);
-	if (!oldprop)
-		return of_add_property(np, newprop);
-
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	next = &np->properties;
-	while (*next) {
+	oldprop = __of_find_property(np, newprop->name, NULL);
+	if (!oldprop) {
+		/* add the new node */
+		rc = __of_add_property(np, newprop);
+	} else while (*next) {
+		/* found the node and then replace it */
 		if (*next == oldprop) {
-			/* found the node */
 			newprop->next = oldprop->next;
 			*next = newprop;
 			oldprop->next = np->deadprops;
 			np->deadprops = oldprop;
-			found = 1;
 			break;
 		}
 		next = &(*next)->next;
@@ -1803,9 +1802,6 @@ int of_update_property(struct device_node *np, struct property *newprop)
 		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
 	__of_add_property_sysfs(np, newprop);
 
-	if (!found)
-		return -ENODEV;
-
 	return 0;
 }
 
-- 
1.8.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] of: fix of_update_property again

[Rob Herring]

On Thu, Apr 17, 2014 at 2:48 AM, Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> The of_update_property() is intented to update a property in a node
> and if the property does not exist, will add it.
>
> The second search of the property is possibly won't be found, that
> maybe removed by other thread just before the second search begain.
>
> Using the __of_find_property() and __of_add_property() instead and
> move them into lock operations.

I have a fix in my tree from Guenter which handles the race of the
property being removed. However, your change will be needed when we
have overlays and the node itself can be removed. So this is 3.16
material and will need to be rebased on Guenter's fix.

Rob

>
> Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  drivers/of/base.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index be2861d..06623d0c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1767,7 +1767,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  {
>         struct property **next, *oldprop;
>         unsigned long flags;
> -       int rc, found = 0;
> +       int rc;
>
>         rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
>         if (rc)
> @@ -1776,20 +1776,19 @@ int of_update_property(struct device_node *np, struct property *newprop)
>         if (!newprop->name)
>                 return -EINVAL;
>
> -       oldprop = of_find_property(np, newprop->name, NULL);
> -       if (!oldprop)
> -               return of_add_property(np, newprop);
> -
>         raw_spin_lock_irqsave(&devtree_lock, flags);
>         next = &np->properties;
> -       while (*next) {
> +       oldprop = __of_find_property(np, newprop->name, NULL);
> +       if (!oldprop) {
> +               /* add the new node */
> +               rc = __of_add_property(np, newprop);
> +       } else while (*next) {
> +               /* found the node and then replace it */
>                 if (*next == oldprop) {
> -                       /* found the node */
>                         newprop->next = oldprop->next;
>                         *next = newprop;
>                         oldprop->next = np->deadprops;
>                         np->deadprops = oldprop;
> -                       found = 1;
>                         break;
>                 }
>                 next = &(*next)->next;
> @@ -1803,9 +1802,6 @@ int of_update_property(struct device_node *np, struct property *newprop)
>                 sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
>         __of_add_property_sysfs(np, newprop);
>
> -       if (!found)
> -               return -ENODEV;
> -
>         return 0;
>  }
>
> --
> 1.8.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv3] of: fix of_update_property again

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3260 bytes --]


> Subject: Re: [PATCHv3] of: fix of_update_property again
> 
> On Thu, Apr 17, 2014 at 2:48 AM, Xiubo Li <Li.Xiubo@freescale.com> wrote:
> > The of_update_property() is intented to update a property in a node
> > and if the property does not exist, will add it.
> >
> > The second search of the property is possibly won't be found, that
> > maybe removed by other thread just before the second search begain.
> >
> > Using the __of_find_property() and __of_add_property() instead and
> > move them into lock operations.
> 
> I have a fix in my tree from Guenter which handles the race of the
> property being removed. However, your change will be needed when we
> have overlays and the node itself can be removed. So this is 3.16
> material and will need to be rebased on Guenter's fix.
> 

Got it.

Thanks,

BRs
Xiubo



> Rob
> 
> >
> > Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> > ---
> >  drivers/of/base.c | 18 +++++++-----------
> >  1 file changed, 7 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index be2861d..06623d0c 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1767,7 +1767,7 @@ int of_update_property(struct device_node *np, struct
> property *newprop)
> >  {
> >         struct property **next, *oldprop;
> >         unsigned long flags;
> > -       int rc, found = 0;
> > +       int rc;
> >
> >         rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
> >         if (rc)
> > @@ -1776,20 +1776,19 @@ int of_update_property(struct device_node *np,
> struct property *newprop)
> >         if (!newprop->name)
> >                 return -EINVAL;
> >
> > -       oldprop = of_find_property(np, newprop->name, NULL);
> > -       if (!oldprop)
> > -               return of_add_property(np, newprop);
> > -
> >         raw_spin_lock_irqsave(&devtree_lock, flags);
> >         next = &np->properties;
> > -       while (*next) {
> > +       oldprop = __of_find_property(np, newprop->name, NULL);
> > +       if (!oldprop) {
> > +               /* add the new node */
> > +               rc = __of_add_property(np, newprop);
> > +       } else while (*next) {
> > +               /* found the node and then replace it */
> >                 if (*next == oldprop) {
> > -                       /* found the node */
> >                         newprop->next = oldprop->next;
> >                         *next = newprop;
> >                         oldprop->next = np->deadprops;
> >                         np->deadprops = oldprop;
> > -                       found = 1;
> >                         break;
> >                 }
> >                 next = &(*next)->next;
> > @@ -1803,9 +1802,6 @@ int of_update_property(struct device_node *np, struct
> property *newprop)
> >                 sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> >         __of_add_property_sysfs(np, newprop);
> >
> > -       if (!found)
> > -               return -ENODEV;
> > -
> >         return 0;
> >  }
> >
> > --
> > 1.8.4
> >
> >
> 

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCHv3] of: fix of_update_property again

[Grant Likely]

On Thu, 17 Apr 2014 09:32:16 -0500, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Apr 17, 2014 at 2:48 AM, Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > The of_update_property() is intented to update a property in a node
> > and if the property does not exist, will add it.
> >
> > The second search of the property is possibly won't be found, that
> > maybe removed by other thread just before the second search begain.
> >
> > Using the __of_find_property() and __of_add_property() instead and
> > move them into lock operations.
> 
> I have a fix in my tree from Guenter which handles the race of the
> property being removed. However, your change will be needed when we
> have overlays and the node itself can be removed. So this is 3.16
> material and will need to be rebased on Guenter's fix.

I didn't think Guenter's patch actually fixed a race. It only fixed a
double checking of the oldprop pointer. Do I have it wrong?

g.

> 
> Rob
> 
> >
> > Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  drivers/of/base.c | 18 +++++++-----------
> >  1 file changed, 7 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index be2861d..06623d0c 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1767,7 +1767,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
> >  {
> >         struct property **next, *oldprop;
> >         unsigned long flags;
> > -       int rc, found = 0;
> > +       int rc;
> >
> >         rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
> >         if (rc)
> > @@ -1776,20 +1776,19 @@ int of_update_property(struct device_node *np, struct property *newprop)
> >         if (!newprop->name)
> >                 return -EINVAL;
> >
> > -       oldprop = of_find_property(np, newprop->name, NULL);
> > -       if (!oldprop)
> > -               return of_add_property(np, newprop);
> > -
> >         raw_spin_lock_irqsave(&devtree_lock, flags);
> >         next = &np->properties;
> > -       while (*next) {
> > +       oldprop = __of_find_property(np, newprop->name, NULL);
> > +       if (!oldprop) {
> > +               /* add the new node */
> > +               rc = __of_add_property(np, newprop);
> > +       } else while (*next) {
> > +               /* found the node and then replace it */
> >                 if (*next == oldprop) {
> > -                       /* found the node */
> >                         newprop->next = oldprop->next;
> >                         *next = newprop;
> >                         oldprop->next = np->deadprops;
> >                         np->deadprops = oldprop;
> > -                       found = 1;
> >                         break;
> >                 }
> >                 next = &(*next)->next;
> > @@ -1803,9 +1802,6 @@ int of_update_property(struct device_node *np, struct property *newprop)
> >                 sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> >         __of_add_property_sysfs(np, newprop);
> >
> > -       if (!found)
> > -               return -ENODEV;
> > -
> >         return 0;
> >  }
> >
> > --
> > 1.8.4
> >
> >

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] of: fix of_update_property again

[Rob Herring]

On Tue, Apr 22, 2014 at 9:06 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, 17 Apr 2014 09:32:16 -0500, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Thu, Apr 17, 2014 at 2:48 AM, Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> > The of_update_property() is intented to update a property in a node
>> > and if the property does not exist, will add it.
>> >
>> > The second search of the property is possibly won't be found, that
>> > maybe removed by other thread just before the second search begain.
>> >
>> > Using the __of_find_property() and __of_add_property() instead and
>> > move them into lock operations.
>>
>> I have a fix in my tree from Guenter which handles the race of the
>> property being removed. However, your change will be needed when we
>> have overlays and the node itself can be removed. So this is 3.16
>> material and will need to be rebased on Guenter's fix.
>
> I didn't think Guenter's patch actually fixed a race. It only fixed a
> double checking of the oldprop pointer. Do I have it wrong?

Yes, the commit message should have been clearer. The problem was that
found was checked after updating the sysfs files.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] of: fix of_update_property again

[Grant Likely]

On Thu, 17 Apr 2014 15:48:29 +0800, Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> The of_update_property() is intented to update a property in a node
> and if the property does not exist, will add it.
> 
> The second search of the property is possibly won't be found, that
> maybe removed by other thread just before the second search begain.
> 
> Using the __of_find_property() and __of_add_property() instead and
> move them into lock operations.

Applied with context fixups, thanks.

g.

> 
> Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  drivers/of/base.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index be2861d..06623d0c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1767,7 +1767,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  {
>  	struct property **next, *oldprop;
>  	unsigned long flags;
> -	int rc, found = 0;
> +	int rc;
>  
>  	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
>  	if (rc)
> @@ -1776,20 +1776,19 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  	if (!newprop->name)
>  		return -EINVAL;
>  
> -	oldprop = of_find_property(np, newprop->name, NULL);
> -	if (!oldprop)
> -		return of_add_property(np, newprop);
> -
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	next = &np->properties;
> -	while (*next) {
> +	oldprop = __of_find_property(np, newprop->name, NULL);
> +	if (!oldprop) {
> +		/* add the new node */
> +		rc = __of_add_property(np, newprop);
> +	} else while (*next) {
> +		/* found the node and then replace it */
>  		if (*next == oldprop) {
> -			/* found the node */
>  			newprop->next = oldprop->next;
>  			*next = newprop;
>  			oldprop->next = np->deadprops;
>  			np->deadprops = oldprop;
> -			found = 1;
>  			break;
>  		}
>  		next = &(*next)->next;
> @@ -1803,9 +1802,6 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
>  	__of_add_property_sysfs(np, newprop);
>  
> -	if (!found)
> -		return -ENODEV;
> -
>  	return 0;
>  }
>  
> -- 
> 1.8.4
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] of: fix of_update_property again

[Guenter Roeck]

On Tue, Apr 22, 2014 at 09:10:35AM -0500, Rob Herring wrote:
> On Tue, Apr 22, 2014 at 9:06 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, 17 Apr 2014 09:32:16 -0500, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> On Thu, Apr 17, 2014 at 2:48 AM, Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> >> > The of_update_property() is intented to update a property in a node
> >> > and if the property does not exist, will add it.
> >> >
> >> > The second search of the property is possibly won't be found, that
> >> > maybe removed by other thread just before the second search begain.
> >> >
> >> > Using the __of_find_property() and __of_add_property() instead and
> >> > move them into lock operations.
> >>
> >> I have a fix in my tree from Guenter which handles the race of the
> >> property being removed. However, your change will be needed when we
> >> have overlays and the node itself can be removed. So this is 3.16
> >> material and will need to be rebased on Guenter's fix.
> >
> > I didn't think Guenter's patch actually fixed a race. It only fixed a
> > double checking of the oldprop pointer. Do I have it wrong?
> 
> Yes, the commit message should have been clearer. The problem was that
> found was checked after updating the sysfs files.
> 
Fixing the race was more of a side effect. It wasn't actually clear to me
at the time that there was a race; I realized it only after I read above
exchange.

To clarify: My understanding is that the property could have been removed
after the call to of_find_property() found it. Thus, the actual search
for it can fail, and the check if it was found is necessary to prevent
action on the (now removed) old property.

Did I get this right ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] of: fix of_update_property again

[Grant Likely]

On Tue, 22 Apr 2014 08:16:09 -0700, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On Tue, Apr 22, 2014 at 09:10:35AM -0500, Rob Herring wrote:
> > On Tue, Apr 22, 2014 at 9:06 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > On Thu, 17 Apr 2014 09:32:16 -0500, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > >> On Thu, Apr 17, 2014 at 2:48 AM, Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > >> > The of_update_property() is intented to update a property in a node
> > >> > and if the property does not exist, will add it.
> > >> >
> > >> > The second search of the property is possibly won't be found, that
> > >> > maybe removed by other thread just before the second search begain.
> > >> >
> > >> > Using the __of_find_property() and __of_add_property() instead and
> > >> > move them into lock operations.
> > >>
> > >> I have a fix in my tree from Guenter which handles the race of the
> > >> property being removed. However, your change will be needed when we
> > >> have overlays and the node itself can be removed. So this is 3.16
> > >> material and will need to be rebased on Guenter's fix.
> > >
> > > I didn't think Guenter's patch actually fixed a race. It only fixed a
> > > double checking of the oldprop pointer. Do I have it wrong?
> > 
> > Yes, the commit message should have been clearer. The problem was that
> > found was checked after updating the sysfs files.
> > 
> Fixing the race was more of a side effect. It wasn't actually clear to me
> at the time that there was a race; I realized it only after I read above
> exchange.
> 
> To clarify: My understanding is that the property could have been removed
> after the call to of_find_property() found it. Thus, the actual search
> for it can fail, and the check if it was found is necessary to prevent
> action on the (now removed) old property.
> 
> Did I get this right ?

Not quite. Your change was correct for the existing code. The second
oldprop check was not needed since there was no way to get there with it
being NULL.

However, your understanding of the race condition is correct. Xiubo
fixed it by moving the of_find_property() call under the spinlock so
there is no possibility for the property to disappear between find and
update.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html