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

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

[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(). 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.

---

Changes in V2:
- Remove of_node_free()
- Rework property allocation to allocate both property and value with
  1 allocation
- Rework node allocation to allocate name at the same time the node is
  allocated
- Remove extern from definitions
- Remove of_property_alloc() value_len parameter and add more
  explanation for the arguments
- Add a check in of_property_free to check OF_DYNAMIC flag
- Add a commit which constify the property argument of
  of_property_check_flags()

Clément Léger (4):
  of: constify of_property_check_flags() prop argument
  of: dynamic: add of_property_alloc() and of_property_free()
  of: dynamic: add of_node_alloc()
  powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

 arch/powerpc/platforms/pseries/dlpar.c        |  51 +------
 .../platforms/pseries/hotplug-memory.c        |  21 +--
 arch/powerpc/platforms/pseries/reconfig.c     |  45 ++----
 drivers/of/dynamic.c                          | 134 ++++++++++++------
 drivers/of/of_private.h                       |  21 ++-
 include/linux/of.h                            |  25 +++-
 6 files changed, 153 insertions(+), 144 deletions(-)

-- 
2.36.0

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

[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(). 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.

---

Changes in V2:
- Remove of_node_free()
- Rework property allocation to allocate both property and value with
  1 allocation
- Rework node allocation to allocate name at the same time the node is
  allocated
- Remove extern from definitions
- Remove of_property_alloc() value_len parameter and add more
  explanation for the arguments
- Add a check in of_property_free to check OF_DYNAMIC flag
- Add a commit which constify the property argument of
  of_property_check_flags()

Clément Léger (4):
  of: constify of_property_check_flags() prop argument
  of: dynamic: add of_property_alloc() and of_property_free()
  of: dynamic: add of_node_alloc()
  powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

 arch/powerpc/platforms/pseries/dlpar.c        |  51 +------
 .../platforms/pseries/hotplug-memory.c        |  21 +--
 arch/powerpc/platforms/pseries/reconfig.c     |  45 ++----
 drivers/of/dynamic.c                          | 134 ++++++++++++------
 drivers/of/of_private.h                       |  21 ++-
 include/linux/of.h                            |  25 +++-
 6 files changed, 153 insertions(+), 144 deletions(-)

-- 
2.36.0

[PATCH v2 1/4] of: constify of_property_check_flags() prop argument

[Clément Léger]

This argument is not modified and thus can be set as const.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 include/linux/of.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index f0a5d6b10c5a..d74fd82a6963 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -207,7 +207,7 @@ static inline void of_node_clear_flag(struct device_node *n, unsigned long flag)
 }
 
 #if defined(CONFIG_OF_DYNAMIC) || defined(CONFIG_SPARC)
-static inline int of_property_check_flag(struct property *p, unsigned long flag)
+static inline int of_property_check_flag(const struct property *p, unsigned long flag)
 {
 	return test_bit(flag, &p->_flags);
 }
@@ -814,7 +814,8 @@ static inline void of_node_clear_flag(struct device_node *n, unsigned long flag)
 {
 }
 
-static inline int of_property_check_flag(struct property *p, unsigned long flag)
+static inline int of_property_check_flag(const struct property *p,
+					 unsigned long flag)
 {
 	return 0;
 }
-- 
2.36.0

[PATCH v2 1/4] of: constify of_property_check_flags() prop argument

[Clément Léger]

This argument is not modified and thus can be set as const.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 include/linux/of.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index f0a5d6b10c5a..d74fd82a6963 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -207,7 +207,7 @@ static inline void of_node_clear_flag(struct device_node *n, unsigned long flag)
 }
 
 #if defined(CONFIG_OF_DYNAMIC) || defined(CONFIG_SPARC)
-static inline int of_property_check_flag(struct property *p, unsigned long flag)
+static inline int of_property_check_flag(const struct property *p, unsigned long flag)
 {
 	return test_bit(flag, &p->_flags);
 }
@@ -814,7 +814,8 @@ static inline void of_node_clear_flag(struct device_node *n, unsigned long flag)
 {
 }
 
-static inline int of_property_check_flag(struct property *p, unsigned long flag)
+static inline int of_property_check_flag(const struct property *p,
+					 unsigned long flag)
 {
 	return 0;
 }
-- 
2.36.0

[PATCH v2 2/4] 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    | 82 ++++++++++++++++++++++++-----------------
 drivers/of/of_private.h | 21 ++++++++++-
 include/linux/of.h      | 14 +++++++
 3 files changed, 82 insertions(+), 35 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cd3821a6444f..c0dcbea31d28 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,66 @@ 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)
