Re: [PATCH v2 12/18] of: overlay: check prevents multiple fragments add or delete same node

[Joe Perches]

On Fri, 2018-10-12 at 21:53 -0700, frowand.list@xxxxxxxxx wrote:
> From: Frank Rowand <frank.rowand@xxxxxxxx>
> 
> 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
[]
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
[]
> @@ -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.

> +						/* 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.

	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;
			}
		}
	}