[PATCH 0/3] of: add of_property_alloc/free() and of_node_alloc/free()

[PATCH 0/3] of: add of_property_alloc/free() and of_node_alloc/free()

[Clément Léger]

In order to be able to create new nodes and properties dynamically from
drivers, add of_property_alloc/free() and of_node_alloc/free(). These
functions can be used to create new nodes and properties flagged with
OF_DYNAMIC and to free them.

Some powerpc code was already doing such operations and thus, these
functions have been used to replace the manual creation of nodes and
properties.

Clément Léger (3):
  of: dynamic: add of_property_alloc() and of_property_free()
  of: dynamic: add of_node_alloc() and of_node_free()
  powerpc/pseries: use of_property_*() and of_node_*() functions

 arch/powerpc/platforms/pseries/dlpar.c        |  51 +-----
 .../platforms/pseries/hotplug-memory.c        |  27 +--
 arch/powerpc/platforms/pseries/reconfig.c     |  44 ++---
 drivers/of/dynamic.c                          | 160 +++++++++++++-----
 include/linux/of.h                            |  25 +++
 5 files changed, 166 insertions(+), 141 deletions(-)

-- 
2.34.1

[PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Clément Léger]

Add function which allows to dynamically allocate and free properties.
Use this function internally for all code that used the same logic
(mainly __of_prop_dup()).

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/of/dynamic.c | 101 ++++++++++++++++++++++++++++++-------------
 include/linux/of.h   |  16 +++++++
 2 files changed, 88 insertions(+), 29 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cd3821a6444f..e8700e509d2e 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -313,9 +313,7 @@ static void property_list_free(struct property *prop_list)
 
 	for (prop = prop_list; prop != NULL; prop = next) {
 		next = prop->next;
-		kfree(prop->name);
-		kfree(prop->value);
-		kfree(prop);
+		of_property_free(prop);
 	}
 }
 
@@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj)
 }
 
 /**
- * __of_prop_dup - Copy a property dynamically.
- * @prop:	Property to copy
+ * of_property_free - Free a property allocated dynamically.
+ * @prop:	Property to be freed
+ */
+void of_property_free(const struct property *prop)
+{
+	kfree(prop->value);
+	kfree(prop->name);
+	kfree(prop);
+}
+EXPORT_SYMBOL(of_property_free);
+
+/**
+ * of_property_alloc - Allocate a property dynamically.
+ * @name:	Name of the new property
+ * @value:	Value that will be copied into the new property value
+ * @value_len:	length of @value to be copied into the new property value
+ * @len:	Length of new property value, must be greater than @value_len
  * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
  *
- * Copy a property by dynamically allocating the memory of both the
+ * Create a property by dynamically allocating the memory of both the
  * property structure and the property name & contents. The property's
  * flags have the OF_DYNAMIC bit set so that we can differentiate between
  * dynamically allocated properties and not.
  *
  * Return: The newly allocated property or NULL on out of memory error.
  */
-struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
+struct property *of_property_alloc(const char *name, const void *value,
+				   int value_len, int len, gfp_t allocflags)
 {
-	struct property *new;
+	int alloc_len = len;
+	struct property *prop;
+
+	if (len < value_len)
+		return NULL;
 
-	new = kzalloc(sizeof(*new), allocflags);
-	if (!new)
+	prop = kzalloc(sizeof(*prop), allocflags);
+	if (!prop)
 		return NULL;
 
+	prop->name = kstrdup(name, allocflags);
+	if (!prop->name)
+		goto out_err;
+
 	/*
-	 * NOTE: There is no check for zero length value.
-	 * In case of a boolean property, this will allocate a value
-	 * of zero bytes. We do this to work around the use
-	 * of of_get_property() calls on boolean values.
+	 * Even if the property has no value, it must be set to a
+	 * non-null value since of_get_property() is used to check
+	 * some values that might or not have a values (ranges for
+	 * instance). Moreover, when the node is released, prop->value
+	 * is kfreed so the memory must come from kmalloc.
 	 */
-	new->name = kstrdup(prop->name, allocflags);
-	new->value = kmemdup(prop->value, prop->length, allocflags);
-	new->length = prop->length;
-	if (!new->name || !new->value)
-		goto err_free;
+	if (!alloc_len)
+		alloc_len = 1;
 
-	/* mark the property as dynamic */
-	of_property_set_flag(new, OF_DYNAMIC);
+	prop->value = kzalloc(alloc_len, allocflags);
+	if (!prop->value)
+		goto out_err;
 
-	return new;
+	if (value)
+		memcpy(prop->value, value, value_len);
+
+	prop->length = len;
+	of_property_set_flag(prop, OF_DYNAMIC);
+
+	return prop;
+
+out_err:
+	of_property_free(prop);
 
- err_free:
-	kfree(new->name);
-	kfree(new->value);
-	kfree(new);
 	return NULL;
 }
+EXPORT_SYMBOL(of_property_alloc);
+
+/**
+ * __of_prop_dup - Copy a property dynamically.
+ * @prop:	Property to copy
+ * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
+ *
+ * Copy a property by dynamically allocating the memory of both the
+ * property structure and the property name & contents. The property's
+ * flags have the OF_DYNAMIC bit set so that we can differentiate between
+ * dynamically allocated properties and not.
+ *
+ * Return: The newly allocated property or NULL on out of memory error.
+ */
+struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
+{
+	return of_property_alloc(prop->name, prop->value, prop->length,
+				 prop->length, allocflags);
+}
 
 /**
  * __of_node_dup() - Duplicate or create an empty device node dynamically.
@@ -447,9 +492,7 @@ struct device_node *__of_node_dup(const struct device_node *np,
 			if (!new_pp)
 				goto err_prop;
 			if (__of_add_property(node, new_pp)) {
-				kfree(new_pp->name);
-				kfree(new_pp->value);
-				kfree(new_pp);
+				of_property_free(new_pp);
 				goto err_prop;
 			}
 		}
diff --git a/include/linux/of.h b/include/linux/of.h
index 04971e85fbc9..6b345eb71c19 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1463,6 +1463,11 @@ enum of_reconfig_change {
 };
 
 #ifdef CONFIG_OF_DYNAMIC
+extern struct property *of_property_alloc(const char *name, const void *value,
+					  int value_len, int len,
+					  gfp_t allocflags);
+extern void of_property_free(const struct property *prop);
+
 extern int of_reconfig_notifier_register(struct notifier_block *);
 extern int of_reconfig_notifier_unregister(struct notifier_block *);
 extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
@@ -1507,6 +1512,17 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
 	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
 }
 #else /* CONFIG_OF_DYNAMIC */
+
+static inline struct property *of_property_alloc(const char *name,
+						 const void *value,
+						 int value_len, int len,
+						 gfp_t allocflags)
+{
+	return NULL;
+}
+
+static inline  void of_property_free(const struct property *prop) {}
+
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
 	return -EINVAL;