+{
+	if (!of_property_check_flag(prop, OF_DYNAMIC))
+		return;
+
+	if (prop->value != prop + 1)
+		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 or NULL
+ *		if only @len allocation is needed.
+ * @len:	Length of new property value and if @value is provided, the
+ * 		length of the value to be copied
  * @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,
+				   size_t len, gfp_t allocflags)
 {
-	struct property *new;
+	struct property *prop;
 
-	new = kzalloc(sizeof(*new), allocflags);
-	if (!new)
+	prop = kzalloc(sizeof(*prop) + len, allocflags);
+	if (!prop)
 		return NULL;
 
-	/*
-	 * 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.
-	 */
-	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;
-
-	/* mark the property as dynamic */
-	of_property_set_flag(new, OF_DYNAMIC);
-
-	return new;
-
- err_free:
-	kfree(new->name);
-	kfree(new->value);
-	kfree(new);
+	prop->name = kstrdup(name, allocflags);
+	if (!prop->name)
+		goto out_err;
+
+	prop->value = prop + 1;
+	if (value)
+		memcpy(prop->value, value, len);
+
+	prop->length = len;
+	of_property_set_flag(prop, OF_DYNAMIC);
+
+	return prop;
+
+out_err:
+	of_property_free(prop);
+
 	return NULL;
 }
+EXPORT_SYMBOL(of_property_alloc);
 
 /**
  * __of_node_dup() - Duplicate or create an empty device node dynamically.
@@ -447,9 +463,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/drivers/of/of_private.h b/drivers/of/of_private.h
index 9324483397f6..1d6459bf705d 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -115,7 +115,26 @@ extern void *__unflatten_device_tree(const void *blob,
  * without taking node references, so you either have to
  * own the devtree lock or work on detached trees only.
  */
-struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
+
+/**
+ * __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.
+ */
+static inline
+struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
+{
+	return of_property_alloc(prop->name, prop->value, prop->length,
+				 allocflags);
+}
+
 struct device_node *__of_node_dup(const struct device_node *np,
 				  const char *full_name);
 
diff --git a/include/linux/of.h b/include/linux/of.h
index d74fd82a6963..f1966f3c3847 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1464,6 +1464,10 @@ enum of_reconfig_change {
 };
 
 #ifdef CONFIG_OF_DYNAMIC
+struct property *of_property_alloc(const char *name, const void *value,
+				   size_t len, gfp_t allocflags);
+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);
@@ -1508,6 +1512,16 @@ 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,
+				   size_t 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.36.0

[PATCH v2 2/4] 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    | 82 ++++++++++++++++++++++++-----------------
 drivers/of/of_private.h | 21 ++++++++++-
 include/linux/of.h      | 14 +++++++
 3 files changed, 82 insertions(+), 35 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cd3821a6444f..c0dcbea31d28 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,66 @@ 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)
+{
+	if (!of_property_check_flag(prop, OF_DYNAMIC))
+		return;
+
+	if (prop->value != prop + 1)
+		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 or NULL
+ *		if only @len allocation is needed.
+ * @len:	Length of new property value and if @value is provided, the
+ * 		length of the value to be copied
  * @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,
+				   size_t len, gfp_t allocflags)
 {
-	struct property *new;
+	struct property *prop;
 
-	new = kzalloc(sizeof(*new), allocflags);
-	if (!new)
+	prop = kzalloc(sizeof(*prop) + len, allocflags);
+	if (!prop)
 		return NULL;
 
-	/*
-	 * 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.
-	 */
-	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;
-
-	/* mark the property as dynamic */
-	of_property_set_flag(new, OF_DYNAMIC);
-
-	return new;
-
- err_free:
-	kfree(new->name);
-	kfree(new->value);
-	kfree(new);
+	prop->name = kstrdup(name, allocflags);
+	if (!prop->name)
+		goto out_err;
+
+	prop->value = prop + 1;
+	if (value)
+		memcpy(prop->value, value, len);
+
+	prop->length = len;
+	of_property_set_flag(prop, OF_DYNAMIC);
+
+	return prop;
+
+out_err:
+	of_property_free(prop);
+
 	return NULL;
 }
+EXPORT_SYMBOL(of_property_alloc);
 
 /**
  * __of_node_dup() - Duplicate or create an empty device node dynamically.
@@ -447,9 +463,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/drivers/of/of_private.h b/drivers/of/of_private.h
index 9324483397f6..1d6459bf705d 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -115,7 +115,26 @@ extern void *__unflatten_device_tree(const void *blob,
  * without taking node references, so you either have to
  * own the devtree lock or work on detached trees only.
  */
-struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
+
+/**
+ * __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.
+ */
+static inline
+struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
+{
+	return of_property_alloc(prop->name, prop->value, prop->length,
+				 allocflags);
+}
+
 struct device_node *__of_node_dup(const struct device_node *np,
 				  const char *full_name);
 
diff --git a/include/linux/of.h b/include/linux/of.h
index d74fd82a6963..f1966f3c3847 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1464,6 +1464,10 @@ enum of_reconfig_change {
 };
 
 #ifdef CONFIG_OF_DYNAMIC
+struct property *of_property_alloc(const char *name, const void *value,
+				   size_t len, gfp_t allocflags);
+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);
@@ -1508,6 +1512,16 @@ 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,
+				   size_t 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.36.0

