[PATCH 00/12] of: overlay: clean up device tree overlay code

[PATCH 00/12] of: overlay: clean up device tree overlay code

[frowand.list]

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

I have found the device tree overlay code to be difficult to read and
maintain.  This patch series attempts to improve that situation.

The cleanup includes some changes visible to users of overlays.  The
only in kernel user of overlays is fixed up for those changes.  The
in kernel user is:

   drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c

Following the cleanup patches are a set of patches to fix various
issues.

The first five patches are intended to not make any functional
changes, and are segrated to ease review.

Frank Rowand (12):
  of: overlay.c: Remove comments that state the obvious, to reduce
    clutter
  of: overlay.c: Convert comparisons to zero or NULL to logical
    expressions
  of: overlay: rename identifiers to more reflect what they do
  of: overlay: rename identifiers in dup_and_fixup_symbol_prop()
  of: overlay: minor restructuring
  of: overlay: detect cases where device tree may become corrupt
  of: overlay: expand check of whether overlay changeset can be removed
  of: overlay: loosen overly strict phandle clash check
  of: overlay: avoid race condition between applying multiple overlays
  of: overlay: simplify applying symbols from an overlay
  of: overlay: remove a dependency on device node full_name
  of: overlay: remove unneeded check for NULL kbasename()

 Documentation/devicetree/overlay-notes.txt   |   12 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |   15 +-
 drivers/of/base.c                            |    2 +-
 drivers/of/dynamic.c                         |  137 +++-
 drivers/of/of_private.h                      |   10 +-
 drivers/of/overlay.c                         | 1024 ++++++++++++++++----------
 drivers/of/unittest.c                        |   80 +-
 include/linux/of.h                           |   33 +-
 8 files changed, 871 insertions(+), 442 deletions(-)

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

[PATCH 01/12] of: overlay.c: Remove comments that state the obvious, to reduce clutter

[frowand.list]

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

Follows recommendations in Documentation/process/coding-style.rst,
section 8, Commenting.