-- 
2.34.1

Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Clément Léger]

Le Thu, 5 May 2022 07:30:47 +0000,
Christophe Leroy <christophe.leroy@csgroup.eu> a écrit :

> >   	/*
> > -	 * NOTE: There is no check for zero length value.
> > -	 * In case of a boolean property, this will allocate a value
> > -	 * of zero bytes. We do this to work around the use
> > -	 * of of_get_property() calls on boolean values.
> > +	 * Even if the property has no value, it must be set to a
> > +	 * non-null value since of_get_property() is used to check
> > +	 * some values that might or not have a values (ranges for
> > +	 * instance). Moreover, when the node is released, prop->value
> > +	 * is kfreed so the memory must come from kmalloc.
> >   	 */
> > -	new->name = kstrdup(prop->name, allocflags);
> > -	new->value = kmemdup(prop->value, prop->length, allocflags);
> > -	new->length = prop->length;
> > -	if (!new->name || !new->value)
> > -		goto err_free;
> > +	if (!alloc_len)
> > +		alloc_len = 1;
> >   
> > -	/* mark the property as dynamic */
> > -	of_property_set_flag(new, OF_DYNAMIC);
> > +	prop->value = kzalloc(alloc_len, allocflags);
> > +	if (!prop->value)
> > +		goto out_err;
> >   
> > -	return new;
> > +	if (value)
> > +		memcpy(prop->value, value, value_len);  
> 
> Could you use kmemdup() instead of kzalloc+memcpy ?

I could but then, we won't be able to allocate a property value that is
larger than the original one. This is used by the powerpc code to
recopy an existing value and add some extra space after it.

> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 04971e85fbc9..6b345eb71c19 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -1463,6 +1463,11 @@ enum of_reconfig_change {
> >   };
> >   
> >   #ifdef CONFIG_OF_DYNAMIC
> > +extern struct property *of_property_alloc(const char *name, const void *value,
> > +					  int value_len, int len,
> > +					  gfp_t allocflags);
> > +extern void of_property_free(const struct property *prop);
> > +  
> 
> 'extern' is pointless for function prototypes, you should not add new 
> ones. Checkpatch complain about it:

I did so that, but I kept that since the existing code is full of them.
Since you mention it, I'll remove the extern.

> 
> CHECK: extern prototypes should be avoided in .h files
> #172: FILE: include/linux/of.h:1466:
> +extern struct property *of_property_alloc(const char *name, const void 
> *value,
> 
> CHECK: extern prototypes should be avoided in .h files
> #175: FILE: include/linux/of.h:1469:
> +extern void of_property_free(const struct property *prop);
> 
> 
> 
> 
> >   extern int of_reconfig_notifier_register(struct notifier_block *);
> >   extern int of_reconfig_notifier_unregister(struct notifier_block *);
> >   extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
> > @@ -1507,6 +1512,17 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
> >   	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
> >   }
> >   #else /* CONFIG_OF_DYNAMIC */
> > +
> > +static inline struct property *of_property_alloc(const char *name,
> > +						 const void *value,
> > +						 int value_len, int len,
> > +						 gfp_t allocflags)  
> 
> Can that fit on less lines ?
> 
> May be:
> 
> static inline struct property
> *of_property_alloc(const char *name, const void *value, int value_len,
> 		   int len, gfp_t allocflags)

Yes, that seems a better split.

Thanks,


-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Rob Herring]

On Thu, May 05, 2022 at 07:30:47AM +0000, Christophe Leroy wrote:
> 
> 
> Le 04/05/2022 à 17:40, Clément Léger a écrit :
> > Add function which allows to dynamically allocate and free properties.
> > Use this function internally for all code that used the same logic
> > (mainly __of_prop_dup()).
> > 
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---
> >   drivers/of/dynamic.c | 101 ++++++++++++++++++++++++++++++-------------
> >   include/linux/of.h   |  16 +++++++
> >   2 files changed, 88 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > index cd3821a6444f..e8700e509d2e 100644
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> > @@ -313,9 +313,7 @@ static void property_list_free(struct property *prop_list)
> >   
> >   	for (prop = prop_list; prop != NULL; prop = next) {
> >   		next = prop->next;
> > -		kfree(prop->name);
> > -		kfree(prop->value);
> > -		kfree(prop);
> > +		of_property_free(prop);
> >   	}
> >   }
> >   
> > @@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj)
> >   }
> >   
> >   /**
> > - * __of_prop_dup - Copy a property dynamically.
> > - * @prop:	Property to copy
> > + * of_property_free - Free a property allocated dynamically.
> > + * @prop:	Property to be freed
> > + */
> > +void of_property_free(const struct property *prop)
> > +{
> > +	kfree(prop->value);
> > +	kfree(prop->name);
> > +	kfree(prop);
> > +}
> > +EXPORT_SYMBOL(of_property_free);
> > +
> > +/**
> > + * of_property_alloc - Allocate a property dynamically.
> > + * @name:	Name of the new property
> > + * @value:	Value that will be copied into the new property value
> > + * @value_len:	length of @value to be copied into the new property value
> > + * @len:	Length of new property value, must be greater than @value_len
> >    * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
> >    *
> > - * Copy a property by dynamically allocating the memory of both the
> > + * Create a property by dynamically allocating the memory of both the
> >    * property structure and the property name & contents. The property's
> >    * flags have the OF_DYNAMIC bit set so that we can differentiate between
> >    * dynamically allocated properties and not.
> >    *
> >    * Return: The newly allocated property or NULL on out of memory error.
> >    */
> > -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> > +struct property *of_property_alloc(const char *name, const void *value,
> > +				   int value_len, int len, gfp_t allocflags)
> >   {
> > -	struct property *new;
> > +	int alloc_len = len;
> > +	struct property *prop;
> > +
> > +	if (len < value_len)
> > +		return NULL;
> >   
> > -	new = kzalloc(sizeof(*new), allocflags);
> > -	if (!new)
> > +	prop = kzalloc(sizeof(*prop), allocflags);
> > +	if (!prop)
> >   		return NULL;
> >   
> > +	prop->name = kstrdup(name, allocflags);
> > +	if (!prop->name)
> > +		goto out_err;
> > +
> >   	/*
> > -	 * NOTE: There is no check for zero length value.
> > -	 * In case of a boolean property, this will allocate a value
> > -	 * of zero bytes. We do this to work around the use
> > -	 * of of_get_property() calls on boolean values.
> > +	 * Even if the property has no value, it must be set to a
> > +	 * non-null value since of_get_property() is used to check
> > +	 * some values that might or not have a values (ranges for
> > +	 * instance). Moreover, when the node is released, prop->value
> > +	 * is kfreed so the memory must come from kmalloc.
> >   	 */
> > -	new->name = kstrdup(prop->name, allocflags);
> > -	new->value = kmemdup(prop->value, prop->length, allocflags);
> > -	new->length = prop->length;
> > -	if (!new->name || !new->value)
> > -		goto err_free;
> > +	if (!alloc_len)
> > +		alloc_len = 1;
> >   
> > -	/* mark the property as dynamic */
> > -	of_property_set_flag(new, OF_DYNAMIC);
> > +	prop->value = kzalloc(alloc_len, allocflags);
> > +	if (!prop->value)
> > +		goto out_err;
> >   
> > -	return new;
> > +	if (value)
> > +		memcpy(prop->value, value, value_len);
> 
> Could you use kmemdup() instead of kzalloc+memcpy ?

I'd prefer there be 1 alloc for struct property and value instead of 2. 
And maybe 'name' gets rolled into it too, but that gets a bit more 
complicated to manage I think. 

With memcpy, note this series[1].

Rob

[1] https://lore.kernel.org/all/20220504014440.3697851-30-keescook@chromium.org/

Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Clément Léger]