[PATCH v2 3/4] of: dynamic: add of_node_alloc()

[Clément Léger]

Add of_node_alloc() which allows to create nodes. The node full_name
field is allocated as part of the node allocation and the kfree() for
this field is checked at release time to allow users using their own
allocated name.

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

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index c0dcbea31d28..67636aafe810 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -359,7 +359,9 @@ void of_node_release(struct kobject *kobj)
 	property_list_free(node->deadprops);
 	fwnode_links_purge(of_fwnode_handle(node));
 
-	kfree(node->full_name);
+	if (node->full_name != (const char *) (node + 1))
+		kfree(node->full_name);
+
 	kfree(node->data);
 	kfree(node);
 }
@@ -426,6 +428,43 @@ struct property *of_property_alloc(const char *name, const void *value,
 }
 EXPORT_SYMBOL(of_property_alloc);
 
+/**
+ * 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;
+	int name_len = 0;
+
+	if (name)
+		name_len = strlen(name) + 1;
+
+	node = kzalloc(sizeof(*node) + name_len, allocflags);
+	if (!node)
+		return NULL;
+
+	if (name) {
+		node->full_name = (const char *) (node + 1);
+		strcpy((char *)node->full_name, name);
+	}
+
+	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
@@ -442,18 +481,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(full_name, 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 f1966f3c3847..c18da685ade1 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1464,6 +1464,7 @@ enum of_reconfig_change {
 };
 
 #ifdef CONFIG_OF_DYNAMIC
+struct device_node *of_node_alloc(const char *name, gfp_t allocflags);
 struct property *of_property_alloc(const char *name, const void *value,
 				   size_t len, gfp_t allocflags);
 void of_property_free(const struct property *prop);
@@ -1512,6 +1513,11 @@ 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
 struct property *of_property_alloc(const char *name, const void *value,
-- 
2.36.0

[PATCH v2 3/4] of: dynamic: add of_node_alloc()

[Clément Léger]

Add of_node_alloc() which allows to create nodes. The node full_name
field is allocated as part of the node allocation and the kfree() for
this field is checked at release time to allow users using their own
allocated name.

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

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index c0dcbea31d28..67636aafe810 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -359,7 +359,9 @@ void of_node_release(struct kobject *kobj)
 	property_list_free(node->deadprops);
 	fwnode_links_purge(of_fwnode_handle(node));
 
-	kfree(node->full_name);
+	if (node->full_name != (const char *) (node + 1))
+		kfree(node->full_name);
+
 	kfree(node->data);
 	kfree(node);
 }
@@ -426,6 +428,43 @@ struct property *of_property_alloc(const char *name, const void *value,
 }
 EXPORT_SYMBOL(of_property_alloc);
 
+/**
+ * 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;
+	int name_len = 0;
+
+	if (name)
+		name_len = strlen(name) + 1;
+
+	node = kzalloc(sizeof(*node) + name_len, allocflags);
+	if (!node)
+		return NULL;
+
+	if (name) {
+		node->full_name = (const char *) (node + 1);
+		strcpy((char *)node->full_name, name);
+	}
+
+	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
@@ -442,18 +481,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(full_name, 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 f1966f3c3847..c18da685ade1 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1464,6 +1464,7 @@ enum of_reconfig_change {
 };
 
 #ifdef CONFIG_OF_DYNAMIC
+struct device_node *of_node_alloc(const char *name, gfp_t allocflags);
 struct property *of_property_alloc(const char *name, const void *value,
 				   size_t len, gfp_t allocflags);
 void of_property_free(const struct property *prop);
@@ -1512,6 +1513,11 @@ 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
 struct property *of_property_alloc(const char *name, const void *value,
-- 
2.36.0

[PATCH v2 4/4] powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

[Clément Léger]

Use of_property_alloc/free() and of_node_alloc() 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        | 21 +-------
 arch/powerpc/platforms/pseries/reconfig.c     | 45 +++++-----------
 3 files changed, 21 insertions(+), 96 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 498d6efcb5ae..5a04566e98a4 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -38,61 +38,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, 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)
@@ -102,11 +66,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_put(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 2e3a317722a8..2ddf2a0ba048 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -69,33 +69,16 @@ 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);
+	struct property *new_prop = of_property_alloc(prop->name, NULL,
+						      prop_size, 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;
 }
 
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index cad7a0c93117..f1a364995e82 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -24,17 +24,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)) {
@@ -55,8 +47,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_put(np);
 	}
 	return err;
 }
@@ -91,9 +82,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);
 	}
 
 }
@@ -167,27 +156,17 @@ 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, NULL, 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;
+	memcpy(prop->value, value, length);
+	*(((char *)prop->value) + length) = 0;
+	prop->next = last;
+
+	return prop;
 }
 
 static int do_add_node(char *buf, size_t bufsize)
-- 
2.36.0

[PATCH v2 4/4] powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

[Clément Léger]

Use of_property_alloc/free() and of_node_alloc() 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        | 21 +-------
 arch/powerpc/platforms/pseries/reconfig.c     | 45 +++++-----------
 3 files changed, 21 insertions(+), 96 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 498d6efcb5ae..5a04566e98a4 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -38,61 +38,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, 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)
@@ -102,11 +66,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_put(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 2e3a317722a8..2ddf2a0ba048 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -69,33 +69,16 @@ 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);
+	struct property *new_prop = of_property_alloc(prop->name, NULL,
+						      prop_size, 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;
 }
 
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index cad7a0c93117..f1a364995e82 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -24,17 +24,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)) {
@@ -55,8 +47,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_put(np);
 	}
 	return err;
 }
@@ -91,9 +82,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);
 	}
 
 }
@@ -167,27 +156,17 @@ 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, NULL, 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;
+	memcpy(prop->value, value, length);
+	*(((char *)prop->value) + length) = 0;
+	prop->next = last;
+
+	return prop;
 }
 
 static int do_add_node(char *buf, size_t bufsize)
-- 
2.36.0

Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property_free()

[Tyrel Datwyler]

On 6/1/22 01:17, 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    | 82 ++++++++++++++++++++++++-----------------
>  drivers/of/of_private.h | 21 ++++++++++-
>  include/linux/of.h      | 14 +++++++
>  3 files changed, 82 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index cd3821a6444f..c0dcbea31d28 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,66 @@ 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)
> +{
> +	if (!of_property_check_flag(prop, OF_DYNAMIC))
> +		return;
> +

This looks wrong to me. From what I understand the value data is allocated as
trailing memory that is part of the property allocation itself. (ie. prop =
kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
of the trailing value data. Calling kfree(prop->value) is bogus since
prop->value wasn't dynamically allocated on its own.

Also, this condition will always fail. You explicitly set prop->value = prop + 1
in alloc.

Maybe I need to go back and look at v1 again.

-Tyrel

> +	if (prop->value != prop + 1)
> +		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 or NULL
> + *		if only @len allocation is needed.
> + * @len:	Length of new property value and if @value is provided, the
> + * 		length of the value to be copied
>   * @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,
> +				   size_t len, gfp_t allocflags)
>  {
> -	struct property *new;
> +	struct property *prop;
> 
> -	new = kzalloc(sizeof(*new), allocflags);
> -	if (!new)
> +	prop = kzalloc(sizeof(*prop) + len, allocflags);
> +	if (!prop)
>  		return NULL;
> 
> -	/*
> -	 * 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.
> -	 */
> -	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;
> -
> -	/* mark the property as dynamic */
> -	of_property_set_flag(new, OF_DYNAMIC);
> -
> -	return new;
> -
> - err_free:
> -	kfree(new->name);
> -	kfree(new->value);
> -	kfree(new);
> +	prop->name = kstrdup(name, allocflags);
> +	if (!prop->name)
> +		goto out_err;
> +
> +	prop->value = prop + 1;
> +	if (value)
> +		memcpy(prop->value, value, len);
> +
> +	prop->length = len;
> +	of_property_set_flag(prop, OF_DYNAMIC);
> +
> +	return prop;
> +
> +out_err:
> +	of_property_free(prop);
> +
>  	return NULL;
>  }
> +EXPORT_SYMBOL(of_property_alloc);
> 
>  /**
>   * __of_node_dup() - Duplicate or create an empty device node dynamically.
> @@ -447,9 +463,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/drivers/of/of_private.h b/drivers/of/of_private.h
> index 9324483397f6..1d6459bf705d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -115,7 +115,26 @@ extern void *__unflatten_device_tree(const void *blob,
>   * without taking node references, so you either have to
>   * own the devtree lock or work on detached trees only.
>   */
> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
> +
> +/**
> + * __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.
> + */
> +static inline
> +struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +{
> +	return of_property_alloc(prop->name, prop->value, prop->length,
> +				 allocflags);
> +}
> +
>  struct device_node *__of_node_dup(const struct device_node *np,
>  				  const char *full_name);
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index d74fd82a6963..f1966f3c3847 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1464,6 +1464,10 @@ enum of_reconfig_change {
>  };
> 
>  #ifdef CONFIG_OF_DYNAMIC
> +struct property *of_property_alloc(const char *name, const void *value,
> +				   size_t len, gfp_t allocflags);
> +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);
> @@ -1508,6 +1512,16 @@ 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,
> +				   size_t 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;

Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property_free()

