On 10/13/18 05:51, Joe Perches wrote:

On Fri, 2018-10-12 at 21:53 -0700, frowand.list@gmail.com wrote:

quoted

From: Frank Rowand <redacted>

Multiple overlay fragments adding or deleting the same node is not
supported.  Replace code comment of such, with check to detect the
attempt and fail the overlay apply.

Devicetree unittest where multiple fragments added the same node was
added in the previous patch in the series.  After applying this patch
the unittest messages will no longer include:

   Duplicate name in motor-1, renamed to "controller#1"
   OF: overlay: of_overlay_apply() err=0
   ### dt-test ### of_overlay_fdt_apply() expected -22, ret=0, overlay_bad_add_dup_node
   ### dt-test ### FAIL of_unittest_overlay_high_level():2419 Adding overlay 'overlay_bad_add_dup_node' failed

   ...

   ### dt-test ### end of unittest - 210 passed, 1 failed

but will instead include:

   OF: overlay: ERROR: multiple overlay fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller

   ...

   ### dt-test ### end of unittest - 211 passed, 0 failed

[]

quoted

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c

[]

quoted

@@ -523,6 +515,54 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
 }
 
 /**
+ * check_changeset_dup_add_node() - changeset validation: duplicate add node
+ * @ovcs:	Overlay changeset
+ *
+ * Check changeset @ovcs->cset for multiple add node entries for the same
+ * node.
+ *
+ * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
+ * invalid overlay in @ovcs->fragments[].
+ */
+static int check_changeset_dup_add_node(struct overlay_changeset *ovcs)
+{
+	struct of_changeset_entry *ce_1, *ce_2;
+	char *fn_1, *fn_2;
+	int name_match;
+
+	list_for_each_entry(ce_1, &ovcs->cset.entries, node) {
+
+		if (ce_1->action == OF_RECONFIG_ATTACH_NODE ||
+		    ce_1->action == OF_RECONFIG_DETACH_NODE) {
+
+			ce_2 = ce_1;
+			list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) {
+				if (ce_2->action == OF_RECONFIG_ATTACH_NODE ||
+				    ce_2->action == OF_RECONFIG_DETACH_NODE) {
+					/* inexpensive name compare */
+					if (!of_node_cmp(ce_1->np->full_name,
+					    ce_2->np->full_name)) {

A bit of odd indentation here.
This line is normally aligned to the second ( on the line above.

Yes, thanks.

quoted

+						/* expensive full path name compare */
+						fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np);
+						fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np);
+						name_match = !strcmp(fn_1, fn_2);
+						kfree(fn_1);
+						kfree(fn_2);
+						if (name_match) {
+							pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n",
+							       ce_1->np);
+							return -EINVAL;
+						}
+					}
+				}
+			}
+		}
+	}
+
+	return 0;
+}

Style trivia:

Using inverted tests and continue would reduce indentation.

Yes, thanks.

-Frank

	list_for_each_entry(ce_1, &ovcs->cset.entries, node) {
		if (ce_1->action != OF_RECONFIG_ATTACH_NODE &&
		    ce_1->action != OF_RECONFIG_DETACH_NODE)
			continue;

		ce_2 = ce_1;
		list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) {
			if (ce_2->action != OF_RECONFIG_ATTACH_NODE &&
			    ce_2->action != OF_RECONFIG_DETACH_NODE)
				continue;

			/* inexpensive name compare */
			if (of_node_cmp(ce_1->np->full_name, ce_2->np->full_name))
				continue;

			/* expensive full path name compare */
			fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np);
			fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np);
			name_match = !strcmp(fn_1, fn_2);
			kfree(fn_1);
			kfree(fn_2);
			if (name_match) {
				pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n",
				       ce_1->np);
				return -EINVAL;
			}
		}
	}