[PATCH 1/1] of: reform prom_update_property function

[PATCH 1/1] of: reform prom_update_property function

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

prom_update_property() currently fails if the property doesn't
actually exist yet which isn't what we want. Change to add-or-update
instead of update-only, then we can remove a lot duplicated lines.

Suggested-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 arch/powerpc/platforms/85xx/p1022_ds.c    |    8 +-------
 arch/powerpc/platforms/pseries/mobility.c |    8 +-------
 arch/powerpc/platforms/pseries/reconfig.c |   13 +++----------
 drivers/of/base.c                         |   15 +++++++++++----
 fs/proc/proc_devtree.c                    |    5 +++++
 include/linux/of.h                        |    3 +--
 6 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
index f700c81..d66631c 100644
--- a/arch/powerpc/platforms/85xx/p1022_ds.c
+++ b/arch/powerpc/platforms/85xx/p1022_ds.c
@@ -348,13 +348,7 @@ void __init p1022_ds_pic_init(void)
  */
 static void __init disable_one_node(struct device_node *np, struct property *new)
 {
-	struct property *old;
-
-	old = of_find_property(np, new->name, NULL);
-	if (old)
-		prom_update_property(np, new, old);
-	else
-		prom_add_property(np, new);
+	prom_update_property(np, new);
 }
 
 /* TRUE if there is a "video=fslfb" command-line parameter. */
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 029a562..dd30b12 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -67,7 +67,6 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
 			      const char *name, u32 vd, char *value)
 {
 	struct property *new_prop = *prop;
-	struct property *old_prop;
 	int more = 0;
 
 	/* A negative 'vd' value indicates that only part of the new property
@@ -117,12 +116,7 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
 	}
 
 	if (!more) {
-		old_prop = of_find_property(dn, new_prop->name, NULL);
-		if (old_prop)
-			prom_update_property(dn, new_prop, old_prop);
-		else
-			prom_add_property(dn, new_prop);
-
+		prom_update_property(dn, new_prop);
 		new_prop = NULL;
 	}
 
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 7b3bf76..4c92f1c 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
 	unsigned char *value;
 	char *name, *end, *next_prop;
 	int rc, length;
-	struct property *newprop, *oldprop;
+	struct property *newprop;
 	buf = parse_node(buf, bufsize, &np);
 	end = buf + bufsize;
 
@@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
 	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
 		slb_set_size(*(int *)value);
 
-	oldprop = of_find_property(np, name,NULL);
-	if (!oldprop) {
-		if (strlen(name))
-			return prom_add_property(np, newprop);
-		return -ENODEV;
-	}
-
 	upd_value.node = np;
 	upd_value.property = newprop;
 	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
 
-	rc = prom_update_property(np, newprop, oldprop);
+	rc = prom_update_property(np, newprop);
 	if (rc)
 		return rc;
 
@@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
 
 		rc = pSeries_reconfig_notify(action, value);
 		if (rc) {
-			prom_update_property(np, oldprop, newprop);
+			prom_update_property(np, newprop);
 			return rc;
 		}
 	}
diff --git a/drivers/of/base.c b/drivers/of/base.c
index d9bfd49..a14f109 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
 }
 
 /*
- * prom_update_property - Update a property in a node.
+ * prom_update_property - Update a property in a node, if the property does
+ * not exist, add it.
  *
  * Note that we don't actually remove it, since we have given out
  * who-knows-how-many pointers to the data using get-property.
@@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
  * and add the new property to the property list
  */
 int prom_update_property(struct device_node *np,
-			 struct property *newprop,
-			 struct property *oldprop)
+			 struct property *newprop)
 {
-	struct property **next;
+	struct property **next, *oldprop;
 	unsigned long flags;
 	int found = 0;
 
+	if (!newprop->name)
+		return -EINVAL;
+
+	oldprop = of_find_property(np, newprop->name, NULL);
+	if (!oldprop)
+		return prom_add_property(np, newprop);
+
 	write_lock_irqsave(&devtree_lock, flags);
 	next = &np->properties;
 	while (*next) {
diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
index 927cbd1..df7dd08 100644
--- a/fs/proc/proc_devtree.c
+++ b/fs/proc/proc_devtree.c
@@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
 {
 	struct proc_dir_entry *ent;
 
+	if (!oldprop) {
+		proc_device_tree_add_prop(pde, newprop);
+		return;
+	}
+
 	for (ent = pde->subdir; ent != NULL; ent = ent->next)
 		if (ent->data == oldprop)
 			break;
diff --git a/include/linux/of.h b/include/linux/of.h
index 2ec1083..b27c871 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
 extern int prom_add_property(struct device_node* np, struct property* prop);
 extern int prom_remove_property(struct device_node *np, struct property *prop);
 extern int prom_update_property(struct device_node *np,
-				struct property *newprop,
-				struct property *oldprop);
+				struct property *newprop);
 
 #if defined(CONFIG_OF_DYNAMIC)
 /* For updating the device tree at runtime */
-- 
1.7.0.4

[PATCH 1/1] of: reform prom_update_property function

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

prom_update_property() currently fails if the property doesn't
actually exist yet which isn't what we want. Change to add-or-update
instead of update-only, then we can remove a lot duplicated lines.

Suggested-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 arch/powerpc/platforms/85xx/p1022_ds.c    |    8 +-------
 arch/powerpc/platforms/pseries/mobility.c |    8 +-------
 arch/powerpc/platforms/pseries/reconfig.c |   13 +++----------
 drivers/of/base.c                         |   15 +++++++++++----
 fs/proc/proc_devtree.c                    |    5 +++++
 include/linux/of.h                        |    3 +--
 6 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
index f700c81..d66631c 100644
--- a/arch/powerpc/platforms/85xx/p1022_ds.c
+++ b/arch/powerpc/platforms/85xx/p1022_ds.c
@@ -348,13 +348,7 @@ void __init p1022_ds_pic_init(void)
  */
 static void __init disable_one_node(struct device_node *np, struct property *new)
 {
-	struct property *old;
-
-	old = of_find_property(np, new->name, NULL);
-	if (old)
-		prom_update_property(np, new, old);
-	else
-		prom_add_property(np, new);
+	prom_update_property(np, new);
 }
 
 /* TRUE if there is a "video=fslfb" command-line parameter. */
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 029a562..dd30b12 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -67,7 +67,6 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
 			      const char *name, u32 vd, char *value)
 {
 	struct property *new_prop = *prop;
-	struct property *old_prop;
 	int more = 0;
 
 	/* A negative 'vd' value indicates that only part of the new property
@@ -117,12 +116,7 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
 	}
 
 	if (!more) {
-		old_prop = of_find_property(dn, new_prop->name, NULL);
-		if (old_prop)
-			prom_update_property(dn, new_prop, old_prop);
-		else
-			prom_add_property(dn, new_prop);
-
+		prom_update_property(dn, new_prop);
 		new_prop = NULL;
 	}
 
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 7b3bf76..4c92f1c 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
 	unsigned char *value;
 	char *name, *end, *next_prop;
 	int rc, length;
-	struct property *newprop, *oldprop;
+	struct property *newprop;
 	buf = parse_node(buf, bufsize, &np);
 	end = buf + bufsize;
 
@@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
 	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
 		slb_set_size(*(int *)value);
 
-	oldprop = of_find_property(np, name,NULL);
-	if (!oldprop) {
-		if (strlen(name))
-			return prom_add_property(np, newprop);
-		return -ENODEV;
-	}
-
 	upd_value.node = np;
 	upd_value.property = newprop;
 	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
 
-	rc = prom_update_property(np, newprop, oldprop);
+	rc = prom_update_property(np, newprop);
 	if (rc)
 		return rc;
 
@@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
 
 		rc = pSeries_reconfig_notify(action, value);
 		if (rc) {
-			prom_update_property(np, oldprop, newprop);
+			prom_update_property(np, newprop);
 			return rc;
 		}
 	}
diff --git a/drivers/of/base.c b/drivers/of/base.c
index d9bfd49..a14f109 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
 }
 
 /*
- * prom_update_property - Update a property in a node.
+ * prom_update_property - Update a property in a node, if the property does
+ * not exist, add it.
  *
  * Note that we don't actually remove it, since we have given out
  * who-knows-how-many pointers to the data using get-property.
@@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
  * and add the new property to the property list
  */
 int prom_update_property(struct device_node *np,
-			 struct property *newprop,
-			 struct property *oldprop)
+			 struct property *newprop)
 {
-	struct property **next;
+	struct property **next, *oldprop;
 	unsigned long flags;
 	int found = 0;
 
+	if (!newprop->name)
+		return -EINVAL;
+
+	oldprop = of_find_property(np, newprop->name, NULL);
+	if (!oldprop)
+		return prom_add_property(np, newprop);
+
 	write_lock_irqsave(&devtree_lock, flags);
 	next = &np->properties;
 	while (*next) {
diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
index 927cbd1..df7dd08 100644
--- a/fs/proc/proc_devtree.c
+++ b/fs/proc/proc_devtree.c
@@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
 {
 	struct proc_dir_entry *ent;
 
+	if (!oldprop) {
+		proc_device_tree_add_prop(pde, newprop);
+		return;
+	}
+
 	for (ent = pde->subdir; ent != NULL; ent = ent->next)
 		if (ent->data == oldprop)
 			break;
diff --git a/include/linux/of.h b/include/linux/of.h
index 2ec1083..b27c871 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
 extern int prom_add_property(struct device_node* np, struct property* prop);
 extern int prom_remove_property(struct device_node *np, struct property *prop);
 extern int prom_update_property(struct device_node *np,
-				struct property *newprop,
-				struct property *oldprop);
+				struct property *newprop);
 
 #if defined(CONFIG_OF_DYNAMIC)
 /* For updating the device tree at runtime */
-- 
1.7.0.4

Re: [PATCH 1/1] of: reform prom_update_property function

[Rob Herring]

On 06/20/2012 12:54 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> prom_update_property() currently fails if the property doesn't
> actually exist yet which isn't what we want. Change to add-or-update
> instead of update-only, then we can remove a lot duplicated lines.
> 
> Suggested-by: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---

Looks fine, but you need to cc powerpc maintainers.

Rob

>  arch/powerpc/platforms/85xx/p1022_ds.c    |    8 +-------
>  arch/powerpc/platforms/pseries/mobility.c |    8 +-------
>  arch/powerpc/platforms/pseries/reconfig.c |   13 +++----------
>  drivers/of/base.c                         |   15 +++++++++++----
>  fs/proc/proc_devtree.c                    |    5 +++++
>  include/linux/of.h                        |    3 +--
>  6 files changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
> index f700c81..d66631c 100644
> --- a/arch/powerpc/platforms/85xx/p1022_ds.c
> +++ b/arch/powerpc/platforms/85xx/p1022_ds.c
> @@ -348,13 +348,7 @@ void __init p1022_ds_pic_init(void)
>   */
>  static void __init disable_one_node(struct device_node *np, struct property *new)
>  {
> -	struct property *old;
> -
> -	old = of_find_property(np, new->name, NULL);
> -	if (old)
> -		prom_update_property(np, new, old);
> -	else
> -		prom_add_property(np, new);
> +	prom_update_property(np, new);
>  }
>  
>  /* TRUE if there is a "video=fslfb" command-line parameter. */
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> index 029a562..dd30b12 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -67,7 +67,6 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
>  			      const char *name, u32 vd, char *value)
>  {
>  	struct property *new_prop = *prop;
> -	struct property *old_prop;
>  	int more = 0;
>  
>  	/* A negative 'vd' value indicates that only part of the new property
> @@ -117,12 +116,7 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
>  	}
>  
>  	if (!more) {
> -		old_prop = of_find_property(dn, new_prop->name, NULL);
> -		if (old_prop)
> -			prom_update_property(dn, new_prop, old_prop);
> -		else
> -			prom_add_property(dn, new_prop);
> -
> +		prom_update_property(dn, new_prop);
>  		new_prop = NULL;
>  	}
>  
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index 7b3bf76..4c92f1c 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
>  	unsigned char *value;
>  	char *name, *end, *next_prop;
>  	int rc, length;
> -	struct property *newprop, *oldprop;
> +	struct property *newprop;
>  	buf = parse_node(buf, bufsize, &np);
>  	end = buf + bufsize;
>  
> @@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
>  	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
>  		slb_set_size(*(int *)value);
>  
> -	oldprop = of_find_property(np, name,NULL);
> -	if (!oldprop) {
> -		if (strlen(name))
> -			return prom_add_property(np, newprop);
> -		return -ENODEV;
> -	}
> -
>  	upd_value.node = np;
>  	upd_value.property = newprop;
>  	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
>  
> -	rc = prom_update_property(np, newprop, oldprop);
> +	rc = prom_update_property(np, newprop);
>  	if (rc)
>  		return rc;
>  
> @@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
>  
>  		rc = pSeries_reconfig_notify(action, value);
>  		if (rc) {
> -			prom_update_property(np, oldprop, newprop);
> +			prom_update_property(np, newprop);
>  			return rc;
>  		}
>  	}
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d9bfd49..a14f109 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
>  }
>  
>  /*
> - * prom_update_property - Update a property in a node.
> + * prom_update_property - Update a property in a node, if the property does
> + * not exist, add it.
>   *
>   * Note that we don't actually remove it, since we have given out
>   * who-knows-how-many pointers to the data using get-property.
> @@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
>   * and add the new property to the property list
>   */
>  int prom_update_property(struct device_node *np,
> -			 struct property *newprop,
> -			 struct property *oldprop)
> +			 struct property *newprop)
>  {
> -	struct property **next;
> +	struct property **next, *oldprop;
>  	unsigned long flags;
>  	int found = 0;
>  
> +	if (!newprop->name)
> +		return -EINVAL;
> +
> +	oldprop = of_find_property(np, newprop->name, NULL);
> +	if (!oldprop)
> +		return prom_add_property(np, newprop);
> +
>  	write_lock_irqsave(&devtree_lock, flags);
>  	next = &np->properties;
>  	while (*next) {
> diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> index 927cbd1..df7dd08 100644
> --- a/fs/proc/proc_devtree.c
> +++ b/fs/proc/proc_devtree.c
> @@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
>  {
>  	struct proc_dir_entry *ent;
>  
> +	if (!oldprop) {
> +		proc_device_tree_add_prop(pde, newprop);
> +		return;
> +	}
> +
>  	for (ent = pde->subdir; ent != NULL; ent = ent->next)
>  		if (ent->data == oldprop)
>  			break;
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 2ec1083..b27c871 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
>  extern int prom_add_property(struct device_node* np, struct property* prop);
>  extern int prom_remove_property(struct device_node *np, struct property *prop);
>  extern int prom_update_property(struct device_node *np,
> -				struct property *newprop,
> -				struct property *oldprop);
> +				struct property *newprop);
>  
>  #if defined(CONFIG_OF_DYNAMIC)
>  /* For updating the device tree at runtime */

Re: [PATCH 1/1] of: reform prom_update_property function

[Rob Herring]

On 06/20/2012 12:54 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> prom_update_property() currently fails if the property doesn't
> actually exist yet which isn't what we want. Change to add-or-update
> instead of update-only, then we can remove a lot duplicated lines.
> 
> Suggested-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Signed-off-by: Dong Aisheng <dong.aisheng-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---

Looks fine, but you need to cc powerpc maintainers.

Rob

>  arch/powerpc/platforms/85xx/p1022_ds.c    |    8 +-------
>  arch/powerpc/platforms/pseries/mobility.c |    8 +-------
>  arch/powerpc/platforms/pseries/reconfig.c |   13 +++----------
>  drivers/of/base.c                         |   15 +++++++++++----
>  fs/proc/proc_devtree.c                    |    5 +++++
>  include/linux/of.h                        |    3 +--
>  6 files changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
> index f700c81..d66631c 100644
> --- a/arch/powerpc/platforms/85xx/p1022_ds.c
> +++ b/arch/powerpc/platforms/85xx/p1022_ds.c
> @@ -348,13 +348,7 @@ void __init p1022_ds_pic_init(void)
>   */
>  static void __init disable_one_node(struct device_node *np, struct property *new)
>  {
> -	struct property *old;
> -
> -	old = of_find_property(np, new->name, NULL);
> -	if (old)
> -		prom_update_property(np, new, old);
> -	else
> -		prom_add_property(np, new);
> +	prom_update_property(np, new);
>  }
>  
>  /* TRUE if there is a "video=fslfb" command-line parameter. */
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> index 029a562..dd30b12 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -67,7 +67,6 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
>  			      const char *name, u32 vd, char *value)
>  {
>  	struct property *new_prop = *prop;
> -	struct property *old_prop;
>  	int more = 0;
>  
>  	/* A negative 'vd' value indicates that only part of the new property
> @@ -117,12 +116,7 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
>  	}
>  
>  	if (!more) {
> -		old_prop = of_find_property(dn, new_prop->name, NULL);
> -		if (old_prop)
> -			prom_update_property(dn, new_prop, old_prop);
> -		else
> -			prom_add_property(dn, new_prop);
> -
> +		prom_update_property(dn, new_prop);
>  		new_prop = NULL;
>  	}
>  
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index 7b3bf76..4c92f1c 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
>  	unsigned char *value;
>  	char *name, *end, *next_prop;
>  	int rc, length;
> -	struct property *newprop, *oldprop;
> +	struct property *newprop;
>  	buf = parse_node(buf, bufsize, &np);
>  	end = buf + bufsize;
>  
> @@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
>  	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
>  		slb_set_size(*(int *)value);
>  
> -	oldprop = of_find_property(np, name,NULL);
> -	if (!oldprop) {
> -		if (strlen(name))
> -			return prom_add_property(np, newprop);
> -		return -ENODEV;
> -	}
> -
>  	upd_value.node = np;
>  	upd_value.property = newprop;
>  	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
>  
> -	rc = prom_update_property(np, newprop, oldprop);
> +	rc = prom_update_property(np, newprop);
>  	if (rc)
>  		return rc;
>  
> @@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
>  
>  		rc = pSeries_reconfig_notify(action, value);
>  		if (rc) {
> -			prom_update_property(np, oldprop, newprop);
> +			prom_update_property(np, newprop);
>  			return rc;
>  		}
>  	}
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d9bfd49..a14f109 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
>  }
>  
>  /*
> - * prom_update_property - Update a property in a node.
> + * prom_update_property - Update a property in a node, if the property does
> + * not exist, add it.
>   *
>   * Note that we don't actually remove it, since we have given out
>   * who-knows-how-many pointers to the data using get-property.
> @@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
>   * and add the new property to the property list
>   */
>  int prom_update_property(struct device_node *np,
> -			 struct property *newprop,
> -			 struct property *oldprop)
> +			 struct property *newprop)
>  {
> -	struct property **next;
> +	struct property **next, *oldprop;
>  	unsigned long flags;
>  	int found = 0;
>  
> +	if (!newprop->name)
> +		return -EINVAL;
> +
> +	oldprop = of_find_property(np, newprop->name, NULL);
> +	if (!oldprop)
> +		return prom_add_property(np, newprop);
> +
>  	write_lock_irqsave(&devtree_lock, flags);
>  	next = &np->properties;
>  	while (*next) {
> diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> index 927cbd1..df7dd08 100644
> --- a/fs/proc/proc_devtree.c
> +++ b/fs/proc/proc_devtree.c
> @@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
>  {
>  	struct proc_dir_entry *ent;
>  
> +	if (!oldprop) {
> +		proc_device_tree_add_prop(pde, newprop);
> +		return;
> +	}
> +
>  	for (ent = pde->subdir; ent != NULL; ent = ent->next)
>  		if (ent->data == oldprop)
>  			break;
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 2ec1083..b27c871 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
>  extern int prom_add_property(struct device_node* np, struct property* prop);
>  extern int prom_remove_property(struct device_node *np, struct property *prop);
>  extern int prom_update_property(struct device_node *np,
> -				struct property *newprop,
> -				struct property *oldprop);
> +				struct property *newprop);
>  
>  #if defined(CONFIG_OF_DYNAMIC)
>  /* For updating the device tree at runtime */

Re: [PATCH 1/1] of: reform prom_update_property function

[Dong Aisheng]

On Wed, Jun 20, 2012 at 09:07:56PM +0800, Rob Herring wrote:
> On 06/20/2012 12:54 AM, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > prom_update_property() currently fails if the property doesn't
> > actually exist yet which isn't what we want. Change to add-or-update
> > instead of update-only, then we can remove a lot duplicated lines.
> > 
> > Suggested-by: Grant Likely <grant.likely@secretlab.ca>
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> 
> Looks fine, but you need to cc powerpc maintainers.
> 
Sorry, i missed that.
Thanks for reminder.

Regards
Dong Aisheng

> 
> >  arch/powerpc/platforms/85xx/p1022_ds.c    |    8 +-------
> >  arch/powerpc/platforms/pseries/mobility.c |    8 +-------
> >  arch/powerpc/platforms/pseries/reconfig.c |   13 +++----------
> >  drivers/of/base.c                         |   15 +++++++++++----
> >  fs/proc/proc_devtree.c                    |    5 +++++
> >  include/linux/of.h                        |    3 +--
> >  6 files changed, 22 insertions(+), 30 deletions(-)
> > 
> > diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
> > index f700c81..d66631c 100644
> > --- a/arch/powerpc/platforms/85xx/p1022_ds.c
> > +++ b/arch/powerpc/platforms/85xx/p1022_ds.c
> > @@ -348,13 +348,7 @@ void __init p1022_ds_pic_init(void)
> >   */
> >  static void __init disable_one_node(struct device_node *np, struct property *new)
> >  {
> > -	struct property *old;
> > -
> > -	old = of_find_property(np, new->name, NULL);
> > -	if (old)
> > -		prom_update_property(np, new, old);
> > -	else
> > -		prom_add_property(np, new);
> > +	prom_update_property(np, new);
> >  }
> >  
> >  /* TRUE if there is a "video=fslfb" command-line parameter. */
> > diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> > index 029a562..dd30b12 100644
> > --- a/arch/powerpc/platforms/pseries/mobility.c
> > +++ b/arch/powerpc/platforms/pseries/mobility.c
> > @@ -67,7 +67,6 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
> >  			      const char *name, u32 vd, char *value)
> >  {
> >  	struct property *new_prop = *prop;
> > -	struct property *old_prop;
> >  	int more = 0;
> >  
> >  	/* A negative 'vd' value indicates that only part of the new property
> > @@ -117,12 +116,7 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
> >  	}
> >  
> >  	if (!more) {
> > -		old_prop = of_find_property(dn, new_prop->name, NULL);
> > -		if (old_prop)
> > -			prom_update_property(dn, new_prop, old_prop);
> > -		else
> > -			prom_add_property(dn, new_prop);
> > -
> > +		prom_update_property(dn, new_prop);
> >  		new_prop = NULL;
> >  	}
> >  
> > diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> > index 7b3bf76..4c92f1c 100644
> > --- a/arch/powerpc/platforms/pseries/reconfig.c
> > +++ b/arch/powerpc/platforms/pseries/reconfig.c
> > @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
> >  	unsigned char *value;
> >  	char *name, *end, *next_prop;
> >  	int rc, length;
> > -	struct property *newprop, *oldprop;
> > +	struct property *newprop;
> >  	buf = parse_node(buf, bufsize, &np);
> >  	end = buf + bufsize;
> >  
> > @@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
> >  	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
> >  		slb_set_size(*(int *)value);
> >  
> > -	oldprop = of_find_property(np, name,NULL);
> > -	if (!oldprop) {
> > -		if (strlen(name))
> > -			return prom_add_property(np, newprop);
> > -		return -ENODEV;
> > -	}
> > -
> >  	upd_value.node = np;
> >  	upd_value.property = newprop;
> >  	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
> >  
> > -	rc = prom_update_property(np, newprop, oldprop);
> > +	rc = prom_update_property(np, newprop);
> >  	if (rc)
> >  		return rc;
> >  
> > @@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
> >  
> >  		rc = pSeries_reconfig_notify(action, value);
> >  		if (rc) {
> > -			prom_update_property(np, oldprop, newprop);
> > +			prom_update_property(np, newprop);
> >  			return rc;
> >  		}
> >  	}
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index d9bfd49..a14f109 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> >  }
> >  
> >  /*
> > - * prom_update_property - Update a property in a node.
> > + * prom_update_property - Update a property in a node, if the property does
> > + * not exist, add it.
> >   *
> >   * Note that we don't actually remove it, since we have given out
> >   * who-knows-how-many pointers to the data using get-property.
> > @@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> >   * and add the new property to the property list
> >   */
> >  int prom_update_property(struct device_node *np,
> > -			 struct property *newprop,
> > -			 struct property *oldprop)
> > +			 struct property *newprop)
> >  {
> > -	struct property **next;
> > +	struct property **next, *oldprop;
> >  	unsigned long flags;
> >  	int found = 0;
> >  
> > +	if (!newprop->name)
> > +		return -EINVAL;
> > +
> > +	oldprop = of_find_property(np, newprop->name, NULL);
> > +	if (!oldprop)
> > +		return prom_add_property(np, newprop);
> > +
> >  	write_lock_irqsave(&devtree_lock, flags);
> >  	next = &np->properties;
> >  	while (*next) {
> > diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> > index 927cbd1..df7dd08 100644
> > --- a/fs/proc/proc_devtree.c
> > +++ b/fs/proc/proc_devtree.c
> > @@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
> >  {
> >  	struct proc_dir_entry *ent;
> >  
> > +	if (!oldprop) {
> > +		proc_device_tree_add_prop(pde, newprop);
> > +		return;
> > +	}
> > +
> >  	for (ent = pde->subdir; ent != NULL; ent = ent->next)
> >  		if (ent->data == oldprop)
> >  			break;
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 2ec1083..b27c871 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
> >  extern int prom_add_property(struct device_node* np, struct property* prop);
> >  extern int prom_remove_property(struct device_node *np, struct property *prop);
> >  extern int prom_update_property(struct device_node *np,
> > -				struct property *newprop,
> > -				struct property *oldprop);
> > +				struct property *newprop);
> >  
> >  #if defined(CONFIG_OF_DYNAMIC)
> >  /* For updating the device tree at runtime */
> 
> 

Re: [PATCH 1/1] of: reform prom_update_property function

[Dong Aisheng]

On Wed, Jun 20, 2012 at 09:07:56PM +0800, Rob Herring wrote:
> On 06/20/2012 12:54 AM, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > prom_update_property() currently fails if the property doesn't
> > actually exist yet which isn't what we want. Change to add-or-update
> > instead of update-only, then we can remove a lot duplicated lines.
> > 
> > Suggested-by: Grant Likely <grant.likely@secretlab.ca>
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> 
> Looks fine, but you need to cc powerpc maintainers.
> 
Sorry, i missed that.
Thanks for reminder.

Regards
Dong Aisheng

> 
> >  arch/powerpc/platforms/85xx/p1022_ds.c    |    8 +-------
> >  arch/powerpc/platforms/pseries/mobility.c |    8 +-------
> >  arch/powerpc/platforms/pseries/reconfig.c |   13 +++----------
> >  drivers/of/base.c                         |   15 +++++++++++----
> >  fs/proc/proc_devtree.c                    |    5 +++++
> >  include/linux/of.h                        |    3 +--
> >  6 files changed, 22 insertions(+), 30 deletions(-)
> > 
> > diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
> > index f700c81..d66631c 100644
> > --- a/arch/powerpc/platforms/85xx/p1022_ds.c
> > +++ b/arch/powerpc/platforms/85xx/p1022_ds.c
> > @@ -348,13 +348,7 @@ void __init p1022_ds_pic_init(void)
> >   */
> >  static void __init disable_one_node(struct device_node *np, struct property *new)
> >  {
> > -	struct property *old;
> > -
> > -	old = of_find_property(np, new->name, NULL);
> > -	if (old)
> > -		prom_update_property(np, new, old);
> > -	else
> > -		prom_add_property(np, new);
> > +	prom_update_property(np, new);
> >  }
> >  
> >  /* TRUE if there is a "video=fslfb" command-line parameter. */
> > diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> > index 029a562..dd30b12 100644
> > --- a/arch/powerpc/platforms/pseries/mobility.c
> > +++ b/arch/powerpc/platforms/pseries/mobility.c
> > @@ -67,7 +67,6 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
> >  			      const char *name, u32 vd, char *value)
> >  {
> >  	struct property *new_prop = *prop;
> > -	struct property *old_prop;
> >  	int more = 0;
> >  
> >  	/* A negative 'vd' value indicates that only part of the new property
> > @@ -117,12 +116,7 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
> >  	}
> >  
> >  	if (!more) {
> > -		old_prop = of_find_property(dn, new_prop->name, NULL);
> > -		if (old_prop)
> > -			prom_update_property(dn, new_prop, old_prop);
> > -		else
> > -			prom_add_property(dn, new_prop);
> > -
> > +		prom_update_property(dn, new_prop);
> >  		new_prop = NULL;
> >  	}
> >  
> > diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> > index 7b3bf76..4c92f1c 100644
> > --- a/arch/powerpc/platforms/pseries/reconfig.c
> > +++ b/arch/powerpc/platforms/pseries/reconfig.c
> > @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
> >  	unsigned char *value;
> >  	char *name, *end, *next_prop;
> >  	int rc, length;
> > -	struct property *newprop, *oldprop;
> > +	struct property *newprop;
> >  	buf = parse_node(buf, bufsize, &np);
> >  	end = buf + bufsize;
> >  
> > @@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
> >  	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
> >  		slb_set_size(*(int *)value);
> >  
> > -	oldprop = of_find_property(np, name,NULL);
> > -	if (!oldprop) {
> > -		if (strlen(name))
> > -			return prom_add_property(np, newprop);
> > -		return -ENODEV;
> > -	}
> > -
> >  	upd_value.node = np;
> >  	upd_value.property = newprop;
> >  	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
> >  
> > -	rc = prom_update_property(np, newprop, oldprop);
> > +	rc = prom_update_property(np, newprop);
> >  	if (rc)
> >  		return rc;
> >  
> > @@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
> >  
> >  		rc = pSeries_reconfig_notify(action, value);
> >  		if (rc) {
> > -			prom_update_property(np, oldprop, newprop);
> > +			prom_update_property(np, newprop);
> >  			return rc;
> >  		}
> >  	}
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index d9bfd49..a14f109 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> >  }
> >  
> >  /*
> > - * prom_update_property - Update a property in a node.
> > + * prom_update_property - Update a property in a node, if the property does
> > + * not exist, add it.
> >   *
> >   * Note that we don't actually remove it, since we have given out
> >   * who-knows-how-many pointers to the data using get-property.
> > @@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> >   * and add the new property to the property list
> >   */
> >  int prom_update_property(struct device_node *np,
> > -			 struct property *newprop,
> > -			 struct property *oldprop)
> > +			 struct property *newprop)
> >  {
> > -	struct property **next;
> > +	struct property **next, *oldprop;
> >  	unsigned long flags;
> >  	int found = 0;
> >  
> > +	if (!newprop->name)
> > +		return -EINVAL;
> > +
> > +	oldprop = of_find_property(np, newprop->name, NULL);
> > +	if (!oldprop)
> > +		return prom_add_property(np, newprop);
> > +
> >  	write_lock_irqsave(&devtree_lock, flags);
> >  	next = &np->properties;
> >  	while (*next) {
> > diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> > index 927cbd1..df7dd08 100644
> > --- a/fs/proc/proc_devtree.c
> > +++ b/fs/proc/proc_devtree.c
> > @@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
> >  {
> >  	struct proc_dir_entry *ent;
> >  
> > +	if (!oldprop) {
> > +		proc_device_tree_add_prop(pde, newprop);
> > +		return;
> > +	}
> > +
> >  	for (ent = pde->subdir; ent != NULL; ent = ent->next)
> >  		if (ent->data == oldprop)
> >  			break;
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 2ec1083..b27c871 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
> >  extern int prom_add_property(struct device_node* np, struct property* prop);
> >  extern int prom_remove_property(struct device_node *np, struct property *prop);
> >  extern int prom_update_property(struct device_node *np,
> > -				struct property *newprop,
> > -				struct property *oldprop);
> > +				struct property *newprop);
> >  
> >  #if defined(CONFIG_OF_DYNAMIC)
> >  /* For updating the device tree at runtime */
> 
> 

Re: [PATCH 1/1] of: reform prom_update_property function

[Benjamin Herrenschmidt]

On Wed, 2012-06-20 at 08:07 -0500, Rob Herring wrote:
> On 06/20/2012 12:54 AM, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > prom_update_property() currently fails if the property doesn't
> > actually exist yet which isn't what we want. Change to add-or-update
> > instead of update-only, then we can remove a lot duplicated lines.
> > 
> > Suggested-by: Grant Likely <grant.likely@secretlab.ca>
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> 
> Looks fine, but you need to cc powerpc maintainers.

Generally fine, just one issue I spotted:

> > diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> > index 7b3bf76..4c92f1c 100644
> > --- a/arch/powerpc/platforms/pseries/reconfig.c
> > +++ b/arch/powerpc/platforms/pseries/reconfig.c
> > @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
> >  	unsigned char *value;
> >  	char *name, *end, *next_prop;
> >  	int rc, length;
> > -	struct property *newprop, *oldprop;
> > +	struct property *newprop;
> >  	buf = parse_node(buf, bufsize, &np);
> >  	end = buf + bufsize;
> >  
> > @@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
> >  	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
> >  		slb_set_size(*(int *)value);
> >  
> > -	oldprop = of_find_property(np, name,NULL);
> > -	if (!oldprop) {
> > -		if (strlen(name))
> > -			return prom_add_property(np, newprop);
> > -		return -ENODEV;
> > -	}
> > -

Here the code would exit the function with an error if the property
didn't already exist, you are losing that semantic.

Additionally there's that oddball test for strlen(name) ... you might
want to check that somewhere... and the strange fact that it would
actually add the new property anyway despite returning an error which is
very very odd semantics but I'd rather not break them since this is
exposed to userspace, so we may have tools relying on them.

> >  	upd_value.node = np;
> >  	upd_value.property = newprop;
> >  	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
> >  
> > -	rc = prom_update_property(np, newprop, oldprop);
> > +	rc = prom_update_property(np, newprop);
> >  	if (rc)
> >  		return rc;
> >  
> > @@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
> >  
> >  		rc = pSeries_reconfig_notify(action, value);
> >  		if (rc) {
> > -			prom_update_property(np, oldprop, newprop);
> > +			prom_update_property(np, newprop);
> >  			return rc;
> >  		}
> >  	}
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index d9bfd49..a14f109 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> >  }
> >  
> >  /*
> > - * prom_update_property - Update a property in a node.
> > + * prom_update_property - Update a property in a node, if the property does
> > + * not exist, add it.
> >   *
> >   * Note that we don't actually remove it, since we have given out
> >   * who-knows-how-many pointers to the data using get-property.
> > @@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> >   * and add the new property to the property list
> >   */
> >  int prom_update_property(struct device_node *np,
> > -			 struct property *newprop,
> > -			 struct property *oldprop)
> > +			 struct property *newprop)
> >  {
> > -	struct property **next;
> > +	struct property **next, *oldprop;
> >  	unsigned long flags;
> >  	int found = 0;
> >  
> > +	if (!newprop->name)
> > +		return -EINVAL;
> > +
> > +	oldprop = of_find_property(np, newprop->name, NULL);
> > +	if (!oldprop)
> > +		return prom_add_property(np, newprop);
> > +
> >  	write_lock_irqsave(&devtree_lock, flags);
> >  	next = &np->properties;
> >  	while (*next) {
> > diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> > index 927cbd1..df7dd08 100644
> > --- a/fs/proc/proc_devtree.c
> > +++ b/fs/proc/proc_devtree.c
> > @@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
> >  {
> >  	struct proc_dir_entry *ent;
> >  
> > +	if (!oldprop) {
> > +		proc_device_tree_add_prop(pde, newprop);
> > +		return;
> > +	}
> > +
> >  	for (ent = pde->subdir; ent != NULL; ent = ent->next)
> >  		if (ent->data == oldprop)
> >  			break;
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 2ec1083..b27c871 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
> >  extern int prom_add_property(struct device_node* np, struct property* prop);
> >  extern int prom_remove_property(struct device_node *np, struct property *prop);
> >  extern int prom_update_property(struct device_node *np,
> > -				struct property *newprop,
> > -				struct property *oldprop);
> > +				struct property *newprop);
> >  
> >  #if defined(CONFIG_OF_DYNAMIC)
> >  /* For updating the device tree at runtime */

Note that another issue you aren't addressing is that this is a
fundamentally leaky operation.

IE. The allocation of the "old" property isn't disposed of. It can't
because today we don't know whether any of those pointers was
dynamically allocated or not. IE they could point to the fdt
string list, data block, or could be bootmem ... or could be
actual kernel memory.

We might want to extend the struct property to contain indications of
the allocation type so we can kfree dynamic properties properly.

But then there's the question of the lifetime of a property... since
they aren't reference counted like nodes are.

Cheers,
Ben.

Re: [PATCH 1/1] of: reform prom_update_property function

[Benjamin Herrenschmidt]

On Wed, 2012-06-20 at 08:07 -0500, Rob Herring wrote:
> On 06/20/2012 12:54 AM, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > 
> > prom_update_property() currently fails if the property doesn't
> > actually exist yet which isn't what we want. Change to add-or-update
> > instead of update-only, then we can remove a lot duplicated lines.
> > 
> > Suggested-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> > Signed-off-by: Dong Aisheng <dong.aisheng-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> 
> Looks fine, but you need to cc powerpc maintainers.

Generally fine, just one issue I spotted:

> > diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> > index 7b3bf76..4c92f1c 100644
> > --- a/arch/powerpc/platforms/pseries/reconfig.c
> > +++ b/arch/powerpc/platforms/pseries/reconfig.c
> > @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
> >  	unsigned char *value;
> >  	char *name, *end, *next_prop;
> >  	int rc, length;
> > -	struct property *newprop, *oldprop;
> > +	struct property *newprop;
> >  	buf = parse_node(buf, bufsize, &np);
> >  	end = buf + bufsize;
> >  
> > @@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
> >  	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
> >  		slb_set_size(*(int *)value);
> >  
> > -	oldprop = of_find_property(np, name,NULL);
> > -	if (!oldprop) {
> > -		if (strlen(name))
> > -			return prom_add_property(np, newprop);
> > -		return -ENODEV;
> > -	}
> > -

Here the code would exit the function with an error if the property
didn't already exist, you are losing that semantic.

Additionally there's that oddball test for strlen(name) ... you might
want to check that somewhere... and the strange fact that it would
actually add the new property anyway despite returning an error which is
very very odd semantics but I'd rather not break them since this is
exposed to userspace, so we may have tools relying on them.

> >  	upd_value.node = np;
> >  	upd_value.property = newprop;
> >  	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
> >  
> > -	rc = prom_update_property(np, newprop, oldprop);
> > +	rc = prom_update_property(np, newprop);
> >  	if (rc)
> >  		return rc;
> >  
> > @@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
> >  
> >  		rc = pSeries_reconfig_notify(action, value);
> >  		if (rc) {
> > -			prom_update_property(np, oldprop, newprop);
> > +			prom_update_property(np, newprop);
> >  			return rc;
> >  		}
> >  	}
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index d9bfd49..a14f109 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> >  }
> >  
> >  /*
> > - * prom_update_property - Update a property in a node.
> > + * prom_update_property - Update a property in a node, if the property does
> > + * not exist, add it.
> >   *
> >   * Note that we don't actually remove it, since we have given out
> >   * who-knows-how-many pointers to the data using get-property.
> > @@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> >   * and add the new property to the property list
> >   */
> >  int prom_update_property(struct device_node *np,
> > -			 struct property *newprop,
> > -			 struct property *oldprop)
> > +			 struct property *newprop)
> >  {
> > -	struct property **next;
> > +	struct property **next, *oldprop;
> >  	unsigned long flags;
> >  	int found = 0;
> >  
> > +	if (!newprop->name)
> > +		return -EINVAL;
> > +
> > +	oldprop = of_find_property(np, newprop->name, NULL);
> > +	if (!oldprop)
> > +		return prom_add_property(np, newprop);
> > +
> >  	write_lock_irqsave(&devtree_lock, flags);
> >  	next = &np->properties;
> >  	while (*next) {
> > diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> > index 927cbd1..df7dd08 100644
> > --- a/fs/proc/proc_devtree.c
> > +++ b/fs/proc/proc_devtree.c
> > @@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
> >  {
> >  	struct proc_dir_entry *ent;
> >  
> > +	if (!oldprop) {
> > +		proc_device_tree_add_prop(pde, newprop);
> > +		return;
> > +	}
> > +
> >  	for (ent = pde->subdir; ent != NULL; ent = ent->next)
> >  		if (ent->data == oldprop)
> >  			break;
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 2ec1083..b27c871 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
> >  extern int prom_add_property(struct device_node* np, struct property* prop);
> >  extern int prom_remove_property(struct device_node *np, struct property *prop);
> >  extern int prom_update_property(struct device_node *np,
> > -				struct property *newprop,
> > -				struct property *oldprop);
> > +				struct property *newprop);
> >  
> >  #if defined(CONFIG_OF_DYNAMIC)
> >  /* For updating the device tree at runtime */

Note that another issue you aren't addressing is that this is a
fundamentally leaky operation.

IE. The allocation of the "old" property isn't disposed of. It can't
because today we don't know whether any of those pointers was
dynamically allocated or not. IE they could point to the fdt
string list, data block, or could be bootmem ... or could be
actual kernel memory.

We might want to extend the struct property to contain indications of
the allocation type so we can kfree dynamic properties properly.

But then there's the question of the lifetime of a property... since
they aren't reference counted like nodes are.

Cheers,
Ben.

Re: [PATCH 1/1] of: reform prom_update_property function

[Dong Aisheng]

On Thu, Jun 21, 2012 at 08:16:36AM +0800, Benjamin Herrenschmidt wrote:
> On Wed, 2012-06-20 at 08:07 -0500, Rob Herring wrote:
> > On 06/20/2012 12:54 AM, Dong Aisheng wrote:
> > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > prom_update_property() currently fails if the property doesn't
> > > actually exist yet which isn't what we want. Change to add-or-update
> > > instead of update-only, then we can remove a lot duplicated lines.
> > > 
> > > Suggested-by: Grant Likely <grant.likely@secretlab.ca>
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > ---
> > 
> > Looks fine, but you need to cc powerpc maintainers.
> 
> Generally fine, just one issue I spotted:
> 
Thanks for the review.

> > > diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> > > index 7b3bf76..4c92f1c 100644
> > > --- a/arch/powerpc/platforms/pseries/reconfig.c
> > > +++ b/arch/powerpc/platforms/pseries/reconfig.c
> > > @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
> > >  	unsigned char *value;
> > >  	char *name, *end, *next_prop;
> > >  	int rc, length;
> > > -	struct property *newprop, *oldprop;
> > > +	struct property *newprop;
> > >  	buf = parse_node(buf, bufsize, &np);
> > >  	end = buf + bufsize;
> > >  
> > > @@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
> > >  	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
> > >  		slb_set_size(*(int *)value);
> > >  
> > > -	oldprop = of_find_property(np, name,NULL);
> > > -	if (!oldprop) {
> > > -		if (strlen(name))
> > > -			return prom_add_property(np, newprop);
> > > -		return -ENODEV;
> > > -	}
> > > -
> 
> Here the code would exit the function with an error if the property
> didn't already exist, you are losing that semantic.
> 
You're correct.

> Additionally there's that oddball test for strlen(name) ... you might
> want to check that somewhere... and the strange fact that it would
> actually add the new property anyway despite returning an error which is
> very very odd semantics but I'd rather not break them since this is
> exposed to userspace, so we may have tools relying on them.
> 
Right, i also feel it's strange.
I do not know that code history, what i can do may be try to keep
the code follow the same as before.

Maybe we could change it as as follows.
It looks then the code follow is the same as before.
Do you think if it's ok?

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 7b3bf76..4c237f4 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize)
        if (!next_prop)
                return -EINVAL;

+       if (!strlen(name)
+               return -ENODEV;
+
        newprop = new_property(name, length, value, NULL);
        if (!newprop)
                return -ENOMEM;
@@ -450,13 +453,6 @@ static int do_update_property(char *buf, size_t bufsize)
        if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
                slb_set_size(*(int *)value);

-       oldprop = of_find_property(np, name,NULL);
-       if (!oldprop) {
-               if (strlen(name))
-                       return prom_add_property(np, newprop);
-               return -ENODEV;
-       }
-

> > >  	upd_value.node = np;
> > >  	upd_value.property = newprop;
> > >  	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
> > >  
> > > -	rc = prom_update_property(np, newprop, oldprop);
> > > +	rc = prom_update_property(np, newprop);
> > >  	if (rc)
> > >  		return rc;
> > >  
> > > @@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
> > >  
> > >  		rc = pSeries_reconfig_notify(action, value);
> > >  		if (rc) {
> > > -			prom_update_property(np, oldprop, newprop);
> > > +			prom_update_property(np, newprop);
> > >  			return rc;
> > >  		}
> > >  	}
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index d9bfd49..a14f109 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> > >  }
> > >  
> > >  /*
> > > - * prom_update_property - Update a property in a node.
> > > + * prom_update_property - Update a property in a node, if the property does
> > > + * not exist, add it.
> > >   *
> > >   * Note that we don't actually remove it, since we have given out
> > >   * who-knows-how-many pointers to the data using get-property.
> > > @@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> > >   * and add the new property to the property list
> > >   */
> > >  int prom_update_property(struct device_node *np,
> > > -			 struct property *newprop,
> > > -			 struct property *oldprop)
> > > +			 struct property *newprop)
> > >  {
> > > -	struct property **next;
> > > +	struct property **next, *oldprop;
> > >  	unsigned long flags;
> > >  	int found = 0;
> > >  
> > > +	if (!newprop->name)
> > > +		return -EINVAL;
> > > +
> > > +	oldprop = of_find_property(np, newprop->name, NULL);
> > > +	if (!oldprop)
> > > +		return prom_add_property(np, newprop);
> > > +
> > >  	write_lock_irqsave(&devtree_lock, flags);
> > >  	next = &np->properties;
> > >  	while (*next) {
> > > diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> > > index 927cbd1..df7dd08 100644
> > > --- a/fs/proc/proc_devtree.c
> > > +++ b/fs/proc/proc_devtree.c
> > > @@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
> > >  {
> > >  	struct proc_dir_entry *ent;
> > >  
> > > +	if (!oldprop) {
> > > +		proc_device_tree_add_prop(pde, newprop);
> > > +		return;
> > > +	}
> > > +
> > >  	for (ent = pde->subdir; ent != NULL; ent = ent->next)
> > >  		if (ent->data == oldprop)
> > >  			break;
> > > diff --git a/include/linux/of.h b/include/linux/of.h
> > > index 2ec1083..b27c871 100644
> > > --- a/include/linux/of.h
> > > +++ b/include/linux/of.h
> > > @@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
> > >  extern int prom_add_property(struct device_node* np, struct property* prop);
> > >  extern int prom_remove_property(struct device_node *np, struct property *prop);
> > >  extern int prom_update_property(struct device_node *np,
> > > -				struct property *newprop,
> > > -				struct property *oldprop);
> > > +				struct property *newprop);
> > >  
> > >  #if defined(CONFIG_OF_DYNAMIC)
> > >  /* For updating the device tree at runtime */
> 
> Note that another issue you aren't addressing is that this is a
> fundamentally leaky operation.
> 
> IE. The allocation of the "old" property isn't disposed of. It can't
> because today we don't know whether any of those pointers was
> dynamically allocated or not. IE they could point to the fdt
Hmm, i did not see static allocated property before.
Where can we see an exist case?

If we really have this issue, it seems of_node_release also has the same issue,
since it frees the property without distinguish whether the property is allocated
dynamically.

> string list, data block, or could be bootmem ... or could be
> actual kernel memory.
> 
> We might want to extend the struct property to contain indications of
> the allocation type so we can kfree dynamic properties properly.
> 
I wonder the simplest way may be not allow static allocated property, like dt
node does i guess.

> But then there's the question of the lifetime of a property... since
> they aren't reference counted like nodes are.
> 
Yes, that's a real exist problem.

Anyway, i guess we could do that work of this problem in another patch
rather than have to in this patch series.

Regards
Dong Aisheng

Re: [PATCH 1/1] of: reform prom_update_property function

[Dong Aisheng]

On Thu, Jun 21, 2012 at 08:16:36AM +0800, Benjamin Herrenschmidt wrote:
> On Wed, 2012-06-20 at 08:07 -0500, Rob Herring wrote:
> > On 06/20/2012 12:54 AM, Dong Aisheng wrote:
> > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > prom_update_property() currently fails if the property doesn't
> > > actually exist yet which isn't what we want. Change to add-or-update
> > > instead of update-only, then we can remove a lot duplicated lines.
> > > 
> > > Suggested-by: Grant Likely <grant.likely@secretlab.ca>
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > ---
> > 
> > Looks fine, but you need to cc powerpc maintainers.
> 
> Generally fine, just one issue I spotted:
> 
Thanks for the review.

> > > diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> > > index 7b3bf76..4c92f1c 100644
> > > --- a/arch/powerpc/platforms/pseries/reconfig.c
> > > +++ b/arch/powerpc/platforms/pseries/reconfig.c
> > > @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
> > >  	unsigned char *value;
> > >  	char *name, *end, *next_prop;
> > >  	int rc, length;
> > > -	struct property *newprop, *oldprop;
> > > +	struct property *newprop;
> > >  	buf = parse_node(buf, bufsize, &np);
> > >  	end = buf + bufsize;
> > >  
> > > @@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t bufsize)
> > >  	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
> > >  		slb_set_size(*(int *)value);
> > >  
> > > -	oldprop = of_find_property(np, name,NULL);
> > > -	if (!oldprop) {
> > > -		if (strlen(name))
> > > -			return prom_add_property(np, newprop);
> > > -		return -ENODEV;
> > > -	}
> > > -
> 
> Here the code would exit the function with an error if the property
> didn't already exist, you are losing that semantic.
> 
You're correct.

> Additionally there's that oddball test for strlen(name) ... you might
> want to check that somewhere... and the strange fact that it would
> actually add the new property anyway despite returning an error which is
> very very odd semantics but I'd rather not break them since this is
> exposed to userspace, so we may have tools relying on them.
> 
Right, i also feel it's strange.
I do not know that code history, what i can do may be try to keep
the code follow the same as before.

Maybe we could change it as as follows.
It looks then the code follow is the same as before.
Do you think if it's ok?

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 7b3bf76..4c237f4 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize)
        if (!next_prop)
                return -EINVAL;

+       if (!strlen(name)
+               return -ENODEV;
+
        newprop = new_property(name, length, value, NULL);
        if (!newprop)
                return -ENOMEM;
@@ -450,13 +453,6 @@ static int do_update_property(char *buf, size_t bufsize)
        if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
                slb_set_size(*(int *)value);

-       oldprop = of_find_property(np, name,NULL);
-       if (!oldprop) {
-               if (strlen(name))
-                       return prom_add_property(np, newprop);
-               return -ENODEV;
-       }
-

> > >  	upd_value.node = np;
> > >  	upd_value.property = newprop;
> > >  	pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
> > >  
> > > -	rc = prom_update_property(np, newprop, oldprop);
> > > +	rc = prom_update_property(np, newprop);
> > >  	if (rc)
> > >  		return rc;
> > >  
> > > @@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
> > >  
> > >  		rc = pSeries_reconfig_notify(action, value);
> > >  		if (rc) {
> > > -			prom_update_property(np, oldprop, newprop);
> > > +			prom_update_property(np, newprop);
> > >  			return rc;
> > >  		}
> > >  	}
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index d9bfd49..a14f109 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> > >  }
> > >  
> > >  /*
> > > - * prom_update_property - Update a property in a node.
> > > + * prom_update_property - Update a property in a node, if the property does
> > > + * not exist, add it.
> > >   *
> > >   * Note that we don't actually remove it, since we have given out
> > >   * who-knows-how-many pointers to the data using get-property.
> > > @@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np, struct property *prop)
> > >   * and add the new property to the property list
> > >   */
> > >  int prom_update_property(struct device_node *np,
> > > -			 struct property *newprop,
> > > -			 struct property *oldprop)
> > > +			 struct property *newprop)
> > >  {
> > > -	struct property **next;
> > > +	struct property **next, *oldprop;
> > >  	unsigned long flags;
> > >  	int found = 0;
> > >  
> > > +	if (!newprop->name)
> > > +		return -EINVAL;
> > > +
> > > +	oldprop = of_find_property(np, newprop->name, NULL);
> > > +	if (!oldprop)
> > > +		return prom_add_property(np, newprop);
> > > +
> > >  	write_lock_irqsave(&devtree_lock, flags);
> > >  	next = &np->properties;
> > >  	while (*next) {
> > > diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> > > index 927cbd1..df7dd08 100644
> > > --- a/fs/proc/proc_devtree.c
> > > +++ b/fs/proc/proc_devtree.c
> > > @@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct proc_dir_entry *pde,
> > >  {
> > >  	struct proc_dir_entry *ent;
> > >  
> > > +	if (!oldprop) {
> > > +		proc_device_tree_add_prop(pde, newprop);
> > > +		return;
> > > +	}
> > > +
> > >  	for (ent = pde->subdir; ent != NULL; ent = ent->next)
> > >  		if (ent->data == oldprop)
> > >  			break;
> > > diff --git a/include/linux/of.h b/include/linux/of.h
> > > index 2ec1083..b27c871 100644
> > > --- a/include/linux/of.h
> > > +++ b/include/linux/of.h
> > > @@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
> > >  extern int prom_add_property(struct device_node* np, struct property* prop);
> > >  extern int prom_remove_property(struct device_node *np, struct property *prop);
> > >  extern int prom_update_property(struct device_node *np,
> > > -				struct property *newprop,
> > > -				struct property *oldprop);
> > > +				struct property *newprop);
> > >  
> > >  #if defined(CONFIG_OF_DYNAMIC)
> > >  /* For updating the device tree at runtime */
> 
> Note that another issue you aren't addressing is that this is a
> fundamentally leaky operation.
> 
> IE. The allocation of the "old" property isn't disposed of. It can't
> because today we don't know whether any of those pointers was
> dynamically allocated or not. IE they could point to the fdt
Hmm, i did not see static allocated property before.
Where can we see an exist case?

If we really have this issue, it seems of_node_release also has the same issue,
since it frees the property without distinguish whether the property is allocated
dynamically.

> string list, data block, or could be bootmem ... or could be
> actual kernel memory.
> 
> We might want to extend the struct property to contain indications of
> the allocation type so we can kfree dynamic properties properly.
> 
I wonder the simplest way may be not allow static allocated property, like dt
node does i guess.

> But then there's the question of the lifetime of a property... since
> they aren't reference counted like nodes are.
> 
Yes, that's a real exist problem.

Anyway, i guess we could do that work of this problem in another patch
rather than have to in this patch series.

Regards
Dong Aisheng

Re: [PATCH 1/1] of: reform prom_update_property function

[Benjamin Herrenschmidt]

On Thu, 2012-06-21 at 17:37 +0800, Dong Aisheng wrote:
> Maybe we could change it as as follows.
> It looks then the code follow is the same as before.
> Do you think if it's ok?
> 
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index 7b3bf76..4c237f4 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize)
>         if (!next_prop)
>                 return -EINVAL;
> 
> +       if (!strlen(name)
> +               return -ENODEV;
> +
>         newprop = new_property(name, length, value, NULL);
>         if (!newprop)
>                 return -ENOMEM;
> @@ -450,13 +453,6 @@ static int do_update_property(char *buf, size_t bufsize)
>         if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
>                 slb_set_size(*(int *)value);
> 
> -       oldprop = of_find_property(np, name,NULL);
> -       if (!oldprop) {
> -               if (strlen(name))
> -                       return prom_add_property(np, newprop);
> -               return -ENODEV;
> -       }

No:

IE. Old code did:

	if (property doesn't exist) {
		if (has a name)
			create_it()
		return -ENODEV;
	}

What you propose is:

	if (!has_a_name)
		return -ENODEV;

Not at all the same semantic.

 .../...
 
> > IE. The allocation of the "old" property isn't disposed of. It can't
> > because today we don't know whether any of those pointers was
> > dynamically allocated or not. IE they could point to the fdt

> Hmm, i did not see static allocated property before.
> Where can we see an exist case?

Almost all your property names and values. They are pointers to the
original fdt block, so you can't free them. But dynamically added
propreties will have kmalloc'ed pointers which should be freed. We need
to add flags to indicate that if we want to avoid leaking memory in very
dynamic environments.

> If we really have this issue, it seems of_node_release also has the same issue,
> since it frees the property without distinguish whether the property is allocated
> dynamically.

Well, actually we do have a flag:

        if (!of_node_check_flag(node, OF_DYNAMIC))
                return;

So we use that. Good. So if update property uses that flag it should be
able to know when to free or not. I forgot we had that :-)

> > string list, data block, or could be bootmem ... or could be
> > actual kernel memory.
> > 
> > We might want to extend the struct property to contain indications of
> > the allocation type so we can kfree dynamic properties properly.
> > 
> I wonder the simplest way may be not allow static allocated property, like dt
> node does i guess.

No way. We generate the device-tree way before we have an allocator
available.

> > But then there's the question of the lifetime of a property... since
> > they aren't reference counted like nodes are.
> > 
> Yes, that's a real exist problem.
> 
> Anyway, i guess we could do that work of this problem in another patch
> rather than have to in this patch series.

Cheers,
Ben.

Re: [PATCH 1/1] of: reform prom_update_property function

[Benjamin Herrenschmidt]

On Thu, 2012-06-21 at 17:37 +0800, Dong Aisheng wrote:
> Maybe we could change it as as follows.
> It looks then the code follow is the same as before.
> Do you think if it's ok?
> 
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index 7b3bf76..4c237f4 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize)
>         if (!next_prop)
>                 return -EINVAL;
> 
> +       if (!strlen(name)
> +               return -ENODEV;
> +
>         newprop = new_property(name, length, value, NULL);
>         if (!newprop)
>                 return -ENOMEM;
> @@ -450,13 +453,6 @@ static int do_update_property(char *buf, size_t bufsize)
>         if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
>                 slb_set_size(*(int *)value);
> 
> -       oldprop = of_find_property(np, name,NULL);
> -       if (!oldprop) {
> -               if (strlen(name))
> -                       return prom_add_property(np, newprop);
> -               return -ENODEV;
> -       }

No:

IE. Old code did:

	if (property doesn't exist) {
		if (has a name)
			create_it()
		return -ENODEV;
	}

What you propose is:

	if (!has_a_name)
		return -ENODEV;

Not at all the same semantic.

 .../...
 
> > IE. The allocation of the "old" property isn't disposed of. It can't
> > because today we don't know whether any of those pointers was
> > dynamically allocated or not. IE they could point to the fdt

> Hmm, i did not see static allocated property before.
> Where can we see an exist case?

Almost all your property names and values. They are pointers to the
original fdt block, so you can't free them. But dynamically added
propreties will have kmalloc'ed pointers which should be freed. We need
to add flags to indicate that if we want to avoid leaking memory in very
dynamic environments.

> If we really have this issue, it seems of_node_release also has the same issue,
> since it frees the property without distinguish whether the property is allocated
> dynamically.

Well, actually we do have a flag:

        if (!of_node_check_flag(node, OF_DYNAMIC))
                return;

So we use that. Good. So if update property uses that flag it should be
able to know when to free or not. I forgot we had that :-)