[Tyrel Datwyler]

On 6/1/22 01:17, 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    | 82 ++++++++++++++++++++++++-----------------
>  drivers/of/of_private.h | 21 ++++++++++-
>  include/linux/of.h      | 14 +++++++
>  3 files changed, 82 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index cd3821a6444f..c0dcbea31d28 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,66 @@ 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)
> +{
> +	if (!of_property_check_flag(prop, OF_DYNAMIC))
> +		return;
> +

This looks wrong to me. From what I understand the value data is allocated as
trailing memory that is part of the property allocation itself. (ie. prop =
kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
of the trailing value data. Calling kfree(prop->value) is bogus since
prop->value wasn't dynamically allocated on its own.

Also, this condition will always fail. You explicitly set prop->value = prop + 1
in alloc.

Maybe I need to go back and look at v1 again.

-Tyrel

> +	if (prop->value != prop + 1)
> +		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 or NULL
> + *		if only @len allocation is needed.
> + * @len:	Length of new property value and if @value is provided, the
> + * 		length of the value to be copied
>   * @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,
> +				   size_t len, gfp_t allocflags)
>  {
> -	struct property *new;
> +	struct property *prop;
> 
> -	new = kzalloc(sizeof(*new), allocflags);
> -	if (!new)
> +	prop = kzalloc(sizeof(*prop) + len, allocflags);
> +	if (!prop)
>  		return NULL;
> 
> -	/*
> -	 * 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.
> -	 */
> -	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;
> -
> -	/* mark the property as dynamic */
> -	of_property_set_flag(new, OF_DYNAMIC);
> -
> -	return new;
> -
> - err_free:
> -	kfree(new->name);
> -	kfree(new->value);
> -	kfree(new);
> +	prop->name = kstrdup(name, allocflags);
> +	if (!prop->name)
> +		goto out_err;
> +
> +	prop->value = prop + 1;
> +	if (value)
> +		memcpy(prop->value, value, len);
> +
> +	prop->length = len;
> +	of_property_set_flag(prop, OF_DYNAMIC);
> +
> +	return prop;
> +
> +out_err:
> +	of_property_free(prop);
> +
>  	return NULL;
>  }
> +EXPORT_SYMBOL(of_property_alloc);
> 
>  /**
>   * __of_node_dup() - Duplicate or create an empty device node dynamically.
> @@ -447,9 +463,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/drivers/of/of_private.h b/drivers/of/of_private.h
> index 9324483397f6..1d6459bf705d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -115,7 +115,26 @@ extern void *__unflatten_device_tree(const void *blob,
>   * without taking node references, so you either have to
>   * own the devtree lock or work on detached trees only.
>   */
> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
> +
> +/**
> + * __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.
> + */
> +static inline
> +struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +{
> +	return of_property_alloc(prop->name, prop->value, prop->length,
> +				 allocflags);
> +}
> +
>  struct device_node *__of_node_dup(const struct device_node *np,
>  				  const char *full_name);
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index d74fd82a6963..f1966f3c3847 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1464,6 +1464,10 @@ enum of_reconfig_change {
>  };
> 
>  #ifdef CONFIG_OF_DYNAMIC
> +struct property *of_property_alloc(const char *name, const void *value,
> +				   size_t len, gfp_t allocflags);
> +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);
> @@ -1508,6 +1512,16 @@ 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,
> +				   size_t 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;

Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property_free()