Le Thu, 5 May 2022 12:37:38 -0500,
Rob Herring <robh@kernel.org> a écrit :

> > >   
> > > -	/* mark the property as dynamic */
> > > -	of_property_set_flag(new, OF_DYNAMIC);
> > > +	prop->value = kzalloc(alloc_len, allocflags);
> > > +	if (!prop->value)
> > > +		goto out_err;
> > >   
> > > -	return new;
> > > +	if (value)
> > > +		memcpy(prop->value, value, value_len);  
> > 
> > Could you use kmemdup() instead of kzalloc+memcpy ?  
> 
> I'd prefer there be 1 alloc for struct property and value instead of 2. 
> And maybe 'name' gets rolled into it too, but that gets a bit more 
> complicated to manage I think. 

At least for value it should be easy indeed. i'll check what I can do
for the name.

> 
> With memcpy, note this series[1].

Ok, good to know, should I base my series on that one ?

> 
> Rob
> 
> [1] https://lore.kernel.org/all/20220504014440.3697851-30-keescook@chromium.org/


-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Rob Herring]

On Wed, May 04, 2022 at 05:40:31PM +0200, Clément Léger wrote:
> Add function which allows to dynamically allocate and free properties.
> Use this function internally for all code that used the same logic
> (mainly __of_prop_dup()).
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/of/dynamic.c | 101 ++++++++++++++++++++++++++++++-------------
>  include/linux/of.h   |  16 +++++++
>  2 files changed, 88 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index cd3821a6444f..e8700e509d2e 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -313,9 +313,7 @@ static void property_list_free(struct property *prop_list)
>  
>  	for (prop = prop_list; prop != NULL; prop = next) {
>  		next = prop->next;
> -		kfree(prop->name);
> -		kfree(prop->value);
> -		kfree(prop);
> +		of_property_free(prop);
>  	}
>  }
>  
> @@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj)
>  }
>  
>  /**
> - * __of_prop_dup - Copy a property dynamically.
> - * @prop:	Property to copy
> + * of_property_free - Free a property allocated dynamically.
> + * @prop:	Property to be freed
> + */
> +void of_property_free(const struct property *prop)
> +{
> +	kfree(prop->value);
> +	kfree(prop->name);
> +	kfree(prop);
> +}
> +EXPORT_SYMBOL(of_property_free);
> +
> +/**
> + * of_property_alloc - Allocate a property dynamically.
> + * @name:	Name of the new property
> + * @value:	Value that will be copied into the new property value
> + * @value_len:	length of @value to be copied into the new property value
> + * @len:	Length of new property value, must be greater than @value_len

What's the usecase for the lengths being different? That doesn't seem 
like a common case, so perhaps handle it with a NULL value and 
non-zero length. Then the caller has to deal with populating 
prop->value.

>   * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
>   *
> - * Copy a property by dynamically allocating the memory of both the
> + * Create a property by dynamically allocating the memory of both the
>   * property structure and the property name & contents. The property's
>   * flags have the OF_DYNAMIC bit set so that we can differentiate between
>   * dynamically allocated properties and not.
>   *
>   * Return: The newly allocated property or NULL on out of memory error.
>   */
> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +struct property *of_property_alloc(const char *name, const void *value,
> +				   int value_len, int len, gfp_t allocflags)
>  {
> -	struct property *new;
> +	int alloc_len = len;
> +	struct property *prop;
> +
> +	if (len < value_len)
> +		return NULL;
>  
> -	new = kzalloc(sizeof(*new), allocflags);
> -	if (!new)
> +	prop = kzalloc(sizeof(*prop), allocflags);
> +	if (!prop)
>  		return NULL;
>  
> +	prop->name = kstrdup(name, allocflags);
> +	if (!prop->name)
> +		goto out_err;
> +
>  	/*
> -	 * NOTE: There is no check for zero length value.
> -	 * In case of a boolean property, this will allocate a value
> -	 * of zero bytes. We do this to work around the use
> -	 * of of_get_property() calls on boolean values.
> +	 * Even if the property has no value, it must be set to a
> +	 * non-null value since of_get_property() is used to check
> +	 * some values that might or not have a values (ranges for
> +	 * instance). Moreover, when the node is released, prop->value
> +	 * is kfreed so the memory must come from kmalloc.

Allowing for NULL value didn't turn out well...

We know that we can do the kfree because OF_DYNAMIC is set IIRC...

If we do 1 allocation for prop and value, then we can test 
for "prop->value == prop + 1" to determine if we need to free or not.

>  	 */
> -	new->name = kstrdup(prop->name, allocflags);
> -	new->value = kmemdup(prop->value, prop->length, allocflags);
> -	new->length = prop->length;
> -	if (!new->name || !new->value)
> -		goto err_free;
> +	if (!alloc_len)
> +		alloc_len = 1;
>  
> -	/* mark the property as dynamic */
> -	of_property_set_flag(new, OF_DYNAMIC);
> +	prop->value = kzalloc(alloc_len, allocflags);
> +	if (!prop->value)
> +		goto out_err;
>  
> -	return new;
> +	if (value)
> +		memcpy(prop->value, value, value_len);
> +
> +	prop->length = len;
> +	of_property_set_flag(prop, OF_DYNAMIC);
> +
> +	return prop;
> +
> +out_err:
> +	of_property_free(prop);
>  
> - err_free:
> -	kfree(new->name);
> -	kfree(new->value);
> -	kfree(new);
>  	return NULL;
>  }
> +EXPORT_SYMBOL(of_property_alloc);
> +
> +/**
> + * __of_prop_dup - Copy a property dynamically.
> + * @prop:	Property to copy
> + * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
> + *
> + * Copy a property by dynamically allocating the memory of both the
> + * property structure and the property name & contents. The property's
> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
> + * dynamically allocated properties and not.
> + *
> + * Return: The newly allocated property or NULL on out of memory error.
> + */
> +struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +{
> +	return of_property_alloc(prop->name, prop->value, prop->length,
> +				 prop->length, allocflags);

This can now be a static inline.

> +}
>  
>  /**
>   * __of_node_dup() - Duplicate or create an empty device node dynamically.
> @@ -447,9 +492,7 @@ struct device_node *__of_node_dup(const struct device_node *np,
>  			if (!new_pp)
>  				goto err_prop;
>  			if (__of_add_property(node, new_pp)) {
> -				kfree(new_pp->name);
> -				kfree(new_pp->value);
> -				kfree(new_pp);
> +				of_property_free(new_pp);
>  				goto err_prop;
>  			}
>  		}
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 04971e85fbc9..6b345eb71c19 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1463,6 +1463,11 @@ enum of_reconfig_change {
>  };
>  
>  #ifdef CONFIG_OF_DYNAMIC
> +extern struct property *of_property_alloc(const char *name, const void *value,
> +					  int value_len, int len,
> +					  gfp_t allocflags);
> +extern void of_property_free(const struct property *prop);
> +
>  extern int of_reconfig_notifier_register(struct notifier_block *);
>  extern int of_reconfig_notifier_unregister(struct notifier_block *);
>  extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
> @@ -1507,6 +1512,17 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>  	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>  }
>  #else /* CONFIG_OF_DYNAMIC */
> +
> +static inline struct property *of_property_alloc(const char *name,
> +						 const void *value,
> +						 int value_len, int len,
> +						 gfp_t allocflags)
> +{
> +	return NULL;
> +}
> +
> +static inline  void of_property_free(const struct property *prop) {}
> +
>  static inline int of_reconfig_notifier_register(struct notifier_block *nb)
>  {
>  	return -EINVAL;
> -- 
> 2.34.1
> 
> 

Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Clément Léger]

