[PATCH v2 0/2] of: remove kbasename() from core

[PATCH v2 0/2] of: remove kbasename() from core

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

(One line description of patch 0/2 is now misleading, but leaving
intact to make it easier to find v1.  One line description of other
patches updated as they will be part of the git commit record.)

Struct device_node full_name no longer includes the full path
name.

Fix some broken overlay code that was not updated to reflect this.

Clean up the unittest changeset test that calls into this
overlay code.

Version 1 of this patch removed kbasename() for other parts of
core devicetree code.  But Geert kindly pointed out that a
devicetree created from Open Firmware (instead from an FDT)
could still contain a full path in the struct device_node
full_path field.  This version (v2) of the patch leaves
kbasename() in place where needed.

Changes since v1:
  - update patch 2/2 one-line description and full description
  - no longer remove kbasename from resolver.c
  - add_changeset_node(): add back kbasename() when comparing nodes
    in the livetree against nodes in the overlay
  - add_changeset_node(): add header comments to document assumptions
    and behavior, and to explain why kbasename() is used
  
Frank Rowand (2):
  of: unittest: clean up changeset test
  of: overlay: do not include path in full_name of added nodes

 drivers/of/dynamic.c    | 21 ++++++++++-----------
 drivers/of/of_private.h |  3 ++-
 drivers/of/overlay.c    | 18 +++++++++++++++---
 drivers/of/unittest.c   | 48 +++++++++++++++++++++++++++++++++++++++---------
 4 files changed, 66 insertions(+), 24 deletions(-)

-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH v2 1/2] of: unittest: clean up changeset test

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

In preparation for fixing __of_node_dup(), clean up the unittest
function that calls it.

Devicetree nodes created from a flattened device tree have a name
property.  Follow this convention for nodes added by a changeset.

For node added by changeset, remove incorrect initialization of
child node pointer.

Add an additional node pointer 'changeset' to more naturally reflect
where in the tree the changeset is added.

Make changeset add property error messages unique.

Add whitespace to break apart logic blocks.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

no changes from v1

Checkpatch warnings are "line over 80 characters" and are consistent
with the style of the surrounding code.


 drivers/of/unittest.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 7a9abaae874d..490bbee0cf87 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -562,42 +562,72 @@ static void __init of_unittest_property_copy(void)
 static void __init of_unittest_changeset(void)
 {
 #ifdef CONFIG_OF_DYNAMIC
-	struct property *ppadd, padd = { .name = "prop-add", .length = 0, .value = "" };
+	struct property *ppadd, padd = { .name = "prop-add", .length = 1, .value = "" };
+	struct property *ppname_n1,  pname_n1  = { .name = "name", .length = 3, .value = "n1"  };
+	struct property *ppname_n2,  pname_n2  = { .name = "name", .length = 3, .value = "n2"  };
+	struct property *ppname_n21, pname_n21 = { .name = "name", .length = 3, .value = "n21" };
 	struct property *ppupdate, pupdate = { .name = "prop-update", .length = 5, .value = "abcd" };
 	struct property *ppremove;
-	struct device_node *n1, *n2, *n21, *nremove, *parent, *np;
+	struct device_node *n1, *n2, *n21, *nchangeset, *nremove, *parent, *np;
 	struct of_changeset chgset;
 
 	n1 = __of_node_dup(NULL, "/testcase-data/changeset/n1");
 	unittest(n1, "testcase setup failure\n");
+
 	n2 = __of_node_dup(NULL, "/testcase-data/changeset/n2");
 	unittest(n2, "testcase setup failure\n");
+
 	n21 = __of_node_dup(NULL, "%s/%s", "/testcase-data/changeset/n2", "n21");
 	unittest(n21, "testcase setup failure %p\n", n21);
-	nremove = of_find_node_by_path("/testcase-data/changeset/node-remove");
+
+	nchangeset = of_find_node_by_path("/testcase-data/changeset");
+	nremove = of_get_child_by_name(nchangeset, "node-remove");
 	unittest(nremove, "testcase setup failure\n");
+
 	ppadd = __of_prop_dup(&padd, GFP_KERNEL);
 	unittest(ppadd, "testcase setup failure\n");
+
+	ppname_n1  = __of_prop_dup(&pname_n1, GFP_KERNEL);
+	unittest(ppname_n1, "testcase setup failure\n");
+
+	ppname_n2  = __of_prop_dup(&pname_n2, GFP_KERNEL);
+	unittest(ppname_n2, "testcase setup failure\n");
+
+	ppname_n21 = __of_prop_dup(&pname_n21, GFP_KERNEL);
+	unittest(ppname_n21, "testcase setup failure\n");
+
 	ppupdate = __of_prop_dup(&pupdate, GFP_KERNEL);
 	unittest(ppupdate, "testcase setup failure\n");
-	parent = nremove->parent;
+
+	parent = nchangeset;
 	n1->parent = parent;
 	n2->parent = parent;
 	n21->parent = n2;
-	n2->child = n21;
+
 	ppremove = of_find_property(parent, "prop-remove", NULL);
 	unittest(ppremove, "failed to find removal prop");
 
 	of_changeset_init(&chgset);
+
 	unittest(!of_changeset_attach_node(&chgset, n1), "fail attach n1\n");
+	unittest(!of_changeset_add_property(&chgset, n1, ppname_n1), "fail add prop name\n");
+
 	unittest(!of_changeset_attach_node(&chgset, n2), "fail attach n2\n");
+	unittest(!of_changeset_add_property(&chgset, n2, ppname_n2), "fail add prop name\n");
+
 	unittest(!of_changeset_detach_node(&chgset, nremove), "fail remove node\n");
+	unittest(!of_changeset_add_property(&chgset, n21, ppname_n21), "fail add prop name\n");
+
 	unittest(!of_changeset_attach_node(&chgset, n21), "fail attach n21\n");
-	unittest(!of_changeset_add_property(&chgset, parent, ppadd), "fail add prop\n");
+
+	unittest(!of_changeset_add_property(&chgset, parent, ppadd), "fail add prop prop-add\n");
 	unittest(!of_changeset_update_property(&chgset, parent, ppupdate), "fail update prop\n");
 	unittest(!of_changeset_remove_property(&chgset, parent, ppremove), "fail remove prop\n");
+
 	unittest(!of_changeset_apply(&chgset), "apply failed\n");
 
+	of_node_put(nchangeset);
+
 	/* Make sure node names are constructed correctly */
 	unittest((np = of_find_node_by_path("/testcase-data/changeset/n2/n21")),
 		 "'%pOF' not added\n", n21);
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH v2 1/2] of: unittest: clean up changeset test