[Clément Léger]

Le Wed, 1 Jun 2022 15:32:29 -0700,
Tyrel Datwyler <tyreld@linux.ibm.com> a écrit :

> >  /**
> > - * __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)
> > +{
> > +	if (!of_property_check_flag(prop, OF_DYNAMIC))
> > +		return;
> > +  
> 
> This looks wrong to me. From what I understand the value data is allocated as
> trailing memory that is part of the property allocation itself. (ie. prop =
> kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
> of the trailing value data. Calling kfree(prop->value) is bogus since
> prop->value wasn't dynamically allocated on its own.

kfree(prop->value) is only called if the value is not the trailing data
of the property so I don't see what is wrong there. In that case, only
kfree(prop) is called.

> 
> Also, this condition will always fail. You explicitly set prop->value = prop + 1
> in alloc.

The user that did allocated the property might want to provide its own
"value". In that case, prop->value would be overwritten by the user
allocated value and thus the check would be true, hence calling
kfree(prop->value).

> 
> Maybe I need to go back and look at v1 again.
> 
> -Tyrel
> 
> > +	if (prop->value != prop + 1)
> > +		kfree(prop->value);
> > +
> > +	kfree(prop->name);
> > +	kfree(prop);
> > +}
> > +EXPORT_SYMBOL(of_property_free);
> > +


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

Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property_free()

[Clément Léger]

Le Wed, 1 Jun 2022 15:32:29 -0700,
Tyrel Datwyler <tyreld@linux.ibm.com> a écrit :

> >  /**
> > - * __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)
> > +{
> > +	if (!of_property_check_flag(prop, OF_DYNAMIC))
> > +		return;
> > +  
> 
> This looks wrong to me. From what I understand the value data is allocated as
> trailing memory that is part of the property allocation itself. (ie. prop =
> kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
> of the trailing value data. Calling kfree(prop->value) is bogus since
> prop->value wasn't dynamically allocated on its own.

kfree(prop->value) is only called if the value is not the trailing data
of the property so I don't see what is wrong there. In that case, only
kfree(prop) is called.

> 
> Also, this condition will always fail. You explicitly set prop->value = prop + 1
> in alloc.

The user that did allocated the property might want to provide its own
"value". In that case, prop->value would be overwritten by the user
allocated value and thus the check would be true, hence calling
kfree(prop->value).

> 
> Maybe I need to go back and look at v1 again.
> 
> -Tyrel
> 
> > +	if (prop->value != prop + 1)
> > +		kfree(prop->value);
> > +
> > +	kfree(prop->name);
> > +	kfree(prop);
> > +}
> > +EXPORT_SYMBOL(of_property_free);
> > +


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

Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property_free()

[Tyrel Datwyler]

On 6/1/22 23:58, Clément Léger wrote:
> Le Wed, 1 Jun 2022 15:32:29 -0700,
> Tyrel Datwyler <tyreld@linux.ibm.com> a écrit :
> 
>>>  /**
>>> - * __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)
>>> +{
>>> +	if (!of_property_check_flag(prop, OF_DYNAMIC))
>>> +		return;
>>> +  
>>
>> This looks wrong to me. From what I understand the value data is allocated as
>> trailing memory that is part of the property allocation itself. (ie. prop =
>> kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
>> of the trailing value data. Calling kfree(prop->value) is bogus since
>> prop->value wasn't dynamically allocated on its own.
> 
> kfree(prop->value) is only called if the value is not the trailing data
> of the property so I don't see what is wrong there. In that case, only
> kfree(prop) is called.

Right, Rob clarified for me in the v1 patch.

> 
>>
>> Also, this condition will always fail. You explicitly set prop->value = prop + 1
>> in alloc.
> 
> The user that did allocated the property might want to provide its own
> "value". In that case, prop->value would be overwritten by the user
> allocated value and thus the check would be true, hence calling
> kfree(prop->value).

So, that was the part I was missing. I think a comment would be helpful so its
clear value can be either trailing or user assigned.

-Tyrel

> 
>>
>> Maybe I need to go back and look at v1 again.
>>
>> -Tyrel
>>
>>> +	if (prop->value != prop + 1)
>>> +		kfree(prop->value);
>>> +
>>> +	kfree(prop->name);
>>> +	kfree(prop);
>>> +}
>>> +EXPORT_SYMBOL(of_property_free);
>>> +
> 
> 

Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property_free()

[Tyrel Datwyler]

On 6/1/22 23:58, Clément Léger wrote:
> Le Wed, 1 Jun 2022 15:32:29 -0700,
> Tyrel Datwyler <tyreld@linux.ibm.com> a écrit :
> 
>>>  /**
>>> - * __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)
>>> +{
>>> +	if (!of_property_check_flag(prop, OF_DYNAMIC))
>>> +		return;
>>> +  
>>
>> This looks wrong to me. From what I understand the value data is allocated as
>> trailing memory that is part of the property allocation itself. (ie. prop =
>> kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
>> of the trailing value data. Calling kfree(prop->value) is bogus since
>> prop->value wasn't dynamically allocated on its own.
> 
> kfree(prop->value) is only called if the value is not the trailing data
> of the property so I don't see what is wrong there. In that case, only
> kfree(prop) is called.

Right, Rob clarified for me in the v1 patch.

> 
>>
>> Also, this condition will always fail. You explicitly set prop->value = prop + 1
>> in alloc.
> 
> The user that did allocated the property might want to provide its own
> "value". In that case, prop->value would be overwritten by the user
> allocated value and thus the check would be true, hence calling
> kfree(prop->value).

So, that was the part I was missing. I think a comment would be helpful so its
clear value can be either trailing or user assigned.

-Tyrel

> 
>>
>> Maybe I need to go back and look at v1 again.
>>
>> -Tyrel
>>
>>> +	if (prop->value != prop + 1)
>>> +		kfree(prop->value);
>>> +
>>> +	kfree(prop->name);
>>> +	kfree(prop);
>>> +}
>>> +EXPORT_SYMBOL(of_property_free);
>>> +
> 
> 

Re: [PATCH v2 4/4] powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

[Rob Herring]

On Wed, Jun 01, 2022 at 10:18:01AM +0200, Clément Léger wrote:
> Use of_property_alloc/free() and of_node_alloc() 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        | 21 +-------
>  arch/powerpc/platforms/pseries/reconfig.c     | 45 +++++-----------
>  3 files changed, 21 insertions(+), 96 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
> index 498d6efcb5ae..5a04566e98a4 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -38,61 +38,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, 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);

Do you have any need for different flags? I can't really see a need for 
atomic or dma allocs or ???, so drop it I think.

>  }
>  
>  static void dlpar_free_one_cc_node(struct device_node *dn)
> @@ -102,11 +66,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);

We should be able to just put the node and all the properties already 
attached will be freed.

Looking at the history of this code, it originally did the kref_init 
much later in dlpar_attach_node(). So there was a window of allocating 
the node and adding properties where you'd need to manually free 
everything. Now that the node is referenced from the start, a put should 
free everything.

>  	}
>  
> -	kfree(dn->full_name);
> -	kfree(dn);
> +	of_node_put(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 2e3a317722a8..2ddf2a0ba048 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -69,33 +69,16 @@ 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);
> +	struct property *new_prop = of_property_alloc(prop->name, NULL,
> +						      prop_size, 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;
>  }
>  
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index cad7a0c93117..f1a364995e82 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -24,17 +24,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)) {
> @@ -55,8 +47,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_put(np);
>  	}
>  	return err;
>  }
> @@ -91,9 +82,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);

Looks like you need this because code does: alloc properties, alloc 
node, add properties, attach node. It would need to be refactored to 
alloc the node first, but that's a bit more complex needing someone to 
test on pSeries.

>  	}
>  
>  }
> @@ -167,27 +156,17 @@ 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, NULL, 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;
> +	memcpy(prop->value, value, length);
> +	*(((char *)prop->value) + length) = 0;

Looks to me like this could be avoided with this change:

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index cad7a0c93117..614753fc5f27 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -148,7 +148,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
        /* now we're on the value */
        *value = tmp;
        tmp += *length;