> > string list, data block, or could be bootmem ... or could be
> > actual kernel memory.
> > 
> > We might want to extend the struct property to contain indications of
> > the allocation type so we can kfree dynamic properties properly.
> > 
> I wonder the simplest way may be not allow static allocated property, like dt
> node does i guess.

No way. We generate the device-tree way before we have an allocator
available.

> > But then there's the question of the lifetime of a property... since
> > they aren't reference counted like nodes are.
> > 
> Yes, that's a real exist problem.
> 
> Anyway, i guess we could do that work of this problem in another patch
> rather than have to in this patch series.

Cheers,
Ben.

Re: [PATCH 1/1] of: reform prom_update_property function

[Dong Aisheng]

On Fri, Jun 22, 2012 at 8:01 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2012-06-21 at 17:37 +0800, Dong Aisheng wrote:
>> Maybe we could change it as as follows.
>> It looks then the code follow is the same as before.
>> Do you think if it's ok?
>>
>> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
>> index 7b3bf76..4c237f4 100644
>> --- a/arch/powerpc/platforms/pseries/reconfig.c
>> +++ b/arch/powerpc/platforms/pseries/reconfig.c
>> @@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize)
>>         if (!next_prop)
>>                 return -EINVAL;
>>
>> +       if (!strlen(name)
>> +               return -ENODEV;
>> +
>>         newprop = new_property(name, length, value, NULL);
>>         if (!newprop)
>>                 return -ENOMEM;
>> @@ -450,13 +453,6 @@ static int do_update_property(char *buf, size_t bufsize)
>>         if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
>>                 slb_set_size(*(int *)value);
>>
>> -       oldprop = of_find_property(np, name,NULL);
>> -       if (!oldprop) {
>> -               if (strlen(name))
>> -                       return prom_add_property(np, newprop);
>> -               return -ENODEV;
>> -       }
>
> No:
>
> IE. Old code did:
>
>        if (property doesn't exist) {
>                if (has a name)
>                        create_it()
>                return -ENODEV;
>        }
>
What i saw is:
       if (property doesn't exist) {
               if (has a name)
                       return create_it()
               return -ENODEV;
       }
Which seems the same behavior as the new prop_update_property api.
The only different is if no name, return -EINVAL;
Am i wrong?

> What you propose is:
>
>        if (!has_a_name)
>                return -ENODEV;
>
> Not at all the same semantic.
>
>  .../...
>
>> > IE. The allocation of the "old" property isn't disposed of. It can't
>> > because today we don't know whether any of those pointers was
>> > dynamically allocated or not. IE they could point to the fdt
>
>> Hmm, i did not see static allocated property before.
>> Where can we see an exist case?
>
> Almost all your property names and values. They are pointers to the
> original fdt block, so you can't free them. But dynamically added
> propreties will have kmalloc'ed pointers which should be freed. We need
> to add flags to indicate that if we want to avoid leaking memory in very
> dynamic environments.
>
Okay, got it, thanks for clarify.

>> If we really have this issue, it seems of_node_release also has the same issue,
>> since it frees the property without distinguish whether the property is allocated
>> dynamically.
>
> Well, actually we do have a flag:
>
>        if (!of_node_check_flag(node, OF_DYNAMIC))
>                return;
>
Oh, i see.

> So we use that. Good. So if update property uses that flag it should be
> able to know when to free or not. I forgot we had that :-)
>
I'm still not sure whether we should free the property in update
property function.
Looking at the code, it seems prom_update_property actually does not remove it.
It only move the property to "dead properties" list.
See the function comment in kernel:
/**
 * prom_remove_property - Remove a property from a node.
 *
 * Note that we don't actually remove it, since we have given out
 * who-knows-how-many pointers to the data using get-property.
 * Instead we just move the property to the "dead properties"
 * list, so it won't be found any more.
 */