[Rob Herring]

On Mon, Feb 26, 2018 at 02:01:22PM -0800, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> In preparation for fixing __of_node_dup(), clean up the unittest
> function that calls it.
> 
> Devicetree nodes created from a flattened device tree have a name
> property.  Follow this convention for nodes added by a changeset.

I'd like to get rid of that property as it should be redundant, but 
that's somewhat orthogonal to this.

It also shouldn't be up to the caller to have to do this either, but 
that's already broken in the current API.

So I've applied these.

> 
> For node added by changeset, remove incorrect initialization of
> child node pointer.
> 
> Add an additional node pointer 'changeset' to more naturally reflect
> where in the tree the changeset is added.
> 
> Make changeset add property error messages unique.
> 
> Add whitespace to break apart logic blocks.
> 
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---

[PATCH v2 2/2] of: overlay: do not include path in full_name of added nodes

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

Struct device_node full_name no longer includes the full path name
when the devicetree is created from a flattened device tree (FDT).
The overlay node creation code was not modified to reflect this
change.  Fix the node full_name generated by overlay code to contain
only the basename.

Unittests call an overlay internal function to create new nodes.
Fix up these calls to provide basename only instead of the full
path.

Fixes: a7e4cfb0a7ca ("of/fdt: only store the device node basename
in full_name")

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

changes since version 1:
  - update patch one-line description and full description
  - no longer remove kbasename from resolver.c
  - add_changeset_node(): add back kbasename() when comparing nodes
    in the livetree against nodes in the overlay
  - add_changeset_node(): add header comments to document assumptions
    and behavior, and to explain why kbasename() is used

 drivers/of/dynamic.c    | 21 ++++++++++-----------
 drivers/of/of_private.h |  3 ++-
 drivers/of/overlay.c    | 18 +++++++++++++++---
 drivers/of/unittest.c   |  6 +++---
 4 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 7bb33d22b4e2..f4f8ed9b5454 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -383,25 +383,24 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
 
 /**
  * __of_node_dup() - Duplicate or create an empty device node dynamically.
- * @fmt: Format string (plus vargs) for new full name of the device node
+ * @np:		if not NULL, contains properties to be duplicated in new node
+ * @full_name:	string value to be duplicated into new node's full_name field
  *
- * Create an device tree node, either by duplicating an empty node or by allocating
- * an empty one suitable for further modification.  The node data are
- * dynamically allocated and all the node flags have the OF_DYNAMIC &
- * OF_DETACHED bits set. Returns the newly allocated node or NULL on out of
- * memory error.
+ * Create a device tree node, optionally duplicating the properties of
+ * another node.  The node data are dynamically allocated and all the node
+ * flags have the OF_DYNAMIC & OF_DETACHED bits set.
+ *
+ * Returns the newly allocated node or NULL on out of memory error.
  */
-struct device_node *__of_node_dup(const struct device_node *np, const char *fmt, ...)
+struct device_node *__of_node_dup(const struct device_node *np,
+				  const char *full_name)
 {
-	va_list vargs;
 	struct device_node *node;
 
 	node = kzalloc(sizeof(*node), GFP_KERNEL);
 	if (!node)
 		return NULL;
-	va_start(vargs, fmt);
-	node->full_name = kvasprintf(GFP_KERNEL, fmt, vargs);
-	va_end(vargs);
+	node->full_name = kstrdup(full_name, GFP_KERNEL);
 	if (!node->full_name) {
 		kfree(node);
 		return NULL;
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 0c609e7d0334..26bb31beb03e 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -104,7 +104,8 @@ extern void *__unflatten_device_tree(const void *blob,
  * own the devtree lock or work on detached trees only.
  */
 struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
-__printf(2, 3) struct device_node *__of_node_dup(const struct device_node *np, const char *fmt, ...);
+struct device_node *__of_node_dup(const struct device_node *np,
+				  const char *full_name);
 
 struct device_node *__of_find_node_by_path(struct device_node *parent,
 						const char *path);
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 3397d7642958..c39df1c663be 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -307,7 +307,20 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
  * If @node has child nodes, add the children recursively via
  * build_changeset_next_level().
  *
- * NOTE: Multiple mods of created nodes not supported.
+ * NOTE_1: A live devicetree created from a flattened device tree (FDT) will
+ *       not contain the full path in node->full_name.  Thus an overlay
+ *       created from an FDT also will not contain the full path in
+ *       node->full_name.  However, a live devicetree created from Open
+ *       Firmware may have the full path in node->full_name.
+ *
+ *       add_changeset_node() follows the FDT convention and does not include
+ *       the full path in node->full_name.  Even though it expects the overlay
+ *       to not contain the full path, it uses kbasename() to remove the
+ *       full path should it exist.  It also uses kbasename() in comparisons
+ *       to nodes in the live devicetree so that it can apply an overlay to
+ *       a live devicetree created from Open Firmware.
+ *
+ * NOTE_2: Multiple mods of created nodes not supported.
  *       If more than one fragment contains a node that does not already exist
  *       in the live tree, then for each fragment of_changeset_attach_node()
  *       will add a changeset entry to add the node.  When the changeset is
@@ -334,8 +347,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
 			break;
 
 	if (!tchild) {
-		tchild = __of_node_dup(node, "%pOF/%s",
-				       target_node, node_kbasename);
+		tchild = __of_node_dup(node, node_kbasename);
 		if (!tchild)
 			return -ENOMEM;
 
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 490bbee0cf87..acf233c34ef7 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -571,13 +571,13 @@ static void __init of_unittest_changeset(void)
 	struct device_node *n1, *n2, *n21, *nchangeset, *nremove, *parent, *np;
 	struct of_changeset chgset;
 
-	n1 = __of_node_dup(NULL, "/testcase-data/changeset/n1");
+	n1 = __of_node_dup(NULL, "n1");
 	unittest(n1, "testcase setup failure\n");
 
-	n2 = __of_node_dup(NULL, "/testcase-data/changeset/n2");
+	n2 = __of_node_dup(NULL, "n2");
 	unittest(n2, "testcase setup failure\n");
 
-	n21 = __of_node_dup(NULL, "%s/%s", "/testcase-data/changeset/n2", "n21");
+	n21 = __of_node_dup(NULL, "n21");
 	unittest(n21, "testcase setup failure %p\n", n21);
 
 	nchangeset = of_find_node_by_path("/testcase-data/changeset");
-- 
Frank Rowand <frank.rowand@sony.com>