-       if (tmp > end) {
+       if (tmp >= end) {
                printk(KERN_ERR "property parse failed in %s at line %d\n",
                       __func__, __LINE__);
                return NULL;
@@ -158,6 +158,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
                       __func__, __LINE__);
                return NULL;
        }
+       *tmp = '\0';
        tmp++;
 
        /* and now we should be on the next name, or the end */


Based on the comments, 'buf' should be nul terminated, so I would think 
that tmp == end would be an error. But I really don't know.

Really need some pSeries people to comment on all this.

Another option is if value is NULL, then of_property_alloc() should 
ensure the buffer is zeroed. Then you just need the memcpy.

Rob

Re: [PATCH v2 4/4] powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

[Rob Herring]

On Wed, Jun 01, 2022 at 10:18:01AM +0200, Clément Léger wrote:
> Use of_property_alloc/free() and of_node_alloc() 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        | 21 +-------
>  arch/powerpc/platforms/pseries/reconfig.c     | 45 +++++-----------
>  3 files changed, 21 insertions(+), 96 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
> index 498d6efcb5ae..5a04566e98a4 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -38,61 +38,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, 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);

Do you have any need for different flags? I can't really see a need for 
atomic or dma allocs or ???, so drop it I think.

>  }
>  
>  static void dlpar_free_one_cc_node(struct device_node *dn)
> @@ -102,11 +66,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);