Finally the dead properties will be freed in of_release_node
if the node has no users after calling of_node_put.

static void of_node_release(struct kref *kref)
{
.....
        struct property *prop = node->properties;
.......
        if (!of_node_check_flag(node, OF_DYNAMIC))
                return;

        while (prop) {
                struct property *next = prop->next;
                kfree(prop->name);
                kfree(prop->value);
                kfree(prop);
                prop = next;

                if (!prop) {
                        prop = node->deadprops;
                        node->deadprops = NULL;
                }
        }
        kfree(node->full_name);
        kfree(node->data);
        kfree(node);
}

So it looks to me there's no memory leak,
did i understand wrong?

>> > string list, data block, or could be bootmem ... or could be
>> > actual kernel memory.
>> >
>> > We might want to extend the struct property to contain indications of
>> > the allocation type so we can kfree dynamic properties properly.
>> >
>> I wonder the simplest way may be not allow static allocated property, like dt
>> node does i guess.
>
> No way. We generate the device-tree way before we have an allocator
> available.
>
Oh, got it.

>> > But then there's the question of the lifetime of a property... since
>> > they aren't reference counted like nodes are.
>> >
>> Yes, that's a real exist problem.
>>
>> Anyway, i guess we could do that work of this problem in another patch
>> rather than have to in this patch series.
>
> Cheers,
> Ben.
>
>