Le Thu, 5 May 2022 14:37:15 -0500,
Rob Herring <robh@kernel.org> a écrit :


> > +
> > +/**
> > + * of_property_alloc - Allocate a property dynamically.
> > + * @name:	Name of the new property
> > + * @value:	Value that will be copied into the new property value
> > + * @value_len:	length of @value to be copied into the new property value
> > + * @len:	Length of new property value, must be greater than @value_len  
> 
> What's the usecase for the lengths being different? That doesn't seem 
> like a common case, so perhaps handle it with a NULL value and 
> non-zero length. Then the caller has to deal with populating 
> prop->value.

That was actually something used by powerpc code but agreed, letting
the user recopy it's values seems fine to me and the usage will be more
clear.

> >  	/*
> > -	 * NOTE: There is no check for zero length value.
> > -	 * In case of a boolean property, this will allocate a value
> > -	 * of zero bytes. We do this to work around the use
> > -	 * of of_get_property() calls on boolean values.
> > +	 * Even if the property has no value, it must be set to a
> > +	 * non-null value since of_get_property() is used to check
> > +	 * some values that might or not have a values (ranges for
> > +	 * instance). Moreover, when the node is released, prop->value
> > +	 * is kfreed so the memory must come from kmalloc.  
> 
> Allowing for NULL value didn't turn out well...
> 
> We know that we can do the kfree because OF_DYNAMIC is set IIRC...
> 
> If we do 1 allocation for prop and value, then we can test 
> for "prop->value == prop + 1" to determine if we need to free or not.

Sounds like a good idea.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Tyrel Datwyler]

On 5/5/22 12:37, Rob Herring wrote:
> On Wed, May 04, 2022 at 05:40:31PM +0200, Clément Léger wrote:
>> Add function which allows to dynamically allocate and free properties.
>> Use this function internally for all code that used the same logic
>> (mainly __of_prop_dup()).
>>
>> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
>> ---
>>  drivers/of/dynamic.c | 101 ++++++++++++++++++++++++++++++-------------
>>  include/linux/of.h   |  16 +++++++
>>  2 files changed, 88 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index cd3821a6444f..e8700e509d2e 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -313,9 +313,7 @@ static void property_list_free(struct property *prop_list)
>>  
>>  	for (prop = prop_list; prop != NULL; prop = next) {
>>  		next = prop->next;
>> -		kfree(prop->name);
>> -		kfree(prop->value);
>> -		kfree(prop);
>> +		of_property_free(prop);
>>  	}
>>  }
>>  
>> @@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj)
>>  }
>>  
>>  /**
>> - * __of_prop_dup - Copy a property dynamically.
>> - * @prop:	Property to copy
>> + * of_property_free - Free a property allocated dynamically.
>> + * @prop:	Property to be freed
>> + */
>> +void of_property_free(const struct property *prop)
>> +{
>> +	kfree(prop->value);
>> +	kfree(prop->name);
>> +	kfree(prop);
>> +}
>> +EXPORT_SYMBOL(of_property_free);
>> +
>> +/**
>> + * of_property_alloc - Allocate a property dynamically.
>> + * @name:	Name of the new property
>> + * @value:	Value that will be copied into the new property value
>> + * @value_len:	length of @value to be copied into the new property value
>> + * @len:	Length of new property value, must be greater than @value_len
> 
> What's the usecase for the lengths being different? That doesn't seem 
> like a common case, so perhaps handle it with a NULL value and 
> non-zero length. Then the caller has to deal with populating 
> prop->value.
> 
>>   * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
>>   *
>> - * Copy a property by dynamically allocating the memory of both the
>> + * Create a property by dynamically allocating the memory of both the
>>   * property structure and the property name & contents. The property's
>>   * flags have the OF_DYNAMIC bit set so that we can differentiate between
>>   * dynamically allocated properties and not.
>>   *
>>   * Return: The newly allocated property or NULL on out of memory error.
>>   */
>> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
>> +struct property *of_property_alloc(const char *name, const void *value,
>> +				   int value_len, int len, gfp_t allocflags)
>>  {
>> -	struct property *new;
>> +	int alloc_len = len;
>> +	struct property *prop;
>> +
>> +	if (len < value_len)
>> +		return NULL;
>>  
>> -	new = kzalloc(sizeof(*new), allocflags);
>> -	if (!new)
>> +	prop = kzalloc(sizeof(*prop), allocflags);
>> +	if (!prop)
>>  		return NULL;
>>  
>> +	prop->name = kstrdup(name, allocflags);
>> +	if (!prop->name)
>> +		goto out_err;
>> +
>>  	/*
>> -	 * NOTE: There is no check for zero length value.
>> -	 * In case of a boolean property, this will allocate a value
>> -	 * of zero bytes. We do this to work around the use
>> -	 * of of_get_property() calls on boolean values.
>> +	 * Even if the property has no value, it must be set to a
>> +	 * non-null value since of_get_property() is used to check
>> +	 * some values that might or not have a values (ranges for
>> +	 * instance). Moreover, when the node is released, prop->value
>> +	 * is kfreed so the memory must come from kmalloc.
> 
> Allowing for NULL value didn't turn out well...
> 
> We know that we can do the kfree because OF_DYNAMIC is set IIRC...
> 
> If we do 1 allocation for prop and value, then we can test 
> for "prop->value == prop + 1" to determine if we need to free or not.

If its a single allocation do we even need a test? Doesn't kfree(prop) take care
of the property and the trailing memory allocated for the value?

-Tyrel

> 
>>  	 */
>> -	new->name = kstrdup(prop->name, allocflags);
>> -	new->value = kmemdup(prop->value, prop->length, allocflags);
>> -	new->length = prop->length;
>> -	if (!new->name || !new->value)
>> -		goto err_free;
>> +	if (!alloc_len)
>> +		alloc_len = 1;
>>  
>> -	/* mark the property as dynamic */
>> -	of_property_set_flag(new, OF_DYNAMIC);
>> +	prop->value = kzalloc(alloc_len, allocflags);
>> +	if (!prop->value)
>> +		goto out_err;
>>  
>> -	return new;
>> +	if (value)
>> +		memcpy(prop->value, value, value_len);
>> +
>> +	prop->length = len;
>> +	of_property_set_flag(prop, OF_DYNAMIC);
>> +
>> +	return prop;
>> +
>> +out_err:
>> +	of_property_free(prop);
>>  
>> - err_free:
>> -	kfree(new->name);
>> -	kfree(new->value);
>> -	kfree(new);
>>  	return NULL;
>>  }
>> +EXPORT_SYMBOL(of_property_alloc);
>> +
>> +/**
>> + * __of_prop_dup - Copy a property dynamically.
>> + * @prop:	Property to copy
>> + * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
>> + *
>> + * Copy a property by dynamically allocating the memory of both the
>> + * property structure and the property name & contents. The property's
>> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
>> + * dynamically allocated properties and not.
>> + *
>> + * Return: The newly allocated property or NULL on out of memory error.
>> + */
>> +struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
>> +{
>> +	return of_property_alloc(prop->name, prop->value, prop->length,
>> +				 prop->length, allocflags);
> 
> This can now be a static inline.
> 
>> +}
>>  
>>  /**
>>   * __of_node_dup() - Duplicate or create an empty device node dynamically.
>> @@ -447,9 +492,7 @@ struct device_node *__of_node_dup(const struct device_node *np,
>>  			if (!new_pp)
>>  				goto err_prop;
>>  			if (__of_add_property(node, new_pp)) {
>> -				kfree(new_pp->name);
>> -				kfree(new_pp->value);
>> -				kfree(new_pp);
>> +				of_property_free(new_pp);
>>  				goto err_prop;
>>  			}
>>  		}
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 04971e85fbc9..6b345eb71c19 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -1463,6 +1463,11 @@ enum of_reconfig_change {
>>  };
>>  
>>  #ifdef CONFIG_OF_DYNAMIC
>> +extern struct property *of_property_alloc(const char *name, const void *value,
>> +					  int value_len, int len,
>> +					  gfp_t allocflags);
>> +extern void of_property_free(const struct property *prop);
>> +
>>  extern int of_reconfig_notifier_register(struct notifier_block *);
>>  extern int of_reconfig_notifier_unregister(struct notifier_block *);
>>  extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
>> @@ -1507,6 +1512,17 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>>  	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>>  }
>>  #else /* CONFIG_OF_DYNAMIC */
>> +
>> +static inline struct property *of_property_alloc(const char *name,
>> +						 const void *value,
>> +						 int value_len, int len,
>> +						 gfp_t allocflags)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline  void of_property_free(const struct property *prop) {}
>> +
>>  static inline int of_reconfig_notifier_register(struct notifier_block *nb)
>>  {
>>  	return -EINVAL;
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Rob Herring]

