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

[PATCH v3 0/5] 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. This code has been more than simply replaced to allow using
of_node_put() rather than a manual deletion of the properties.
Unfortunately, as I don't own a powerpc platform, it would need to be
tested.

---

Changes in V3:
- Remove gfpflag attribute from of_node_alloc() and of_property_alloc().
- Removed allocflags from __of_node_dup().
- Rework powerpc code to only use of_node_put().
- Fix properties free using of_node_property in OF unittests.

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 (5):
  of: constify of_property_check_flags() prop argument
  of: remove __of_node_dup() allocflags parameter
  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        |  62 +-------
 .../platforms/pseries/hotplug-memory.c        |  21 +--
 arch/powerpc/platforms/pseries/reconfig.c     | 123 ++++++----------
 drivers/of/dynamic.c                          | 137 ++++++++++++------
 drivers/of/of_private.h                       |  19 ++-
 drivers/of/overlay.c                          |   2 +-
 drivers/of/unittest.c                         |  24 ++-
 include/linux/of.h                            |  24 ++-
 8 files changed, 191 insertions(+), 221 deletions(-)

-- 
2.36.1

[PATCH v3 1/5] 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.1

Re: [PATCH v3 1/5] of: constify of_property_check_flags() prop argument

[Rob Herring]

On Mon, Jun 20, 2022 at 12:41:19PM +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(-)

I already applied this patch, don't resend it.

Rob

[PATCH v3 2/5] of: remove __of_node_dup() allocflags parameter

[Clément Léger]