Regards
Dong Aisheng

Re: [PATCH 1/1] of: reform prom_update_property function

[Dong Aisheng]

On Fri, Jun 22, 2012 at 8:01 AM, Benjamin Herrenschmidt
<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> On Thu, 2012-06-21 at 17:37 +0800, Dong Aisheng wrote:
>> Maybe we could change it as as follows.
>> It looks then the code follow is the same as before.
>> Do you think if it's ok?
>>
>> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
>> index 7b3bf76..4c237f4 100644
>> --- a/arch/powerpc/platforms/pseries/reconfig.c
>> +++ b/arch/powerpc/platforms/pseries/reconfig.c
>> @@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize)
>>         if (!next_prop)
>>                 return -EINVAL;
>>
>> +       if (!strlen(name)
>> +               return -ENODEV;
>> +
>>         newprop = new_property(name, length, value, NULL);
>>         if (!newprop)
>>                 return -ENOMEM;
>> @@ -450,13 +453,6 @@ static int do_update_property(char *buf, size_t bufsize)
>>         if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
>>                 slb_set_size(*(int *)value);
>>
>> -       oldprop = of_find_property(np, name,NULL);
>> -       if (!oldprop) {
>> -               if (strlen(name))
>> -                       return prom_add_property(np, newprop);
>> -               return -ENODEV;
>> -       }
>
> No:
>
> IE. Old code did:
>
>        if (property doesn't exist) {
>                if (has a name)
>                        create_it()
>                return -ENODEV;
>        }
>
What i saw is:
       if (property doesn't exist) {
               if (has a name)
                       return create_it()
               return -ENODEV;
       }
Which seems the same behavior as the new prop_update_property api.
The only different is if no name, return -EINVAL;
Am i wrong?

> What you propose is:
>
>        if (!has_a_name)
>                return -ENODEV;
>
> Not at all the same semantic.
>
>  .../...
>
>> > IE. The allocation of the "old" property isn't disposed of. It can't
>> > because today we don't know whether any of those pointers was
>> > dynamically allocated or not. IE they could point to the fdt
>
>> Hmm, i did not see static allocated property before.
>> Where can we see an exist case?
>
> Almost all your property names and values. They are pointers to the
> original fdt block, so you can't free them. But dynamically added
> propreties will have kmalloc'ed pointers which should be freed. We need
> to add flags to indicate that if we want to avoid leaking memory in very
> dynamic environments.
>
Okay, got it, thanks for clarify.

>> If we really have this issue, it seems of_node_release also has the same issue,
>> since it frees the property without distinguish whether the property is allocated
>> dynamically.
>
> Well, actually we do have a flag:
>
>        if (!of_node_check_flag(node, OF_DYNAMIC))
>                return;
>
Oh, i see.

> So we use that. Good. So if update property uses that flag it should be
> able to know when to free or not. I forgot we had that :-)
>
I'm still not sure whether we should free the property in update
property function.
Looking at the code, it seems prom_update_property actually does not remove it.
It only move the property to "dead properties" list.
See the function comment in kernel:
/**
 * prom_remove_property - Remove a property from a node.
 *
 * Note that we don't actually remove it, since we have given out
 * who-knows-how-many pointers to the data using get-property.
 * Instead we just move the property to the "dead properties"
 * list, so it won't be found any more.
 */