On Wed, Jun 1, 2022 at 5:31 PM Tyrel Datwyler <tyreld@linux.ibm.com> wrote:
>
> On 5/5/22 12:37, Rob Herring wrote:
> > On Wed, May 04, 2022 at 05:40:31PM +0200, Clément Léger wrote:
> >> Add function which allows to dynamically allocate and free properties.
> >> Use this function internally for all code that used the same logic
> >> (mainly __of_prop_dup()).
> >>
> >> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> >> ---
> >>  drivers/of/dynamic.c | 101 ++++++++++++++++++++++++++++++-------------
> >>  include/linux/of.h   |  16 +++++++
> >>  2 files changed, 88 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> >> index cd3821a6444f..e8700e509d2e 100644
> >> --- a/drivers/of/dynamic.c
> >> +++ b/drivers/of/dynamic.c
> >> @@ -313,9 +313,7 @@ static void property_list_free(struct property *prop_list)
> >>
> >>      for (prop = prop_list; prop != NULL; prop = next) {
> >>              next = prop->next;
> >> -            kfree(prop->name);
> >> -            kfree(prop->value);
> >> -            kfree(prop);
> >> +            of_property_free(prop);
> >>      }
> >>  }
> >>
> >> @@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj)
> >>  }
> >>
> >>  /**
> >> - * __of_prop_dup - Copy a property dynamically.
> >> - * @prop:   Property to copy
> >> + * of_property_free - Free a property allocated dynamically.
> >> + * @prop:   Property to be freed
> >> + */
> >> +void of_property_free(const struct property *prop)
> >> +{
> >> +    kfree(prop->value);
> >> +    kfree(prop->name);
> >> +    kfree(prop);
> >> +}
> >> +EXPORT_SYMBOL(of_property_free);
> >> +
> >> +/**
> >> + * of_property_alloc - Allocate a property dynamically.
> >> + * @name:   Name of the new property
> >> + * @value:  Value that will be copied into the new property value
> >> + * @value_len:      length of @value to be copied into the new property value
> >> + * @len:    Length of new property value, must be greater than @value_len
> >
> > What's the usecase for the lengths being different? That doesn't seem
> > like a common case, so perhaps handle it with a NULL value and
> > non-zero length. Then the caller has to deal with populating
> > prop->value.
> >
> >>   * @allocflags:     Allocation flags (typically pass GFP_KERNEL)
> >>   *
> >> - * Copy a property by dynamically allocating the memory of both the
> >> + * Create a property by dynamically allocating the memory of both the
> >>   * property structure and the property name & contents. The property's
> >>   * flags have the OF_DYNAMIC bit set so that we can differentiate between
> >>   * dynamically allocated properties and not.
> >>   *
> >>   * Return: The newly allocated property or NULL on out of memory error.
> >>   */
> >> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> >> +struct property *of_property_alloc(const char *name, const void *value,
> >> +                               int value_len, int len, gfp_t allocflags)
> >>  {
> >> -    struct property *new;
> >> +    int alloc_len = len;
> >> +    struct property *prop;
> >> +
> >> +    if (len < value_len)
> >> +            return NULL;
> >>
> >> -    new = kzalloc(sizeof(*new), allocflags);
> >> -    if (!new)
> >> +    prop = kzalloc(sizeof(*prop), allocflags);
> >> +    if (!prop)
> >>              return NULL;
> >>
> >> +    prop->name = kstrdup(name, allocflags);
> >> +    if (!prop->name)
> >> +            goto out_err;
> >> +
> >>      /*
> >> -     * NOTE: There is no check for zero length value.
> >> -     * In case of a boolean property, this will allocate a value
> >> -     * of zero bytes. We do this to work around the use
> >> -     * of of_get_property() calls on boolean values.
> >> +     * Even if the property has no value, it must be set to a
> >> +     * non-null value since of_get_property() is used to check
> >> +     * some values that might or not have a values (ranges for
> >> +     * instance). Moreover, when the node is released, prop->value
> >> +     * is kfreed so the memory must come from kmalloc.
> >
> > Allowing for NULL value didn't turn out well...
> >
> > We know that we can do the kfree because OF_DYNAMIC is set IIRC...
> >
> > If we do 1 allocation for prop and value, then we can test
> > for "prop->value == prop + 1" to determine if we need to free or not.
>
> If its a single allocation do we even need a test? Doesn't kfree(prop) take care
> of the property and the trailing memory allocated for the value?

Yes, it does when it's a single alloc, but it's testing for when
prop->value is not a single allocation because we could have either.

Rob

Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Tyrel Datwyler]

On 6/2/22 07:06, Rob Herring wrote:
> On Wed, Jun 1, 2022 at 5:31 PM Tyrel Datwyler <tyreld@linux.ibm.com> wrote:
>>
>> On 5/5/22 12:37, Rob Herring wrote:
>>> On Wed, May 04, 2022 at 05:40:31PM +0200, Clément Léger wrote:
>>>> Add function which allows to dynamically allocate and free properties.
>>>> Use this function internally for all code that used the same logic
>>>> (mainly __of_prop_dup()).
>>>>
>>>> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
>>>> ---
>>>>  drivers/of/dynamic.c | 101 ++++++++++++++++++++++++++++++-------------
>>>>  include/linux/of.h   |  16 +++++++
>>>>  2 files changed, 88 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>>>> index cd3821a6444f..e8700e509d2e 100644
>>>> --- a/drivers/of/dynamic.c
>>>> +++ b/drivers/of/dynamic.c
>>>> @@ -313,9 +313,7 @@ static void property_list_free(struct property *prop_list)
>>>>
>>>>      for (prop = prop_list; prop != NULL; prop = next) {
>>>>              next = prop->next;
>>>> -            kfree(prop->name);
>>>> -            kfree(prop->value);
>>>> -            kfree(prop);
>>>> +            of_property_free(prop);
>>>>      }
>>>>  }
>>>>
>>>> @@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj)
>>>>  }
>>>>
>>>>  /**
>>>> - * __of_prop_dup - Copy a property dynamically.
>>>> - * @prop:   Property to copy
>>>> + * of_property_free - Free a property allocated dynamically.
>>>> + * @prop:   Property to be freed
>>>> + */
>>>> +void of_property_free(const struct property *prop)
>>>> +{
>>>> +    kfree(prop->value);
>>>> +    kfree(prop->name);
>>>> +    kfree(prop);
>>>> +}
>>>> +EXPORT_SYMBOL(of_property_free);
>>>> +
>>>> +/**
>>>> + * of_property_alloc - Allocate a property dynamically.
>>>> + * @name:   Name of the new property
>>>> + * @value:  Value that will be copied into the new property value
>>>> + * @value_len:      length of @value to be copied into the new property value
>>>> + * @len:    Length of new property value, must be greater than @value_len
>>>
>>> What's the usecase for the lengths being different? That doesn't seem
>>> like a common case, so perhaps handle it with a NULL value and
>>> non-zero length. Then the caller has to deal with populating
>>> prop->value.
>>>
>>>>   * @allocflags:     Allocation flags (typically pass GFP_KERNEL)
>>>>   *
>>>> - * Copy a property by dynamically allocating the memory of both the
>>>> + * Create a property by dynamically allocating the memory of both the
>>>>   * property structure and the property name & contents. The property's
>>>>   * flags have the OF_DYNAMIC bit set so that we can differentiate between
>>>>   * dynamically allocated properties and not.
>>>>   *
>>>>   * Return: The newly allocated property or NULL on out of memory error.
>>>>   */
>>>> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
>>>> +struct property *of_property_alloc(const char *name, const void *value,
>>>> +                               int value_len, int len, gfp_t allocflags)
>>>>  {
>>>> -    struct property *new;
>>>> +    int alloc_len = len;
>>>> +    struct property *prop;
>>>> +
>>>> +    if (len < value_len)
>>>> +            return NULL;
>>>>
>>>> -    new = kzalloc(sizeof(*new), allocflags);
>>>> -    if (!new)
>>>> +    prop = kzalloc(sizeof(*prop), allocflags);
>>>> +    if (!prop)
>>>>              return NULL;
>>>>
>>>> +    prop->name = kstrdup(name, allocflags);
>>>> +    if (!prop->name)
>>>> +            goto out_err;
>>>> +
>>>>      /*
>>>> -     * NOTE: There is no check for zero length value.
>>>> -     * In case of a boolean property, this will allocate a value
>>>> -     * of zero bytes. We do this to work around the use
>>>> -     * of of_get_property() calls on boolean values.
>>>> +     * Even if the property has no value, it must be set to a
>>>> +     * non-null value since of_get_property() is used to check
>>>> +     * some values that might or not have a values (ranges for
>>>> +     * instance). Moreover, when the node is released, prop->value
>>>> +     * is kfreed so the memory must come from kmalloc.
>>>
>>> Allowing for NULL value didn't turn out well...
>>>
>>> We know that we can do the kfree because OF_DYNAMIC is set IIRC...
>>>
>>> If we do 1 allocation for prop and value, then we can test
>>> for "prop->value == prop + 1" to determine if we need to free or not.
>>
>> If its a single allocation do we even need a test? Doesn't kfree(prop) take care
>> of the property and the trailing memory allocated for the value?
> 
> Yes, it does when it's a single alloc, but it's testing for when
> prop->value is not a single allocation because we could have either.
> 

Ok, that is the part I was missing. Thanks for the clarification.

-Tyrel

[PATCH 2/3] of: dynamic: add of_node_alloc() and of_node_free()

[Clément Léger]

Add functions which allows to create and free nodes.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/of/dynamic.c | 59 ++++++++++++++++++++++++++++++++++++--------
 include/linux/of.h   |  9 +++++++
 2 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e8700e509d2e..ec28e5ba2969 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -455,6 +455,54 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
 				 prop->length, allocflags);
 }
 
+/**
+ * of_node_free - Free a node allocated dynamically.
+ * @node:	Node to be freed
+ */
+void of_node_free(const struct device_node *node)
+{
+	kfree(node->full_name);
+	kfree(node->data);
+	kfree(node);
+}
+EXPORT_SYMBOL(of_node_free);
+
+/**
+ * of_node_alloc - Allocate a node dynamically.
+ * @name:	Node name
+ * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
+ *
+ * Create a node by dynamically allocating the memory of both the
+ * node structure and the node name & contents. The node's
+ * flags have the OF_DYNAMIC & OF_DETACHED bit set so that we can
+ * differentiate between dynamically allocated nodes and not.
+ *
+ * Return: The newly allocated node or NULL on out of memory error.
+ */
+struct device_node *of_node_alloc(const char *name, gfp_t allocflags)
+{
+	struct device_node *node;
+
+	node = kzalloc(sizeof(*node), allocflags);
+	if (!node)
+		return NULL;
+
+	if (name) {
+		node->full_name = kstrdup(name, allocflags);
+		if (!node->full_name) {
+			kfree(node);
+			return NULL;
+		}
+	}
+
+	of_node_set_flag(node, OF_DYNAMIC);
+	of_node_set_flag(node, OF_DETACHED);
+	of_node_init(node);
+
+	return node;
+}
+EXPORT_SYMBOL(of_node_alloc);
+
 /**
  * __of_node_dup() - Duplicate or create an empty device node dynamically.
  * @np:		if not NULL, contains properties to be duplicated in new node
@@ -471,18 +519,9 @@ struct device_node *__of_node_dup(const struct device_node *np,
 {
 	struct device_node *node;
 
-	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	node = of_node_alloc(NULL, GFP_KERNEL);
 	if (!node)
 		return NULL;
-	node->full_name = kstrdup(full_name, GFP_KERNEL);
-	if (!node->full_name) {
-		kfree(node);
-		return NULL;
-	}
-
-	of_node_set_flag(node, OF_DYNAMIC);
-	of_node_set_flag(node, OF_DETACHED);
-	of_node_init(node);
 
 	/* Iterate over and duplicate all properties */
 	if (np) {
diff --git a/include/linux/of.h b/include/linux/of.h
index 6b345eb71c19..749ac07f4960 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1463,6 +1463,8 @@ enum of_reconfig_change {
 };
 
 #ifdef CONFIG_OF_DYNAMIC
+extern struct device_node *of_node_alloc(const char *name, gfp_t allocflags);
+extern void of_node_free(const struct device_node *node);
 extern struct property *of_property_alloc(const char *name, const void *value,
 					  int value_len, int len,
 					  gfp_t allocflags);
@@ -1512,6 +1514,13 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
 	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
 }
 #else /* CONFIG_OF_DYNAMIC */
+static inline struct device_node *of_node_alloc(const char *name,
+						gfp_t allocflags)
+{
+	return NULL;
+}
+
+static inline void of_node_free(const struct device_node *node) {}
 
 static inline struct property *of_property_alloc(const char *name,
 						 const void *value,
-- 
2.34.1

Re: [PATCH 2/3] of: dynamic: add of_node_alloc() and of_node_free()

[Rob Herring]

On Wed, May 04, 2022 at 05:40:32PM +0200, Clément Léger wrote:
> Add functions which allows to create and free nodes.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/of/dynamic.c | 59 ++++++++++++++++++++++++++++++++++++--------
>  include/linux/of.h   |  9 +++++++
>  2 files changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index e8700e509d2e..ec28e5ba2969 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -455,6 +455,54 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
>  				 prop->length, allocflags);
>  }
>  
> +/**
> + * of_node_free - Free a node allocated dynamically.
> + * @node:	Node to be freed
> + */
> +void of_node_free(const struct device_node *node)
> +{
> +	kfree(node->full_name);
> +	kfree(node->data);
> +	kfree(node);
> +}
> +EXPORT_SYMBOL(of_node_free);

This shouldn't be needed. Nodes are refcounted, so any caller should 
just do a put.

Rob

Re: [PATCH 2/3] of: dynamic: add of_node_alloc() and of_node_free()

[Clément Léger]

Le Thu, 5 May 2022 14:43:54 -0500,
Rob Herring <robh@kernel.org> a écrit :

> On Wed, May 04, 2022 at 05:40:32PM +0200, Clément Léger wrote:
> > Add functions which allows to create and free nodes.
> > 
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---
> >  drivers/of/dynamic.c | 59 ++++++++++++++++++++++++++++++++++++--------
> >  include/linux/of.h   |  9 +++++++
> >  2 files changed, 58 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > index e8700e509d2e..ec28e5ba2969 100644
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> > @@ -455,6 +455,54 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> >  				 prop->length, allocflags);
> >  }
> >  
> > +/**
> > + * of_node_free - Free a node allocated dynamically.
> > + * @node:	Node to be freed
> > + */
> > +void of_node_free(const struct device_node *node)
> > +{
> > +	kfree(node->full_name);
> > +	kfree(node->data);
> > +	kfree(node);
> > +}
> > +EXPORT_SYMBOL(of_node_free);  
> 
> This shouldn't be needed. Nodes are refcounted, so any caller should 
> just do a put.

