[PATCH] of: fix of_update_property [v2]

[PATCH] of: fix of_update_property [v2]

[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 | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 418a4ff9..34acb37 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,25 +1776,26 @@ 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 node */
+		rc = __of_add_property(np, newprop);
+	} else {
+		next = &np->properties;
+		while (*next && *next != oldprop)
+			next = &(*next)->next;
+
 		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;
 	}
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
 	if (rc)
 		return rc;
 
@@ -1803,9 +1804,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: [PATCH] of: fix of_update_property [v2]

[Grant Likely]

On Tue, 1 Apr 2014 13:51:26 +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.
> 
> Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  drivers/of/base.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 418a4ff9..34acb37 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,25 +1776,26 @@ 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 node */
> +		rc = __of_add_property(np, newprop);
> +	} else {

if you changed this line to:
	} else while (*next) {
then most of the other changes go away. You don't need the separate
while loop and the function remains largely identical aside from moving
the __of_find_property() into the spinlock.

> +		next = &np->properties;
> +		while (*next && *next != oldprop)
> +			next = &(*next)->next;
> +
>  		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;
>  	}
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +
>  	if (rc)
>  		return rc;
>  
> @@ -1803,9 +1804,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: [PATCH] of: fix of_update_property [v2]

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> > @@ -1776,25 +1776,26 @@ 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 node */
> > +		rc = __of_add_property(np, newprop);
> > +	} else {
> 
> if you changed this line to:
> 	} else while (*next) {
> then most of the other changes go away. You don't need the separate
> while loop 

Yes, I will fix this.


> and the function remains largely identical aside from moving
> the __of_find_property() into the spinlock.
>

But, from the following codes, we can see that, if oldprop !=  NULL 
Meaning that we have found it, and should just do the updatation later:
+++++++++++++++
	oldprop = of_find_property(np, newprop->name, NULL);
	if (!oldprop)
		return of_add_property(np, newprop);
---------------


 
> >  			/* found the node */
> >  			newprop->next = oldprop->next;
> >  			*next = newprop;
> >  			oldprop->next = np->deadprops;
> >  			np->deadprops = oldprop;
> > -			found = 1;

And why the 'found' flag is here is that the oldprop maybe removed
just before the spin_lock and after of_find_property().

And so use and move __of_find_property() and __of_add_property() into
the spinlock could avoid this...


Thanks,
--

BRs,
Xiubo


> > -			break;
> >  		}
> > -		next = &(*next)->next;
> >  	}
> >  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > +
> >  	if (rc)
> >  		return rc;
> >
> > @@ -1803,9 +1804,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: [PATCH] of: fix of_update_property [v2]

[Grant Likely]

On Wed, 2 Apr 2014 05:29:53 +0000, "Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org" <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> 
> > > @@ -1776,25 +1776,26 @@ 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 node */
> > > +		rc = __of_add_property(np, newprop);
> > > +	} else {
> > 
> > if you changed this line to:
> > 	} else while (*next) {
> > then most of the other changes go away. You don't need the separate
> > while loop 
> 
> Yes, I will fix this.
> 
> 
> > and the function remains largely identical aside from moving
> > the __of_find_property() into the spinlock.
> >
> 
> But, from the following codes, we can see that, if oldprop !=  NULL 
> Meaning that we have found it, and should just do the updatation later:
> +++++++++++++++
> 	oldprop = of_find_property(np, newprop->name, NULL);
> 	if (!oldprop)
> 		return of_add_property(np, newprop);
> ---------------
> 
> 
>  
> > >  			/* found the node */
> > >  			newprop->next = oldprop->next;
> > >  			*next = newprop;
> > >  			oldprop->next = np->deadprops;
> > >  			np->deadprops = oldprop;
> > > -			found = 1;
> 
> And why the 'found' flag is here is that the oldprop maybe removed
> just before the spin_lock and after of_find_property().
> 
> And so use and move __of_find_property() and __of_add_property() into
> the spinlock could avoid this...

Isn't that what I said?

g.

> 
> 
> Thanks,
> --
> 
> BRs,
> Xiubo
> 
> 
> > > -			break;
> > >  		}
> > > -		next = &(*next)->next;
> > >  	}
> > >  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > > +
> > >  	if (rc)
> > >  		return rc;
> > >
> > > @@ -1803,9 +1804,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: [PATCH] of: fix of_update_property [v2]

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> > > and the function remains largely identical aside from moving
> > > the __of_find_property() into the spinlock.
> > >
> >
> > But, from the following codes, we can see that, if oldprop !=  NULL
> > Meaning that we have found it, and should just do the updatation later:
> > +++++++++++++++
> > 	oldprop = of_find_property(np, newprop->name, NULL);
> > 	if (!oldprop)
> > 		return of_add_property(np, newprop);
> > ---------------
> >
> >
> >
> > > >  			/* found the node */
> > > >  			newprop->next = oldprop->next;
> > > >  			*next = newprop;
> > > >  			oldprop->next = np->deadprops;
> > > >  			np->deadprops = oldprop;
> > > > -			found = 1;
> >
> > And why the 'found' flag is here is that the oldprop maybe removed
> > just before the spin_lock and after of_find_property().
> >
> > And so use and move __of_find_property() and __of_add_property() into
> > the spinlock could avoid this...
> 
> Isn't that what I said?