Finally the dead properties will be freed in of_release_node
if the node has no users after calling of_node_put.

static void of_node_release(struct kref *kref)
{
.....
        struct property *prop = node->properties;
.......
        if (!of_node_check_flag(node, OF_DYNAMIC))
                return;

        while (prop) {
                struct property *next = prop->next;
                kfree(prop->name);
                kfree(prop->value);
                kfree(prop);
                prop = next;

                if (!prop) {
                        prop = node->deadprops;
                        node->deadprops = NULL;
                }
        }
        kfree(node->full_name);
        kfree(node->data);
        kfree(node);
}

So it looks to me there's no memory leak,
did i understand wrong?

>> > string list, data block, or could be bootmem ... or could be
>> > actual kernel memory.
>> >
>> > We might want to extend the struct property to contain indications of
>> > the allocation type so we can kfree dynamic properties properly.
>> >
>> I wonder the simplest way may be not allow static allocated property, like dt
>> node does i guess.
>
> No way. We generate the device-tree way before we have an allocator
> available.
>
Oh, got it.

>> > But then there's the question of the lifetime of a property... since
>> > they aren't reference counted like nodes are.
>> >
>> Yes, that's a real exist problem.
>>
>> Anyway, i guess we could do that work of this problem in another patch
>> rather than have to in this patch series.
>
> Cheers,
> Ben.
>
>

Regards
Dong Aisheng

Re: [PATCH 1/1] of: reform prom_update_property function

[Benjamin Herrenschmidt]

On Fri, 2012-06-22 at 13:25 +0800, Dong Aisheng wrote:
> Which seems the same behavior as the new prop_update_property api.
> The only different is if no name, return -EINVAL;
> Am i wrong?

No, you are right. Sorry for the noise :-)

Cheers,
Ben.

Re: [PATCH 1/1] of: reform prom_update_property function

[Benjamin Herrenschmidt]

On Fri, 2012-06-22 at 13:25 +0800, Dong Aisheng wrote:
> Which seems the same behavior as the new prop_update_property api.
> The only different is if no name, return -EINVAL;
> Am i wrong?

No, you are right. Sorry for the noise :-)

Cheers,
Ben.