Acked. Do you want the name to be allocated as part of the node
allocation also ?

> 
> Rob



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH 2/3] of: dynamic: add of_node_alloc() and of_node_free()

[Rob Herring]

On Fri, May 6, 2022 at 5:45 AM Clément Léger <clement.leger@bootlin.com> wrote:
>
> Le Thu, 5 May 2022 14:43:54 -0500,
> Rob Herring <robh@kernel.org> a écrit :
>
> > On Wed, May 04, 2022 at 05:40:32PM +0200, Clément Léger wrote:
> > > Add functions which allows to create and free nodes.
> > >
> > > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > > ---
> > >  drivers/of/dynamic.c | 59 ++++++++++++++++++++++++++++++++++++--------
> > >  include/linux/of.h   |  9 +++++++
> > >  2 files changed, 58 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > > index e8700e509d2e..ec28e5ba2969 100644
> > > --- a/drivers/of/dynamic.c
> > > +++ b/drivers/of/dynamic.c
> > > @@ -455,6 +455,54 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> > >                              prop->length, allocflags);
> > >  }
> > >
> > > +/**
> > > + * of_node_free - Free a node allocated dynamically.
> > > + * @node:  Node to be freed
> > > + */
> > > +void of_node_free(const struct device_node *node)
> > > +{
> > > +   kfree(node->full_name);
> > > +   kfree(node->data);
> > > +   kfree(node);
> > > +}
> > > +EXPORT_SYMBOL(of_node_free);
> >
> > This shouldn't be needed. Nodes are refcounted, so any caller should
> > just do a put.
>
> Acked. Do you want the name to be allocated as part of the node
> allocation also ?