We should be able to just put the node and all the properties already 
attached will be freed.

Looking at the history of this code, it originally did the kref_init 
much later in dlpar_attach_node(). So there was a window of allocating 
the node and adding properties where you'd need to manually free 
everything. Now that the node is referenced from the start, a put should 
free everything.

>  	}
>  
> -	kfree(dn->full_name);
> -	kfree(dn);
> +	of_node_put(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 2e3a317722a8..2ddf2a0ba048 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -69,33 +69,16 @@ 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);
> +	struct property *new_prop = of_property_alloc(prop->name, NULL,
> +						      prop_size, 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;
>  }
>  
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index cad7a0c93117..f1a364995e82 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -24,17 +24,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)) {
> @@ -55,8 +47,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_put(np);
>  	}
>  	return err;
>  }
> @@ -91,9 +82,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);

Looks like you need this because code does: alloc properties, alloc 
node, add properties, attach node. It would need to be refactored to 
alloc the node first, but that's a bit more complex needing someone to 
test on pSeries.

>  	}
>  
>  }
> @@ -167,27 +156,17 @@ 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, NULL, 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;
> +	memcpy(prop->value, value, length);
> +	*(((char *)prop->value) + length) = 0;

Looks to me like this could be avoided with this change:

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index cad7a0c93117..614753fc5f27 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -148,7 +148,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
        /* now we're on the value */
        *value = tmp;
        tmp += *length;
-       if (tmp > end) {
+       if (tmp >= end) {
                printk(KERN_ERR "property parse failed in %s at line %d\n",
                       __func__, __LINE__);
                return NULL;
@@ -158,6 +158,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
                       __func__, __LINE__);
                return NULL;
        }
+       *tmp = '\0';
        tmp++;
 
        /* and now we should be on the next name, or the end */


Based on the comments, 'buf' should be nul terminated, so I would think 
that tmp == end would be an error. But I really don't know.

Really need some pSeries people to comment on all this.

Another option is if value is NULL, then of_property_alloc() should 
ensure the buffer is zeroed. Then you just need the memcpy.

Rob

Re: [PATCH v2 1/4] of: constify of_property_check_flags() prop argument

[Rob Herring]

On Wed, 01 Jun 2022 10:17:58 +0200, Clément Léger wrote:
> This argument is not modified and thus can be set as const.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  include/linux/of.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

As this looks independent of the rest of the series, applied, thanks!

Re: [PATCH v2 1/4] of: constify of_property_check_flags() prop argument

[Rob Herring]

On Wed, 01 Jun 2022 10:17:58 +0200, Clément Léger wrote:
> This argument is not modified and thus can be set as const.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  include/linux/of.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