The alloclags are always set to GFP_KERNEL so remove this specific flag.
Moreover, this function is going to be based on one that does not
provides passing gfp flags, so be prepared for this.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/of/dynamic.c    |  9 ++++-----
 drivers/of/of_private.h |  2 +-
 drivers/of/overlay.c    |  2 +-
 drivers/of/unittest.c   | 16 ++++++++--------
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cd3821a6444f..5560e501aedf 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -369,7 +369,6 @@ void of_node_release(struct kobject *kobj)
 /**
  * __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
@@ -378,11 +377,11 @@ void of_node_release(struct kobject *kobj)
  *
  * 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_prop_dup(const struct property *prop)
 {
 	struct property *new;
 
-	new = kzalloc(sizeof(*new), allocflags);
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
 	if (!new)
 		return NULL;
 
@@ -392,8 +391,8 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
 	 * 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->name = kstrdup(prop->name, GFP_KERNEL);
+	new->value = kmemdup(prop->value, prop->length, GFP_KERNEL);
 	new->length = prop->length;
 	if (!new->name || !new->value)
 		goto err_free;
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 9324483397f6..e319d8c2bf8b 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -115,7 +115,7 @@ 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);
+struct property *__of_prop_dup(const struct property *prop);
 struct device_node *__of_node_dup(const struct device_node *np,
 				  const char *full_name);
 
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 4044ddcb02c6..9c026bcbf6ab 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -339,7 +339,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
 			return -EINVAL;
 		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
 	} else {
-		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
+		new_prop = __of_prop_dup(overlay_prop);
 	}
 
 	if (!new_prop)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 7f6bba18c515..23ccb74ef408 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -772,13 +772,13 @@ static void __init of_unittest_property_copy(void)
 	struct property p2 = { .name = "p2", .length = 5, .value = "abcd" };
 	struct property *new;
 
-	new = __of_prop_dup(&p1, GFP_KERNEL);
+	new = __of_prop_dup(&p1);
 	unittest(new && propcmp(&p1, new), "empty property didn't copy correctly\n");
 	kfree(new->value);
 	kfree(new->name);
 	kfree(new);
 
-	new = __of_prop_dup(&p2, GFP_KERNEL);
+	new = __of_prop_dup(&p2);
 	unittest(new && propcmp(&p2, new), "non-empty property didn't copy correctly\n");
 	kfree(new->value);
 	kfree(new->name);
@@ -811,19 +811,19 @@ static void __init of_unittest_changeset(void)
 	nremove = of_get_child_by_name(nchangeset, "node-remove");
 	unittest(nremove, "testcase setup failure\n");
 
-	ppadd = __of_prop_dup(&padd, GFP_KERNEL);
+	ppadd = __of_prop_dup(&padd);
 	unittest(ppadd, "testcase setup failure\n");
 
-	ppname_n1  = __of_prop_dup(&pname_n1, GFP_KERNEL);
+	ppname_n1  = __of_prop_dup(&pname_n1);
 	unittest(ppname_n1, "testcase setup failure\n");
 
-	ppname_n2  = __of_prop_dup(&pname_n2, GFP_KERNEL);
+	ppname_n2  = __of_prop_dup(&pname_n2);
 	unittest(ppname_n2, "testcase setup failure\n");
 
-	ppname_n21 = __of_prop_dup(&pname_n21, GFP_KERNEL);
+	ppname_n21 = __of_prop_dup(&pname_n21);
 	unittest(ppname_n21, "testcase setup failure\n");
 
-	ppupdate = __of_prop_dup(&pupdate, GFP_KERNEL);
+	ppupdate = __of_prop_dup(&pupdate);
 	unittest(ppupdate, "testcase setup failure\n");
 
 	parent = nchangeset;
@@ -3333,7 +3333,7 @@ static __init void of_unittest_overlay_high_level(void)
 		struct property *new_prop;
 		for_each_property_of_node(overlay_base_symbols, prop) {
 
-			new_prop = __of_prop_dup(prop, GFP_KERNEL);
+			new_prop = __of_prop_dup(prop);
 			if (!new_prop) {
 				unittest(0, "__of_prop_dup() of '%s' from overlay_base node __symbols__",
 					 prop->name);
-- 
2.36.1

Re: [PATCH v3 2/5] of: remove __of_node_dup() allocflags parameter

[Bjorn Helgaas]

On Mon, Jun 20, 2022 at 12:41:20PM +0200, Clément Léger wrote:
> The alloclags are always set to GFP_KERNEL so remove this specific flag.
> Moreover, this function is going to be based on one that does not
> provides passing gfp flags, so be prepared for this.

s/alloclags/allocflags/

s/provides passing/supports passing/

[PATCH v3 3/5] 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.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/of/dynamic.c    | 83 ++++++++++++++++++++++++-----------------
 drivers/of/of_private.h | 19 +++++++++-
 drivers/of/unittest.c   |  8 +---
 include/linux/of.h      | 14 +++++++
 4 files changed, 83 insertions(+), 41 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 5560e501aedf..9e29000571d1 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,47 +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
  *
- * 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)
+
+struct property *of_property_alloc(const char *name, const void *value,
+				   size_t len)
 {
-	struct property *new;
+	struct property *prop;
 
-	new = kzalloc(sizeof(*new), GFP_KERNEL);
-	if (!new)
+	prop = kzalloc(sizeof(*prop) + len, GFP_KERNEL);
+	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, GFP_KERNEL);
-	new->value = kmemdup(prop->value, prop->length, GFP_KERNEL);
-	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, GFP_KERNEL);
+	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.
@@ -446,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 e319d8c2bf8b..7bed5a4a8033 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -115,7 +115,24 @@ 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);
+
+/**
+ * __of_prop_dup - Copy a property dynamically.
+ * @prop:	Property to copy
+ *
+ * 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)
+{
+	return of_property_alloc(prop->name, prop->value, prop->length);
+}
+
 struct device_node *__of_node_dup(const struct device_node *np,
 				  const char *full_name);
 
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 23ccb74ef408..789ed9f46e3c 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -774,15 +774,11 @@ static void __init of_unittest_property_copy(void)
 
 	new = __of_prop_dup(&p1);
 	unittest(new && propcmp(&p1, new), "empty property didn't copy correctly\n");
-	kfree(new->value);
-	kfree(new->name);
-	kfree(new);
+	of_property_free(new);
 
 	new = __of_prop_dup(&p2);
 	unittest(new && propcmp(&p2, new), "non-empty property didn't copy correctly\n");
-	kfree(new->value);
-	kfree(new->name);
-	kfree(new);
+	of_property_free(new);
 #endif
 }
 
diff --git a/include/linux/of.h b/include/linux/of.h
index d74fd82a6963..157248a77c4e 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);
+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)
+{
+	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.1

[PATCH v3 4/5] 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 | 53 ++++++++++++++++++++++++++++++++++----------
 include/linux/of.h   |  5 +++++
 2 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 9e29000571d1..422dfaa5aaa5 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,42 @@ 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
+ *
+ * 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)
+{
+	struct device_node *node;
+	int name_len = 0;
+
+	if (name)
+		name_len = strlen(name) + 1;
+
+	node = kzalloc(sizeof(*node) + name_len, GFP_KERNEL);
+	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,24 +480,15 @@ 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);
 	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) {
 		struct property *pp, *new_pp;
 		for_each_property_of_node(np, pp) {
-			new_pp = __of_prop_dup(pp, GFP_KERNEL);
+			new_pp = __of_prop_dup(pp);
 			if (!new_pp)
 				goto err_prop;
 			if (__of_add_property(node, new_pp)) {
diff --git a/include/linux/of.h b/include/linux/of.h
index 157248a77c4e..650b1ac0075b 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);
 struct property *of_property_alloc(const char *name, const void *value,
 				   size_t len);
 void of_property_free(const struct property *prop);
@@ -1512,6 +1513,10 @@ 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)
+{
+	return NULL;
+}
 
 static inline
 struct property *of_property_alloc(const char *name, const void *value,
-- 
2.36.1

[PATCH v3 5/5] 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. In order to obtain something cleaner
and allow using only of_node_put() instead of manual property deletion,
a rework of the usage of node in reconfig.c has been done.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 arch/powerpc/platforms/pseries/dlpar.c        |  62 +--------
 .../platforms/pseries/hotplug-memory.c        |  21 +--
 arch/powerpc/platforms/pseries/reconfig.c     | 123 ++++++------------
 3 files changed, 50 insertions(+), 156 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 498d6efcb5ae..c17e7b6f1dd2 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -38,75 +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);
 }
 
 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;
-}
-
-static void dlpar_free_one_cc_node(struct device_node *dn)
-{
-	struct property *prop;
-
-	while (dn->properties) {
-		prop = dn->properties;
-		dn->properties = prop->next;
-		dlpar_free_cc_property(prop);
-	}
-
-	kfree(dn->full_name);
-	kfree(dn);
+	return of_node_alloc(name);
 }
 
 void dlpar_free_cc_nodes(struct device_node *dn)
@@ -117,7 +67,7 @@ void dlpar_free_cc_nodes(struct device_node *dn)
 	if (dn->sibling)
 		dlpar_free_cc_nodes(dn->sibling);
 
-	dlpar_free_one_cc_node(dn);
+	of_node_put(dn);
 }
 
 #define COMPLETE	0
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 2e3a317722a8..e583814414a1 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);
 	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..8704c541de3c 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -19,46 +19,29 @@
 
 #include "of_helpers.h"
 
-static int pSeries_reconfig_add_node(const char *path, struct property *proplist)
+static struct device_node *pSeries_reconfig_add_node(const char *path)
 {
-	struct device_node *np;
-	int err = -ENOMEM;
-
-	np = kzalloc(sizeof(*np), 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);
+	struct device_node *np, *parent;
 
-	np->parent = pseries_of_derive_parent(path);
-	if (IS_ERR(np->parent)) {
-		err = PTR_ERR(np->parent);
-		goto out_err;
+	np = of_find_node_by_path(path)
+	if (np) {
+		of_node_put(np);
+		return ERR_PTR(-EINVAL);
 	}
 
-	err = of_attach_node(np);
-	if (err) {
-		printk(KERN_ERR "Failed to add device node %s\n", path);
-		goto out_err;
-	}
+	parent = pseries_of_derive_parent(path);
+	if (IS_ERR(parent))
+		return parent;
 
-	of_node_put(np->parent);
+	np = of_node_alloc(kbasename(path));
+	if (!np) {
+		of_node_put(parent);
+		return ERR_PTR(-ENOMEM);
+	}
 
-	return 0;
+	np->parent = parent;
 
-out_err:
-	if (np) {
-		of_node_put(np->parent);
-		kfree(np->full_name);
-		kfree(np);
-	}
-	return err;
+	return np;
 }
 
 static int pSeries_reconfig_remove_node(struct device_node *np)
@@ -80,24 +63,6 @@ static int pSeries_reconfig_remove_node(struct device_node *np)
 	return 0;
 }
 
-/*
- * /proc/powerpc/ofdt - yucky binary interface for adding and removing
- * OF device nodes.  Should be deprecated as soon as we get an
- * in-kernel wrapper for the RTAS ibm,configure-connector call.
- */
-
-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);
-	}
-
-}
-
 /**
  * parse_next_property - process the next property from raw input buffer
  * @buf: input buffer, must be nul-terminated
@@ -148,7 +113,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 +123,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 */
@@ -167,27 +133,15 @@ 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);
+	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;
+	prop->next = last;
+
+	return prop;
 }
 
 static int do_add_node(char *buf, size_t bufsize)