Some in function comments are promoted to function header comments.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 53 ++++++++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 8ecfee31ab6d..26f63f10f4b0 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -143,7 +143,6 @@ static struct property *dup_and_fixup_symbol_prop(struct of_overlay *ov,
 	strcpy(new->value, target_path);
 	strcpy(new->value + target_path_len, label_path);
 
-	/* mark the property as dynamic */
 	of_property_set_flag(new, OF_DYNAMIC);
 
 	return new;
@@ -157,23 +156,24 @@ static struct property *dup_and_fixup_symbol_prop(struct of_overlay *ov,
 
 }
 
+/*
+ * Some special properties are not updated (no error returned).
+ * Update of property in symbols node is not allowed.
+ */
 static int of_overlay_apply_single_property(struct of_overlay *ov,
 		struct device_node *target, struct property *prop,
 		bool is_symbols_node)
 {
 	struct property *propn = NULL, *tprop;
 
-	/* NOTE: Multiple changes of single properties not supported */
 	tprop = of_find_property(target, prop->name, NULL);
 
-	/* special properties are not meant to be updated (silent NOP) */
 	if (of_prop_cmp(prop->name, "name") == 0 ||
 	    of_prop_cmp(prop->name, "phandle") == 0 ||
 	    of_prop_cmp(prop->name, "linux,phandle") == 0)
 		return 0;
 
 	if (is_symbols_node) {
-		/* changing a property in __symbols__ node not allowed */
 		if (tprop)
 			return -EINVAL;
 		propn = dup_and_fixup_symbol_prop(ov, prop);
@@ -184,14 +184,19 @@ static int of_overlay_apply_single_property(struct of_overlay *ov,
 	if (propn == NULL)
 		return -ENOMEM;
 
-	/* not found? add */
 	if (tprop == NULL)
 		return of_changeset_add_property(&ov->cset, target, propn);
 
-	/* found? update */
 	return of_changeset_update_property(&ov->cset, target, propn);
 }
 
+/*
+ * NOTE: Multiple mods of created nodes not supported.
+ *
+ * Return
+ *  -ENOMEM if memory allocation fails
+ *  -EINVAL if existing node has a phandle and overlay node has a phandle
+ */
 static int of_overlay_apply_single_device_node(struct of_overlay *ov,
 		struct device_node *target, struct device_node *child)
 {
@@ -203,13 +208,11 @@ static int of_overlay_apply_single_device_node(struct of_overlay *ov,
 	if (cname == NULL)
 		return -ENOMEM;
 
-	/* NOTE: Multiple mods of created nodes not supported */
 	for_each_child_of_node(target, tchild)
 		if (!of_node_cmp(cname, kbasename(tchild->full_name)))
 			break;
 
 	if (tchild != NULL) {
-		/* new overlay phandle value conflicts with existing value */
 		if (child->phandle)
 			return -EINVAL;
 
@@ -217,12 +220,10 @@ static int of_overlay_apply_single_device_node(struct of_overlay *ov,
 		ret = of_overlay_apply_one(ov, tchild, child, 0);
 		of_node_put(tchild);
 	} else {
-		/* create empty tree as a target */
 		tchild = __of_node_dup(child, "%pOF/%s", target, cname);
 		if (!tchild)
 			return -ENOMEM;
 
-		/* point to parent */
 		tchild->parent = target;
 
 		ret = of_changeset_attach_node(&ov->cset, tchild);
@@ -243,6 +244,8 @@ static int of_overlay_apply_single_device_node(struct of_overlay *ov,
  * Note that the in case of an error the target node is left
  * in a inconsistent state. Error recovery should be performed
  * by using the changeset.
+ *
+ * Do not allow symbols node to have any children.
  */
 static int of_overlay_apply_one(struct of_overlay *ov,
 		struct device_node *target, const struct device_node *overlay,
@@ -262,7 +265,6 @@ static int of_overlay_apply_one(struct of_overlay *ov,
 		}
 	}
 
-	/* do not allow symbols node to have any children */
 	if (is_symbols_node)
 		return 0;
 
@@ -292,7 +294,6 @@ static int of_overlay_apply(struct of_overlay *ov)
 {
 	int i, err;
 
-	/* first we apply the overlays atomically */
 	for (i = 0; i < ov->count; i++) {
 		struct of_overlay_info *ovinfo = &ov->ovinfo_tab[i];
 
@@ -309,10 +310,10 @@ static int of_overlay_apply(struct of_overlay *ov)
 
 /*
  * Find the target node using a number of different strategies
- * in order of preference
+ * in order of preference:
  *
- * "target" property containing the phandle of the target
- * "target-path" property containing the path of the target
+ * 1) "target" property containing the phandle of the target
+ * 2) "target-path" property containing the path of the target
  */
 static struct device_node *find_target_node(struct device_node *info_node)
 {
@@ -320,12 +321,10 @@ static struct device_node *find_target_node(struct device_node *info_node)
 	u32 val;
 	int ret;
 
-	/* first try to go by using the target as a phandle */
 	ret = of_property_read_u32(info_node, "target", &val);
 	if (ret == 0)
 		return of_find_node_by_phandle(val);
 
-	/* now try to locate by path */
 	ret = of_property_read_string(info_node, "target-path", &path);
 	if (ret == 0)
 		return of_find_node_by_path(path);
@@ -390,7 +389,6 @@ static int of_build_overlay_info(struct of_overlay *ov,
 	struct of_overlay_info *ovinfo;
 	int cnt, err;
 
-	/* worst case; every child is a node */
 	cnt = 0;
 	for_each_child_of_node(tree, node)
 		cnt++;
@@ -423,7 +421,6 @@ static int of_build_overlay_info(struct of_overlay *ov,
 		cnt++;
 	}
 
-	/* if nothing filled, return error */
 	if (cnt == 0) {
 		kfree(ovinfo);
 		return -ENODEV;
@@ -479,7 +476,6 @@ int of_overlay_create(struct device_node *tree)
 	struct of_overlay *ov;
 	int err, id;
 
-	/* allocate the overlay structure */
 	ov = kzalloc(sizeof(*ov), GFP_KERNEL);
 	if (ov == NULL)
 		return -ENOMEM;
@@ -498,7 +494,6 @@ int of_overlay_create(struct device_node *tree)
 	}
 	ov->id = id;
 
-	/* build the overlay info structures */
 	err = of_build_overlay_info(ov, tree);
 	if (err) {
 		pr_err("of_build_overlay_info() failed for tree@%pOF\n",
@@ -513,18 +508,15 @@ int of_overlay_create(struct device_node *tree)
 		goto err_free_idr;
 	}
 
-	/* apply the overlay */
 	err = of_overlay_apply(ov);
 	if (err)
 		goto err_abort_trans;
 
-	/* apply the changeset */
 	err = __of_changeset_apply(&ov->cset);
 	if (err)
 		goto err_revert_overlay;
 
 
-	/* add to the tail of the overlay list */
 	list_add_tail(&ov->node, &ov_list);
 
 	of_overlay_notify(ov, OF_OVERLAY_POST_APPLY);
@@ -547,13 +539,15 @@ int of_overlay_create(struct device_node *tree)
 }
 EXPORT_SYMBOL_GPL(of_overlay_create);
 
-/* check whether the given node, lies under the given tree */
+/*
+ * check whether the given node, lies under the given tree
+ * return 1 if under tree, else 0
+ */
 static int overlay_subtree_check(struct device_node *tree,
 		struct device_node *dn)
 {
 	struct device_node *child;
 
-	/* match? */
 	if (tree == dn)
 		return 1;
 
@@ -567,7 +561,10 @@ static int overlay_subtree_check(struct device_node *tree,
 	return 0;
 }
 
-/* check whether this overlay is the topmost */
+/*
+ * check whether this overlay is the topmost
+ * return 1 if topmost, else 0
+ */
 static int overlay_is_topmost(struct of_overlay *ov, struct device_node *dn)
 {
 	struct of_overlay *ovt;
@@ -588,7 +585,6 @@ static int overlay_is_topmost(struct of_overlay *ov, struct device_node *dn)
 		}
 	}
 
-	/* overlay is topmost */
 	return 1;
 }
 
@@ -638,7 +634,6 @@ int of_overlay_destroy(int id)
 		goto out;
 	}
 
-	/* check whether the overlay is safe to remove */
 	if (!overlay_removal_is_ok(ov)) {
 		err = -EBUSY;
 		goto out;
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 02/12] of: overlay.c: Convert comparisons to zero or NULL to logical expressions

[frowand.list]

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

Use normal shorthand for comparing a variable to zero.
For variable "XXX":
   convert (XXX == 0) to (!XXX)
   convert (XXX != 0) to (XXX)

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 26f63f10f4b0..8e0c7eb4858c 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -168,9 +168,9 @@ static int of_overlay_apply_single_property(struct of_overlay *ov,
 
 	tprop = of_find_property(target, prop->name, NULL);
 
-	if (of_prop_cmp(prop->name, "name") == 0 ||
-	    of_prop_cmp(prop->name, "phandle") == 0 ||
-	    of_prop_cmp(prop->name, "linux,phandle") == 0)
+	if (!of_prop_cmp(prop->name, "name") ||
+	    !of_prop_cmp(prop->name, "phandle") ||
+	    !of_prop_cmp(prop->name, "linux,phandle"))
 		return 0;
 
 	if (is_symbols_node) {
@@ -181,10 +181,10 @@ static int of_overlay_apply_single_property(struct of_overlay *ov,
 		propn = __of_prop_dup(prop, GFP_KERNEL);
 	}
 
-	if (propn == NULL)
+	if (!propn)
 		return -ENOMEM;
 
-	if (tprop == NULL)
+	if (!tprop)
 		return of_changeset_add_property(&ov->cset, target, propn);
 
 	return of_changeset_update_property(&ov->cset, target, propn);
@@ -205,14 +205,14 @@ static int of_overlay_apply_single_device_node(struct of_overlay *ov,
 	int ret = 0;
 
 	cname = kbasename(child->full_name);
-	if (cname == NULL)
+	if (!cname)
 		return -ENOMEM;
 
 	for_each_child_of_node(target, tchild)
 		if (!of_node_cmp(cname, kbasename(tchild->full_name)))
 			break;
 
-	if (tchild != NULL) {
+	if (tchild) {
 		if (child->phandle)
 			return -EINVAL;
 
@@ -270,7 +270,7 @@ static int of_overlay_apply_one(struct of_overlay *ov,
 
 	for_each_child_of_node(overlay, child) {
 		ret = of_overlay_apply_single_device_node(ov, target, child);
-		if (ret != 0) {
+		if (ret) {
 			pr_err("Failed to apply single node @%pOF/%s\n",
 			       target, child->name);
 			of_node_put(child);
@@ -299,7 +299,7 @@ static int of_overlay_apply(struct of_overlay *ov)
 
 		err = of_overlay_apply_one(ov, ovinfo->target, ovinfo->overlay,
 					   ovinfo->is_symbols_node);
-		if (err != 0) {
+		if (err) {
 			pr_err("apply failed '%pOF'\n", ovinfo->target);
 			return err;
 		}
@@ -322,11 +322,11 @@ static struct device_node *find_target_node(struct device_node *info_node)
 	int ret;
 
 	ret = of_property_read_u32(info_node, "target", &val);
-	if (ret == 0)
+	if (!ret)
 		return of_find_node_by_phandle(val);
 
 	ret = of_property_read_string(info_node, "target-path", &path);
-	if (ret == 0)
+	if (!ret)
 		return of_find_node_by_path(path);
 
 	pr_err("Failed to find target for node %p (%s)\n",
@@ -353,11 +353,11 @@ static int of_fill_overlay_info(struct of_overlay *ov,
 		struct device_node *info_node, struct of_overlay_info *ovinfo)
 {
 	ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__");
-	if (ovinfo->overlay == NULL)
+	if (!ovinfo->overlay)
 		goto err_fail;
 
 	ovinfo->target = find_target_node(info_node);
-	if (ovinfo->target == NULL)
+	if (!ovinfo->target)
 		goto err_fail;
 
 	return 0;
@@ -397,13 +397,13 @@ static int of_build_overlay_info(struct of_overlay *ov,
 		cnt++;
 
 	ovinfo = kcalloc(cnt, sizeof(*ovinfo), GFP_KERNEL);
-	if (ovinfo == NULL)
+	if (!ovinfo)
 		return -ENOMEM;
 
 	cnt = 0;
 	for_each_child_of_node(tree, node) {
 		err = of_fill_overlay_info(ov, node, &ovinfo[cnt]);
-		if (err == 0)
+		if (!err)
 			cnt++;
 	}
 
@@ -421,7 +421,7 @@ static int of_build_overlay_info(struct of_overlay *ov,
 		cnt++;
 	}
 
-	if (cnt == 0) {
+	if (!cnt) {
 		kfree(ovinfo);
 		return -ENODEV;
 	}
@@ -477,7 +477,7 @@ int of_overlay_create(struct device_node *tree)
 	int err, id;
 
 	ov = kzalloc(sizeof(*ov), GFP_KERNEL);
-	if (ov == NULL)
+	if (!ov)
 		return -ENOMEM;
 	ov->id = -1;
 
@@ -628,7 +628,7 @@ int of_overlay_destroy(int id)
 	mutex_lock(&of_mutex);
 
 	ov = idr_find(&ov_idr, id);
-	if (ov == NULL) {
+	if (!ov) {
 		err = -ENODEV;
 		pr_err("destroy: Could not find overlay #%d\n", id);
 		goto out;
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 03/12] of: overlay: rename identifiers to more reflect what they do

[frowand.list]

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

This patch is aimed primarily at drivers/of/overlay.c, but those
changes also have a small impact in a few other files.

overlay.c is difficult to read and maintain.  Improve readability:
  - Rename functions, types and variables to better reflect what
    they do and to be consistent with names in other places,
    such as the device tree overlay FDT (flattened device tree),
    and make the algorithms more clear
  - Use the same names consistently throughout the file
  - Update comments for name changes
  - Fix incorrect comments

This patch is intended to not introduce any functional change.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 Documentation/devicetree/overlay-notes.txt   |  12 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |   5 +-
 drivers/of/dynamic.c                         |   2 +-
 drivers/of/overlay.c                         | 506 ++++++++++++++-------------
 drivers/of/unittest.c                        |  20 +-
 include/linux/of.h                           |  12 +-
 6 files changed, 294 insertions(+), 263 deletions(-)

diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
index eb7f2685fda1..c4aa0adf13ec 100644
--- a/Documentation/devicetree/overlay-notes.txt
+++ b/Documentation/devicetree/overlay-notes.txt
@@ -87,15 +87,15 @@ Overlay in-kernel API
 
 The API is quite easy to use.
 
-1. Call of_overlay_create() to create and apply an overlay. The return value
-is a cookie identifying this overlay.
+1. Call of_overlay_apply() to create and apply an overlay changeset. The return
+value is an error or a cookie identifying this overlay.
 
-2. Call of_overlay_destroy() to remove and cleanup the overlay previously
-created via the call to of_overlay_create(). Removal of an overlay that
-is stacked by another will not be permitted.
+2. Call of_overlay_remove() to remove and cleanup the overlay changeset
+previously created via the call to of_overlay_apply(). Removal of an overlay
+changeset that is stacked by another will not be permitted.
 
 Finally, if you need to remove all overlays in one-go, just call
-of_overlay_destroy_all() which will remove every single one in the correct
+of_overlay_remove_all() which will remove every single one in the correct
 order.
 
 Overlay DTS Format
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index 623a9140493c..5f5b7ba35f1d 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -247,9 +247,10 @@ static void __init tilcdc_convert_slave_node(void)
 
 	tilcdc_node_disable(slave);
 
-	ret = of_overlay_create(overlay);
+	ret = of_overlay_apply(overlay);
 	if (ret)
-		pr_err("%s: Creating overlay failed: %d\n", __func__, ret);
+		pr_err("%s: Applying overlay changeset failed: %d\n",
+			__func__, ret);
 	else
 		pr_info("%s: ti,tilcdc,slave node successfully converted\n",
 			__func__);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 301b6db2b48d..124510d56421 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -775,7 +775,7 @@ int of_changeset_revert(struct of_changeset *ocs)
 EXPORT_SYMBOL_GPL(of_changeset_revert);
 
 /**
- * of_changeset_action - Perform a changeset action
+ * of_changeset_action - Add an action to the tail of the changeset list
  *
  * @ocs:	changeset pointer
  * @action:	action to perform
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 8e0c7eb4858c..397ef10d1f26 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -25,67 +25,63 @@
 #include "of_private.h"
 
 /**
- * struct of_overlay_info - Holds a single overlay info
+ * struct fragment - info about fragment nodes in overlay expanded device tree
  * @target:	target of the overlay operation
- * @overlay:	pointer to the overlay contents node
- *
- * Holds a single overlay state, including all the overlay logs &
- * records.
+ * @overlay:	pointer to the __overlay__ node
  */
-struct of_overlay_info {
+struct fragment {
 	struct device_node *target;
 	struct device_node *overlay;
 	bool is_symbols_node;
 };
 
 /**
- * struct of_overlay - Holds a complete overlay transaction
- * @node:	List on which we are located
- * @count:	Count of ovinfo structures
- * @ovinfo_tab:	Overlay info table (count sized)
- * @cset:	Changeset to be used
- *
- * Holds a complete overlay transaction
+ * struct overlay_changeset
+ * @ovcs_list:	list on which we are located
+ * @count:	count of @fragments structures
+ * @fragments:	info about fragment nodes in overlay expanded device tree
+ * @cset:	changeset to apply fragments to live device tree
  */
-struct of_overlay {
+struct overlay_changeset {
 	int id;
-	struct list_head node;
+	struct list_head ovcs_list;
 	int count;
-	struct of_overlay_info *ovinfo_tab;
+	struct fragment *fragments;
 	struct of_changeset cset;
 };
 
-static int of_overlay_apply_one(struct of_overlay *ov,
-		struct device_node *target, const struct device_node *overlay,
+static int build_changeset_next_level(struct overlay_changeset *ovcs,
+		struct device_node *target_node,
+		const struct device_node *overlay_node,
 		bool is_symbols_node);
 
-static BLOCKING_NOTIFIER_HEAD(of_overlay_chain);
+static BLOCKING_NOTIFIER_HEAD(overlay_notify_chain);
 
 int of_overlay_notifier_register(struct notifier_block *nb)
 {
-	return blocking_notifier_chain_register(&of_overlay_chain, nb);
+	return blocking_notifier_chain_register(&overlay_notify_chain, nb);
 }
 EXPORT_SYMBOL_GPL(of_overlay_notifier_register);
 
 int of_overlay_notifier_unregister(struct notifier_block *nb)
 {
-	return blocking_notifier_chain_unregister(&of_overlay_chain, nb);
+	return blocking_notifier_chain_unregister(&overlay_notify_chain, nb);
 }
 EXPORT_SYMBOL_GPL(of_overlay_notifier_unregister);
 
-static int of_overlay_notify(struct of_overlay *ov,
-			     enum of_overlay_notify_action action)
+static int overlay_notify(struct overlay_changeset *ovcs,
+		enum of_overlay_notify_action action)
 {
 	struct of_overlay_notify_data nd;
 	int i, ret;
 
-	for (i = 0; i < ov->count; i++) {
-		struct of_overlay_info *ovinfo = &ov->ovinfo_tab[i];
+	for (i = 0; i < ovcs->count; i++) {
+		struct fragment *fragment = &ovcs->fragments[i];
 
-		nd.target = ovinfo->target;
-		nd.overlay = ovinfo->overlay;
+		nd.target = fragment->target;
+		nd.overlay = fragment->overlay;
 
-		ret = blocking_notifier_call_chain(&of_overlay_chain,
+		ret = blocking_notifier_call_chain(&overlay_notify_chain,
 						   action, &nd);
 		if (ret)
 			return notifier_to_errno(ret);
@@ -94,10 +90,10 @@ static int of_overlay_notify(struct of_overlay *ov,
 	return 0;
 }
 
-static struct property *dup_and_fixup_symbol_prop(struct of_overlay *ov,
-		const struct property *prop)
+static struct property *dup_and_fixup_symbol_prop(
+		struct overlay_changeset *ovcs, const struct property *prop)
 {
-	struct of_overlay_info *ovinfo;
+	struct fragment *fragment;
 	struct property *new;
 	const char *overlay_name;
 	char *label_path;
@@ -116,18 +112,18 @@ static struct property *dup_and_fixup_symbol_prop(struct of_overlay *ov,
 	if (!new)
 		return NULL;
 
-	for (k = 0; k < ov->count; k++) {
-		ovinfo = &ov->ovinfo_tab[k];
-		overlay_name = ovinfo->overlay->full_name;
+	for (k = 0; k < ovcs->count; k++) {
+		fragment = &ovcs->fragments[k];
+		overlay_name = fragment->overlay->full_name;
 		overlay_name_len = strlen(overlay_name);
 		if (!strncasecmp(symbol_path, overlay_name, overlay_name_len))
 			break;
 	}
 
-	if (k >= ov->count)
+	if (k >= ovcs->count)
 		goto err_free;
 
-	target_path = ovinfo->target->full_name;
+	target_path = fragment->target->full_name;
 	target_path_len = strlen(target_path);
 
 	label_path = symbol_path + overlay_name_len;
@@ -156,81 +152,110 @@ static struct property *dup_and_fixup_symbol_prop(struct of_overlay *ov,
 
 }
 
-/*
+/**
+ * add_changeset_property() - add @overlay_prop to overlay changeset
+ * @ovcs:		overlay changeset
+ * @target_node:	where to place @overlay_prop in live tree
+ * @overlay_prop:	property to add or update, from overlay tree
+ * is_symbols_node:	1 if @target_node is "/__symbols__"
+ *
+ * If @overlay_prop does not already exist in @target_node, add changeset entry
+ * to add @overlay_prop in @target_node, else add changeset entry to update
+ * value of @overlay_prop.
+ *
  * Some special properties are not updated (no error returned).
+ *
  * Update of property in symbols node is not allowed.
+ *
+ * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
+ * invalid @overlay.
  */
-static int of_overlay_apply_single_property(struct of_overlay *ov,
-		struct device_node *target, struct property *prop,
+static int add_changeset_property(struct overlay_changeset *ovcs,
+		struct device_node *target_node,
+		struct property *overlay_prop,
 		bool is_symbols_node)
 {
-	struct property *propn = NULL, *tprop;
+	struct property *new_prop = NULL, *prop;
 
-	tprop = of_find_property(target, prop->name, NULL);
+	prop = of_find_property(target_node, overlay_prop->name, NULL);
 
-	if (!of_prop_cmp(prop->name, "name") ||
-	    !of_prop_cmp(prop->name, "phandle") ||
-	    !of_prop_cmp(prop->name, "linux,phandle"))
+	if (!of_prop_cmp(overlay_prop->name, "name") ||
+	    !of_prop_cmp(overlay_prop->name, "phandle") ||
+	    !of_prop_cmp(overlay_prop->name, "linux,phandle"))
 		return 0;
 
 	if (is_symbols_node) {
-		if (tprop)
+		if (prop)
 			return -EINVAL;
-		propn = dup_and_fixup_symbol_prop(ov, prop);
+		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
 	} else {
-		propn = __of_prop_dup(prop, GFP_KERNEL);
+		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
 	}
 
-	if (!propn)
+	if (!new_prop)
 		return -ENOMEM;
 
-	if (!tprop)
-		return of_changeset_add_property(&ov->cset, target, propn);
+	if (!prop)
+		return of_changeset_add_property(&ovcs->cset, target_node,
+						 new_prop);
 
-	return of_changeset_update_property(&ov->cset, target, propn);
+	return of_changeset_update_property(&ovcs->cset, target_node, new_prop);
 }
 
-/*
+/**
+ * add_changeset_node() - add @node (and children) to overlay changeset
+ * @ovcs:		overlay changeset
+ * @target_node:	where to place @node in live tree
+ * @node:		node from within overlay device tree fragment
+ *
+ * If @node does not already exist in @target_node, add changeset entry
+ * to add @node in @target_node.
+ *
+ * If @node already exists in @target_node, and the existing node has
+ * a phandle, the overlay node is not allowed to have a phandle.
+ *
+ * If @node has child nodes, add the children recursively via
+ * build_changeset_next_level().
+ *
  * NOTE: Multiple mods of created nodes not supported.
  *
- * Return
- *  -ENOMEM if memory allocation fails
- *  -EINVAL if existing node has a phandle and overlay node has a phandle
+ * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
+ * invalid @overlay.
  */
-static int of_overlay_apply_single_device_node(struct of_overlay *ov,
-		struct device_node *target, struct device_node *child)
+static int add_changeset_node(struct overlay_changeset *ovcs,
+		struct device_node *target_node, struct device_node *node)
 {
-	const char *cname;
+	const char *node_kbasename;
 	struct device_node *tchild;
 	int ret = 0;
 
-	cname = kbasename(child->full_name);
-	if (!cname)
+	node_kbasename = kbasename(node->full_name);
+	if (!node_kbasename)
 		return -ENOMEM;
 
-	for_each_child_of_node(target, tchild)
-		if (!of_node_cmp(cname, kbasename(tchild->full_name)))
+	for_each_child_of_node(target_node, tchild)
+		if (!of_node_cmp(node_kbasename, kbasename(tchild->full_name)))
 			break;
 
 	if (tchild) {
-		if (child->phandle)
+		if (node->phandle)
 			return -EINVAL;
 
-		/* apply overlay recursively */
-		ret = of_overlay_apply_one(ov, tchild, child, 0);
+		ret = build_changeset_next_level(ovcs, tchild, node, 0);
 		of_node_put(tchild);
 	} else {
-		tchild = __of_node_dup(child, "%pOF/%s", target, cname);
+		tchild = __of_node_dup(node, "%pOF/%s",
+				       target_node, node_kbasename);
 		if (!tchild)
 			return -ENOMEM;
 
-		tchild->parent = target;
+		tchild->parent = target_node;
 
-		ret = of_changeset_attach_node(&ov->cset, tchild);
+		ret = of_changeset_attach_node(&ovcs->cset, tchild);
 		if (ret)
 			return ret;
 
-		ret = of_overlay_apply_one(ov, tchild, child, 0);
+		ret = build_changeset_next_level(ovcs, tchild, node, 0);
 		if (ret)
 			return ret;
 	}
@@ -238,29 +263,37 @@ static int of_overlay_apply_single_device_node(struct of_overlay *ov,
 	return ret;
 }
 
-/*
- * Apply a single overlay node recursively.
+/**
+ * build_changeset_next_level() - add level of overlay changeset
+ * @ovcs:		overlay changeset
+ * @target_node:	where to place @overlay_node in live tree
+ * @overlay_node:	node from within an overlay device tree fragment
+ * @is_symbols_node:	@overlay_node is node "/__symbols__"
  *
- * Note that the in case of an error the target node is left
- * in a inconsistent state. Error recovery should be performed
- * by using the changeset.
+ * Add the properties (if any) and nodes (if any) from @overlay_node to the
+ * @ovcs->cset changeset.  If an added node has child nodes, they will
+ * be added recursively.
  *
  * Do not allow symbols node to have any children.
+ *
+ * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
+ * invalid @overlay_node.
  */
-static int of_overlay_apply_one(struct of_overlay *ov,
-		struct device_node *target, const struct device_node *overlay,
+static int build_changeset_next_level(struct overlay_changeset *ovcs,
+		struct device_node *target_node,
+		const struct device_node *overlay_node,
 		bool is_symbols_node)
 {
 	struct device_node *child;
 	struct property *prop;
 	int ret;
 
-	for_each_property_of_node(overlay, prop) {
-		ret = of_overlay_apply_single_property(ov, target, prop,
-						       is_symbols_node);
+	for_each_property_of_node(overlay_node, prop) {
+		ret = add_changeset_property(ovcs, target_node, prop,
+					     is_symbols_node);
 		if (ret) {
 			pr_err("Failed to apply prop @%pOF/%s\n",
-			       target, prop->name);
+			       target_node, prop->name);
 			return ret;
 		}
 	}
@@ -268,11 +301,11 @@ static int of_overlay_apply_one(struct of_overlay *ov,
 	if (is_symbols_node)
 		return 0;
 
-	for_each_child_of_node(overlay, child) {
-		ret = of_overlay_apply_single_device_node(ov, target, child);
+	for_each_child_of_node(overlay_node, child) {
+		ret = add_changeset_node(ovcs, target_node, child);
 		if (ret) {
-			pr_err("Failed to apply single node @%pOF/%s\n",
-			       target, child->name);
+			pr_err("Failed to apply node @%pOF/%s\n",
+			       target_node, child->name);
 			of_node_put(child);
 			return ret;
 		}
@@ -282,26 +315,30 @@ static int of_overlay_apply_one(struct of_overlay *ov,
 }
 
 /**
- * of_overlay_apply() - Apply @count overlays pointed at by @ovinfo_tab
- * @ov:		Overlay to apply
+ * build_changeset() - populate overlay changeset in @ovcs from @ovcs->fragments
+ * @ovcs:	Overlay changeset
  *
- * Applies the overlays given, while handling all error conditions
- * appropriately. Either the operation succeeds, or if it fails the
- * live tree is reverted to the state before the attempt.
- * Returns 0, or an error if the overlay attempt failed.
+ * Create changeset @ovcs->cset to contain the nodes and properties of the
+ * overlay device tree fragments in @ovcs->fragments[].  If an error occurs,
+ * any portions of the changeset that were successfully created will remain
+ * in @ovcs->cset.
+ *
+ * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
+ * invalid overlay in @ovcs->fragments[].
  */
-static int of_overlay_apply(struct of_overlay *ov)
+static int build_changeset(struct overlay_changeset *ovcs)
 {
-	int i, err;
+	int i, ret;
 
-	for (i = 0; i < ov->count; i++) {
-		struct of_overlay_info *ovinfo = &ov->ovinfo_tab[i];
+	for (i = 0; i < ovcs->count; i++) {
+		struct fragment *fragment = &ovcs->fragments[i];
 
-		err = of_overlay_apply_one(ov, ovinfo->target, ovinfo->overlay,
-					   ovinfo->is_symbols_node);
-		if (err) {
-			pr_err("apply failed '%pOF'\n", ovinfo->target);
-			return err;
+		ret = build_changeset_next_level(ovcs, fragment->target,
+					       fragment->overlay,
+					       fragment->is_symbols_node);
+		if (ret) {
+			pr_err("apply failed '%pOF'\n", fragment->target);
+			return ret;
 		}
 	}
 
@@ -371,23 +408,24 @@ static int of_fill_overlay_info(struct of_overlay *ov,
 }
 
 /**
- * of_build_overlay_info() - Build an overlay info array
- * @ov		Overlay to build
- * @tree:	Device node containing all the overlays
+ * init_overlay_changeset() - initialize overlay changeset from overlay tree
+ * @ovcs	Overlay changeset to build
+ * @tree:	Contains all the overlay fragments and overlay fixup nodes
  *
- * Helper function that given a tree containing overlay information,
- * allocates and builds an overlay info array containing it, ready
- * for use using of_overlay_apply.
+ * Initialize @ovcs.  Populate @ovcs->fragments with node information from
+ * the top level of @tree.  The relevant top level nodes are the fragment
+ * nodes and the __symbols__ node.  Any other top level node will be ignored.
  *
- * Returns 0 on success with the @cntp @ovinfop pointers valid,
- * while on error a negative error value is returned.
+ * Returns 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
+ * detected in @tree, or -ENODEV if no valid nodes found.
  */
-static int of_build_overlay_info(struct of_overlay *ov,
+static int init_overlay_changeset(struct overlay_changeset *ovcs,
 		struct device_node *tree)
 {
 	struct device_node *node;
-	struct of_overlay_info *ovinfo;
-	int cnt, err;
+	struct fragment *fragment;
+	struct fragment *fragments;
+	int cnt, ret;
 
 	cnt = 0;
 	for_each_child_of_node(tree, node)
@@ -396,24 +434,25 @@ static int of_build_overlay_info(struct of_overlay *ov,
 	if (of_get_child_by_name(tree, "__symbols__"))
 		cnt++;
 
-	ovinfo = kcalloc(cnt, sizeof(*ovinfo), GFP_KERNEL);
-	if (!ovinfo)
+	fragments = kcalloc(cnt, sizeof(*fragments), GFP_KERNEL);
+	if (!fragments)
 		return -ENOMEM;
 
 	cnt = 0;
 	for_each_child_of_node(tree, node) {
-		err = of_fill_overlay_info(ov, node, &ovinfo[cnt]);
-		if (!err)
+		ret = of_fill_overlay_info(ovcs, node, &fragments[cnt]);
+		if (!ret)
 			cnt++;
 	}
 
 	node = of_get_child_by_name(tree, "__symbols__");
 	if (node) {
-		ovinfo[cnt].overlay = node;
-		ovinfo[cnt].target = of_find_node_by_path("/__symbols__");
-		ovinfo[cnt].is_symbols_node = 1;
+		fragment = &fragments[cnt];
+		fragment->overlay = node;
+		fragment->target = of_find_node_by_path("/__symbols__");
+		fragment->is_symbols_node = 1;
 
-		if (!ovinfo[cnt].target) {
+		if (!fragment->target) {
 			pr_err("no symbols in root of device tree.\n");
 			return -EINVAL;
 		}
@@ -422,137 +461,127 @@ static int of_build_overlay_info(struct of_overlay *ov,
 	}
 
 	if (!cnt) {
-		kfree(ovinfo);
+		kfree(fragments);
 		return -ENODEV;
 	}
 
-	ov->count = cnt;
-	ov->ovinfo_tab = ovinfo;
+	ovcs->count = cnt;
+	ovcs->fragments = fragments;
 
 	return 0;
 }
 
 /**
- * of_free_overlay_info() - Free an overlay info array
- * @ov		Overlay to free the overlay info from
- * @ovinfo_tab:	Array of overlay_info's to free
+ * free_overlay_fragments() - Free a fragments array
+ * @ovcs	Overlay to free the overlay info from
  *
- * Releases the memory of a previously allocated ovinfo array
- * by of_build_overlay_info.
- * Returns 0, or an error if the arguments are bogus.
+ * Frees the memory of an ovcs->fragments[] array.
  */
-static int of_free_overlay_info(struct of_overlay *ov)
+static void free_overlay_fragments(struct overlay_changeset *ovcs)
 {
-	struct of_overlay_info *ovinfo;
 	int i;
 
 	/* do it in reverse */
-	for (i = ov->count - 1; i >= 0; i--) {
-		ovinfo = &ov->ovinfo_tab[i];
-
-		of_node_put(ovinfo->target);
-		of_node_put(ovinfo->overlay);
+	for (i = ovcs->count - 1; i >= 0; i--) {
+		of_node_put(ovcs->fragments[i].target);
+		of_node_put(ovcs->fragments[i].overlay);
 	}
-	kfree(ov->ovinfo_tab);
 
-	return 0;
+	kfree(ovcs->fragments);
 }
 
-static LIST_HEAD(ov_list);
-static DEFINE_IDR(ov_idr);
+static LIST_HEAD(ovcs_list);
+static DEFINE_IDR(ovcs_idr);
 
 /**
- * of_overlay_create() - Create and apply an overlay
- * @tree:	Device node containing all the overlays
+ * of_overlay_apply() - Create and apply an overlay changeset
+ * @tree:	Expanded overlay device tree
  *
- * Creates and applies an overlay while also keeping track
- * of the overlay in a list. This list can be used to prevent
- * illegal overlay removals.
+ * Creates and applies an overlay changeset.  If successful, the overlay
+ * changeset is added to the overlay changeset list.
  *
- * Returns the id of the created overlay, or a negative error number
+ * Returns the id of the created overlay changeset, or a negative error number
  */
-int of_overlay_create(struct device_node *tree)
+int of_overlay_apply(struct device_node *tree)
 {
-	struct of_overlay *ov;
-	int err, id;
+	struct overlay_changeset *ovcs;
+	int id, ret;
 
-	ov = kzalloc(sizeof(*ov), GFP_KERNEL);
-	if (!ov)
+	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
+	if (!ovcs)
 		return -ENOMEM;
-	ov->id = -1;
+	ovcs->id = -1;
 
-	INIT_LIST_HEAD(&ov->node);
+	INIT_LIST_HEAD(&ovcs->ovcs_list);
 
-	of_changeset_init(&ov->cset);
+	of_changeset_init(&ovcs->cset);
 
 	mutex_lock(&of_mutex);
 
-	id = idr_alloc(&ov_idr, ov, 0, 0, GFP_KERNEL);
+	id = idr_alloc(&ovcs_idr, ovcs, 0, 0, GFP_KERNEL);
 	if (id < 0) {
-		err = id;
+		ret = id;
 		goto err_destroy_trans;
 	}
-	ov->id = id;
+	ovcs->id = id;
 
-	err = of_build_overlay_info(ov, tree);
-	if (err) {
-		pr_err("of_build_overlay_info() failed for tree@%pOF\n",
+	ret = init_overlay_changeset(ovcs, tree);
+	if (ret) {
+		pr_err("init_overlay_changeset() failed for tree@%pOF\n",
 		       tree);
 		goto err_free_idr;
 	}
 
-	err = of_overlay_notify(ov, OF_OVERLAY_PRE_APPLY);
-	if (err < 0) {
-		pr_err("%s: Pre-apply notifier failed (err=%d)\n",
-		       __func__, err);
-		goto err_free_idr;
+	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
+	if (ret < 0) {
+		pr_err("%s: Pre-apply notifier failed (ret=%d)\n",
+		       __func__, ret);
+		goto err_free_overlay_fragments;
 	}
 
-	err = of_overlay_apply(ov);
-	if (err)
-		goto err_abort_trans;
+	ret = build_changeset(ovcs);
+	if (ret)
+		goto err_free_overlay_fragments;
 
-	err = __of_changeset_apply(&ov->cset);
-	if (err)
-		goto err_revert_overlay;
+	ret = __of_changeset_apply(&ovcs->cset);
+	if (ret)
+		goto err_free_overlay_fragments;
 
+	list_add_tail(&ovcs->ovcs_list, &ovcs_list);
 
-	list_add_tail(&ov->node, &ov_list);
-
-	of_overlay_notify(ov, OF_OVERLAY_POST_APPLY);
+	overlay_notify(ovcs, OF_OVERLAY_POST_APPLY);
 
 	mutex_unlock(&of_mutex);
 
 	return id;
 
-err_revert_overlay:
-err_abort_trans:
-	of_free_overlay_info(ov);
+err_free_overlay_fragments:
+	free_overlay_fragments(ovcs);
 err_free_idr:
-	idr_remove(&ov_idr, ov->id);
+	idr_remove(&ovcs_idr, ovcs->id);
 err_destroy_trans:
-	of_changeset_destroy(&ov->cset);
-	kfree(ov);
+	of_changeset_destroy(&ovcs->cset);
+	kfree(ovcs);
 	mutex_unlock(&of_mutex);
 
-	return err;
+	return ret;
 }
-EXPORT_SYMBOL_GPL(of_overlay_create);
+EXPORT_SYMBOL_GPL(of_overlay_apply);
 
 /*
- * check whether the given node, lies under the given tree
- * return 1 if under tree, else 0
+ * Find @np in @tree.
+ *
+ * Returns 1 if @np is @tree or is contained in @tree, else 0
  */
-static int overlay_subtree_check(struct device_node *tree,
-		struct device_node *dn)
+static int find_node(struct device_node *tree, struct device_node *np)
 {
 	struct device_node *child;
 
-	if (tree == dn)
+	if (tree == np)
 		return 1;
 
 	for_each_child_of_node(tree, child) {
-		if (overlay_subtree_check(child, dn)) {
+		if (find_node(child, np)) {
 			of_node_put(child);
 			return 1;
 		}
@@ -562,30 +591,32 @@ static int overlay_subtree_check(struct device_node *tree,
 }
 
 /*
- * check whether this overlay is the topmost
- * return 1 if topmost, else 0
+ * Is @remove_ce_np a child of or the same as any
+ * node in an overlay changeset more topmost than @remove_ovcs?
+ *
+ * Returns 1 if found, else 0
  */
-static int overlay_is_topmost(struct of_overlay *ov, struct device_node *dn)
+static int node_in_later_cs(struct overlay_changeset *remove_ovcs,
+		struct device_node *remove_ce_np)
 {
-	struct of_overlay *ovt;
+	struct overlay_changeset *ovcs;
 	struct of_changeset_entry *ce;
 
-	list_for_each_entry_reverse(ovt, &ov_list, node) {
-		/* if we hit ourselves, we're done */
-		if (ovt == ov)
+	list_for_each_entry_reverse(ovcs, &ovcs_list, ovcs_list) {
+		if (ovcs == remove_ovcs)
 			break;
 
-		/* check against each subtree affected by this overlay */
-		list_for_each_entry(ce, &ovt->cset.entries, node) {
-			if (overlay_subtree_check(ce->np, dn)) {
+		list_for_each_entry(ce, &ovcs->cset.entries, node) {
+			if (find_node(ce->np, remove_ce_np)) {
 				pr_err("%s: #%d clashes #%d @%pOF\n",
-					__func__, ov->id, ovt->id, dn);
-				return 0;
+					__func__, remove_ovcs->id, ovcs->id,
+					remove_ce_np);
+				return 1;
 			}
 		}
 	}
 
-	return 1;
+	return 0;
 }
 
 /*
@@ -598,13 +629,13 @@ static int overlay_is_topmost(struct of_overlay *ov, struct device_node *dn)
  * the one closest to the tail. If another overlay has affected this
  * device node and is closest to the tail, then removal is not permited.
  */
-static int overlay_removal_is_ok(struct of_overlay *ov)
+static int overlay_removal_is_ok(struct overlay_changeset *remove_ovcs)
 {
-	struct of_changeset_entry *ce;
+	struct of_changeset_entry *remove_ce;
 
-	list_for_each_entry(ce, &ov->cset.entries, node) {
-		if (!overlay_is_topmost(ov, ce->np)) {
-			pr_err("overlay #%d is not topmost\n", ov->id);
+	list_for_each_entry(remove_ce, &remove_ovcs->cset.entries, node) {
+		if (node_in_later_cs(remove_ovcs, remove_ce->np)) {
+			pr_err("overlay #%d is not topmost\n", remove_ovcs->id);
 			return 0;
 		}
 	}
@@ -613,74 +644,73 @@ static int overlay_removal_is_ok(struct of_overlay *ov)
 }
 
 /**
- * of_overlay_destroy() - Removes an overlay
- * @id:	Overlay id number returned by a previous call to of_overlay_create
+ * of_overlay_remove() - Revert and free an overlay changeset
+ * @ovcs_id:	Overlay changeset id number
  *
- * Removes an overlay if it is permissible.
+ * Removes an overlay if it is permissible.  ovcs_id was previously returned
+ * by of_overlay_apply().
  *
  * Returns 0 on success, or a negative error number
  */
-int of_overlay_destroy(int id)
+int of_overlay_remove(int ovcs_id)
 {
-	struct of_overlay *ov;
-	int err;
+	struct overlay_changeset *ovcs;
+	int ret = 0;
 
 	mutex_lock(&of_mutex);
 
-	ov = idr_find(&ov_idr, id);
-	if (!ov) {
-		err = -ENODEV;
-		pr_err("destroy: Could not find overlay #%d\n", id);
+	ovcs = idr_find(&ovcs_idr, ovcs_id);
+	if (!ovcs) {
+		ret = -ENODEV;
+		pr_err("remove: Could not find overlay #%d\n", ovcs_id);
 		goto out;
 	}
 
-	if (!overlay_removal_is_ok(ov)) {
-		err = -EBUSY;
+	if (!overlay_removal_is_ok(ovcs)) {
+		ret = -EBUSY;
 		goto out;
 	}
 
-	of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
-	list_del(&ov->node);
-	__of_changeset_revert(&ov->cset);
-	of_overlay_notify(ov, OF_OVERLAY_POST_REMOVE);
-	of_free_overlay_info(ov);
-	idr_remove(&ov_idr, id);
-	of_changeset_destroy(&ov->cset);
-	kfree(ov);
-
-	err = 0;
+	overlay_notify(ovcs, OF_OVERLAY_PRE_REMOVE);
+	list_del(&ovcs->ovcs_list);
+	__of_changeset_revert(&ovcs->cset);
+	overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
+	free_overlay_fragments(ovcs);
+	idr_remove(&ovcs_idr, ovcs_id);
+	of_changeset_destroy(&ovcs->cset);
+	kfree(ovcs);
 
 out:
 	mutex_unlock(&of_mutex);
 
-	return err;
+	return ret;
 }
-EXPORT_SYMBOL_GPL(of_overlay_destroy);
+EXPORT_SYMBOL_GPL(of_overlay_remove);
 
 /**
- * of_overlay_destroy_all() - Removes all overlays from the system
+ * of_overlay_remove_all() - Reverts and frees all overlay changesets
  *
  * Removes all overlays from the system in the correct order.
  *
  * Returns 0 on success, or a negative error number
  */
-int of_overlay_destroy_all(void)
+int of_overlay_remove_all(void)
 {
-	struct of_overlay *ov, *ovn;
+	struct overlay_changeset *ovcs, *ovcs_n;
 
 	mutex_lock(&of_mutex);
 
 	/* the tail of list is guaranteed to be safe to remove */
-	list_for_each_entry_safe_reverse(ov, ovn, &ov_list, node) {
-		list_del(&ov->node);
-		__of_changeset_revert(&ov->cset);
-		of_free_overlay_info(ov);
-		idr_remove(&ov_idr, ov->id);
-		kfree(ov);
+	list_for_each_entry_safe_reverse(ovcs, ovcs_n, &ovcs_list, ovcs_list) {
+		list_del(&ovcs->ovcs_list);
+		__of_changeset_revert(&ovcs->cset);
+		free_overlay_fragments(ovcs);
+		idr_remove(&ovcs_idr, ovcs->id);
+		kfree(ovcs);
 	}
 
 	mutex_unlock(&of_mutex);
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(of_overlay_destroy_all);
+EXPORT_SYMBOL_GPL(of_overlay_remove_all);
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 29a35cb1da64..e19dcd80e7a7 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1231,7 +1231,7 @@ static void of_unittest_destroy_tracked_overlays(void)
 			if (!(overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id)))
 				continue;
 
-			ret = of_overlay_destroy(id + overlay_first_id);
+			ret = of_overlay_remove(id + overlay_first_id);
 			if (ret == -ENODEV) {
 				pr_warn("%s: no overlay to destroy for #%d\n",
 					__func__, id + overlay_first_id);
@@ -1263,7 +1263,7 @@ static int of_unittest_apply_overlay(int overlay_nr, int unittest_nr,
 		goto out;
 	}
 
-	ret = of_overlay_create(np);
+	ret = of_overlay_apply(np);
 	if (ret < 0) {
 		unittest(0, "could not create overlay from \"%s\"\n",
 				overlay_path(overlay_nr));
@@ -1348,7 +1348,7 @@ static int of_unittest_apply_revert_overlay_check(int overlay_nr,
 		return -EINVAL;
 	}
 
-	ret = of_overlay_destroy(ov_id);
+	ret = of_overlay_remove(ov_id);
 	if (ret != 0) {
 		unittest(0, "overlay @\"%s\" failed to be destroyed @\"%s\"\n",
 				overlay_path(overlay_nr),
@@ -1477,7 +1477,7 @@ static void of_unittest_overlay_6(void)
 			return;
 		}
 
-		ret = of_overlay_create(np);
+		ret = of_overlay_apply(np);
 		if (ret < 0)  {
 			unittest(0, "could not create overlay from \"%s\"\n",
 					overlay_path(overlay_nr + i));
@@ -1501,7 +1501,7 @@ static void of_unittest_overlay_6(void)
 	}
 
 	for (i = 1; i >= 0; i--) {
-		ret = of_overlay_destroy(ov_id[i]);
+		ret = of_overlay_remove(ov_id[i]);
 		if (ret != 0) {
 			unittest(0, "overlay @\"%s\" failed destroy @\"%s\"\n",
 					overlay_path(overlay_nr + i),
@@ -1547,7 +1547,7 @@ static void of_unittest_overlay_8(void)
 			return;
 		}
 
-		ret = of_overlay_create(np);
+		ret = of_overlay_apply(np);
 		if (ret < 0)  {
 			unittest(0, "could not create overlay from \"%s\"\n",
 					overlay_path(overlay_nr + i));
@@ -1558,7 +1558,7 @@ static void of_unittest_overlay_8(void)
 	}
 
 	/* now try to remove first overlay (it should fail) */
-	ret = of_overlay_destroy(ov_id[0]);
+	ret = of_overlay_remove(ov_id[0]);
 	if (ret == 0) {
 		unittest(0, "overlay @\"%s\" was destroyed @\"%s\"\n",
 				overlay_path(overlay_nr + 0),
@@ -1569,7 +1569,7 @@ static void of_unittest_overlay_8(void)
 
 	/* removing them in order should work */
 	for (i = 1; i >= 0; i--) {
-		ret = of_overlay_destroy(ov_id[i]);
+		ret = of_overlay_remove(ov_id[i]);
 		if (ret != 0) {
 			unittest(0, "overlay @\"%s\" not destroyed @\"%s\"\n",
 					overlay_path(overlay_nr + i),
@@ -2151,9 +2151,9 @@ static int __init overlay_data_add(int onum)
 		goto out_free_np_overlay;
 	}
 
-	ret = of_overlay_create(info->np_overlay);
+	ret = of_overlay_apply(info->np_overlay);
 	if (ret < 0) {
-		pr_err("of_overlay_create() (ret=%d), %d\n", ret, onum);
+		pr_err("of_overlay_apply() (ret=%d), %d\n", ret, onum);
 		goto out_free_np_overlay;
 	} else {
 		info->overlay_id = ret;
diff --git a/include/linux/of.h b/include/linux/of.h
index cfc34117fc92..45e787e28ce7 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1306,26 +1306,26 @@ struct of_overlay_notify_data {
 #ifdef CONFIG_OF_OVERLAY
 
 /* ID based overlays; the API for external users */
-int of_overlay_create(struct device_node *tree);
-int of_overlay_destroy(int id);
-int of_overlay_destroy_all(void);
+int of_overlay_apply(struct device_node *tree);
+int of_overlay_remove(int id);
+int of_overlay_remove_all(void);
 
 int of_overlay_notifier_register(struct notifier_block *nb);
 int of_overlay_notifier_unregister(struct notifier_block *nb);
 
 #else
 
-static inline int of_overlay_create(struct device_node *tree)
+static inline int of_overlay_apply(struct device_node *tree)
 {
 	return -ENOTSUPP;
 }
 
-static inline int of_overlay_destroy(int id)
+static inline int of_overlay_remove(int id)
 {
 	return -ENOTSUPP;
 }
 
-static inline int of_overlay_destroy_all(void)
+static inline int of_overlay_remove_all(void)
 {
 	return -ENOTSUPP;
 }
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 04/12] of: overlay: rename identifiers in dup_and_fixup_symbol_prop()

[frowand.list]

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

More renaming of identifiers to better reflect what they do.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 397ef10d1f26..c350742ed2a2 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -90,17 +90,29 @@ static int overlay_notify(struct overlay_changeset *ovcs,
 	return 0;
 }
 
+/*
+ * The properties in the "/__symbols__" node are "symbols".
+ *
+ * The value of properties in the "/__symbols__" node is the path of a
+ * node in the subtree of a fragment node's "__overlay__" node, for
+ * example "/fragment@0/__overlay__/symbol_path_tail".  Symbol_path_tail
+ * can be a single node or it may be a multi-node path.
+ *
+ * The duplicated property value will be modified by replacing the
+ * "/fragment_name/__overlay/" portion of the value  with the target
+ * path from the fragment node.
+ */
 static struct property *dup_and_fixup_symbol_prop(
 		struct overlay_changeset *ovcs, const struct property *prop)
 {
 	struct fragment *fragment;
 	struct property *new;
 	const char *overlay_name;
-	char *label_path;
+	char *symbol_path_tail;
 	char *symbol_path;
 	const char *target_path;
 	int k;
-	int label_path_len;
+	int symbol_path_tail_len;
 	int overlay_name_len;
 	int target_path_len;
 
@@ -126,18 +138,18 @@ static struct property *dup_and_fixup_symbol_prop(
 	target_path = fragment->target->full_name;
 	target_path_len = strlen(target_path);
 
-	label_path = symbol_path + overlay_name_len;
-	label_path_len = strlen(label_path);
+	symbol_path_tail = symbol_path + overlay_name_len;
+	symbol_path_tail_len = strlen(symbol_path_tail);
 
 	new->name = kstrdup(prop->name, GFP_KERNEL);
-	new->length = target_path_len + label_path_len + 1;
+	new->length = target_path_len + symbol_path_tail_len + 1;
 	new->value = kzalloc(new->length, GFP_KERNEL);
 
 	if (!new->name || !new->value)
 		goto err_free;
 
 	strcpy(new->value, target_path);
-	strcpy(new->value + target_path_len, label_path);
+	strcpy(new->value + target_path_len, symbol_path_tail);
 
 	of_property_set_flag(new, OF_DYNAMIC);
 
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 05/12] of: overlay: minor restructuring

[frowand.list]

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

Continue improving the readability of overlay.c.  The previous patches
renamed identifiers.  This patch is split out from the previous patches
to make the previous patches easier to review.

Changes are:
  - minor code restructuring
  - some initialization of an overlay changeset occurred outside of
    init_overlay_changeset(), move that into init_overlay_changeset()
  - consolidate freeing an overlay changeset into free_overlay_changeset()

This patch is intended to not introduce any functional change.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 205 +++++++++++++++++++++++----------------------------
 1 file changed, 92 insertions(+), 113 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index c350742ed2a2..0f92a5c26748 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -55,6 +55,9 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
 		const struct device_node *overlay_node,
 		bool is_symbols_node);
 
+static LIST_HEAD(ovcs_list);
+static DEFINE_IDR(ovcs_idr);
+
 static BLOCKING_NOTIFIER_HEAD(overlay_notify_chain);
 
 int of_overlay_notifier_register(struct notifier_block *nb)
@@ -160,8 +163,6 @@ static struct property *dup_and_fixup_symbol_prop(
 	kfree(new->value);
 	kfree(new);
 	return NULL;
-
-
 }
 
 /**
@@ -249,13 +250,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
 		if (!of_node_cmp(node_kbasename, kbasename(tchild->full_name)))
 			break;
 
-	if (tchild) {
-		if (node->phandle)
-			return -EINVAL;
-
-		ret = build_changeset_next_level(ovcs, tchild, node, 0);
-		of_node_put(tchild);
-	} else {
+	if (!tchild) {
 		tchild = __of_node_dup(node, "%pOF/%s",
 				       target_node, node_kbasename);
 		if (!tchild)
@@ -267,11 +262,15 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
 		if (ret)
 			return ret;
 
-		ret = build_changeset_next_level(ovcs, tchild, node, 0);
-		if (ret)
-			return ret;
+		return build_changeset_next_level(ovcs, tchild, node, 0);
 	}
 
+	if (node->phandle)
+		return -EINVAL;
+
+	ret = build_changeset_next_level(ovcs, tchild, node, 0);
+	of_node_put(tchild);
+
 	return ret;
 }
 
@@ -385,41 +384,6 @@ static struct device_node *find_target_node(struct device_node *info_node)
 }
 
 /**
- * of_fill_overlay_info() - Fill an overlay info structure
- * @ov		Overlay to fill
- * @info_node:	Device node containing the overlay
- * @ovinfo:	Pointer to the overlay info structure to fill
- *
- * Fills an overlay info structure with the overlay information
- * from a device node. This device node must have a target property
- * which contains a phandle of the overlay target node, and an
- * __overlay__ child node which has the overlay contents.
- * Both ovinfo->target & ovinfo->overlay have their references taken.
- *
- * Returns 0 on success, or a negative error value.
- */
-static int of_fill_overlay_info(struct of_overlay *ov,
-		struct device_node *info_node, struct of_overlay_info *ovinfo)
-{
-	ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__");
-	if (!ovinfo->overlay)
-		goto err_fail;
-
-	ovinfo->target = find_target_node(info_node);
-	if (!ovinfo->target)
-		goto err_fail;
-
-	return 0;
-
-err_fail:
-	of_node_put(ovinfo->target);
-	of_node_put(ovinfo->overlay);
-
-	memset(ovinfo, 0, sizeof(*ovinfo));
-	return -EINVAL;
-}
-
-/**
  * init_overlay_changeset() - initialize overlay changeset from overlay tree
  * @ovcs	Overlay changeset to build
  * @tree:	Contains all the overlay fragments and overlay fixup nodes
@@ -429,32 +393,61 @@ static int of_fill_overlay_info(struct of_overlay *ov,
  * nodes and the __symbols__ node.  Any other top level node will be ignored.
  *
  * Returns 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
- * detected in @tree, or -ENODEV if no valid nodes found.
+ * detected in @tree, or -ENOSPC if idr_alloc() error.
  */
 static int init_overlay_changeset(struct overlay_changeset *ovcs,
 		struct device_node *tree)
 {
-	struct device_node *node;
+	struct device_node *node, *overlay_node;
 	struct fragment *fragment;
 	struct fragment *fragments;
 	int cnt, ret;
 
+	INIT_LIST_HEAD(&ovcs->ovcs_list);
+
+	of_changeset_init(&ovcs->cset);
+
+	ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
+	if (ovcs->id <= 0)
+		return ovcs->id;
+
 	cnt = 0;
-	for_each_child_of_node(tree, node)
-		cnt++;
 
-	if (of_get_child_by_name(tree, "__symbols__"))
+	/* fragment nodes */
+	for_each_child_of_node(tree, node) {
+		overlay_node = of_get_child_by_name(node, "__overlay__");
+		if (overlay_node) {
+			cnt++;
+			of_node_put(overlay_node);
+		}
+	}
+
+	node = of_get_child_by_name(tree, "__symbols__");
+	if (node) {
 		cnt++;
+		of_node_put(node);
+	}
 
 	fragments = kcalloc(cnt, sizeof(*fragments), GFP_KERNEL);
-	if (!fragments)
-		return -ENOMEM;
+	if (!fragments) {
+		ret = -ENOMEM;
+		goto err_free_idr;
+	}
 
 	cnt = 0;
 	for_each_child_of_node(tree, node) {
-		ret = of_fill_overlay_info(ovcs, node, &fragments[cnt]);
-		if (!ret)
-			cnt++;
+		fragment = &fragments[cnt];
+		fragment->overlay = of_get_child_by_name(node, "__overlay__");
+		if (fragment->overlay) {
+			fragment->target = find_target_node(node);
+			if (!fragment->target) {
+				of_node_put(fragment->overlay);
+				ret = -EINVAL;
+				goto err_free_fragments;
+			} else {
+				cnt++;
+			}
+		}
 	}
 
 	node = of_get_child_by_name(tree, "__symbols__");
@@ -466,44 +459,51 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 
 		if (!fragment->target) {
 			pr_err("no symbols in root of device tree.\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_free_fragments;
 		}
 
 		cnt++;
 	}
 
 	if (!cnt) {
-		kfree(fragments);
-		return -ENODEV;
+		ret = -EINVAL;
+		goto err_free_fragments;
 	}
 
 	ovcs->count = cnt;
 	ovcs->fragments = fragments;
 
 	return 0;
+
+
+err_free_fragments:
+	kfree(fragments);
+err_free_idr:
+	idr_remove(&ovcs_idr, ovcs->id);
+
+	return ret;
 }
 
-/**
- * free_overlay_fragments() - Free a fragments array
- * @ovcs	Overlay to free the overlay info from
- *
- * Frees the memory of an ovcs->fragments[] array.
- */
-static void free_overlay_fragments(struct overlay_changeset *ovcs)
+static void free_overlay_changeset(struct overlay_changeset *ovcs)
 {
 	int i;
 
-	/* do it in reverse */
-	for (i = ovcs->count - 1; i >= 0; i--) {
+	if (!ovcs->cset.entries.next)
+		return;
+	of_changeset_destroy(&ovcs->cset);
+
+	if (ovcs->id)
+		idr_remove(&ovcs_idr, ovcs->id);
+
+	for (i = 0; i < ovcs->count; i++) {
 		of_node_put(ovcs->fragments[i].target);
 		of_node_put(ovcs->fragments[i].overlay);
 	}
-
 	kfree(ovcs->fragments);
-}
 
-static LIST_HEAD(ovcs_list);
-static DEFINE_IDR(ovcs_idr);
+	kfree(ovcs);
+}
 
 /**
  * of_overlay_apply() - Create and apply an overlay changeset
@@ -517,47 +517,34 @@ static void free_overlay_fragments(struct overlay_changeset *ovcs)
 int of_overlay_apply(struct device_node *tree)
 {
 	struct overlay_changeset *ovcs;
-	int id, ret;
+	int ret;
 
 	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
 	if (!ovcs)
 		return -ENOMEM;
-	ovcs->id = -1;
-
-	INIT_LIST_HEAD(&ovcs->ovcs_list);
-
-	of_changeset_init(&ovcs->cset);
 
 	mutex_lock(&of_mutex);
 
-	id = idr_alloc(&ovcs_idr, ovcs, 0, 0, GFP_KERNEL);
-	if (id < 0) {
-		ret = id;
-		goto err_destroy_trans;
-	}
-	ovcs->id = id;
-
 	ret = init_overlay_changeset(ovcs, tree);
 	if (ret) {
-		pr_err("init_overlay_changeset() failed for tree@%pOF\n",
-		       tree);
-		goto err_free_idr;
+		pr_err("init_overlay_changeset() failed, ret = %d\n", ret);
+		goto err_free_overlay_changeset;
 	}
 
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
 	if (ret < 0) {
 		pr_err("%s: Pre-apply notifier failed (ret=%d)\n",
 		       __func__, ret);
-		goto err_free_overlay_fragments;
+		goto err_free_overlay_changeset;
 	}
 
 	ret = build_changeset(ovcs);
 	if (ret)
-		goto err_free_overlay_fragments;
+		goto err_free_overlay_changeset;
 
 	ret = __of_changeset_apply(&ovcs->cset);
 	if (ret)
-		goto err_free_overlay_fragments;
+		goto err_free_overlay_changeset;
 
 	list_add_tail(&ovcs->ovcs_list, &ovcs_list);
 
@@ -565,15 +552,11 @@ int of_overlay_apply(struct device_node *tree)
 
 	mutex_unlock(&of_mutex);
 
-	return id;
+	return ovcs->id;
+
+err_free_overlay_changeset:
+	free_overlay_changeset(ovcs);
 
-err_free_overlay_fragments:
-	free_overlay_fragments(ovcs);
-err_free_idr:
-	idr_remove(&ovcs_idr, ovcs->id);
-err_destroy_trans:
-	of_changeset_destroy(&ovcs->cset);
-	kfree(ovcs);
 	mutex_unlock(&of_mutex);
 
 	return ret;
@@ -684,13 +667,14 @@ int of_overlay_remove(int ovcs_id)
 	}
 
 	overlay_notify(ovcs, OF_OVERLAY_PRE_REMOVE);
+
 	list_del(&ovcs->ovcs_list);
+
 	__of_changeset_revert(&ovcs->cset);
+
 	overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
-	free_overlay_fragments(ovcs);
-	idr_remove(&ovcs_idr, ovcs_id);
-	of_changeset_destroy(&ovcs->cset);
-	kfree(ovcs);
+
+	free_overlay_changeset(ovcs);
 
 out:
 	mutex_unlock(&of_mutex);
@@ -709,20 +693,15 @@ int of_overlay_remove(int ovcs_id)
 int of_overlay_remove_all(void)
 {
 	struct overlay_changeset *ovcs, *ovcs_n;
-
-	mutex_lock(&of_mutex);
+	int ret;
 
 	/* the tail of list is guaranteed to be safe to remove */
 	list_for_each_entry_safe_reverse(ovcs, ovcs_n, &ovcs_list, ovcs_list) {
-		list_del(&ovcs->ovcs_list);
-		__of_changeset_revert(&ovcs->cset);
-		free_overlay_fragments(ovcs);
-		idr_remove(&ovcs_idr, ovcs->id);
-		kfree(ovcs);
+		ret = of_overlay_remove(ovcs->id);
+		if (ret)
+			return ret;
 	}
 
-	mutex_unlock(&of_mutex);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_overlay_remove_all);
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 06/12] of: overlay: detect cases where device tree may become corrupt

[frowand.list]

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

When an attempt to apply an overlay changeset fails, an effort
is made to revert any partial application of the changeset.
When an attempt to remove an overlay changeset fails, an effort
is made to re-apply any partial reversion of the changeset.

The existing code does not check for failure to recover a failed
overlay changeset application or overlay changeset revert.

Add the missing checks and flag the devicetree as corrupt if the
state of the devicetree can not be determined.

Improve and expand the returned errors to more fully reflect the
result of the effort to undo the partial effects of a failed attempt
to apply or remove an overlay changeset.

If the device tree might be corrupt, do not allow further attempts
to apply or remove an overlay changeset.

When creating an overlay changeset from an overlay device tree,
add some additional warnings if the state of the overlay device
tree is not as expected.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |   5 +-
 drivers/of/dynamic.c                         | 135 +++++++++++---
 drivers/of/of_private.h                      |   8 +-
 drivers/of/overlay.c                         | 253 ++++++++++++++++++++++-----
 drivers/of/unittest.c                        |  57 +++---
 include/linux/of.h                           |  10 +-
 6 files changed, 372 insertions(+), 96 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index 5f5b7ba35f1d..7a7be0515bfd 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -204,7 +204,7 @@ static void __init tilcdc_convert_slave_node(void)
 	/* For all memory needed for the overlay tree. This memory can
 	   be freed after the overlay has been applied. */
 	struct kfree_table kft;
-	int ret;
+	int ovcs_id, ret;
 
 	if (kfree_table_init(&kft))
 		return;
@@ -247,7 +247,8 @@ static void __init tilcdc_convert_slave_node(void)
 
 	tilcdc_node_disable(slave);
 
-	ret = of_overlay_apply(overlay);
+	ovcs_id = 0;
+	ret = of_overlay_apply(overlay, &ovcs_id);
 	if (ret)
 		pr_err("%s: Applying overlay changeset failed: %d\n",
 			__func__, ret);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 124510d56421..c1026efd6f9e 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -508,11 +508,12 @@ static void __of_changeset_entry_invert(struct of_changeset_entry *ce,
 	}
 }
 
-static void __of_changeset_entry_notify(struct of_changeset_entry *ce, bool revert)
+static int __of_changeset_entry_notify(struct of_changeset_entry *ce,
+		bool revert)
 {
 	struct of_reconfig_data rd;
 	struct of_changeset_entry ce_inverted;
-	int ret;
+	int ret = 0;
 
 	if (revert) {
 		__of_changeset_entry_invert(ce, &ce_inverted);
@@ -534,11 +535,12 @@ static void __of_changeset_entry_notify(struct of_changeset_entry *ce, bool reve
 	default:
 		pr_err("invalid devicetree changeset action: %i\n",
 			(int)ce->action);
-		return;
+		ret = -EINVAL;
 	}
 
 	if (ret)
 		pr_err("changeset notifier error @%pOF\n", ce->np);
+	return ret;
 }
 
 static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
@@ -672,32 +674,82 @@ void of_changeset_destroy(struct of_changeset *ocs)
 }
 EXPORT_SYMBOL_GPL(of_changeset_destroy);
 
-int __of_changeset_apply(struct of_changeset *ocs)
+/*
+ * Apply the changeset entries in @ocs.
+ * If apply fails, an attempt is made to revert the entries that were
+ * successfully applied.
+ *
+ * If multiple revert errors occur then only the final revert error is reported.
+ *
+ * Returns 0 on success, a negative error value in case of an error.
+ * If a revert error occurs, it is returned in *ret_revert.
+ */
+int __of_changeset_apply_entries(struct of_changeset *ocs, int *ret_revert)
 {
 	struct of_changeset_entry *ce;
-	int ret;
+	int ret, ret_tmp;
 
-	/* perform the rest of the work */
 	pr_debug("changeset: applying...\n");
 	list_for_each_entry(ce, &ocs->entries, node) {
 		ret = __of_changeset_entry_apply(ce);
 		if (ret) {
 			pr_err("Error applying changeset (%d)\n", ret);
-			list_for_each_entry_continue_reverse(ce, &ocs->entries, node)
-				__of_changeset_entry_revert(ce);
+			list_for_each_entry_continue_reverse(ce, &ocs->entries,
+							     node) {
+				ret_tmp = __of_changeset_entry_revert(ce);
+				if (ret_tmp)
+					*ret_revert = ret_tmp;
+			}
 			return ret;
 		}
 	}
-	pr_debug("changeset: applied, emitting notifiers.\n");
+
+	return 0;
+}
+
+/*
+ * Returns 0 on success, a negative error value in case of an error.
+ *
+ * If multiple changset entry notification errors occur then only the
+ * final notification error is reported.
+ */
+int __of_changeset_apply_notify(struct of_changeset *ocs)
+{
+	struct of_changeset_entry *ce;
+	int ret = 0, ret_tmp;
+
+	pr_debug("changeset: emitting notifiers.\n");
 
 	/* drop the global lock while emitting notifiers */
 	mutex_unlock(&of_mutex);
-	list_for_each_entry(ce, &ocs->entries, node)
-		__of_changeset_entry_notify(ce, 0);
+	list_for_each_entry(ce, &ocs->entries, node) {
+		ret_tmp = __of_changeset_entry_notify(ce, 0);
+		if (ret_tmp)
+			ret = ret_tmp;
+	}
 	mutex_lock(&of_mutex);
 	pr_debug("changeset: notifiers sent.\n");
 
-	return 0;
+	return ret;
+}
+
+/*
+ * Returns 0 on success, a negative error value in case of an error.
+ *
+ * If a changeset entry apply fails, an attempt is made to revert any
+ * previous entries in the changeset.  If any of the reverts fails,
+ * that failure is not reported.  Thus the state of the device tree
+ * is unknown if an apply error occurs.
+ */
+static int __of_changeset_apply(struct of_changeset *ocs)
+{
+	int ret, ret_revert = 0;
+
+	ret = __of_changeset_apply_entries(ocs, &ret_revert);
+	if (!ret)
+		ret = __of_changeset_apply_notify(ocs);
+
+	return ret;
 }
 
 /**
@@ -724,31 +776,74 @@ int of_changeset_apply(struct of_changeset *ocs)
 }
 EXPORT_SYMBOL_GPL(of_changeset_apply);
 
-int __of_changeset_revert(struct of_changeset *ocs)
+/*
+ * Revert the changeset entries in @ocs.
+ * If revert fails, an attempt is made to re-apply the entries that were
+ * successfully removed.
+ *
+ * If multiple re-apply errors occur then only the final apply error is
+ * reported.
+ *
+ * Returns 0 on success, a negative error value in case of an error.
+ * If an apply error occurs, it is returned in *ret_apply.
+ */
+int __of_changeset_revert_entries(struct of_changeset *ocs, int *ret_apply)
 {
 	struct of_changeset_entry *ce;
-	int ret;
+	int ret, ret_tmp;
 
 	pr_debug("changeset: reverting...\n");
 	list_for_each_entry_reverse(ce, &ocs->entries, node) {
 		ret = __of_changeset_entry_revert(ce);
 		if (ret) {
 			pr_err("Error reverting changeset (%d)\n", ret);
-			list_for_each_entry_continue(ce, &ocs->entries, node)
-				__of_changeset_entry_apply(ce);
+			list_for_each_entry_continue(ce, &ocs->entries, node) {
+				ret_tmp = __of_changeset_entry_apply(ce);
+				if (ret_tmp)
+					*ret_apply = ret_tmp;
+			}
 			return ret;
 		}
 	}
-	pr_debug("changeset: reverted, emitting notifiers.\n");
+
+	return 0;
+}
+
+/*
+ * If multiple changset entry notification errors occur then only the
+ * final notification error is reported.
+ */
+int __of_changeset_revert_notify(struct of_changeset *ocs)
+{
+	struct of_changeset_entry *ce;
+	int ret = 0, ret_tmp;
+
+	pr_debug("changeset: emitting notifiers.\n");
 
 	/* drop the global lock while emitting notifiers */
 	mutex_unlock(&of_mutex);
-	list_for_each_entry_reverse(ce, &ocs->entries, node)
-		__of_changeset_entry_notify(ce, 1);
+	list_for_each_entry_reverse(ce, &ocs->entries, node) {
+		ret_tmp = __of_changeset_entry_notify(ce, 1);
+		if (ret_tmp)
+			ret = ret_tmp;
+	}
 	mutex_lock(&of_mutex);
 	pr_debug("changeset: notifiers sent.\n");
 
-	return 0;
+	return ret;
+}
+
+static int __of_changeset_revert(struct of_changeset *ocs)
+{
+	int ret, ret_reply;
+
+	ret_reply = 0;
+	ret = __of_changeset_revert_entries(ocs, &ret_reply);
+
+	if (!ret)
+		ret = __of_changeset_revert_notify(ocs);
+
+	return ret;
 }
 
 /**
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 3ae12ffbf547..b66e8a812147 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -45,8 +45,12 @@ static inline struct device_node *kobj_to_device_node(struct kobject *kobj)
 extern int of_property_notify(int action, struct device_node *np,
 			      struct property *prop, struct property *old_prop);
 extern void of_node_release(struct kobject *kobj);
-extern int __of_changeset_apply(struct of_changeset *ocs);
-extern int __of_changeset_revert(struct of_changeset *ocs);
+extern int __of_changeset_apply_entries(struct of_changeset *ocs,
+					int *ret_revert);
+extern int __of_changeset_apply_notify(struct of_changeset *ocs);
+extern int __of_changeset_revert_entries(struct of_changeset *ocs,
+					 int *ret_apply);
+extern int __of_changeset_revert_notify(struct of_changeset *ocs);
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_property_notify(int action, struct device_node *np,
 				     struct property *prop, struct property *old_prop)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 0f92a5c26748..c7526186b1c8 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -50,6 +50,22 @@ struct overlay_changeset {
 	struct of_changeset cset;
 };
 
+/* flags are sticky - once set, do not reset */
+static int devicetree_state_flags;
+#define DTSF_APPLY_FAIL		0x01
+#define DTSF_REVERT_FAIL	0x02
+
+/*
+ * If a changeset apply or revert encounters an error, an attempt will
+ * be made to undo partial changes, but may fail.  If the undo fails
+ * we do not know the state of the devicetree.
+ */
+static int devicetree_corrupt(void)
+{
+	return devicetree_state_flags &
+		(DTSF_APPLY_FAIL | DTSF_REVERT_FAIL);
+}
+
 static int build_changeset_next_level(struct overlay_changeset *ovcs,
 		struct device_node *target_node,
 		const struct device_node *overlay_node,
@@ -72,6 +88,13 @@ int of_overlay_notifier_unregister(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(of_overlay_notifier_unregister);
 
+static char *of_overlay_action_name[] = {
+	"pre-apply",
+	"post-apply",
+	"pre-remove",
+	"post-remove",
+};
+
 static int overlay_notify(struct overlay_changeset *ovcs,
 		enum of_overlay_notify_action action)
 {
@@ -86,8 +109,14 @@ static int overlay_notify(struct overlay_changeset *ovcs,
 
 		ret = blocking_notifier_call_chain(&overlay_notify_chain,
 						   action, &nd);
-		if (ret)
-			return notifier_to_errno(ret);
+		if (ret == NOTIFY_STOP)
+			return 0;
+		if (ret) {
+			ret = notifier_to_errno(ret);
+			pr_err("overlay changeset %s notifier error %d, target: %pOF\n",
+			       of_overlay_action_name[action], ret, nd.target);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -231,6 +260,14 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
  * build_changeset_next_level().
  *
  * NOTE: 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
+ *       applied, __of_attach_node() will attach the node twice (once for
+ *       each fragment).  At this point the device tree will be corrupted.
+ *
+ *       TODO: add integrity check to ensure that multiple fragments do not
+ *             create the same node.
  *
  * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
  * invalid @overlay.
@@ -303,8 +340,8 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
 		ret = add_changeset_property(ovcs, target_node, prop,
 					     is_symbols_node);
 		if (ret) {
-			pr_err("Failed to apply prop @%pOF/%s\n",
-			       target_node, prop->name);
+			pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
+				 target_node, prop->name, ret);
 			return ret;
 		}
 	}
@@ -315,8 +352,8 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
 	for_each_child_of_node(overlay_node, child) {
 		ret = add_changeset_node(ovcs, target_node, child);
 		if (ret) {
-			pr_err("Failed to apply node @%pOF/%s\n",
-			       target_node, child->name);
+			pr_debug("Failed to apply node @%pOF/%s, err=%d\n",
+				 target_node, child->name, ret);
 			of_node_put(child);
 			return ret;
 		}
@@ -348,7 +385,7 @@ static int build_changeset(struct overlay_changeset *ovcs)
 					       fragment->overlay,
 					       fragment->is_symbols_node);
 		if (ret) {
-			pr_err("apply failed '%pOF'\n", fragment->target);
+			pr_debug("apply failed '%pOF'\n", fragment->target);
 			return ret;
 		}
 	}
@@ -403,6 +440,19 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	struct fragment *fragments;
 	int cnt, ret;
 
+	/*
+	 * Warn for some issues.  Can not return -EINVAL for these until
+	 * of_unittest_apply_overlay() is fixed to pass these checks.
+	 */
+	if (!of_node_check_flag(tree, OF_DYNAMIC))
+		pr_debug("%s() tree is not dynamic\n", __func__);
+
+	if (!of_node_check_flag(tree, OF_DETACHED))
+		pr_debug("%s() tree is not detached\n", __func__);
+
+	if (!of_node_is_root(tree))
+		pr_debug("%s() tree is not root\n", __func__);
+
 	INIT_LIST_HEAD(&ovcs->ovcs_list);
 
 	of_changeset_init(&ovcs->cset);
@@ -476,12 +526,13 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 
 	return 0;
 
-
 err_free_fragments:
 	kfree(fragments);
 err_free_idr:
 	idr_remove(&ovcs_idr, ovcs->id);
 
+	pr_err("%s() failed, ret = %d\n", __func__, ret);
+
 	return ret;
 }
 
@@ -508,33 +559,71 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
 /**
  * of_overlay_apply() - Create and apply an overlay changeset
  * @tree:	Expanded overlay device tree
+ * @ovcs_id:	Pointer to overlay changeset id
+ *
+ * Creates and applies an overlay changeset.
  *
- * Creates and applies an overlay changeset.  If successful, the overlay
- * changeset is added to the overlay changeset list.
+ * If an error occurs in a pre-apply notifier, then no changes are made
+ * to the device tree.
  *
- * Returns the id of the created overlay changeset, or a negative error number
+
+ * A non-zero return value will not have created the changeset if error is from:
+ *   - parameter checks
+ *   - building the changeset
+ *   - overlay changset pre-apply notifier
+ *
+ * If an error is returned by an overlay changeset pre-apply notifier
+ * then no further overlay changeset pre-apply notifier will be called.
+ *
+ * A non-zero return value will have created the changeset if error is from:
+ *   - overlay changeset entry notifier
+ *   - overlay changset post-apply notifier
+ *
+ * If an error is returned by an overlay changeset post-apply notifier
+ * then no further overlay changeset post-apply notifier will be called.
+ *
+ * If more than one notifier returns an error, then the last notifier
+ * error to occur is returned.
+ *
+ * If an error occurred while applying the overlay changeset, then an
+ * attempt is made to revert any changes that were made to the
+ * device tree.  If there were any errors during the revert attempt
+ * then the state of the device tree can not be determined, and any
+ * following attempt to apply or remove an overlay changeset will be
+ * refused.
+ *
+ * Returns 0 on success, or a negative error number.  Overlay changeset
+ * id is returned to *ovcs_id.
  */
-int of_overlay_apply(struct device_node *tree)
+
+int of_overlay_apply(struct device_node *tree, int *ovcs_id)
 {
 	struct overlay_changeset *ovcs;
-	int ret;
+	int ret = 0, ret_revert, ret_tmp;
+
+	*ovcs_id = 0;
+
+	if (devicetree_corrupt()) {
+		pr_err("devicetree state suspect, refuse to apply overlay\n");
+		ret = -EBUSY;
+		goto out;
+	}
 
 	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
-	if (!ovcs)
-		return -ENOMEM;
+	if (!ovcs) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	mutex_lock(&of_mutex);
 
 	ret = init_overlay_changeset(ovcs, tree);
-	if (ret) {
-		pr_err("init_overlay_changeset() failed, ret = %d\n", ret);
+	if (ret)
 		goto err_free_overlay_changeset;
-	}
 
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
-	if (ret < 0) {
-		pr_err("%s: Pre-apply notifier failed (ret=%d)\n",
-		       __func__, ret);
+	if (ret) {
+		pr_err("overlay changeset pre-apply notify error %d\n", ret);
 		goto err_free_overlay_changeset;
 	}
 
@@ -542,23 +631,46 @@ int of_overlay_apply(struct device_node *tree)
 	if (ret)
 		goto err_free_overlay_changeset;
 
-	ret = __of_changeset_apply(&ovcs->cset);
-	if (ret)
+	ret_revert = 0;
+	ret = __of_changeset_apply_entries(&ovcs->cset, &ret_revert);
+	if (ret) {
+		if (ret_revert) {
+			pr_debug("overlay changeset revert error %d\n",
+				 ret_revert);
+			devicetree_state_flags |= DTSF_APPLY_FAIL;
+		}
 		goto err_free_overlay_changeset;
+	} else {
+		ret = __of_changeset_apply_notify(&ovcs->cset);
+		if (ret)
+			pr_err("overlay changeset entry notify error %d\n",
+			       ret);
+		/* fall through */
+	}
 
 	list_add_tail(&ovcs->ovcs_list, &ovcs_list);
-
-	overlay_notify(ovcs, OF_OVERLAY_POST_APPLY);
+	*ovcs_id = ovcs->id;
+
+	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_APPLY);
+	if (ret_tmp) {
+		pr_err("overlay changeset post-apply notify error %d\n",
+		       ret_tmp);
+		if (!ret)
+			ret = ret_tmp;
+	}
 
 	mutex_unlock(&of_mutex);
 
-	return ovcs->id;
+	goto out;
 
 err_free_overlay_changeset:
 	free_overlay_changeset(ovcs);
 
 	mutex_unlock(&of_mutex);
 
+out:
+	pr_debug("%s() err=%d\n", __func__, ret);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_overlay_apply);
@@ -640,45 +752,106 @@ static int overlay_removal_is_ok(struct overlay_changeset *remove_ovcs)
 
 /**
  * of_overlay_remove() - Revert and free an overlay changeset
- * @ovcs_id:	Overlay changeset id number
+ * @ovcs_id:	Pointer to overlay changeset id
  *
- * Removes an overlay if it is permissible.  ovcs_id was previously returned
+ * Removes an overlay if it is permissible.  @ovcs_id was previously returned
  * by of_overlay_apply().
  *
- * Returns 0 on success, or a negative error number
+ * If an error occurred while attempting to revert the overlay changeset,
+ * then an attempt is made to re-apply any changeset entry that was
+ * reverted.  If an error occurs on re-apply then the state of the device
+ * tree can not be determined, and any following attempt to apply or remove
+ * an overlay changeset will be refused.
+ *
+ * A non-zero return value will not revert the changeset if error is from:
+ *   - parameter checks
+ *   - overlay changset pre-remove notifier
+ *   - overlay changeset entry revert
+ *
+ * If an error is returned by an overlay changeset pre-remove notifier
+ * then no further overlay changeset pre-remove notifier will be called.
+ *
+ * If more than one notifier returns an error, then the last notifier
+ * error to occur is returned.
+ *
+ * A non-zero return value will revert the changeset if error is from:
+ *   - overlay changeset entry notifier
+ *   - overlay changset post-remove notifier
+ *
+ * If an error is returned by an overlay changeset post-remove notifier
+ * then no further overlay changeset post-remove notifier will be called.
+ *
+ * Returns 0 on success, or a negative error number.  *ovcs_id is set to
+ * zero after reverting the changeset, even if a subsequent error occurs.
  */
-int of_overlay_remove(int ovcs_id)
+int of_overlay_remove(int *ovcs_id)
 {
 	struct overlay_changeset *ovcs;
-	int ret = 0;
+	int ret, ret_apply, ret_tmp;
+
+	ret = 0;
+
+	if (devicetree_corrupt()) {
+		pr_err("suspect devicetree state, refuse to remove overlay\n");
+		ret = -EBUSY;
+		goto out;
+	}
 
 	mutex_lock(&of_mutex);
 
-	ovcs = idr_find(&ovcs_idr, ovcs_id);
+	ovcs = idr_find(&ovcs_idr, *ovcs_id);
 	if (!ovcs) {
 		ret = -ENODEV;
-		pr_err("remove: Could not find overlay #%d\n", ovcs_id);
-		goto out;
+		pr_err("remove: Could not find overlay #%d\n", *ovcs_id);
+		goto out_unlock;
 	}
 
 	if (!overlay_removal_is_ok(ovcs)) {
 		ret = -EBUSY;
-		goto out;
+		goto out_unlock;
 	}
 
-	overlay_notify(ovcs, OF_OVERLAY_PRE_REMOVE);
+	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_REMOVE);
+	if (ret) {
+		pr_err("overlay changeset pre-remove notify error %d\n", ret);
+		goto out_unlock;
+	}
 
 	list_del(&ovcs->ovcs_list);
 
-	__of_changeset_revert(&ovcs->cset);
+	ret_apply = 0;
+	ret = __of_changeset_revert_entries(&ovcs->cset, &ret_apply);
+	if (ret) {
+		if (ret_apply)
+			devicetree_state_flags |= DTSF_REVERT_FAIL;
+		goto out_unlock;
+	} else {
+		ret = __of_changeset_revert_notify(&ovcs->cset);
+		if (ret) {
+			pr_err("overlay changeset entry notify error %d\n",
+			       ret);
+			/* fall through - changeset was reverted */
+		}
+	}
 
-	overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
+	*ovcs_id = 0;
+
+	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
+	if (ret_tmp) {
+		pr_err("overlay changeset post-remove notify error %d\n",
+		       ret_tmp);
+		if (!ret)
+			ret = ret_tmp;
+	}
 
 	free_overlay_changeset(ovcs);
 
-out:
+out_unlock:
 	mutex_unlock(&of_mutex);
 
+out:
+	pr_debug("%s() err=%d\n", __func__, ret);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_overlay_remove);
@@ -697,7 +870,7 @@ int of_overlay_remove_all(void)
 
 	/* the tail of list is guaranteed to be safe to remove */
 	list_for_each_entry_safe_reverse(ovcs, ovcs_n, &ovcs_list, ovcs_list) {
-		ret = of_overlay_remove(ovcs->id);
+		ret = of_overlay_remove(&ovcs->id);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e19dcd80e7a7..db2f170186de 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1218,7 +1218,7 @@ static void of_unittest_untrack_overlay(int id)
 
 static void of_unittest_destroy_tracked_overlays(void)
 {
-	int id, ret, defers;
+	int id, ret, defers, ovcs_id;
 
 	if (overlay_first_id < 0)
 		return;
@@ -1231,7 +1231,8 @@ static void of_unittest_destroy_tracked_overlays(void)
 			if (!(overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id)))
 				continue;
 
-			ret = of_overlay_remove(id + overlay_first_id);
+			ovcs_id = id + overlay_first_id;
+			ret = of_overlay_remove(&ovcs_id);
 			if (ret == -ENODEV) {
 				pr_warn("%s: no overlay to destroy for #%d\n",
 					__func__, id + overlay_first_id);
@@ -1253,7 +1254,7 @@ static int of_unittest_apply_overlay(int overlay_nr, int unittest_nr,
 		int *overlay_id)
 {
 	struct device_node *np = NULL;
-	int ret, id = -1;
+	int ret;
 
 	np = of_find_node_by_path(overlay_path(overlay_nr));
 	if (np == NULL) {
@@ -1263,23 +1264,20 @@ static int of_unittest_apply_overlay(int overlay_nr, int unittest_nr,
 		goto out;
 	}
 
-	ret = of_overlay_apply(np);
+	*overlay_id = 0;
+	ret = of_overlay_apply(np, overlay_id);
 	if (ret < 0) {
 		unittest(0, "could not create overlay from \"%s\"\n",
 				overlay_path(overlay_nr));
 		goto out;
 	}
-	id = ret;
-	of_unittest_track_overlay(id);
+	of_unittest_track_overlay(*overlay_id);
 
 	ret = 0;
 
 out:
 	of_node_put(np);
 
-	if (overlay_id)
-		*overlay_id = id;
-
 	return ret;
 }
 
@@ -1287,7 +1285,7 @@ static int of_unittest_apply_overlay(int overlay_nr, int unittest_nr,
 static int of_unittest_apply_overlay_check(int overlay_nr, int unittest_nr,
 		int before, int after, enum overlay_type ovtype)
 {
-	int ret;
+	int ret, ovcs_id;
 
 	/* unittest device must not be in before state */
 	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
@@ -1298,7 +1296,8 @@ static int of_unittest_apply_overlay_check(int overlay_nr, int unittest_nr,
 		return -EINVAL;
 	}
 
-	ret = of_unittest_apply_overlay(overlay_nr, unittest_nr, NULL);
+	ovcs_id = 0;
+	ret = of_unittest_apply_overlay(overlay_nr, unittest_nr, &ovcs_id);
 	if (ret != 0) {
 		/* of_unittest_apply_overlay already called unittest() */
 		return ret;
@@ -1321,7 +1320,7 @@ static int of_unittest_apply_revert_overlay_check(int overlay_nr,
 		int unittest_nr, int before, int after,
 		enum overlay_type ovtype)
 {
-	int ret, ov_id;
+	int ret, ovcs_id;
 
 	/* unittest device must be in before state */
 	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
@@ -1333,7 +1332,8 @@ static int of_unittest_apply_revert_overlay_check(int overlay_nr,
 	}
 
 	/* apply the overlay */
-	ret = of_unittest_apply_overlay(overlay_nr, unittest_nr, &ov_id);
+	ovcs_id = 0;
+	ret = of_unittest_apply_overlay(overlay_nr, unittest_nr, &ovcs_id);
 	if (ret != 0) {
 		/* of_unittest_apply_overlay already called unittest() */
 		return ret;
@@ -1348,7 +1348,7 @@ static int of_unittest_apply_revert_overlay_check(int overlay_nr,
 		return -EINVAL;
 	}
 
-	ret = of_overlay_remove(ov_id);
+	ret = of_overlay_remove(&ovcs_id);
 	if (ret != 0) {
 		unittest(0, "overlay @\"%s\" failed to be destroyed @\"%s\"\n",
 				overlay_path(overlay_nr),
@@ -1450,7 +1450,7 @@ static void of_unittest_overlay_5(void)
 static void of_unittest_overlay_6(void)
 {
 	struct device_node *np;
-	int ret, i, ov_id[2];
+	int ret, i, ov_id[2], ovcs_id;
 	int overlay_nr = 6, unittest_nr = 6;
 	int before = 0, after = 1;
 
@@ -1477,13 +1477,14 @@ static void of_unittest_overlay_6(void)
 			return;
 		}
 
-		ret = of_overlay_apply(np);
+		ovcs_id = 0;
+		ret = of_overlay_apply(np, &ovcs_id);
 		if (ret < 0)  {
 			unittest(0, "could not create overlay from \"%s\"\n",
 					overlay_path(overlay_nr + i));
 			return;
 		}
-		ov_id[i] = ret;
+		ov_id[i] = ovcs_id;
 		of_unittest_track_overlay(ov_id[i]);
 	}
 
@@ -1501,7 +1502,8 @@ static void of_unittest_overlay_6(void)
 	}
 
 	for (i = 1; i >= 0; i--) {
-		ret = of_overlay_remove(ov_id[i]);
+		ovcs_id = ov_id[i];
+		ret = of_overlay_remove(&ovcs_id);
 		if (ret != 0) {
 			unittest(0, "overlay @\"%s\" failed destroy @\"%s\"\n",
 					overlay_path(overlay_nr + i),
@@ -1532,7 +1534,7 @@ static void of_unittest_overlay_6(void)
 static void of_unittest_overlay_8(void)
 {
 	struct device_node *np;
-	int ret, i, ov_id[2];
+	int ret, i, ov_id[2], ovcs_id;
 	int overlay_nr = 8, unittest_nr = 8;
 
 	/* we don't care about device state in this test */
@@ -1547,18 +1549,20 @@ static void of_unittest_overlay_8(void)
 			return;
 		}
 
-		ret = of_overlay_apply(np);
+		ovcs_id = 0;
+		ret = of_overlay_apply(np, &ovcs_id);
 		if (ret < 0)  {
 			unittest(0, "could not create overlay from \"%s\"\n",
 					overlay_path(overlay_nr + i));
 			return;
 		}
-		ov_id[i] = ret;
+		ov_id[i] = ovcs_id;
 		of_unittest_track_overlay(ov_id[i]);
 	}
 
 	/* now try to remove first overlay (it should fail) */
-	ret = of_overlay_remove(ov_id[0]);
+	ovcs_id = ov_id[0];
+	ret = of_overlay_remove(&ovcs_id);
 	if (ret == 0) {
 		unittest(0, "overlay @\"%s\" was destroyed @\"%s\"\n",
 				overlay_path(overlay_nr + 0),
@@ -1569,7 +1573,8 @@ static void of_unittest_overlay_8(void)
 
 	/* removing them in order should work */
 	for (i = 1; i >= 0; i--) {
-		ret = of_overlay_remove(ov_id[i]);
+		ovcs_id = ov_id[i];
+		ret = of_overlay_remove(&ovcs_id);
 		if (ret != 0) {
 			unittest(0, "overlay @\"%s\" not destroyed @\"%s\"\n",
 					overlay_path(overlay_nr + i),
@@ -2151,13 +2156,11 @@ static int __init overlay_data_add(int onum)
 		goto out_free_np_overlay;
 	}
 
-	ret = of_overlay_apply(info->np_overlay);
+	info->overlay_id = 0;
+	ret = of_overlay_apply(info->np_overlay, &info->overlay_id);
 	if (ret < 0) {
 		pr_err("of_overlay_apply() (ret=%d), %d\n", ret, onum);
 		goto out_free_np_overlay;
-	} else {
-		info->overlay_id = ret;
-		ret = 0;
 	}
 
 	pr_debug("__dtb_overlay_begin applied, overlay id %d\n", ret);
diff --git a/include/linux/of.h b/include/linux/of.h
index 45e787e28ce7..49e5f24fb390 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1292,7 +1292,7 @@ static inline bool of_device_is_system_power_controller(const struct device_node
  */
 
 enum of_overlay_notify_action {
-	OF_OVERLAY_PRE_APPLY,
+	OF_OVERLAY_PRE_APPLY = 0,
 	OF_OVERLAY_POST_APPLY,
 	OF_OVERLAY_PRE_REMOVE,
 	OF_OVERLAY_POST_REMOVE,
@@ -1306,8 +1306,8 @@ struct of_overlay_notify_data {
 #ifdef CONFIG_OF_OVERLAY
 
 /* ID based overlays; the API for external users */
-int of_overlay_apply(struct device_node *tree);
-int of_overlay_remove(int id);
+int of_overlay_apply(struct device_node *tree, int *ovcs_id);
+int of_overlay_remove(int *ovcs_id);
 int of_overlay_remove_all(void);
 
 int of_overlay_notifier_register(struct notifier_block *nb);
@@ -1315,12 +1315,12 @@ struct of_overlay_notify_data {
 
 #else
 
-static inline int of_overlay_apply(struct device_node *tree)
+static inline int of_overlay_apply(struct device_node *tree, int *ovcs_id)
 {
 	return -ENOTSUPP;
 }
 
-static inline int of_overlay_remove(int id)
+static inline int of_overlay_remove(int *ovcs_id)
 {
 	return -ENOTSUPP;
 }
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 07/12] of: overlay: expand check of whether overlay changeset can be removed

[frowand.list]

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

The test of whether it is safe to remove an overlay changeset
looked at whether any node in the overlay changeset was in a
subtree rooted at any more recently applied overlay changeset
node.

The test failed to determine whether any node in the overlay
changeset was the root of a subtree that contained a more
recently applied overlay changeset node.  Add this additional
check to the test.

The test is still lacking any check for any phandle dependencies.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index c7526186b1c8..015d8b112f60 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -698,13 +698,13 @@ static int find_node(struct device_node *tree, struct device_node *np)
 }
 
 /*
- * Is @remove_ce_np a child of or the same as any
+ * Is @remove_ce_node a child of, a parent of, or the same as any
  * node in an overlay changeset more topmost than @remove_ovcs?
  *
  * Returns 1 if found, else 0
  */
-static int node_in_later_cs(struct overlay_changeset *remove_ovcs,
-		struct device_node *remove_ce_np)
+static int node_overlaps_later_cs(struct overlay_changeset *remove_ovcs,
+		struct device_node *remove_ce_node)
 {
 	struct overlay_changeset *ovcs;
 	struct of_changeset_entry *ce;
@@ -714,10 +714,16 @@ static int node_in_later_cs(struct overlay_changeset *remove_ovcs,
 			break;
 
 		list_for_each_entry(ce, &ovcs->cset.entries, node) {
-			if (find_node(ce->np, remove_ce_np)) {
-				pr_err("%s: #%d clashes #%d @%pOF\n",
+			if (find_node(ce->np, remove_ce_node)) {
+				pr_err("%s: #%d overlaps with #%d @%pOF\n",
 					__func__, remove_ovcs->id, ovcs->id,
-					remove_ce_np);
+					remove_ce_node);
+				return 1;
+			}
+			if (find_node(remove_ce_node, ce->np)) {
+				pr_err("%s: #%d overlaps with #%d @%pOF\n",
+					__func__, remove_ovcs->id, ovcs->id,
+					remove_ce_node);
 				return 1;
 			}
 		}
@@ -741,7 +747,7 @@ static int overlay_removal_is_ok(struct overlay_changeset *remove_ovcs)
 	struct of_changeset_entry *remove_ce;
 
 	list_for_each_entry(remove_ce, &remove_ovcs->cset.entries, node) {
-		if (node_in_later_cs(remove_ovcs, remove_ce->np)) {
+		if (node_overlaps_later_cs(remove_ovcs, remove_ce->np)) {
 			pr_err("overlay #%d is not topmost\n", remove_ovcs->id);
 			return 0;
 		}
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 08/12] of: overlay: loosen overly strict phandle clash check

[frowand.list]

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

When an overlay contains a node that already exists in
the live device tree, the overlay node is not allowed
to change the phandle of the existing node.

The existing check refused to allow an overlay node to
set the node phandle even when the existing node did
not have a phandle.  Relax the check to allow an
overlay node to set the phandle value if the existing
node does not have a phandle.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 015d8b112f60..a0d3222febdc 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -302,10 +302,10 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
 		return build_changeset_next_level(ovcs, tchild, node, 0);
 	}
 
-	if (node->phandle)
-		return -EINVAL;
-
-	ret = build_changeset_next_level(ovcs, tchild, node, 0);
+	if (node->phandle && tchild->phandle)
+		ret = -EINVAL;
+	else
+		ret = build_changeset_next_level(ovcs, tchild, node, 0);
 	of_node_put(tchild);
 
 	return ret;
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[frowand.list]

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

The process of applying an overlay consists of:
  - unflatten an overlay FDT (flattened device tree) into an
    EDT (expanded device tree)
  - fixup the phandle values in the overlay EDT to fit in a
    range above the phandle values in the live device tree
  - create the overlay changeset to reflect the contents of
    the overlay EDT
  - apply the overlay changeset, to modify the live device tree,
    potentially changing the maximum phandle value in the live
    device tree

There is currently no protection against two overlay applies
concurrently determining what range of phandle values are in use
in the live device tree, and subsequently changing that range.
Add a mutex to prevent multiple overlay applies from occurring
simultaneously.

Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
so that the WARN() string will be more easily grepped.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++++++
 drivers/of/overlay.c                         | 22 ++++++++++++++++++++++
 drivers/of/unittest.c                        | 21 +++++++++++++++++++++
 include/linux/of.h                           | 19 +++++++++++++++++++
 4 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index 7a7be0515bfd..c99f7924b1c6 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
 		goto out;
 	}
 
+	/*
+	 * protect from of_resolve_phandles() through of_overlay_apply()
+	 */
+	of_overlay_mutex_lock();
+
 	overlay = tilcdc_get_overlay(&kft);
 	if (!overlay)
 		goto out;
@@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
 		pr_info("%s: ti,tilcdc,slave node successfully converted\n",
 			__func__);
 out:
+	of_overlay_mutex_unlock();
+
 	kfree_table_free(&kft);
 	of_node_put(i2c);
 	of_node_put(slave);
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index a0d3222febdc..4ed372af6ce7 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -71,6 +71,28 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
 		const struct device_node *overlay_node,
 		bool is_symbols_node);
 
+/*
+ * of_resolve_phandles() finds the largest phandle in the live tree.
+ * of_overlay_apply() may add a larger phandle to the live tree.
+ * Do not allow race between two overlays being applied simultaneously:
+ *    mutex_lock(&of_overlay_phandle_mutex)
+ *    of_resolve_phandles()
+ *    of_overlay_apply()
+ *    mutex_unlock(&of_overlay_phandle_mutex)
+ */
+static DEFINE_MUTEX(of_overlay_phandle_mutex);
+
+void of_overlay_mutex_lock(void)
+{
+	mutex_lock(&of_overlay_phandle_mutex);
+}
+
+void of_overlay_mutex_unlock(void)
+{
+	mutex_unlock(&of_overlay_phandle_mutex);
+}
+
+
 static LIST_HEAD(ovcs_list);
 static DEFINE_IDR(ovcs_idr);
 
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index db2f170186de..f4c8aff21320 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -994,9 +994,17 @@ static int __init unittest_data_add(void)
 		return -ENODATA;
 	}
 	of_node_set_flag(unittest_data_node, OF_DETACHED);
+
+	/*
+	 * This lock normally encloses of_overlay_apply() as well as
+	 * of_resolve_phandles().
+	 */
+	of_overlay_mutex_lock();
+
 	rc = of_resolve_phandles(unittest_data_node);
 	if (rc) {
 		pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
+		of_overlay_mutex_unlock();
 		return -EINVAL;
 	}
 
@@ -1006,6 +1014,7 @@ static int __init unittest_data_add(void)
 			__of_attach_node_sysfs(np);
 		of_aliases = of_find_node_by_path("/aliases");
 		of_chosen = of_find_node_by_path("/chosen");
+		of_overlay_mutex_unlock();
 		return 0;
 	}
 
@@ -1018,6 +1027,9 @@ static int __init unittest_data_add(void)
 		attach_node_and_children(np);
 		np = next;
 	}
+
+	of_overlay_mutex_unlock();
+
 	return 0;
 }
 
@@ -2150,9 +2162,12 @@ static int __init overlay_data_add(int onum)
 	}
 	of_node_set_flag(info->np_overlay, OF_DETACHED);
 
+	of_overlay_mutex_lock();
+
 	ret = of_resolve_phandles(info->np_overlay);
 	if (ret) {
 		pr_err("resolve ot phandles (ret=%d), %d\n", ret, onum);
+		of_overlay_mutex_unlock();
 		goto out_free_np_overlay;
 	}
 
@@ -2160,9 +2175,12 @@ static int __init overlay_data_add(int onum)
 	ret = of_overlay_apply(info->np_overlay, &info->overlay_id);
 	if (ret < 0) {
 		pr_err("of_overlay_apply() (ret=%d), %d\n", ret, onum);
+		of_overlay_mutex_unlock();
 		goto out_free_np_overlay;
 	}
 
+	of_overlay_mutex_unlock();
+
 	pr_debug("__dtb_overlay_begin applied, overlay id %d\n", ret);
 
 	goto out;
@@ -2209,7 +2227,10 @@ static __init void of_unittest_overlay_high_level(void)
 	 * Could not fixup phandles in unittest_unflatten_overlay_base()
 	 * because kmalloc() was not yet available.
 	 */
+	of_overlay_mutex_lock();
 	of_resolve_phandles(overlay_base_root);
+	of_overlay_mutex_unlock();
+
 
 	/*
 	 * do not allow overlay_base to duplicate any node already in
diff --git a/include/linux/of.h b/include/linux/of.h
index 49e5f24fb390..eb60eddf83c2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1306,6 +1306,9 @@ struct of_overlay_notify_data {
 #ifdef CONFIG_OF_OVERLAY
 
 /* ID based overlays; the API for external users */
+void of_overlay_mutex_lock(void);
+void of_overlay_mutex_unlock(void);
+
 int of_overlay_apply(struct device_node *tree, int *ovcs_id);
 int of_overlay_remove(int *ovcs_id);
 int of_overlay_remove_all(void);
@@ -1315,6 +1318,22 @@ struct of_overlay_notify_data {
 
 #else
 
+static inline void of_overlay_mutex_lock(void)
+{
+#ifndef CONFIG_OF_RESOLVE
+	/* avoid warning in unittest.c */
+	WARN(1, "of_overlay_mutex_lock() ifdef'ed out\n");
+#endif
+}
+
+static inline void of_overlay_mutex_unlock(void)
+{
+#ifndef CONFIG_OF_RESOLVE
+	/* avoid warning in unittest.c */
+	WARN(1, "of_overlay_mutex_unlock() ifdef'ed out\n");
+#endif
+}
+
 static inline int of_overlay_apply(struct device_node *tree, int *ovcs_id)
 {
 	return -ENOTSUPP;
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 10/12] of: overlay: simplify applying symbols from an overlay

[frowand.list]

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

The code to apply symbols from an overlay to the live device tree
was implemented with the intent to be minimally intrusive on the
existing code.  After recent restructuring of the overlay apply
code, it is easier to disintangle the code that applies the
symbols, and to make the overlay changeset creation code more
straight forward and understandable.

Remove the extra complexity, and make the code more obvious.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 91 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 26 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 4ed372af6ce7..172807d3f375 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -32,21 +32,22 @@
 struct fragment {
 	struct device_node *target;
 	struct device_node *overlay;
-	bool is_symbols_node;
 };
 
 /**
  * struct overlay_changeset
- * @ovcs_list:	list on which we are located
- * @count:	count of @fragments structures
- * @fragments:	info about fragment nodes in overlay expanded device tree
- * @cset:	changeset to apply fragments to live device tree
+ * @ovcs_list:		list on which we are located
+ * @count:		count of fragment structures
+ * @fragments:		fragment nodes in the overlay expanded device tree
+ * @symbols_fragment:	last element of @fragments[] is the  __symbols__ node
+ * @cset:		changeset to apply fragments to live device tree
  */
 struct overlay_changeset {
 	int id;
 	struct list_head ovcs_list;
 	int count;
 	struct fragment *fragments;
+	bool symbols_fragment;
 	struct of_changeset cset;
 };
 
@@ -68,8 +69,7 @@ static int devicetree_corrupt(void)
 
 static int build_changeset_next_level(struct overlay_changeset *ovcs,
 		struct device_node *target_node,
-		const struct device_node *overlay_node,
-		bool is_symbols_node);
+		const struct device_node *overlay_node);
 
 /*
  * of_resolve_phandles() finds the largest phandle in the live tree.
@@ -221,7 +221,7 @@ static struct property *dup_and_fixup_symbol_prop(
  * @ovcs:		overlay changeset
  * @target_node:	where to place @overlay_prop in live tree
  * @overlay_prop:	property to add or update, from overlay tree
- * is_symbols_node:	1 if @target_node is "/__symbols__"
+ * @is_symbols_prop:	1 if @overlay_prop is from node "/__symbols__"
  *
  * If @overlay_prop does not already exist in @target_node, add changeset entry
  * to add @overlay_prop in @target_node, else add changeset entry to update
@@ -237,7 +237,7 @@ static struct property *dup_and_fixup_symbol_prop(
 static int add_changeset_property(struct overlay_changeset *ovcs,
 		struct device_node *target_node,
 		struct property *overlay_prop,
-		bool is_symbols_node)
+		bool is_symbols_prop)
 {
 	struct property *new_prop = NULL, *prop;
 
@@ -248,7 +248,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
 	    !of_prop_cmp(overlay_prop->name, "linux,phandle"))
 		return 0;
 
-	if (is_symbols_node) {
+	if (is_symbols_prop) {
 		if (prop)
 			return -EINVAL;
 		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
@@ -321,13 +321,13 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
 		if (ret)
 			return ret;
 
-		return build_changeset_next_level(ovcs, tchild, node, 0);
+		return build_changeset_next_level(ovcs, tchild, node);
 	}
 
 	if (node->phandle && tchild->phandle)
 		ret = -EINVAL;
 	else
-		ret = build_changeset_next_level(ovcs, tchild, node, 0);
+		ret = build_changeset_next_level(ovcs, tchild, node);
 	of_node_put(tchild);
 
 	return ret;
@@ -338,7 +338,6 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
  * @ovcs:		overlay changeset
  * @target_node:	where to place @overlay_node in live tree
  * @overlay_node:	node from within an overlay device tree fragment
- * @is_symbols_node:	@overlay_node is node "/__symbols__"
  *
  * Add the properties (if any) and nodes (if any) from @overlay_node to the
  * @ovcs->cset changeset.  If an added node has child nodes, they will
@@ -351,16 +350,14 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
  */
 static int build_changeset_next_level(struct overlay_changeset *ovcs,
 		struct device_node *target_node,
-		const struct device_node *overlay_node,
-		bool is_symbols_node)
+		const struct device_node *overlay_node)
 {
 	struct device_node *child;
 	struct property *prop;
 	int ret;
 
 	for_each_property_of_node(overlay_node, prop) {
-		ret = add_changeset_property(ovcs, target_node, prop,
-					     is_symbols_node);
+		ret = add_changeset_property(ovcs, target_node, prop, 0);
 		if (ret) {
 			pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
 				 target_node, prop->name, ret);
@@ -368,9 +365,6 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
 		}
 	}
 
-	if (is_symbols_node)
-		return 0;
-
 	for_each_child_of_node(overlay_node, child) {
 		ret = add_changeset_node(ovcs, target_node, child);
 		if (ret) {
@@ -384,6 +378,28 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
 	return 0;
 }
 
+/*
+ * Add the properties from __overlay__ node to the @ovcs->cset changeset.
+ */
+static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
+		struct device_node *target_node,
+		const struct device_node *overlay_symbols_node)
+{
+	struct property *prop;
+	int ret;
+
+	for_each_property_of_node(overlay_symbols_node, prop) {
+		ret = add_changeset_property(ovcs, target_node, prop, 1);
+		if (ret) {
+			pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
+				 target_node, prop->name, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * build_changeset() - populate overlay changeset in @ovcs from @ovcs->fragments
  * @ovcs:	Overlay changeset
@@ -398,14 +414,33 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
  */
 static int build_changeset(struct overlay_changeset *ovcs)
 {
-	int i, ret;
+	struct fragment *fragment;
+	int fragments_count, i, ret;
 
-	for (i = 0; i < ovcs->count; i++) {
-		struct fragment *fragment = &ovcs->fragments[i];
+	/*
+	 * if there is a symbols fragment in ovcs->fragments[i] it is
+	 * the final element in the array
+	 */
+	if (ovcs->symbols_fragment)
+		fragments_count = ovcs->count - 1;
+	else
+		fragments_count = ovcs->count;
+
+	for (i = 0; i < fragments_count; i++) {
+		fragment = &ovcs->fragments[i];
 
 		ret = build_changeset_next_level(ovcs, fragment->target,
-					       fragment->overlay,
-					       fragment->is_symbols_node);
+						 fragment->overlay);
+		if (ret) {
+			pr_debug("apply failed '%pOF'\n", fragment->target);
+			return ret;
+		}
+	}
+
+	if (ovcs->symbols_fragment) {
+		fragment = &ovcs->fragments[ovcs->count - 1];
+		ret = build_changeset_symbols_node(ovcs, fragment->target,
+						   fragment->overlay);
 		if (ret) {
 			pr_debug("apply failed '%pOF'\n", fragment->target);
 			return ret;
@@ -522,12 +557,16 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 		}
 	}
 
+	/*
+	 * if there is a symbols fragment in ovcs->fragments[i] it is
+	 * the final element in the array
+	 */
 	node = of_get_child_by_name(tree, "__symbols__");
 	if (node) {
+		ovcs->symbols_fragment = 1;
 		fragment = &fragments[cnt];
 		fragment->overlay = node;
 		fragment->target = of_find_node_by_path("/__symbols__");
-		fragment->is_symbols_node = 1;
 
 		if (!fragment->target) {
 			pr_err("no symbols in root of device tree.\n");
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 11/12] of: overlay: remove a dependency on device node full_name

[frowand.list]

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

The "%pOF" printf format was recently added to print the
full name of a device tree node, with the intent of changing
the node full_name field to contain only the node name instead
of the full path of the node.

dup_and_fixup_symbol_prop() duplicates a property from the
"/__symbols__" node of an overlay device tree.  The value
of each duplicated property must be fixed up to include
the full path of a node in the live device tree.  The
current code uses the node's full_name for that purpose.
Update the code to use the "%pOF" printf format to
determine the node's full path.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/base.c       |  2 +-
 drivers/of/of_private.h |  2 ++
 drivers/of/overlay.c    | 90 ++++++++++++++++++++++++++++++-------------------
 3 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 260d33c0f26c..6f91fa67e5bb 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -760,7 +760,7 @@ struct device_node *of_get_child_by_name(const struct device_node *node,
 }
 EXPORT_SYMBOL(of_get_child_by_name);
 
-static struct device_node *__of_find_node_by_path(struct device_node *parent,
+struct device_node *__of_find_node_by_path(struct device_node *parent,
 						const char *path)
 {
 	struct device_node *child;
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index b66e8a812147..0c9e473801f2 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -81,6 +81,8 @@ extern void *__unflatten_device_tree(const void *blob,
 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_find_node_by_path(struct device_node *parent,
+						const char *path);
 struct device_node *__of_find_node_by_full_path(struct device_node *node,
 						const char *path);
 
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 172807d3f375..81881e45f273 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -37,6 +37,7 @@ struct fragment {
 /**
  * struct overlay_changeset
  * @ovcs_list:		list on which we are located
+ * @overlay_tree:	expanded device tree that contains the fragment nodes
  * @count:		count of fragment structures
  * @fragments:		fragment nodes in the overlay expanded device tree
  * @symbols_fragment:	last element of @fragments[] is the  __symbols__ node
@@ -45,6 +46,7 @@ struct fragment {
 struct overlay_changeset {
 	int id;
 	struct list_head ovcs_list;
+	struct device_node *overlay_tree;
 	int count;
 	struct fragment *fragments;
 	bool symbols_fragment;
@@ -145,12 +147,13 @@ static int overlay_notify(struct overlay_changeset *ovcs,
 }
 
 /*
- * The properties in the "/__symbols__" node are "symbols".
+ * The values of properties in the "/__symbols__" node are paths in
+ * the ovcs->overlay_tree.  When duplicating the properties, the paths
+ * need to be adjusted to be the correct path for the live device tree.
  *
- * The value of properties in the "/__symbols__" node is the path of a
- * node in the subtree of a fragment node's "__overlay__" node, for
- * example "/fragment@0/__overlay__/symbol_path_tail".  Symbol_path_tail
- * can be a single node or it may be a multi-node path.
+ * The paths refer to a node in the subtree of a fragment node's "__overlay__"
+ * node, for example "/fragment@0/__overlay__/symbol_path_tail",
+ * where symbol_path_tail can be a single node or it may be a multi-node path.
  *
  * The duplicated property value will be modified by replacing the
  * "/fragment_name/__overlay/" portion of the value  with the target
@@ -160,59 +163,76 @@ static struct property *dup_and_fixup_symbol_prop(
 		struct overlay_changeset *ovcs, const struct property *prop)
 {
 	struct fragment *fragment;
-	struct property *new;
-	const char *overlay_name;
-	char *symbol_path_tail;
-	char *symbol_path;
+	struct property *new_prop;
+	struct device_node *fragment_node;
+	struct device_node *overlay_node;
+	const char *path;
+	const char *path_tail;
 	const char *target_path;
 	int k;
-	int symbol_path_tail_len;
 	int overlay_name_len;
+	int path_len;
+	int path_tail_len;
 	int target_path_len;
 
 	if (!prop->value)
 		return NULL;
-	symbol_path = prop->value;
+	if (strnlen(prop->value, prop->length) >= prop->length)
+		return NULL;
+	path = prop->value;
+	path_len = strlen(path);
 
-	new = kzalloc(sizeof(*new), GFP_KERNEL);
-	if (!new)
+	if (path_len < 1)
 		return NULL;
+	fragment_node = __of_find_node_by_path(ovcs->overlay_tree, path + 1);
+	overlay_node = __of_find_node_by_path(fragment_node, "__overlay__/");
+	of_node_put(fragment_node);
+	of_node_put(overlay_node);
 
 	for (k = 0; k < ovcs->count; k++) {
 		fragment = &ovcs->fragments[k];
-		overlay_name = fragment->overlay->full_name;
-		overlay_name_len = strlen(overlay_name);
-		if (!strncasecmp(symbol_path, overlay_name, overlay_name_len))
+		if (fragment->overlay == overlay_node)
 			break;
 	}
-
 	if (k >= ovcs->count)
-		goto err_free;
+		return NULL;
+
+	overlay_name_len = snprintf(NULL, 0, "%pOF", fragment->overlay);
 
-	target_path = fragment->target->full_name;
+	if (overlay_name_len > path_len)
+		return NULL;
+	path_tail = path + overlay_name_len;
+	path_tail_len = strlen(path_tail);
+
+	target_path = kasprintf(GFP_KERNEL, "%pOF", fragment->target);
+	if (!target_path)
+		return NULL;
 	target_path_len = strlen(target_path);
 
-	symbol_path_tail = symbol_path + overlay_name_len;
-	symbol_path_tail_len = strlen(symbol_path_tail);
+	new_prop = kzalloc(sizeof(*new_prop), GFP_KERNEL);
+	if (!new_prop)
+		goto err_free_target_path;
 
-	new->name = kstrdup(prop->name, GFP_KERNEL);
-	new->length = target_path_len + symbol_path_tail_len + 1;
-	new->value = kzalloc(new->length, GFP_KERNEL);
+	new_prop->name = kstrdup(prop->name, GFP_KERNEL);
+	new_prop->length = target_path_len + path_tail_len + 1;
+	new_prop->value = kzalloc(new_prop->length, GFP_KERNEL);
+	if (!new_prop->name || !new_prop->value)
+		goto err_free_new_prop;
 
-	if (!new->name || !new->value)
-		goto err_free;
+	strcpy(new_prop->value, target_path);
+	strcpy(new_prop->value + target_path_len, path_tail);
 
-	strcpy(new->value, target_path);
-	strcpy(new->value + target_path_len, symbol_path_tail);
+	of_property_set_flag(new_prop, OF_DYNAMIC);
 
-	of_property_set_flag(new, OF_DYNAMIC);
+	return new_prop;
 
-	return new;
+err_free_new_prop:
+	kfree(new_prop->name);
+	kfree(new_prop->value);
+	kfree(new_prop);
+err_free_target_path:
+	kfree(target_path);
 
- err_free:
-	kfree(new->name);
-	kfree(new->value);
-	kfree(new);
 	return NULL;
 }
 
@@ -510,6 +530,8 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	if (!of_node_is_root(tree))
 		pr_debug("%s() tree is not root\n", __func__);
 
+	ovcs->overlay_tree = tree;
+
 	INIT_LIST_HEAD(&ovcs->ovcs_list);
 
 	of_changeset_init(&ovcs->cset);
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 12/12] of: overlay: remove unneeded check for NULL kbasename()

[frowand.list]

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

kbasename() will not return NULL if passed a valid string.  If
the parameter passed to kbasename() in this case is already NULL
then the devicetree has been corrupted.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 81881e45f273..88df2986b03f 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -322,8 +322,6 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
 	int ret = 0;
 
 	node_kbasename = kbasename(node->full_name);
-	if (!node_kbasename)
-		return -ENOMEM;
 
 	for_each_child_of_node(target_node, tchild)
 		if (!of_node_cmp(node_kbasename, kbasename(tchild->full_name)))
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH 00/12] of: overlay: clean up device tree overlay code

[Rob Herring]

On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> I have found the device tree overlay code to be difficult to read and
> maintain.  This patch series attempts to improve that situation.
>
> The cleanup includes some changes visible to users of overlays.  The
> only in kernel user of overlays is fixed up for those changes.  The
> in kernel user is:
>
>    drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c

At what point can we remove this? I'm assuming at some point users
will need to update their dtb's for other reasons and this becomes
obsolete.

Rob

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Rob Herring]

On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> The process of applying an overlay consists of:
>   - unflatten an overlay FDT (flattened device tree) into an
>     EDT (expanded device tree)
>   - fixup the phandle values in the overlay EDT to fit in a
>     range above the phandle values in the live device tree
>   - create the overlay changeset to reflect the contents of
>     the overlay EDT
>   - apply the overlay changeset, to modify the live device tree,
>     potentially changing the maximum phandle value in the live
>     device tree
>
> There is currently no protection against two overlay applies
> concurrently determining what range of phandle values are in use
> in the live device tree, and subsequently changing that range.
> Add a mutex to prevent multiple overlay applies from occurring
> simultaneously.
>
> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
> so that the WARN() string will be more easily grepped.
>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++++++
>  drivers/of/overlay.c                         | 22 ++++++++++++++++++++++
>  drivers/of/unittest.c                        | 21 +++++++++++++++++++++
>  include/linux/of.h                           | 19 +++++++++++++++++++
>  4 files changed, 69 insertions(+)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> index 7a7be0515bfd..c99f7924b1c6 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
>                 goto out;
>         }
>
> +       /*
> +        * protect from of_resolve_phandles() through of_overlay_apply()
> +        */
> +       of_overlay_mutex_lock();
> +

We can't be relying on callers to get the locking right...

>         overlay = tilcdc_get_overlay(&kft);
>         if (!overlay)
>                 goto out;
> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
>                 pr_info("%s: ti,tilcdc,slave node successfully converted\n",
>                         __func__);
>  out:
> +       of_overlay_mutex_unlock();
> +
>         kfree_table_free(&kft);
>         of_node_put(i2c);
>         of_node_put(slave);
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index a0d3222febdc..4ed372af6ce7 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>                 const struct device_node *overlay_node,
>                 bool is_symbols_node);
>
> +/*
> + * of_resolve_phandles() finds the largest phandle in the live tree.
> + * of_overlay_apply() may add a larger phandle to the live tree.
> + * Do not allow race between two overlays being applied simultaneously:
> + *    mutex_lock(&of_overlay_phandle_mutex)
> + *    of_resolve_phandles()
> + *    of_overlay_apply()
> + *    mutex_unlock(&of_overlay_phandle_mutex)

Why do these need to be separate functions? I think I mentioned it
before, but essentially overlay_data_add() should be part of the
overlay API. We may need to allow for callers to do each step, but
generally I think the interface should just be "apply this fdt blob".

Rob

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Frank Rowand]

On 10/04/17 08:19, Rob Herring wrote:
> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> The process of applying an overlay consists of:
>>   - unflatten an overlay FDT (flattened device tree) into an
>>     EDT (expanded device tree)
>>   - fixup the phandle values in the overlay EDT to fit in a
>>     range above the phandle values in the live device tree
>>   - create the overlay changeset to reflect the contents of
>>     the overlay EDT
>>   - apply the overlay changeset, to modify the live device tree,
>>     potentially changing the maximum phandle value in the live
>>     device tree
>>
>> There is currently no protection against two overlay applies
>> concurrently determining what range of phandle values are in use
>> in the live device tree, and subsequently changing that range.
>> Add a mutex to prevent multiple overlay applies from occurring
>> simultaneously.
>>
>> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
>> so that the WARN() string will be more easily grepped.
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++++++
>>  drivers/of/overlay.c                         | 22 ++++++++++++++++++++++
>>  drivers/of/unittest.c                        | 21 +++++++++++++++++++++
>>  include/linux/of.h                           | 19 +++++++++++++++++++
>>  4 files changed, 69 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>> index 7a7be0515bfd..c99f7924b1c6 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
>>                 goto out;
>>         }
>>
>> +       /*
>> +        * protect from of_resolve_phandles() through of_overlay_apply()
>> +        */
>> +       of_overlay_mutex_lock();
>> +
> 
> We can't be relying on callers to get the locking right...

Agreed.


> 
>>         overlay = tilcdc_get_overlay(&kft);
>>         if (!overlay)
>>                 goto out;
>> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
>>                 pr_info("%s: ti,tilcdc,slave node successfully converted\n",
>>                         __func__);
>>  out:
>> +       of_overlay_mutex_unlock();
>> +
>>         kfree_table_free(&kft);
>>         of_node_put(i2c);
>>         of_node_put(slave);
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index a0d3222febdc..4ed372af6ce7 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>                 const struct device_node *overlay_node,
>>                 bool is_symbols_node);
>>
>> +/*
>> + * of_resolve_phandles() finds the largest phandle in the live tree.
>> + * of_overlay_apply() may add a larger phandle to the live tree.
>> + * Do not allow race between two overlays being applied simultaneously:
>> + *    mutex_lock(&of_overlay_phandle_mutex)
>> + *    of_resolve_phandles()
>> + *    of_overlay_apply()
>> + *    mutex_unlock(&of_overlay_phandle_mutex)
> 
> Why do these need to be separate functions? I think I mentioned it
> before, but essentially overlay_data_add() should be part of the
> overlay API. We may need to allow for callers to do each step, but
> generally I think the interface should just be "apply this fdt blob".

Yes, that is where I want to end up.

> 
> Rob
> 

Re: [PATCH 00/12] of: overlay: clean up device tree overlay code

[Tomi Valkeinen]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 04/10/17 17:56, Rob Herring wrote:
> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> I have found the device tree overlay code to be difficult to read and
>> maintain.  This patch series attempts to improve that situation.
>>
>> The cleanup includes some changes visible to users of overlays.  The
>> only in kernel user of overlays is fixed up for those changes.  The
>> in kernel user is:
>>
>>    drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> 
> At what point can we remove this? I'm assuming at some point users
> will need to update their dtb's for other reasons and this becomes
> obsolete.

To be honest, I have no idea, or how to find that out.

Do we need to get rid of it? Afaik, we haven't do much (or any?)
maintenance on tilcdc_slave_compat.c since it was written, so from our
perspective it's been a minimal burden. Is it creating burden for others?

Is the approach done with tilcdc_slave_compat.c something that's not
recommended? I'm sure similar situations happen with other drivers too,
and I think it's a good idea to have a recommended way of keeping
compatibility.

 Tomi

Re: [PATCH 00/12] of: overlay: clean up device tree overlay code

[Jyri Sarha]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 10/05/17 09:53, Tomi Valkeinen wrote:
> On 04/10/17 17:56, Rob Herring wrote:
>> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
>>> From: Frank Rowand <frank.rowand@sony.com>
>>>
>>> I have found the device tree overlay code to be difficult to read and
>>> maintain.  This patch series attempts to improve that situation.
>>>
>>> The cleanup includes some changes visible to users of overlays.  The
>>> only in kernel user of overlays is fixed up for those changes.  The
>>> in kernel user is:
>>>
>>>    drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>
>> At what point can we remove this? I'm assuming at some point users
>> will need to update their dtb's for other reasons and this becomes
>> obsolete.
> 
> To be honest, I have no idea, or how to find that out.
> 

I think the first approach could be setting the DRM_TILCDC_SLAVE_COMPAT
default to n and listen if there is any reports about breakage.

> Do we need to get rid of it? Afaik, we haven't do much (or any?)
> maintenance on tilcdc_slave_compat.c since it was written, so from our
> perspective it's been a minimal burden. Is it creating burden for others?
> 
> Is the approach done with tilcdc_slave_compat.c something that's not
> recommended? I'm sure similar situations happen with other drivers too,
> and I think it's a good idea to have a recommended way of keeping
> compatibility.
> 

For tilcdc I would say that we soon need a similar mechanism to get rid
off tilcdc internal panel driver and to start using generic panel
drivers instead. That is, if we want to keep the kernel compatible with
old devicetree blobs.

Best regards,
Jyri

Re: [PATCH 00/12] of: overlay: clean up device tree overlay code

[Rob Herring]

On Thu, Oct 5, 2017 at 1:53 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
> On 04/10/17 17:56, Rob Herring wrote:
>> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
>>> From: Frank Rowand <frank.rowand@sony.com>
>>>
>>> I have found the device tree overlay code to be difficult to read and
>>> maintain.  This patch series attempts to improve that situation.
>>>
>>> The cleanup includes some changes visible to users of overlays.  The
>>> only in kernel user of overlays is fixed up for those changes.  The
>>> in kernel user is:
>>>
>>>    drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>
>> At what point can we remove this? I'm assuming at some point users
>> will need to update their dtb's for other reasons and this becomes
>> obsolete.
>
> To be honest, I have no idea, or how to find that out.
>
> Do we need to get rid of it? Afaik, we haven't do much (or any?)
> maintenance on tilcdc_slave_compat.c since it was written, so from our
> perspective it's been a minimal burden. Is it creating burden for others?

Well, Frank had to deal with it. It's dealing with DT at a pretty low
level and impacts further clean-ups we want to do.

> Is the approach done with tilcdc_slave_compat.c something that's not
> recommended? I'm sure similar situations happen with other drivers too,
> and I think it's a good idea to have a recommended way of keeping
> compatibility.

If we do want this, I think we need some infrastructure to handle
these fixups and I don't really want to see dts files scattered around
the kernel. Most folks probably just break compatibility which I guess
for some platforms is fine. We do need to support maintaining
compatibility, but for how long depends on the platform.

This was added in 4.2. Looks like 2 boards used the old binding as of
4.1. Here's probably an incomplete list of changes since then:

$ git log v4.2.. --oneline -- arch/arm/boot/dts/am335x-base0033.dts
arch/arm/boot/dts/am335x-igep0033.dtsi
05e7d622f153 ARM: dts: omap: Add generic compatible string for I2C EEPROM
278cb79cc113 ARM: dts: am335x: Add missing unit name to memory nodes
c731abd99121 ARM: dts: am335x/437x/57xx: remove unneeded unit name for
gpio-leds nodes
4c049a5b7c89 ARM: dts: am335x/am437x: remove unneeded unit name for
fixed regulators
42647f947210 ARM: dts: am335x: Update elm phandle binding
63015d73f345 ARM: dts: am335x: Provide NAND ready pin
db0f68529a6a ARM: dts: am335x: Disable wait pin monitoring for NAND
037521483846 ARM: dts: am335x: Fix NAND device nodes
e9a267702d32 ARM: dts: am335x-base0033: Use IOPAD pinmux macro
8a3ecb217f11 ARM: dts: am335x-igep0033: Use IOPAD pinmux macro


And unfortunately, no one ever updated the am335x-base0033.dts to the
new binding. Though it doesn't have an nxp,tda998x node either, so
maybe it is not used? Otherwise, compatibility is probably still
needed since the clock has not even started on that board. Minimally,
this dts should be updated and the kernel should throw a WARN if the
dtb should be updated.


$ git log v4.1.. --oneline --no-merges
arch/arm/boot/dts/am335x-boneblack*
arch/arm/boot/dts/am335x-bone-common.dtsi
7e697ac3c4fb ARM: dts: tps65217: Add power button interrupt to the
common tps65217.dtsi file
6a80131e9dd2 ARM: dts: tps65217: Add charger interrupts to the common
tps65217.dtsi file
05e7d622f153 ARM: dts: omap: Add generic compatible string for I2C EEPROM
c2498af5c036 arm: dts: boneblack-wireless: add WL1835 Bluetooth device node
b9cb2ba71848 ARM: dts: Use - instead of @ for DT OPP entries for TI SoCs
bc4b1736f246 ARM: dts: am335x-boneblack: Enable 1GHz OPP for cpu
d680414d0f42 ARM: dts: Configure BeagleBone peripheral USB VBUS irq
fbb5850bd3c1 ARM: dts: Add am335x-boneblack-wireless
2876cc4a773c ARM: dts: Move most of am335x-boneblack.dts to
am335x-boneblack-common.dtsi
be53e38f0df2 dt-bindings: mfd: Remove TPS65217 interrupts
30aa2e48962c ARM: dts: am335x: Fix the interrupt name of TPS65217
a291b6b3d287 ARM: dts: am335x-boneblack: Add blue-and-red-wiring
-property to LCDC node
17fad5f3ab61 ARM: dts: AM335X-bone-common: Add the internal and
external clock nodes for rtc
eb3e4bbebac4 ARM: dts: am335x: Add the power button interrupt
1934e89a769b ARM: dts: am335x: Add the charger interrupt
2d63b9ce2136 ARM: dts: am335x: Support the PMIC interrupt
e3659351da09 Revert "ARM: dts: am335x-boneblack: Enable 1GHz OPP for cpu"
df0bd1e8f3c5 ARM: dts: am335x-boneblack: Add HDMI audio support
278cb79cc113 ARM: dts: am335x: Add missing unit name to memory nodes
c731abd99121 ARM: dts: am335x/437x/57xx: remove unneeded unit name for
gpio-leds nodes
4c049a5b7c89 ARM: dts: am335x/am437x: remove unneeded unit name for
fixed regulators
f03a881a8a8d ARM: dts: am335x-bone-common: use stdout-path in Beaglebone boards.
fd4eeada1bf1 ARM: dts: am335x-bone-common: Mark MAC as having only one PHY
c36e6ec90487 ARM: dts: am335x-boneblack: Enable 1GHz OPP for cpu
fb515b8e384d ARM: dts: am335x: Update MPU regulator range for TI boards
e327b3f56403 Revert "regulator: tps65217: remove tps65217.dtsi file"
8e6ebfaa9b38 regulator: tps65217: remove tps65217.dtsi file
df10eadfce35 ARM: dts: am335x-boneblack: Use pinctrl constants and
AM33XX_IOPAD macro
e03b2a26d6bf ARM: dts: am335x-bone-common: Use AM33XX_IOPAD pinmux macro
c7ce74bc921b ARM: dts: am335x: fix cd-gpios definition as per hardware
design and dt binding docs
34c900a60bb0 ARM: dts: am335x-boneblack: Use new binding for HDMI
5c250adb51ca Revert "ARM: dts: am335x-boneblack: disable RTC-only sleep"
5d1a2961adf9 ARM: dts: Beaglebone i2c definitions

Looks to me like any user would want to update and have some of these changes.

Rob

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Rob Herring]

On Wed, Oct 04, 2017 at 08:29:59PM -0700, Frank Rowand wrote:
> On 10/04/17 08:19, Rob Herring wrote:
> > On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
> >> From: Frank Rowand <frank.rowand@sony.com>
> >>
> >> The process of applying an overlay consists of:
> >>   - unflatten an overlay FDT (flattened device tree) into an
> >>     EDT (expanded device tree)
> >>   - fixup the phandle values in the overlay EDT to fit in a
> >>     range above the phandle values in the live device tree
> >>   - create the overlay changeset to reflect the contents of
> >>     the overlay EDT
> >>   - apply the overlay changeset, to modify the live device tree,
> >>     potentially changing the maximum phandle value in the live
> >>     device tree
> >>
> >> There is currently no protection against two overlay applies
> >> concurrently determining what range of phandle values are in use
> >> in the live device tree, and subsequently changing that range.
> >> Add a mutex to prevent multiple overlay applies from occurring
> >> simultaneously.
> >>
> >> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
> >> so that the WARN() string will be more easily grepped.
> >>
> >> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> >> ---
> >>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++++++
> >>  drivers/of/overlay.c                         | 22 ++++++++++++++++++++++
> >>  drivers/of/unittest.c                        | 21 +++++++++++++++++++++
> >>  include/linux/of.h                           | 19 +++++++++++++++++++
> >>  4 files changed, 69 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> >> index 7a7be0515bfd..c99f7924b1c6 100644
> >> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> >> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> >> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
> >>                 goto out;
> >>         }
> >>
> >> +       /*
> >> +        * protect from of_resolve_phandles() through of_overlay_apply()
> >> +        */
> >> +       of_overlay_mutex_lock();
> >> +
> > 
> > We can't be relying on callers to get the locking right...
> 
> Agreed.
> 
> 
> > 
> >>         overlay = tilcdc_get_overlay(&kft);
> >>         if (!overlay)
> >>                 goto out;
> >> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
> >>                 pr_info("%s: ti,tilcdc,slave node successfully converted\n",
> >>                         __func__);
> >>  out:
> >> +       of_overlay_mutex_unlock();
> >> +
> >>         kfree_table_free(&kft);
> >>         of_node_put(i2c);
> >>         of_node_put(slave);
> >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >> index a0d3222febdc..4ed372af6ce7 100644
> >> --- a/drivers/of/overlay.c
> >> +++ b/drivers/of/overlay.c
> >> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
> >>                 const struct device_node *overlay_node,
> >>                 bool is_symbols_node);
> >>
> >> +/*
> >> + * of_resolve_phandles() finds the largest phandle in the live tree.
> >> + * of_overlay_apply() may add a larger phandle to the live tree.
> >> + * Do not allow race between two overlays being applied simultaneously:
> >> + *    mutex_lock(&of_overlay_phandle_mutex)
> >> + *    of_resolve_phandles()
> >> + *    of_overlay_apply()
> >> + *    mutex_unlock(&of_overlay_phandle_mutex)
> > 
> > Why do these need to be separate functions? I think I mentioned it
> > before, but essentially overlay_data_add() should be part of the
> > overlay API. We may need to allow for callers to do each step, but
> > generally I think the interface should just be "apply this fdt blob".
> 
> Yes, that is where I want to end up.

So, is that not doable now? To put it another way, why does 
of_resolve_phandles need to be a separate call? Seems like an internal 
detail of how you apply an overlay to me.

Rob

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Frank Rowand]

On 10/10/17 11:40, Rob Herring wrote:
> On Wed, Oct 04, 2017 at 08:29:59PM -0700, Frank Rowand wrote:
>> On 10/04/17 08:19, Rob Herring wrote:
>>> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
>>>> From: Frank Rowand <frank.rowand@sony.com>
>>>>
>>>> The process of applying an overlay consists of:
>>>>   - unflatten an overlay FDT (flattened device tree) into an
>>>>     EDT (expanded device tree)
>>>>   - fixup the phandle values in the overlay EDT to fit in a
>>>>     range above the phandle values in the live device tree
>>>>   - create the overlay changeset to reflect the contents of
>>>>     the overlay EDT
>>>>   - apply the overlay changeset, to modify the live device tree,
>>>>     potentially changing the maximum phandle value in the live
>>>>     device tree
>>>>
>>>> There is currently no protection against two overlay applies
>>>> concurrently determining what range of phandle values are in use
>>>> in the live device tree, and subsequently changing that range.
>>>> Add a mutex to prevent multiple overlay applies from occurring
>>>> simultaneously.
>>>>
>>>> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
>>>> so that the WARN() string will be more easily grepped.
>>>>
>>>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>>>> ---
>>>>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++++++
>>>>  drivers/of/overlay.c                         | 22 ++++++++++++++++++++++
>>>>  drivers/of/unittest.c                        | 21 +++++++++++++++++++++
>>>>  include/linux/of.h                           | 19 +++++++++++++++++++
>>>>  4 files changed, 69 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>> index 7a7be0515bfd..c99f7924b1c6 100644
>>>> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
>>>>                 goto out;
>>>>         }
>>>>
>>>> +       /*
>>>> +        * protect from of_resolve_phandles() through of_overlay_apply()
>>>> +        */
>>>> +       of_overlay_mutex_lock();
>>>> +
>>>
>>> We can't be relying on callers to get the locking right...
>>
>> Agreed.
>>
>>
>>>
>>>>         overlay = tilcdc_get_overlay(&kft);
>>>>         if (!overlay)
>>>>                 goto out;
>>>> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
>>>>                 pr_info("%s: ti,tilcdc,slave node successfully converted\n",
>>>>                         __func__);
>>>>  out:
>>>> +       of_overlay_mutex_unlock();
>>>> +
>>>>         kfree_table_free(&kft);
>>>>         of_node_put(i2c);
>>>>         of_node_put(slave);
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index a0d3222febdc..4ed372af6ce7 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>>>                 const struct device_node *overlay_node,
>>>>                 bool is_symbols_node);
>>>>
>>>> +/*
>>>> + * of_resolve_phandles() finds the largest phandle in the live tree.
>>>> + * of_overlay_apply() may add a larger phandle to the live tree.
>>>> + * Do not allow race between two overlays being applied simultaneously:
>>>> + *    mutex_lock(&of_overlay_phandle_mutex)
>>>> + *    of_resolve_phandles()
>>>> + *    of_overlay_apply()
>>>> + *    mutex_unlock(&of_overlay_phandle_mutex)
>>>
>>> Why do these need to be separate functions? I think I mentioned it
>>> before, but essentially overlay_data_add() should be part of the
>>> overlay API. We may need to allow for callers to do each step, but
>>> generally I think the interface should just be "apply this fdt blob".
>>
>> Yes, that is where I want to end up.
> 
> So, is that not doable now? To put it another way, why does 
> of_resolve_phandles need to be a separate call? Seems like an internal 
> detail of how you apply an overlay to me.
> 
> Rob

Yes, of_resolve_phandles() should become an internal call made from
the "apply this fdt blob" function.

The biggest obstacle is drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
using overlays in a convoluted manner.  The second obstacle will
probably be the older overlay tests in drivers/of/unittest.c.  I
need to look at how to convert them to using actual overlays.

There are other fixes and improvements to the overlay code that
need to occur, but it is like pulling on a loose thread in a
sweater - it just goes on and on.  I'd like to get this set of
patches in, with whatever changes are absolutely essential,
then continue on with more patch sets.  This code will be
much easier for me to modify in the future if this patch set
is applied.

-Frank

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Rob Herring]

On Tue, Oct 10, 2017 at 2:39 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 10/10/17 11:40, Rob Herring wrote:
>> On Wed, Oct 04, 2017 at 08:29:59PM -0700, Frank Rowand wrote:
>>> On 10/04/17 08:19, Rob Herring wrote:
>>>> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
>>>>> From: Frank Rowand <frank.rowand@sony.com>
>>>>>
>>>>> The process of applying an overlay consists of:
>>>>>   - unflatten an overlay FDT (flattened device tree) into an
>>>>>     EDT (expanded device tree)
>>>>>   - fixup the phandle values in the overlay EDT to fit in a
>>>>>     range above the phandle values in the live device tree
>>>>>   - create the overlay changeset to reflect the contents of
>>>>>     the overlay EDT
>>>>>   - apply the overlay changeset, to modify the live device tree,
>>>>>     potentially changing the maximum phandle value in the live
>>>>>     device tree
>>>>>
>>>>> There is currently no protection against two overlay applies
>>>>> concurrently determining what range of phandle values are in use
>>>>> in the live device tree, and subsequently changing that range.
>>>>> Add a mutex to prevent multiple overlay applies from occurring
>>>>> simultaneously.
>>>>>
>>>>> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
>>>>> so that the WARN() string will be more easily grepped.
>>>>>
>>>>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>>>>> ---
>>>>>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++++++
>>>>>  drivers/of/overlay.c                         | 22 ++++++++++++++++++++++
>>>>>  drivers/of/unittest.c                        | 21 +++++++++++++++++++++
>>>>>  include/linux/of.h                           | 19 +++++++++++++++++++
>>>>>  4 files changed, 69 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>>> index 7a7be0515bfd..c99f7924b1c6 100644
>>>>> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>>> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
>>>>>                 goto out;
>>>>>         }
>>>>>
>>>>> +       /*
>>>>> +        * protect from of_resolve_phandles() through of_overlay_apply()
>>>>> +        */
>>>>> +       of_overlay_mutex_lock();
>>>>> +
>>>>
>>>> We can't be relying on callers to get the locking right...
>>>
>>> Agreed.
>>>
>>>
>>>>
>>>>>         overlay = tilcdc_get_overlay(&kft);
>>>>>         if (!overlay)
>>>>>                 goto out;
>>>>> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
>>>>>                 pr_info("%s: ti,tilcdc,slave node successfully converted\n",
>>>>>                         __func__);
>>>>>  out:
>>>>> +       of_overlay_mutex_unlock();
>>>>> +
>>>>>         kfree_table_free(&kft);
>>>>>         of_node_put(i2c);
>>>>>         of_node_put(slave);
>>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>>> index a0d3222febdc..4ed372af6ce7 100644
>>>>> --- a/drivers/of/overlay.c
>>>>> +++ b/drivers/of/overlay.c
>>>>> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>>>>                 const struct device_node *overlay_node,
>>>>>                 bool is_symbols_node);
>>>>>
>>>>> +/*
>>>>> + * of_resolve_phandles() finds the largest phandle in the live tree.
>>>>> + * of_overlay_apply() may add a larger phandle to the live tree.
>>>>> + * Do not allow race between two overlays being applied simultaneously:
>>>>> + *    mutex_lock(&of_overlay_phandle_mutex)
>>>>> + *    of_resolve_phandles()
>>>>> + *    of_overlay_apply()
>>>>> + *    mutex_unlock(&of_overlay_phandle_mutex)
>>>>
>>>> Why do these need to be separate functions? I think I mentioned it
>>>> before, but essentially overlay_data_add() should be part of the
>>>> overlay API. We may need to allow for callers to do each step, but
>>>> generally I think the interface should just be "apply this fdt blob".
>>>
>>> Yes, that is where I want to end up.
>>
>> So, is that not doable now? To put it another way, why does
>> of_resolve_phandles need to be a separate call? Seems like an internal
>> detail of how you apply an overlay to me.
>>
>> Rob
>
> Yes, of_resolve_phandles() should become an internal call made from
> the "apply this fdt blob" function.

I mean just moving of_resolve_phandles into of_overlay_apply. Not the
unflattening too.

> The biggest obstacle is drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> using overlays in a convoluted manner.  The second obstacle will
> probably be the older overlay tests in drivers/of/unittest.c.  I
> need to look at how to convert them to using actual overlays.
>
> There are other fixes and improvements to the overlay code that
> need to occur, but it is like pulling on a loose thread in a
> sweater - it just goes on and on.  I'd like to get this set of
> patches in, with whatever changes are absolutely essential,
> then continue on with more patch sets.  This code will be
> much easier for me to modify in the future if this patch set
> is applied.

AFAICT, I don't think anything between of_resolve_phandles and
of_overlay_apply calls in tilcdc depends on the phandles being fixed
up. And for the unittests that don't call of_resolve_phandles, would
there be any side effect of calling of_resolve_phandles?

Rob

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Frank Rowand]

On 10/10/17 14:06, Rob Herring wrote:
> On Tue, Oct 10, 2017 at 2:39 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 10/10/17 11:40, Rob Herring wrote:
>>> On Wed, Oct 04, 2017 at 08:29:59PM -0700, Frank Rowand wrote:
>>>> On 10/04/17 08:19, Rob Herring wrote:
>>>>> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
>>>>>> From: Frank Rowand <frank.rowand@sony.com>
>>>>>>
>>>>>> The process of applying an overlay consists of:
>>>>>>   - unflatten an overlay FDT (flattened device tree) into an
>>>>>>     EDT (expanded device tree)
>>>>>>   - fixup the phandle values in the overlay EDT to fit in a
>>>>>>     range above the phandle values in the live device tree
>>>>>>   - create the overlay changeset to reflect the contents of
>>>>>>     the overlay EDT
>>>>>>   - apply the overlay changeset, to modify the live device tree,
>>>>>>     potentially changing the maximum phandle value in the live
>>>>>>     device tree
>>>>>>
>>>>>> There is currently no protection against two overlay applies
>>>>>> concurrently determining what range of phandle values are in use
>>>>>> in the live device tree, and subsequently changing that range.
>>>>>> Add a mutex to prevent multiple overlay applies from occurring
>>>>>> simultaneously.
>>>>>>
>>>>>> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
>>>>>> so that the WARN() string will be more easily grepped.
>>>>>>
>>>>>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++++++
>>>>>>  drivers/of/overlay.c                         | 22 ++++++++++++++++++++++
>>>>>>  drivers/of/unittest.c                        | 21 +++++++++++++++++++++
>>>>>>  include/linux/of.h                           | 19 +++++++++++++++++++
>>>>>>  4 files changed, 69 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>>>> index 7a7be0515bfd..c99f7924b1c6 100644
>>>>>> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>>>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>>>> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
>>>>>>                 goto out;
>>>>>>         }
>>>>>>
>>>>>> +       /*
>>>>>> +        * protect from of_resolve_phandles() through of_overlay_apply()
>>>>>> +        */
>>>>>> +       of_overlay_mutex_lock();
>>>>>> +
>>>>>
>>>>> We can't be relying on callers to get the locking right...
>>>>
>>>> Agreed.
>>>>
>>>>
>>>>>
>>>>>>         overlay = tilcdc_get_overlay(&kft);
>>>>>>         if (!overlay)
>>>>>>                 goto out;
>>>>>> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
>>>>>>                 pr_info("%s: ti,tilcdc,slave node successfully converted\n",
>>>>>>                         __func__);
>>>>>>  out:
>>>>>> +       of_overlay_mutex_unlock();
>>>>>> +
>>>>>>         kfree_table_free(&kft);
>>>>>>         of_node_put(i2c);
>>>>>>         of_node_put(slave);
>>>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>>>> index a0d3222febdc..4ed372af6ce7 100644
>>>>>> --- a/drivers/of/overlay.c
>>>>>> +++ b/drivers/of/overlay.c
>>>>>> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>>>>>>                 const struct device_node *overlay_node,
>>>>>>                 bool is_symbols_node);
>>>>>>
>>>>>> +/*
>>>>>> + * of_resolve_phandles() finds the largest phandle in the live tree.
>>>>>> + * of_overlay_apply() may add a larger phandle to the live tree.
>>>>>> + * Do not allow race between two overlays being applied simultaneously:
>>>>>> + *    mutex_lock(&of_overlay_phandle_mutex)
>>>>>> + *    of_resolve_phandles()
>>>>>> + *    of_overlay_apply()
>>>>>> + *    mutex_unlock(&of_overlay_phandle_mutex)
>>>>>
>>>>> Why do these need to be separate functions? I think I mentioned it
>>>>> before, but essentially overlay_data_add() should be part of the
>>>>> overlay API. We may need to allow for callers to do each step, but
>>>>> generally I think the interface should just be "apply this fdt blob".
>>>>
>>>> Yes, that is where I want to end up.
>>>
>>> So, is that not doable now? To put it another way, why does
>>> of_resolve_phandles need to be a separate call? Seems like an internal
>>> detail of how you apply an overlay to me.
>>>
>>> Rob
>>
>> Yes, of_resolve_phandles() should become an internal call made from
>> the "apply this fdt blob" function.
> 
> I mean just moving of_resolve_phandles into of_overlay_apply. Not the
> unflattening too.

OK, I can do that.  I'll send another patch that shows what is involved.


>> The biggest obstacle is drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>> using overlays in a convoluted manner.  The second obstacle will
>> probably be the older overlay tests in drivers/of/unittest.c.  I
>> need to look at how to convert them to using actual overlays.
>>
>> There are other fixes and improvements to the overlay code that
>> need to occur, but it is like pulling on a loose thread in a
>> sweater - it just goes on and on.  I'd like to get this set of
>> patches in, with whatever changes are absolutely essential,
>> then continue on with more patch sets.  This code will be
>> much easier for me to modify in the future if this patch set
>> is applied.
> 
> AFAICT, I don't think anything between of_resolve_phandles and
> of_overlay_apply calls in tilcdc depends on the phandles being fixed
> up.

I started looking at that, then decided that it wasn't worth the
aggravation at the moment.  That use of overlays is fragile and
really needs to go away.  With this latest change on top of all
the others, someone who has that hardware really needs to test
the patches to make sure nothing broke.


> And for the unittests that don't call of_resolve_phandles, would
> there be any side effect of calling of_resolve_phandles?

Yes.  The old style overlay tests do not use true overlays.  Those
tests were created before dtc was able to handle overlays, so they
are somewhat hand crafted.

This has two results.  First, the subtrees that are passed to
of_overlay_apply() are deeper in the tree than a true overlay
would be.  Thus they do not contain the __local_fixups__ node
that would be in a true overlay.  The __local_fixups__ node
is at the root level of the testcases.dtb tree, and fixups
for the entire tree are done in one step, instead of fixing
up each overlay separately.  So when of_resolve_phandles()
is called on each overlay tree, there is no __local_fixups__
node visible, and a second fixup is not attempted.  The
second result is that the overlay trees are not detached,
so I temporarily disabled one the detached check in
of_resolve_phandles() so overlay_apply() will not fail
for these overlays.

I'll have to rework the old style overlay tests to use true
overlays soon.  But that is another activity that I would
like to do as a subsequent step to this patch set.

-Frank

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Frank Rowand]

On 10/10/17 14:06, Rob Herring wrote:
> On Tue, Oct 10, 2017 at 2:39 PM, Frank Rowand <frowand.list@gmail.com> wrote:

< snip >

> 
> AFAICT, I don't think anything between of_resolve_phandles and
> of_overlay_apply calls in tilcdc depends on the phandles being fixed
> up.

I think you are correct, after another quick scan of that code.

< snip >

-Frank

Re: [PATCH 00/12] of: overlay: clean up device tree overlay code

[Frank Rowand]

Hi Rob,

On 10/02/17 20:53, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> I have found the device tree overlay code to be difficult to read and
> maintain.  This patch series attempts to improve that situation.
> 
> The cleanup includes some changes visible to users of overlays.  The
> only in kernel user of overlays is fixed up for those changes.  The
> in kernel user is:
> 
>    drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> 
> Following the cleanup patches are a set of patches to fix various
> issues.
> 
> The first five patches are intended to not make any functional
> changes, and are segrated to ease review.
> 
> Frank Rowand (12):
>   of: overlay.c: Remove comments that state the obvious, to reduce
>     clutter
>   of: overlay.c: Convert comparisons to zero or NULL to logical
>     expressions
>   of: overlay: rename identifiers to more reflect what they do
>   of: overlay: rename identifiers in dup_and_fixup_symbol_prop()
>   of: overlay: minor restructuring
>   of: overlay: detect cases where device tree may become corrupt
>   of: overlay: expand check of whether overlay changeset can be removed
>   of: overlay: loosen overly strict phandle clash check
>   of: overlay: avoid race condition between applying multiple overlays
>   of: overlay: simplify applying symbols from an overlay
>   of: overlay: remove a dependency on device node full_name
>   of: overlay: remove unneeded check for NULL kbasename()
> 
>  Documentation/devicetree/overlay-notes.txt   |   12 +-
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |   15 +-
>  drivers/of/base.c                            |    2 +-
>  drivers/of/dynamic.c                         |  137 +++-
>  drivers/of/of_private.h                      |   10 +-
>  drivers/of/overlay.c                         | 1024 ++++++++++++++++----------
>  drivers/of/unittest.c                        |   80 +-
>  include/linux/of.h                           |   33 +-
>  8 files changed, 871 insertions(+), 442 deletions(-)
> 

What is the status on this series?  Did I resolve all of the issues that
you found?  Is there anything else I need to do?

The last issue that I recall, was a question about "[PATCH 09/12] of: overlay: 
avoid race condition between applying multiple overlays", were you asked if
of_resolve_phandles() could be moved into of_overlay_apply().  I sent an
additional patch "[PATCH] of: overlay: move resolve phandles into
of_overlay_apply()" [1] that applies on top of this series to do so.

There is a trickle of new patches against the same files as in my
series, so I would like to get my series applied sooner that later,
if possible.

Thanks,

Frank

[1] https://lkml.org/lkml/2017/10/10/1389

Re: [PATCH 00/12] of: overlay: clean up device tree overlay code

[Rob Herring]

On Fri, Oct 13, 2017 at 5:03 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> Hi Rob,
>
> On 10/02/17 20:53, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> I have found the device tree overlay code to be difficult to read and
>> maintain.  This patch series attempts to improve that situation.
>>
>> The cleanup includes some changes visible to users of overlays.  The
>> only in kernel user of overlays is fixed up for those changes.  The
>> in kernel user is:
>>
>>    drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>
>> Following the cleanup patches are a set of patches to fix various
>> issues.
>>
>> The first five patches are intended to not make any functional
>> changes, and are segrated to ease review.
>>
>> Frank Rowand (12):
>>   of: overlay.c: Remove comments that state the obvious, to reduce
>>     clutter
>>   of: overlay.c: Convert comparisons to zero or NULL to logical
>>     expressions
>>   of: overlay: rename identifiers to more reflect what they do
>>   of: overlay: rename identifiers in dup_and_fixup_symbol_prop()
>>   of: overlay: minor restructuring
>>   of: overlay: detect cases where device tree may become corrupt
>>   of: overlay: expand check of whether overlay changeset can be removed
>>   of: overlay: loosen overly strict phandle clash check
>>   of: overlay: avoid race condition between applying multiple overlays
>>   of: overlay: simplify applying symbols from an overlay
>>   of: overlay: remove a dependency on device node full_name
>>   of: overlay: remove unneeded check for NULL kbasename()
>>
>>  Documentation/devicetree/overlay-notes.txt   |   12 +-
>>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |   15 +-
>>  drivers/of/base.c                            |    2 +-
>>  drivers/of/dynamic.c                         |  137 +++-
>>  drivers/of/of_private.h                      |   10 +-
>>  drivers/of/overlay.c                         | 1024 ++++++++++++++++----------
>>  drivers/of/unittest.c                        |   80 +-
>>  include/linux/of.h                           |   33 +-
>>  8 files changed, 871 insertions(+), 442 deletions(-)
>>
>
> What is the status on this series?  Did I resolve all of the issues that
> you found?  Is there anything else I need to do?
>
> The last issue that I recall, was a question about "[PATCH 09/12] of: overlay:
> avoid race condition between applying multiple overlays", were you asked if
> of_resolve_phandles() could be moved into of_overlay_apply().  I sent an
> additional patch "[PATCH] of: overlay: move resolve phandles into
> of_overlay_apply()" [1] that applies on top of this series to do so.

Sorry, found my draft on [1] unsent.

> There is a trickle of new patches against the same files as in my
> series, so I would like to get my series applied sooner that later,
> if possible.

I've applied 1-8, but haven't pushed them out. I was hoping to do the
whole series at once.

Rob

Re: [PATCH 00/12] of: overlay: clean up device tree overlay code

[Jyri Sarha]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 10/05/17 11:33, Jyri Sarha wrote:
> On 10/05/17 09:53, Tomi Valkeinen wrote:
>> On 04/10/17 17:56, Rob Herring wrote:
>>> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.list@gmail.com> wrote:
>>>> From: Frank Rowand <frank.rowand@sony.com>
>>>>
>>>> I have found the device tree overlay code to be difficult to read and
>>>> maintain.  This patch series attempts to improve that situation.
>>>>
>>>> The cleanup includes some changes visible to users of overlays.  The
>>>> only in kernel user of overlays is fixed up for those changes.  The
>>>> in kernel user is:
>>>>
>>>>    drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>
>>> At what point can we remove this? I'm assuming at some point users
>>> will need to update their dtb's for other reasons and this becomes
>>> obsolete.
>>
>> To be honest, I have no idea, or how to find that out.
>>
> 
> I think the first approach could be setting the DRM_TILCDC_SLAVE_COMPAT
> default to n and listen if there is any reports about breakage.
> 

After giving it more thought. Maybe we can drop the
DRM_TILCDC_SLAVE_COMPAT in v4.16. 2017 LTS is out already, so there
should be plenty of time for whoever is still using the legacy DTBs to
get rid of them.

>> Do we need to get rid of it? Afaik, we haven't do much (or any?)
>> maintenance on tilcdc_slave_compat.c since it was written, so from our
>> perspective it's been a minimal burden. Is it creating burden for others?
>>
>> Is the approach done with tilcdc_slave_compat.c something that's not
>> recommended? I'm sure similar situations happen with other drivers too,
>> and I think it's a good idea to have a recommended way of keeping
>> compatibility.
>>
> 
> For tilcdc I would say that we soon need a similar mechanism to get rid
> off tilcdc internal panel driver and to start using generic panel
> drivers instead. That is, if we want to keep the kernel compatible with
> old devicetree blobs.
> 

Actually for tilcdc this is not that bad. The messy tilcdc slave
mechanism has been gotten rid of. The rest of tilcdc specific legacy
drivers - the tilcdc legacy panel support and the tilcdc legacy tfp410
driver - do not have any external dependencies and they can basically
remain there for ever for backward compatibility.

But in general, a generic mechanism to translate legacy DTBs to follow a
new binding would be a handy tool in keeping the drivers clean while
keeping up the support for legacy DTBs.

Best regards,
Jyri