As this looks independent of the rest of the series, applied, thanks!

Re: [PATCH v2 4/4] powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

[Clément Léger]

Le Fri, 3 Jun 2022 15:14:07 -0500,
Rob Herring <robh@kernel.org> a écrit :

> >  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);  
> 
> Do you have any need for different flags? I can't really see a need for 
> atomic or dma allocs or ???, so drop it I think.

Hum no, i copied this behavior from an existing function. I'll remove
that.

> 
> >  }
> >  
> >  static void dlpar_free_one_cc_node(struct device_node *dn)
> > @@ -102,11 +66,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);  
> 
> We should be able to just put the node and all the properties already 
> attached will be freed.

Indeed !

> 
> Looking at the history of this code, it originally did the kref_init 
> much later in dlpar_attach_node(). So there was a window of allocating 
> the node and adding properties where you'd need to manually free 
> everything. Now that the node is referenced from the start, a put should 
> free everything.
> 
> > @@ -91,9 +82,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);  
> 
> Looks like you need this because code does: alloc properties, alloc 
> node, add properties, attach node. It would need to be refactored to 
> alloc the node first, but that's a bit more complex needing someone to 
> test on pSeries.

Acked.

> 
> >  	}
> >  
> >  }
> > @@ -167,27 +156,17 @@ 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, NULL, 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;
> > +	memcpy(prop->value, value, length);
> > +	*(((char *)prop->value) + length) = 0;  
> 
> Looks to me like this could be avoided with this change:
> 
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index cad7a0c93117..614753fc5f27 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -148,7 +148,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
>         /* now we're on the value */
>         *value = tmp;
>         tmp += *length;
> -       if (tmp > end) {
> +       if (tmp >= end) {
>                 printk(KERN_ERR "property parse failed in %s at line %d\n",
>                        __func__, __LINE__);
>                 return NULL;
> @@ -158,6 +158,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
>                        __func__, __LINE__);
>                 return NULL;
>         }
> +       *tmp = '\0';
>         tmp++;
>  
>         /* and now we should be on the next name, or the end */
> 
> 
> Based on the comments, 'buf' should be nul terminated, so I would think 
> that tmp == end would be an error. But I really don't know.
> 
> Really need some pSeries people to comment on all this.
> 
> Another option is if value is NULL, then of_property_alloc() should 
> ensure the buffer is zeroed. Then you just need the memcpy.

Probably looks like a safe behavior anyway to zero the value buffer.
I'll add that.

Thanks,

Clément

> 
> Rob



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

Re: [PATCH v2 4/4] powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

[Clément Léger]

Le Fri, 3 Jun 2022 15:14:07 -0500,
Rob Herring <robh@kernel.org> a écrit :

> >  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);  
> 
> Do you have any need for different flags? I can't really see a need for 
> atomic or dma allocs or ???, so drop it I think.

Hum no, i copied this behavior from an existing function. I'll remove
that.

> 
> >  }
> >  
> >  static void dlpar_free_one_cc_node(struct device_node *dn)
> > @@ -102,11 +66,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);  
> 
> We should be able to just put the node and all the properties already 
> attached will be freed.

Indeed !

> 
> Looking at the history of this code, it originally did the kref_init 
> much later in dlpar_attach_node(). So there was a window of allocating 
> the node and adding properties where you'd need to manually free 
> everything. Now that the node is referenced from the start, a put should 
> free everything.
> 
> > @@ -91,9 +82,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);  
> 
> Looks like you need this because code does: alloc properties, alloc 
> node, add properties, attach node. It would need to be refactored to 
> alloc the node first, but that's a bit more complex needing someone to 
> test on pSeries.

Acked.

> 
> >  	}
> >  
> >  }
> > @@ -167,27 +156,17 @@ 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, NULL, 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;
> > +	memcpy(prop->value, value, length);
> > +	*(((char *)prop->value) + length) = 0;  
> 
> Looks to me like this could be avoided with this change:
> 
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index cad7a0c93117..614753fc5f27 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -148,7 +148,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
>         /* now we're on the value */
>         *value = tmp;
>         tmp += *length;
> -       if (tmp > end) {
> +       if (tmp >= end) {
>                 printk(KERN_ERR "property parse failed in %s at line %d\n",
>                        __func__, __LINE__);
>                 return NULL;
> @@ -158,6 +158,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
>                        __func__, __LINE__);
>                 return NULL;
>         }
> +       *tmp = '\0';
>         tmp++;
>  
>         /* and now we should be on the next name, or the end */
> 
> 
> Based on the comments, 'buf' should be nul terminated, so I would think 
> that tmp == end would be an error. But I really don't know.
> 
> Really need some pSeries people to comment on all this.
> 
> Another option is if value is NULL, then of_property_alloc() should 
> ensure the buffer is zeroed. Then you just need the memcpy.

Probably looks like a safe behavior anyway to zero the value buffer.
I'll add that.

Thanks,

Clément

> 
> Rob



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