Yeah, I think that would be fine.

Rob

[PATCH 3/3] powerpc/pseries: use of_property_*() and of_node_*() functions

[Clément Léger]

Use of_property_alloc/free() and of_node_alloc/free() to create and free
device-tree nodes and properties.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 arch/powerpc/platforms/pseries/dlpar.c        | 51 +++----------------
 .../platforms/pseries/hotplug-memory.c        | 27 +---------
 arch/powerpc/platforms/pseries/reconfig.c     | 44 ++++------------
 3 files changed, 20 insertions(+), 102 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index b1f01ac0c29e..ebecde6c7256 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -39,61 +39,25 @@ struct cc_workarea {
 	__be32	prop_offset;
 };
 
-void dlpar_free_cc_property(struct property *prop)
-{
-	kfree(prop->name);
-	kfree(prop->value);
-	kfree(prop);
-}
-
 static struct property *dlpar_parse_cc_property(struct cc_workarea *ccwa)
 {
-	struct property *prop;
-	char *name;
-	char *value;
-
-	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
-	if (!prop)
-		return NULL;
+	int length;
+	char *name, *value;
 
 	name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
-	prop->name = kstrdup(name, GFP_KERNEL);
-	if (!prop->name) {
-		dlpar_free_cc_property(prop);
-		return NULL;
-	}
-
-	prop->length = be32_to_cpu(ccwa->prop_length);
+	length = be32_to_cpu(ccwa->prop_length);
 	value = (char *)ccwa + be32_to_cpu(ccwa->prop_offset);
-	prop->value = kmemdup(value, prop->length, GFP_KERNEL);
-	if (!prop->value) {
-		dlpar_free_cc_property(prop);
-		return NULL;
-	}
 
-	return prop;
+	return of_property_alloc(name, value, length, length, GFP_KERNEL);
 }
 
 static struct device_node *dlpar_parse_cc_node(struct cc_workarea *ccwa)
 {
-	struct device_node *dn;
 	const char *name;
 
-	dn = kzalloc(sizeof(*dn), GFP_KERNEL);
-	if (!dn)
-		return NULL;
-
 	name = (const char *)ccwa + be32_to_cpu(ccwa->name_offset);
-	dn->full_name = kstrdup(name, GFP_KERNEL);
-	if (!dn->full_name) {
-		kfree(dn);
-		return NULL;
-	}
-
-	of_node_set_flag(dn, OF_DYNAMIC);
-	of_node_init(dn);
 
-	return dn;
+	return of_node_alloc(name, GFP_KERNEL);
 }
 
 static void dlpar_free_one_cc_node(struct device_node *dn)