@@ -203,13 +157,13 @@ static int do_add_node(char *buf, size_t bufsize)
 	buf = strchr(buf, ' ');
 	if (!buf)
 		return -EINVAL;
+
 	*buf = '\0';
 	buf++;
 
-	if ((np = of_find_node_by_path(path))) {
-		of_node_put(np);
-		return -EINVAL;
-	}
+	np = pSeries_reconfig_add_node(path);
+	if (IS_ERR(np))
+		return PTR_ERR(np);
 
 	/* rv = build_prop_list(tmp, bufsize - (tmp - buf), &proplist); */
 	while (buf < end &&
@@ -217,22 +171,29 @@ static int do_add_node(char *buf, size_t bufsize)
 		struct property *last = prop;
 
 		prop = new_property(name, length, value, last);
-		if (!prop) {
-			rv = -ENOMEM;
-			prop = last;
+		if (!prop)
 			goto out;
-		}
+
+		np->properties = prop;
 	}
 	if (!buf) {
 		rv = -EINVAL;
 		goto out;
 	}
 
-	rv = pSeries_reconfig_add_node(path, prop);
+	rv = of_attach_node(np);
+	if (rv) {
+		pr_err("Failed to attach node %pOF\n", np);
+		goto out;
+	}
 
+	of_node_put(np->parent);
+
+	return 0;
 out:
-	if (rv)
-		release_prop_list(prop);
+	of_node_put(np->parent);
+	of_node_put(np);
+
 	return rv;
 }
 
-- 
2.36.1

Re: [PATCH v3 5/5] powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

[Clément Léger]

Le Tue, 28 Jun 2022 17:00:45 +0000,
Christophe Leroy <christophe.leroy@csgroup.eu> a écrit :

> > -static int pSeries_reconfig_add_node(const char *path, struct property *proplist)
> > +static struct device_node *pSeries_reconfig_add_node(const char *path)
> >   {
> > -	struct device_node *np;
> > -	int err = -ENOMEM;
> > -
> > -	np = kzalloc(sizeof(*np), 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);
> > +	struct device_node *np, *parent;
> >   
> > -	np->parent = pseries_of_derive_parent(path);
> > -	if (IS_ERR(np->parent)) {
> > -		err = PTR_ERR(np->parent);
> > -		goto out_err;
> > +	np = of_find_node_by_path(path)  
> 
> Missing ;
> 
> Did you test build ?

I generally test build for powerpc but it seems like I forgot to rebuild
my powerpc kernel build after this modification. It seems like only
this error did not pass through the test build. I'll try to be more
careful next time.

Sorry for that.

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

Re: [PATCH v3 0/5] of: add of_property_alloc/free() and of_node_alloc()

[Frank Rowand]

Sorry for the lack of response, it's been a busy week.  I will get to this
soon.

-Frank

On 6/20/22 06:41, Clément Léger wrote:
> 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. This code has been more than simply replaced to allow using
> of_node_put() rather than a manual deletion of the properties.
> Unfortunately, as I don't own a powerpc platform, it would need to be
> tested.
> 
> ---
> 
> Changes in V3:
> - Remove gfpflag attribute from of_node_alloc() and of_property_alloc().
> - Removed allocflags from __of_node_dup().
> - Rework powerpc code to only use of_node_put().
> - Fix properties free using of_node_property in OF unittests.
> 
> 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 (5):
>   of: constify of_property_check_flags() prop argument
>   of: remove __of_node_dup() allocflags parameter
>   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        |  62 +-------
>  .../platforms/pseries/hotplug-memory.c        |  21 +--
>  arch/powerpc/platforms/pseries/reconfig.c     | 123 ++++++----------
>  drivers/of/dynamic.c                          | 137 ++++++++++++------
>  drivers/of/of_private.h                       |  19 ++-
>  drivers/of/overlay.c                          |   2 +-
>  drivers/of/unittest.c                         |  24 ++-
>  include/linux/of.h                            |  24 ++-
>  8 files changed, 191 insertions(+), 221 deletions(-)
>