Yes, maybe my not thoroughly understanding of your last mail before.

Thanks,

BRs
Xiubo
--
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: [PATCH] of: fix of_update_property [v2]

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> Subject: RE: [PATCH] of: fix of_update_property [v2]
> 
> On Wed, 2 Apr 2014 05:29:53 +0000, "Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org"
> <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> >
> >
> > > > @@ -1776,25 +1776,26 @@ 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 node */
> > > > +		rc = __of_add_property(np, newprop);
> > > > +	} else {
> > >
> > > if you changed this line to:
> > > 	} else while (*next) {
> > > then most of the other changes go away. You don't need the separate
> > > while loop
> >

Well, this will output some errors by using the './scripts/checkpatch.pl'
Checking.

$ ./scripts/checkpatch.pl

ERROR: trailing statements should be on next line
#1785: FILE: of/base.c:1785:
+       else while (*next) {

ERROR: trailing statements should be on next line
#1785: FILE: of/base.c:1785:
+       else while (*next) {

Should be split into two lines...

Thanks,

BRs
Xiubo


> > Yes, I will fix this.
> >
> >
> > > and the function remains largely identical aside from moving
> > > the __of_find_property() into the spinlock.
> > >
> >
> > But, from the following codes, we can see that, if oldprop !=  NULL
> > Meaning that we have found it, and should just do the updatation later:
> > +++++++++++++++
> > 	oldprop = of_find_property(np, newprop->name, NULL);
> > 	if (!oldprop)
> > 		return of_add_property(np, newprop);
> > ---------------
> >
> >
> >
> > > >  			/* found the node */
> > > >  			newprop->next = oldprop->next;
> > > >  			*next = newprop;
> > > >  			oldprop->next = np->deadprops;
> > > >  			np->deadprops = oldprop;
> > > > -			found = 1;
> >
> > And why the 'found' flag is here is that the oldprop maybe removed
> > just before the spin_lock and after of_find_property().
> >
> > And so use and move __of_find_property() and __of_add_property() into
> > the spinlock could avoid this...
> 
> Isn't that what I said?
> 
> g.
> 
> >
> >
> > Thanks,
> > --
> >
> > BRs,
> > Xiubo
> >
> >
> > > > -			break;
> > > >  		}
> > > > -		next = &(*next)->next;
> > > >  	}
> > > >  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > > > +
> > > >  	if (rc)
> > > >  		return rc;
> > > >
> > > > @@ -1803,9 +1804,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: [PATCH] of: fix of_update_property [v2]

[Grant Likely]

On Fri, 4 Apr 2014 08:30:42 +0000, "Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org" <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > Subject: RE: [PATCH] of: fix of_update_property [v2]
> > 
> > On Wed, 2 Apr 2014 05:29:53 +0000, "Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org"
> > <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > >
> > >
> > > > > @@ -1776,25 +1776,26 @@ 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 node */
> > > > > +		rc = __of_add_property(np, newprop);
> > > > > +	} else {
> > > >
> > > > if you changed this line to:
> > > > 	} else while (*next) {
> > > > then most of the other changes go away. You don't need the separate
> > > > while loop
> > >
> 
> Well, this will output some errors by using the './scripts/checkpatch.pl'
> Checking.
> 
> $ ./scripts/checkpatch.pl
> 
> ERROR: trailing statements should be on next line
> #1785: FILE: of/base.c:1785:
> +       else while (*next) {
> 
> ERROR: trailing statements should be on next line
> #1785: FILE: of/base.c:1785:
> +       else while (*next) {
> 
> Should be split into two lines...

Checkpatch is wrong. I won't reject that approach.

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

RE: [PATCH] of: fix of_update_property [v2]

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> Subject: RE: [PATCH] of: fix of_update_property [v2]
> 
> On Fri, 4 Apr 2014 08:30:42 +0000, "Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org"
> <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > Subject: RE: [PATCH] of: fix of_update_property [v2]
> > >
> > > On Wed, 2 Apr 2014 05:29:53 +0000, "Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org"
> > > <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > >
> > > >
> > > > > > @@ -1776,25 +1776,26 @@ 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 node */
> > > > > > +		rc = __of_add_property(np, newprop);
> > > > > > +	} else {
> > > > >
> > > > > if you changed this line to:
> > > > > 	} else while (*next) {
> > > > > then most of the other changes go away. You don't need the separate
> > > > > while loop
> > > >
> >
> > Well, this will output some errors by using the './scripts/checkpatch.pl'
> > Checking.
> >
> > $ ./scripts/checkpatch.pl
> >
> > ERROR: trailing statements should be on next line
> > #1785: FILE: of/base.c:1785:
> > +       else while (*next) {
> >
> > ERROR: trailing statements should be on next line
> > #1785: FILE: of/base.c:1785:
> > +       else while (*next) {
> >
> > Should be split into two lines...
> 
> Checkpatch is wrong. I won't reject that approach.
> 

Okey, I have sent one version, should I send it again ?

Thanks,
Brs
Xiubo


> 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