@@ -103,11 +67,10 @@ static void dlpar_free_one_cc_node(struct device_node *dn)
 	while (dn->properties) {
 		prop = dn->properties;
 		dn->properties = prop->next;
-		dlpar_free_cc_property(prop);
+		of_property_free(prop);
 	}
 
-	kfree(dn->full_name);
-	kfree(dn);
+	of_node_free(dn);
 }
 
 void dlpar_free_cc_nodes(struct device_node *dn)
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 91cf23495ccb..591727b05f36 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -70,34 +70,11 @@ unsigned long pseries_memory_block_size(void)
 	return memblock_size;
 }
 
-static void dlpar_free_property(struct property *prop)
-{
-	kfree(prop->name);
-	kfree(prop->value);
-	kfree(prop);
-}
-
 static struct property *dlpar_clone_property(struct property *prop,
 					     u32 prop_size)
 {
-	struct property *new_prop;
-
-	new_prop = kzalloc(sizeof(*new_prop), GFP_KERNEL);
-	if (!new_prop)
-		return NULL;
-
-	new_prop->name = kstrdup(prop->name, GFP_KERNEL);
-	new_prop->value = kzalloc(prop_size, GFP_KERNEL);
-	if (!new_prop->name || !new_prop->value) {
-		dlpar_free_property(new_prop);
-		return NULL;
-	}
-
-	memcpy(new_prop->value, prop->value, prop->length);
-	new_prop->length = prop_size;
-
-	of_property_set_flag(new_prop, OF_DYNAMIC);
-	return new_prop;
+	return of_property_alloc(prop->name, prop->value, prop->length,
+				 prop_size, GFP_KERNEL);
 }
 
 static bool find_aa_index(struct device_node *dr_node,
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 7f7369fec46b..08c2f9088537 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -25,17 +25,9 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
 	struct device_node *np;
 	int err = -ENOMEM;
 
-	np = kzalloc(sizeof(*np), GFP_KERNEL);
+	np = of_node_alloc(kbasename(path), GFP_KERNEL);
 	if (!np)
-		goto out_err;
-
-	np->full_name = kstrdup(kbasename(path), GFP_KERNEL);
-	if (!np->full_name)
-		goto out_err;
-
-	np->properties = proplist;
-	of_node_set_flag(np, OF_DYNAMIC);
-	of_node_init(np);
+		return -ENOMEM;
 
 	np->parent = pseries_of_derive_parent(path);
 	if (IS_ERR(np->parent)) {
@@ -56,8 +48,7 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
 out_err:
 	if (np) {
 		of_node_put(np->parent);
-		kfree(np->full_name);
-		kfree(np);
+		of_node_free(np);
 	}
 	return err;
 }
@@ -92,9 +83,7 @@ static void release_prop_list(const struct property *prop)
 	struct property *next;
 	for (; prop; prop = next) {
 		next = prop->next;
-		kfree(prop->name);
-		kfree(prop->value);
-		kfree(prop);
+		of_property_free(prop);
 	}
 
 }
@@ -168,27 +157,16 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
 static struct property *new_property(const char *name, const int length,
 				     const unsigned char *value, struct property *last)
 {
-	struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
+	struct property *prop;
 
-	if (!new)
+	prop = of_property_alloc(name, value, length, length + 1, GFP_KERNEL);
+	if (!prop)
 		return NULL;
 
-	if (!(new->name = kstrdup(name, GFP_KERNEL)))
-		goto cleanup;
-	if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
-		goto cleanup;
-
-	memcpy(new->value, value, length);
-	*(((char *)new->value) + length) = 0;
-	new->length = length;
-	new->next = last;
-	return new;
-
-cleanup:
-	kfree(new->name);
-	kfree(new->value);
-	kfree(new);
-	return NULL;
+	*(((char *)prop->value) + length) = 0;
+	prop->next = last;
+
+	return prop;
 }
 
 static int do_add_node(char *buf, size_t bufsize)
-- 
2.34.1