[PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[Uwe Kleine-König]

A phandle in an overlay is not supposed to overwrite a phandle that
already exists in the base dtb as this breaks references to the
respective node in the base.

So add another iteration over the fdto that checks for such overwrites
and fixes the fdto phandle's value to match the fdt's.

A test is added that checks that newly added phandles and existing
phandles work as expected.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

last year I already tried to address the issue of phandle overwriting in
overlays. See
https://lore.kernel.org/all/20230426100231.484497-1-u.kleine-koenig@pengutronix.de

The approach I took back then required allocating a mapping. This isn't
suiteable for libfdt though because it's expected to be embedded in code
that cannot allocate dynamic memory. So here I implement an algorithm
that doesn't allocate memory at the cost of a higher run time because it
iterates over the whole dtbo for each conflict found. But there isn't a
better way to do it without allocating memory.

Best regards
Uwe

 libfdt/fdt_overlay.c              | 230 ++++++++++++++++++++++++++++++
 tests/overlay_base_phandle.dts    |  21 +++
 tests/overlay_overlay_phandle.dts |  30 ++++
 tests/run_tests.sh                |  23 +++
 4 files changed, 304 insertions(+)
 create mode 100644 tests/overlay_base_phandle.dts
 create mode 100644 tests/overlay_overlay_phandle.dts

diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index 5c0c3981b89d..914acc5b14a6 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -520,6 +520,228 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
 	return 0;
 }
 
+static int overlay_adjust_node_conflicting_phandle(void *fdto, int node,
+						   uint32_t fdt_phandle,
+						   uint32_t fdto_phandle)
+{
+	const fdt32_t *php;
+	int len, ret;
+	int child;
+
+	php = fdt_getprop(fdto, node, "phandle", &len);
+	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
+		ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
+		if (ret)
+			return ret;
+	}
+
+	php = fdt_getprop(fdto, node, "linux,phandle", &len);
+	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
+		ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
+		if (ret)
+			return ret;
+	}
+
+	fdt_for_each_subnode(child, fdto, node) {
+		ret = overlay_adjust_node_conflicting_phandle(fdto, child,
+							      fdt_phandle, fdto_phandle);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int overlay_adjust_local_conflicting_phandle(void *fdto,
+						    uint32_t fdt_phandle,
+						    uint32_t fdto_phandle)
+{
+	return overlay_adjust_node_conflicting_phandle(fdto, 0, fdt_phandle,
+						       fdto_phandle);
+}
+
+static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
+						      int fixup_node,
+						      uint32_t fdt_phandle,
+						      uint32_t fdto_phandle)
+{
+	int fixup_prop;
+	int fixup_child;
+	int ret;
+
+	fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
+
+		const fdt32_t *fixup_val;
+                const char *tree_val;
+                const char *name;
+                int fixup_len;
+                int tree_len;
+                int i;
+
+                fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
+                                                  &name, &fixup_len);
+                if (!fixup_val)
+                        return fixup_len;
+
+                if (fixup_len % sizeof(uint32_t))
+                        return -FDT_ERR_BADOVERLAY;
+                fixup_len /= sizeof(uint32_t);
+
+                tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
+                if (!tree_val) {
+                        if (tree_len == -FDT_ERR_NOTFOUND)
+                                return -FDT_ERR_BADOVERLAY;
+
+                        return tree_len;
+                }
+
+		for (i = 0; i < fixup_len; i++) {
+			fdt32_t adj_val;
+			uint32_t poffset;
+
+			poffset = fdt32_to_cpu(fixup_val[i]);
+
+			/*
+			 * phandles to fixup can be unaligned.
+			 *
+			 * Use a memcpy for the architectures that do
+			 * not support unaligned accesses.
+			 */
+			memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));
+
+			if (fdt32_to_cpu(adj_val) == fdto_phandle) {
+
+				adj_val = cpu_to_fdt32(fdt_phandle);
+
+				ret = fdt_setprop_inplace_namelen_partial(fdto,
+									  tree_node,
+									  name,
+									  strlen(name),
+									  poffset,
+									  &adj_val,
+									  sizeof(adj_val));
+				if (ret == -FDT_ERR_NOSPACE)
+					return -FDT_ERR_BADOVERLAY;
+
+				if (ret)
+					return ret;
+			}
+		}
+	}
+
+	fdt_for_each_subnode(fixup_child, fdto, fixup_node) {
+		const char *fixup_child_name = fdt_get_name(fdto, fixup_child, NULL);
+		int tree_child;
+
+		tree_child = fdt_subnode_offset(fdto, tree_node, fixup_child_name);
+
+		if (tree_child == -FDT_ERR_NOTFOUND)
+			return -FDT_ERR_BADOVERLAY;
+		if (tree_child < 0)
+			return tree_child;
+
+		ret = overlay_update_node_conflicting_references(fdto, tree_child,
+                                                      fixup_child,
+                                                      fdt_phandle,
+                                                      fdto_phandle);
+	}
+
+	return 0;
+}
+
+static int overlay_update_local_conflicting_references(void *fdto,
+						       uint32_t fdt_phandle,
+						       uint32_t fdto_phandle)
+{
+	int fixups;
+
+	fixups = fdt_path_offset(fdto, "/__local_fixups__");
+	if (fixups == -FDT_ERR_NOTFOUND)
+		return 0;
+	if (fixups < 0)
+		return fixups;
+
+	return overlay_update_node_conflicting_references(fdto, 0, fixups,
+							  fdt_phandle,
+							  fdto_phandle);
+}
+
+static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
+						  void *fdto, int fdtonode)
+{
+	uint32_t fdt_phandle, fdto_phandle;
+	int fdtochild;
+
+	fdt_phandle = fdt_get_phandle(fdt, fdtnode);
+	fdto_phandle = fdt_get_phandle(fdto, fdtonode);
+
+	if (fdt_phandle && fdto_phandle) {
+		int ret;
+
+		ret = overlay_adjust_local_conflicting_phandle(fdto,
+							       fdt_phandle,
+							       fdto_phandle);
+		if (ret)
+			return ret;
+
+		ret = overlay_update_local_conflicting_references(fdto,
+								  fdt_phandle,
+								  fdto_phandle);
+		if (ret)
+			return ret;
+	}
+
+	fdt_for_each_subnode(fdtochild, fdto, fdtonode) {
+		const char *name = fdt_get_name(fdto, fdtochild, NULL);
+		int fdtchild;
+		int ret;
+
+		fdtchild = fdt_subnode_offset(fdt, fdtnode, name);
+		if (fdtchild == -FDT_ERR_NOTFOUND)
+			/*
+			 * no further overwrites possible here as this node is
+			 * new
+			 */
+			continue;
+
+		ret = overlay_prevent_phandle_overwrite_node(fdt, fdtchild,
+							     fdto, fdtochild);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int overlay_prevent_phandle_overwrite(void *fdt, void *fdto)
+{
+	int fragment;
+
+	fdt_for_each_subnode(fragment, fdto, 0) {
+		int overlay;
+		int target;
+		int ret;
+
+		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
+		if (overlay == -FDT_ERR_NOTFOUND)
+			continue;
+
+		if (overlay < 0)
+			return overlay;
+
+		target = fdt_overlay_target_offset(fdt, fdto, fragment, NULL);
+		if (target < 0)
+			return target;
+
+		ret = overlay_prevent_phandle_overwrite_node(fdt, target,
+							     fdto, overlay);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  * overlay_apply_node - Merges a node into the base device tree
  * @fdt: Base Device Tree blob
@@ -824,18 +1046,26 @@ int fdt_overlay_apply(void *fdt, void *fdto)
 	if (ret)
 		goto err;
 
+	/* Increase all phandles in the fdto by delta */
 	ret = overlay_adjust_local_phandles(fdto, delta);
 	if (ret)
 		goto err;
 
+	/* Adapt the phandle values in fdto to the above increase */
 	ret = overlay_update_local_references(fdto, delta);
 	if (ret)
 		goto err;
 
+	/* Update fdto's phandles using symbols from fdt */
 	ret = overlay_fixup_phandles(fdt, fdto);
 	if (ret)
 		goto err;
 
+	/* Don't overwrite phandles in fdt */
+	ret = overlay_prevent_phandle_overwrite(fdt, fdto);
+	if (ret)
+		goto err;
+
 	ret = overlay_merge(fdt, fdto);
 	if (ret)
 		goto err;
diff --git a/tests/overlay_base_phandle.dts b/tests/overlay_base_phandle.dts
new file mode 100644
index 000000000000..36f013c772a2
--- /dev/null
+++ b/tests/overlay_base_phandle.dts
@@ -0,0 +1,21 @@
+/*
+ * Copyright (c) 2024 Uwe Kleine-König
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+
+/ {
+	node_a: a {
+		value = <17>;
+	};
+
+	node_b: b {
+		a = <&node_a>;
+	};
+
+	node_c: c {
+		b = <&node_b>;
+	};
+};
diff --git a/tests/overlay_overlay_phandle.dts b/tests/overlay_overlay_phandle.dts
new file mode 100644
index 000000000000..218e5266b992
--- /dev/null
+++ b/tests/overlay_overlay_phandle.dts
@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2024 Uwe Kleine-König
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+/plugin/;
+
+&{/} {
+	/* /a already has a label "node_a" in the base dts */
+	node_b2: b {
+		value = <42>;
+		c = <&node_c>;
+	};
+
+	c {
+		a = <&node_a>;
+		d = <&node_d>;
+	};
+
+	node_d: d {
+		value = <23>;
+		b = <&node_b2>;
+	};
+};
+
+&node_a {
+	value = <32>;
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 3225a12bbb06..256472b623a3 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -1040,6 +1040,28 @@ fdtoverlay_tests() {
     run_dtc_test -@ -I dts -O dtb -o $stacked_addlabeldtb $stacked_addlabel
 
     run_fdtoverlay_test baz "/foonode/barnode/baznode" "baz-property" "-ts" ${stacked_base_nolabeldtb} ${stacked_addlabel_targetdtb} ${stacked_addlabeldtb} ${stacked_bardtb} ${stacked_bazdtb}
+
+    # verify that labels are not overwritten
+    run_dtc_test -@ -I dts -O dtb -o overlay_base_phandle.test.dtb "$SRCDIR/overlay_base_phandle.dts"
+    run_dtc_test -@ -I dts -O dtb -o overlay_overlay_phandle.test.dtb "$SRCDIR/overlay_overlay_phandle.dts"
+    run_wrap_test $FDTOVERLAY -i overlay_base_phandle.test.dtb -o overlay_base_phandleO.test.dtb overlay_overlay_phandle.test.dtb
+
+    pa=$($DTGET overlay_base_phandleO.test.dtb /a phandle)
+    pb=$($DTGET overlay_base_phandleO.test.dtb /b phandle)
+    pc=$($DTGET overlay_base_phandleO.test.dtb /c phandle)
+    pd=$($DTGET overlay_base_phandleO.test.dtb /d phandle)
+    ba=$($DTGET overlay_base_phandleO.test.dtb /b a)
+    bc=$($DTGET overlay_base_phandleO.test.dtb /b c)
+    ca=$($DTGET overlay_base_phandleO.test.dtb /c a)
+    cb=$($DTGET overlay_base_phandleO.test.dtb /c b)
+    cd=$($DTGET overlay_base_phandleO.test.dtb /c d)
+    db=$($DTGET overlay_base_phandleO.test.dtb /d b)
+    shorten_echo "check phandle wasn't overwritten:	"
+    if test "$ba-$bc-$ca-$cb-$cd-$db" = "$pa-$pc-$pa-$pb-$pd-$pb"; then
+	    PASS
+    else
+	    FAIL
+    fi
 }
 
 pylibfdt_tests () {
-- 
2.43.0

Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3739 bytes --]

Hello,

On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote:
> A phandle in an overlay is not supposed to overwrite a phandle that
> already exists in the base dtb as this breaks references to the
> respective node in the base.
> 
> So add another iteration over the fdto that checks for such overwrites
> and fixes the fdto phandle's value to match the fdt's.
> 
> A test is added that checks that newly added phandles and existing
> phandles work as expected.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

After sending the patch out I had a nice idea for an optimisation here.
It doesn't make the process faster, but improves the result.

If phandle with the (shifted) value 17 is changed to 5, there is no
phandle with the value 17 in the end. So while iterating over the fdto
to replace all phandles with value 17 by 5 all values > 17 can be
reduced by one. This way the phandle allocation in the resulting dtb is
continuous if both inputs have continuous numbers.

This can be implemented by the patch below on top of the patch I'm
replying to.

Best regards
Uwe

diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index 914acc5b14a6..bfdba50dee50 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -529,15 +529,25 @@ static int overlay_adjust_node_conflicting_phandle(void *fdto, int node,
 	int child;
 
 	php = fdt_getprop(fdto, node, "phandle", &len);
-	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
-		ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
+	if (php && len == sizeof(*php)) {
+		if (fdt32_to_cpu(*php) == fdto_phandle)
+			ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
+		else if (fdt32_to_cpu(*php) > fdto_phandle)
+			ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1);
+		else
+			ret = 0;
 		if (ret)
 			return ret;
 	}
 
 	php = fdt_getprop(fdto, node, "linux,phandle", &len);
-	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
-		ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
+	if (php && len == sizeof(*php)) {
+		if (fdt32_to_cpu(*php) == fdto_phandle)
+			ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
+		else if (fdt32_to_cpu(*php) > fdto_phandle)
+			ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1);
+		else
+			ret = 0;
 		if (ret)
 			return ret;
 	}
@@ -609,23 +619,27 @@ static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
 			 */
 			memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));
 
-			if (fdt32_to_cpu(adj_val) == fdto_phandle) {
+			if (fdt32_to_cpu(adj_val) < fdto_phandle)
+				continue;
 
+			if (fdt32_to_cpu(adj_val) == fdto_phandle)
 				adj_val = cpu_to_fdt32(fdt_phandle);
+			else
+				adj_val = cpu_to_fdt32(fdt32_to_cpu(adj_val) - 1);
 
-				ret = fdt_setprop_inplace_namelen_partial(fdto,
-									  tree_node,
-									  name,
-									  strlen(name),
-									  poffset,
-									  &adj_val,
-									  sizeof(adj_val));
-				if (ret == -FDT_ERR_NOSPACE)
-					return -FDT_ERR_BADOVERLAY;
 
-				if (ret)
-					return ret;
-			}
+			ret = fdt_setprop_inplace_namelen_partial(fdto,
+								  tree_node,
+								  name,
+								  strlen(name),
+								  poffset,
+								  &adj_val,
+								  sizeof(adj_val));
+			if (ret == -FDT_ERR_NOSPACE)
+				return -FDT_ERR_BADOVERLAY;
+
+			if (ret)
+				return ret;
 		}
 	}
 
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 14014 bytes --]

On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote:
> A phandle in an overlay is not supposed to overwrite a phandle that
> already exists in the base dtb as this breaks references to the
> respective node in the base.
> 
> So add another iteration over the fdto that checks for such overwrites
> and fixes the fdto phandle's value to match the fdt's.
> 
> A test is added that checks that newly added phandles and existing
> phandles work as expected.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> last year I already tried to address the issue of phandle overwriting in
> overlays. See
> https://lore.kernel.org/all/20230426100231.484497-1-u.kleine-koenig@pengutronix.de
> 
> The approach I took back then required allocating a mapping. This isn't
> suiteable for libfdt though because it's expected to be embedded in code
> that cannot allocate dynamic memory. So here I implement an algorithm
> that doesn't allocate memory at the cost of a higher run time because it
> iterates over the whole dtbo for each conflict found. But there isn't a
> better way to do it without allocating memory.

Thanks for following this up.  I think I finally got my head around
what's going on here.  I really wish there were a more elegant way of
dealing with it, but I don't think there is.  That said, there are
some smaller improvements that I think can be made, detailed comments
below.

>  libfdt/fdt_overlay.c              | 230 ++++++++++++++++++++++++++++++
>  tests/overlay_base_phandle.dts    |  21 +++
>  tests/overlay_overlay_phandle.dts |  30 ++++
>  tests/run_tests.sh                |  23 +++
>  4 files changed, 304 insertions(+)
>  create mode 100644 tests/overlay_base_phandle.dts
>  create mode 100644 tests/overlay_overlay_phandle.dts
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 5c0c3981b89d..914acc5b14a6 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -520,6 +520,228 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
>  	return 0;
>  }
>  

All of these new functions could do with descriptive comments saying
what they do (see the existing code for examples).

> +static int overlay_adjust_node_conflicting_phandle(void *fdto, int node,
> +						   uint32_t fdt_phandle,
> +						   uint32_t fdto_phandle)
> +{
> +	const fdt32_t *php;
> +	int len, ret;
> +	int child;
> +
> +	php = fdt_getprop(fdto, node, "phandle", &len);
> +	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
> +		ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	php = fdt_getprop(fdto, node, "linux,phandle", &len);
> +	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
> +		ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(child, fdto, node) {
> +		ret = overlay_adjust_node_conflicting_phandle(fdto, child,
> +							      fdt_phandle, fdto_phandle);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int overlay_adjust_local_conflicting_phandle(void *fdto,
> +						    uint32_t fdt_phandle,
> +						    uint32_t fdto_phandle)
> +{
> +	return overlay_adjust_node_conflicting_phandle(fdto, 0, fdt_phandle,
> +						       fdto_phandle);
> +}

I don't believe you need the above two functions: see further comments
at the call site.

> +static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
> +						      int fixup_node,
> +						      uint32_t fdt_phandle,
> +						      uint32_t fdto_phandle)
> +{
> +	int fixup_prop;
> +	int fixup_child;
> +	int ret;
> +
> +	fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
> +

Extraneous blank line.

> +		const fdt32_t *fixup_val;
> +                const char *tree_val;

libfdt style is to use tab indents, looks like most of this block has
space indents instead.

> +                const char *name;
> +                int fixup_len;
> +                int tree_len;
> +                int i;
> +
> +                fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
> +                                                  &name, &fixup_len);
> +                if (!fixup_val)
> +                        return fixup_len;
> +
> +                if (fixup_len % sizeof(uint32_t))
> +                        return -FDT_ERR_BADOVERLAY;
> +                fixup_len /= sizeof(uint32_t);
> +
> +                tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> +                if (!tree_val) {
> +                        if (tree_len == -FDT_ERR_NOTFOUND)
> +                                return -FDT_ERR_BADOVERLAY;
> +
> +                        return tree_len;
> +                }
> +
> +		for (i = 0; i < fixup_len; i++) {
> +			fdt32_t adj_val;
> +			uint32_t poffset;
> +
> +			poffset = fdt32_to_cpu(fixup_val[i]);

You can use fdt32_ld_() here to slightly simplify this

> +
> +			/*
> +			 * phandles to fixup can be unaligned.
> +			 *
> +			 * Use a memcpy for the architectures that do
> +			 * not support unaligned accesses.
> +			 */
> +			memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));

And here you can use fdt32_ld() which has unaligned access handling
built in.

> +
> +			if (fdt32_to_cpu(adj_val) == fdto_phandle) {
> +
> +				adj_val = cpu_to_fdt32(fdt_phandle);
> +
> +				ret = fdt_setprop_inplace_namelen_partial(fdto,
> +									  tree_node,
> +									  name,
> +									  strlen(name),
> +									  poffset,
> +									  &adj_val,
> +									  sizeof(adj_val));
> +				if (ret == -FDT_ERR_NOSPACE)
> +					return -FDT_ERR_BADOVERLAY;
> +
> +				if (ret)
> +					return ret;

And here you can use fdt32_st() on (tree_val + poffset), avoiding
quite some complexity.

> +			}
> +		}
> +	}
> +
> +	fdt_for_each_subnode(fixup_child, fdto, fixup_node) {
> +		const char *fixup_child_name = fdt_get_name(fdto, fixup_child, NULL);
> +		int tree_child;
> +
> +		tree_child = fdt_subnode_offset(fdto, tree_node, fixup_child_name);
> +
> +		if (tree_child == -FDT_ERR_NOTFOUND)
> +			return -FDT_ERR_BADOVERLAY;
> +		if (tree_child < 0)
> +			return tree_child;
> +
> +		ret = overlay_update_node_conflicting_references(fdto, tree_child,
> +                                                      fixup_child,
> +                                                      fdt_phandle,
> +                                                      fdto_phandle);
> +	}
> +
> +	return 0;
> +}
> +
> +static int overlay_update_local_conflicting_references(void *fdto,
> +						       uint32_t fdt_phandle,
> +						       uint32_t fdto_phandle)
> +{
> +	int fixups;
> +
> +	fixups = fdt_path_offset(fdto, "/__local_fixups__");
> +	if (fixups == -FDT_ERR_NOTFOUND)
> +		return 0;
> +	if (fixups < 0)
> +		return fixups;
> +
> +	return overlay_update_node_conflicting_references(fdto, 0, fixups,
> +							  fdt_phandle,
> +							  fdto_phandle);
> +}
> +
> +static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
> +						  void *fdto, int fdtonode)

Maybe "overlay_resolve_phandle_conflict()" for a slightly shorter
function name.

> +{
> +	uint32_t fdt_phandle, fdto_phandle;
> +	int fdtochild;
> +
> +	fdt_phandle = fdt_get_phandle(fdt, fdtnode);
> +	fdto_phandle = fdt_get_phandle(fdto, fdtonode);
> +
> +	if (fdt_phandle && fdto_phandle) {
> +		int ret;
> +
> +		ret = overlay_adjust_local_conflicting_phandle(fdto,
> +							       fdt_phandle,
> +							       fdto_phandle);
> +		if (ret)
> +			return ret;

So while, as you've seen, there can be conflicting phandles between
the fdt and the fdto, within the fdto a particular phandle should only
appear on a single node.  If a phandle is duplicated across nodes
witin a single fdto, you have bigger problems.  Therefore, having
found a conflict, you're already found the only place you need to
change the phandle property itself - you don't need to scan the full
fdto again.

So I believe you can replace the above call with just setting the
phandle property on fdtonode.

> +
> +		ret = overlay_update_local_conflicting_references(fdto,
> +								  fdt_phandle,
> +								  fdto_phandle);
> +		if (ret)
> +			return ret;

You do, of course, have to scan the whole fdto to update references.

> +	}
> +
> +	fdt_for_each_subnode(fdtochild, fdto, fdtonode) {
> +		const char *name = fdt_get_name(fdto, fdtochild, NULL);
> +		int fdtchild;
> +		int ret;
> +
> +		fdtchild = fdt_subnode_offset(fdt, fdtnode, name);
> +		if (fdtchild == -FDT_ERR_NOTFOUND)
> +			/*
> +			 * no further overwrites possible here as this node is
> +			 * new
> +			 */
> +			continue;
> +
> +		ret = overlay_prevent_phandle_overwrite_node(fdt, fdtchild,
> +							     fdto, fdtochild);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int overlay_prevent_phandle_overwrite(void *fdt, void *fdto)
> +{
> +	int fragment;
> +
> +	fdt_for_each_subnode(fragment, fdto, 0) {
> +		int overlay;
> +		int target;
> +		int ret;
> +
> +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +		if (overlay == -FDT_ERR_NOTFOUND)
> +			continue;
> +
> +		if (overlay < 0)
> +			return overlay;
> +
> +		target = fdt_overlay_target_offset(fdt, fdto, fragment, NULL);
> +		if (target < 0)
> +			return target;
> +
> +		ret = overlay_prevent_phandle_overwrite_node(fdt, target,
> +							     fdto, overlay);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * overlay_apply_node - Merges a node into the base device tree
>   * @fdt: Base Device Tree blob
> @@ -824,18 +1046,26 @@ int fdt_overlay_apply(void *fdt, void *fdto)
>  	if (ret)
>  		goto err;
>  
> +	/* Increase all phandles in the fdto by delta */
>  	ret = overlay_adjust_local_phandles(fdto, delta);
>  	if (ret)
>  		goto err;
>  
> +	/* Adapt the phandle values in fdto to the above increase */
>  	ret = overlay_update_local_references(fdto, delta);
>  	if (ret)
>  		goto err;
>  
> +	/* Update fdto's phandles using symbols from fdt */
>  	ret = overlay_fixup_phandles(fdt, fdto);
>  	if (ret)
>  		goto err;
>  
> +	/* Don't overwrite phandles in fdt */
> +	ret = overlay_prevent_phandle_overwrite(fdt, fdto);
> +	if (ret)
> +		goto err;

I think this should go above overlay_fixup_phandles(), so that we're
completing all the __local_fixups__ based adjustments before going to
the __fixups__ based adjustments.

> +
>  	ret = overlay_merge(fdt, fdto);
>  	if (ret)
>  		goto err;
> diff --git a/tests/overlay_base_phandle.dts b/tests/overlay_base_phandle.dts
> new file mode 100644
> index 000000000000..36f013c772a2
> --- /dev/null
> +++ b/tests/overlay_base_phandle.dts
> @@ -0,0 +1,21 @@
> +/*
> + * Copyright (c) 2024 Uwe Kleine-König
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +	node_a: a {
> +		value = <17>;
> +	};
> +
> +	node_b: b {
> +		a = <&node_a>;
> +	};
> +
> +	node_c: c {
> +		b = <&node_b>;
> +	};
> +};
> diff --git a/tests/overlay_overlay_phandle.dts b/tests/overlay_overlay_phandle.dts
> new file mode 100644
> index 000000000000..218e5266b992
> --- /dev/null
> +++ b/tests/overlay_overlay_phandle.dts
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright (c) 2024 Uwe Kleine-König
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +&{/} {
> +	/* /a already has a label "node_a" in the base dts */
> +	node_b2: b {
> +		value = <42>;
> +		c = <&node_c>;
> +	};
> +
> +	c {
> +		a = <&node_a>;
> +		d = <&node_d>;
> +	};
> +
> +	node_d: d {
> +		value = <23>;
> +		b = <&node_b2>;
> +	};
> +};
> +
> +&node_a {
> +	value = <32>;
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 3225a12bbb06..256472b623a3 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -1040,6 +1040,28 @@ fdtoverlay_tests() {
>      run_dtc_test -@ -I dts -O dtb -o $stacked_addlabeldtb $stacked_addlabel
>  
>      run_fdtoverlay_test baz "/foonode/barnode/baznode" "baz-property" "-ts" ${stacked_base_nolabeldtb} ${stacked_addlabel_targetdtb} ${stacked_addlabeldtb} ${stacked_bardtb} ${stacked_bazdtb}
> +
> +    # verify that labels are not overwritten
> +    run_dtc_test -@ -I dts -O dtb -o overlay_base_phandle.test.dtb "$SRCDIR/overlay_base_phandle.dts"
> +    run_dtc_test -@ -I dts -O dtb -o overlay_overlay_phandle.test.dtb "$SRCDIR/overlay_overlay_phandle.dts"
> +    run_wrap_test $FDTOVERLAY -i overlay_base_phandle.test.dtb -o overlay_base_phandleO.test.dtb overlay_overlay_phandle.test.dtb
> +
> +    pa=$($DTGET overlay_base_phandleO.test.dtb /a phandle)
> +    pb=$($DTGET overlay_base_phandleO.test.dtb /b phandle)
> +    pc=$($DTGET overlay_base_phandleO.test.dtb /c phandle)
> +    pd=$($DTGET overlay_base_phandleO.test.dtb /d phandle)
> +    ba=$($DTGET overlay_base_phandleO.test.dtb /b a)
> +    bc=$($DTGET overlay_base_phandleO.test.dtb /b c)
> +    ca=$($DTGET overlay_base_phandleO.test.dtb /c a)
> +    cb=$($DTGET overlay_base_phandleO.test.dtb /c b)
> +    cd=$($DTGET overlay_base_phandleO.test.dtb /c d)
> +    db=$($DTGET overlay_base_phandleO.test.dtb /d b)
> +    shorten_echo "check phandle wasn't overwritten:	"
> +    if test "$ba-$bc-$ca-$cb-$cd-$db" = "$pa-$pc-$pa-$pb-$pd-$pb"; then

I'd prefer to see each of these pairs as a separate test case.

> +	    PASS
> +    else
> +	    FAIL
> +    fi
>  }
>  
>  pylibfdt_tests () {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4191 bytes --]

On Fri, Feb 16, 2024 at 10:06:37PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote:
> > A phandle in an overlay is not supposed to overwrite a phandle that
> > already exists in the base dtb as this breaks references to the
> > respective node in the base.
> > 
> > So add another iteration over the fdto that checks for such overwrites
> > and fixes the fdto phandle's value to match the fdt's.
> > 
> > A test is added that checks that newly added phandles and existing
> > phandles work as expected.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> After sending the patch out I had a nice idea for an optimisation here.
> It doesn't make the process faster, but improves the result.
> 
> If phandle with the (shifted) value 17 is changed to 5, there is no
> phandle with the value 17 in the end. So while iterating over the fdto
> to replace all phandles with value 17 by 5 all values > 17 can be
> reduced by one. This way the phandle allocation in the resulting dtb is
> continuous if both inputs have continuous numbers.

Honestly, I don't think the marginal advantage of improving the
chances of contiguous phandles is worth the extra complexity.

> 
> This can be implemented by the patch below on top of the patch I'm
> replying to.
> 
> Best regards
> Uwe
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 914acc5b14a6..bfdba50dee50 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -529,15 +529,25 @@ static int overlay_adjust_node_conflicting_phandle(void *fdto, int node,
>  	int child;
>  
>  	php = fdt_getprop(fdto, node, "phandle", &len);
> -	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
> -		ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
> +	if (php && len == sizeof(*php)) {
> +		if (fdt32_to_cpu(*php) == fdto_phandle)
> +			ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
> +		else if (fdt32_to_cpu(*php) > fdto_phandle)
> +			ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1);
> +		else
> +			ret = 0;
>  		if (ret)
>  			return ret;
>  	}
>  
>  	php = fdt_getprop(fdto, node, "linux,phandle", &len);
> -	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
> -		ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
> +	if (php && len == sizeof(*php)) {
> +		if (fdt32_to_cpu(*php) == fdto_phandle)
> +			ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
> +		else if (fdt32_to_cpu(*php) > fdto_phandle)
> +			ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1);
> +		else
> +			ret = 0;
>  		if (ret)
>  			return ret;
>  	}
> @@ -609,23 +619,27 @@ static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
>  			 */
>  			memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));
>  
> -			if (fdt32_to_cpu(adj_val) == fdto_phandle) {
> +			if (fdt32_to_cpu(adj_val) < fdto_phandle)
> +				continue;
>  
> +			if (fdt32_to_cpu(adj_val) == fdto_phandle)
>  				adj_val = cpu_to_fdt32(fdt_phandle);
> +			else
> +				adj_val = cpu_to_fdt32(fdt32_to_cpu(adj_val) - 1);
>  
> -				ret = fdt_setprop_inplace_namelen_partial(fdto,
> -									  tree_node,
> -									  name,
> -									  strlen(name),
> -									  poffset,
> -									  &adj_val,
> -									  sizeof(adj_val));
> -				if (ret == -FDT_ERR_NOSPACE)
> -					return -FDT_ERR_BADOVERLAY;
>  
> -				if (ret)
> -					return ret;
> -			}
> +			ret = fdt_setprop_inplace_namelen_partial(fdto,
> +								  tree_node,
> +								  name,
> +								  strlen(name),
> +								  poffset,
> +								  &adj_val,
> +								  sizeof(adj_val));
> +			if (ret == -FDT_ERR_NOSPACE)
> +				return -FDT_ERR_BADOVERLAY;
> +
> +			if (ret)
> +				return ret;
>  		}
>  	}
>  



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4605 bytes --]

Hello David,

On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote:
> On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote:
> > [...]
> > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> > index 5c0c3981b89d..914acc5b14a6 100644
> > --- a/libfdt/fdt_overlay.c
> > +++ b/libfdt/fdt_overlay.c
> > @@ -520,6 +520,228 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
> >  	return 0;
> >  }
> >  
> 
> All of these new functions could do with descriptive comments saying
> what they do (see the existing code for examples).

Yeah, I was lazy. I didn't expect this patch to go in without comments
and avoided investing time documenting functions that maybe will change
in the course of the review-resend loop. :-)

> > [...]
> > +static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
> > +						      int fixup_node,
> > +						      uint32_t fdt_phandle,
> > +						      uint32_t fdto_phandle)
> > +{
> > +	int fixup_prop;
> > +	int fixup_child;
> > +	int ret;
> > +
> > +	fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
> > +
> 
> Extraneous blank line.
> 
> > +		const fdt32_t *fixup_val;
> > +                const char *tree_val;
> 
> libfdt style is to use tab indents, looks like most of this block has
> space indents instead.

Huh, I'm surprised about myself that I didn't notice this alone.

> > +
> > +			/*
> > +			 * phandles to fixup can be unaligned.
> > +			 *
> > +			 * Use a memcpy for the architectures that do
> > +			 * not support unaligned accesses.
> > +			 */
> > +			memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));
> 
> And here you can use fdt32_ld() which has unaligned access handling
> built in.

I copied this from somewhere, so I guess there is some potential for
improvement in existing code. Will take a look.
 
> > [...]
> > +
> > +static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
> > +						  void *fdto, int fdtonode)
> 
> Maybe "overlay_resolve_phandle_conflict()" for a slightly shorter
> function name.

Sounds good, wasn't too lucky with
overlay_prevent_phandle_overwrite_node() either.
 
> > +{
> > +	uint32_t fdt_phandle, fdto_phandle;
> > +	int fdtochild;
> > +
> > +	fdt_phandle = fdt_get_phandle(fdt, fdtnode);
> > +	fdto_phandle = fdt_get_phandle(fdto, fdtonode);
> > +
> > +	if (fdt_phandle && fdto_phandle) {
> > +		int ret;
> > +
> > +		ret = overlay_adjust_local_conflicting_phandle(fdto,
> > +							       fdt_phandle,
> > +							       fdto_phandle);
> > +		if (ret)
> > +			return ret;
> 
> So while, as you've seen, there can be conflicting phandles between
> the fdt and the fdto, within the fdto a particular phandle should only
> appear on a single node.  If a phandle is duplicated across nodes
> witin a single fdto, you have bigger problems.  Therefore, having
> found a conflict, you're already found the only place you need to
> change the phandle property itself - you don't need to scan the full
> fdto again.
> 
> So I believe you can replace the above call with just setting the
> phandle property on fdtonode.

Oh right!
 
> > [...]
> >  /**
> >   * overlay_apply_node - Merges a node into the base device tree
> >   * @fdt: Base Device Tree blob
> > @@ -824,18 +1046,26 @@ int fdt_overlay_apply(void *fdt, void *fdto)
> >  	if (ret)
> >  		goto err;
> >  
> > +	/* Increase all phandles in the fdto by delta */
> >  	ret = overlay_adjust_local_phandles(fdto, delta);
> >  	if (ret)
> >  		goto err;
> >  
> > +	/* Adapt the phandle values in fdto to the above increase */
> >  	ret = overlay_update_local_references(fdto, delta);
> >  	if (ret)
> >  		goto err;
> >  
> > +	/* Update fdto's phandles using symbols from fdt */
> >  	ret = overlay_fixup_phandles(fdt, fdto);
> >  	if (ret)
> >  		goto err;
> >  
> > +	/* Don't overwrite phandles in fdt */
> > +	ret = overlay_prevent_phandle_overwrite(fdt, fdto);
> > +	if (ret)
> > +		goto err;
> 
> I think this should go above overlay_fixup_phandles(), so that we're
> completing all the __local_fixups__ based adjustments before going to
> the __fixups__ based adjustments.

This is only to please the reader, right? Or is there a corner case I
failed to see?

The feedback I removed in this mail I agree with. Expect a v3 soon.

Thanks for your time to review my patch,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5039 bytes --]

On Thu, Feb 22, 2024 at 08:22:48AM +0100, Uwe Kleine-König wrote:
> Hello David,
> 
> On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote:
> > On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote:
> > > [...]
> > > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> > > index 5c0c3981b89d..914acc5b14a6 100644
> > > --- a/libfdt/fdt_overlay.c
> > > +++ b/libfdt/fdt_overlay.c
> > > @@ -520,6 +520,228 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
> > >  	return 0;
> > >  }
> > >  
> > 
> > All of these new functions could do with descriptive comments saying
> > what they do (see the existing code for examples).
> 
> Yeah, I was lazy. I didn't expect this patch to go in without comments
> and avoided investing time documenting functions that maybe will change
> in the course of the review-resend loop. :-)
> 
> > > [...]
> > > +static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
> > > +						      int fixup_node,
> > > +						      uint32_t fdt_phandle,
> > > +						      uint32_t fdto_phandle)
> > > +{
> > > +	int fixup_prop;
> > > +	int fixup_child;
> > > +	int ret;
> > > +
> > > +	fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
> > > +
> > 
> > Extraneous blank line.
> > 
> > > +		const fdt32_t *fixup_val;
> > > +                const char *tree_val;
> > 
> > libfdt style is to use tab indents, looks like most of this block has
> > space indents instead.
> 
> Huh, I'm surprised about myself that I didn't notice this alone.
> 
> > > +
> > > +			/*
> > > +			 * phandles to fixup can be unaligned.
> > > +			 *
> > > +			 * Use a memcpy for the architectures that do
> > > +			 * not support unaligned accesses.
> > > +			 */
> > > +			memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));
> > 
> > And here you can use fdt32_ld() which has unaligned access handling
> > built in.
> 
> I copied this from somewhere, so I guess there is some potential for
> improvement in existing code. Will take a look.
>  
> > > [...]
> > > +
> > > +static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
> > > +						  void *fdto, int fdtonode)
> > 
> > Maybe "overlay_resolve_phandle_conflict()" for a slightly shorter
> > function name.
> 
> Sounds good, wasn't too lucky with
> overlay_prevent_phandle_overwrite_node() either.
>  
> > > +{
> > > +	uint32_t fdt_phandle, fdto_phandle;
> > > +	int fdtochild;
> > > +
> > > +	fdt_phandle = fdt_get_phandle(fdt, fdtnode);
> > > +	fdto_phandle = fdt_get_phandle(fdto, fdtonode);
> > > +
> > > +	if (fdt_phandle && fdto_phandle) {
> > > +		int ret;
> > > +
> > > +		ret = overlay_adjust_local_conflicting_phandle(fdto,
> > > +							       fdt_phandle,
> > > +							       fdto_phandle);
> > > +		if (ret)
> > > +			return ret;
> > 
> > So while, as you've seen, there can be conflicting phandles between
> > the fdt and the fdto, within the fdto a particular phandle should only
> > appear on a single node.  If a phandle is duplicated across nodes
> > witin a single fdto, you have bigger problems.  Therefore, having
> > found a conflict, you're already found the only place you need to
> > change the phandle property itself - you don't need to scan the full
> > fdto again.
> > 
> > So I believe you can replace the above call with just setting the
> > phandle property on fdtonode.
> 
> Oh right!
>  
> > > [...]
> > >  /**
> > >   * overlay_apply_node - Merges a node into the base device tree
> > >   * @fdt: Base Device Tree blob
> > > @@ -824,18 +1046,26 @@ int fdt_overlay_apply(void *fdt, void *fdto)
> > >  	if (ret)
> > >  		goto err;
> > >  
> > > +	/* Increase all phandles in the fdto by delta */
> > >  	ret = overlay_adjust_local_phandles(fdto, delta);
> > >  	if (ret)
> > >  		goto err;
> > >  
> > > +	/* Adapt the phandle values in fdto to the above increase */
> > >  	ret = overlay_update_local_references(fdto, delta);
> > >  	if (ret)
> > >  		goto err;
> > >  
> > > +	/* Update fdto's phandles using symbols from fdt */
> > >  	ret = overlay_fixup_phandles(fdt, fdto);
> > >  	if (ret)
> > >  		goto err;
> > >  
> > > +	/* Don't overwrite phandles in fdt */
> > > +	ret = overlay_prevent_phandle_overwrite(fdt, fdto);
> > > +	if (ret)
> > > +		goto err;
> > 
> > I think this should go above overlay_fixup_phandles(), so that we're
> > completing all the __local_fixups__ based adjustments before going to
> > the __fixups__ based adjustments.
> 
> This is only to please the reader, right? Or is there a corner case I
> failed to see?

Correct.  At least as far as I've seen, too.

> The feedback I removed in this mail I agree with. Expect a v3 soon.
> 
> Thanks for your time to review my patch,
> Uwe
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3996 bytes --]

On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote:
> On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote:
> > +static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
> > +						      int fixup_node,
> > +						      uint32_t fdt_phandle,
> > +						      uint32_t fdto_phandle)
> > +{
> > +	int fixup_prop;
> > +	int fixup_child;
> > +	int ret;
> > +
> > +	fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
> > +
> > +		const fdt32_t *fixup_val;
> > +                const char *tree_val;
> > +                const char *name;
> > +                int fixup_len;
> > +                int tree_len;
> > +                int i;
> > +
> > +                fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
> > +                                                  &name, &fixup_len);
> > +                if (!fixup_val)
> > +                        return fixup_len;
> > +
> > +                if (fixup_len % sizeof(uint32_t))
> > +                        return -FDT_ERR_BADOVERLAY;
> > +                fixup_len /= sizeof(uint32_t);
> > +
> > +                tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> > +                if (!tree_val) {
> > +                        if (tree_len == -FDT_ERR_NOTFOUND)
> > +                                return -FDT_ERR_BADOVERLAY;
> > +
> > +                        return tree_len;
> > +                }
> > +
> > +		for (i = 0; i < fixup_len; i++) {
> > +			fdt32_t adj_val;
> > +			uint32_t poffset;
> > +
> > +			poffset = fdt32_to_cpu(fixup_val[i]);
> 
> You can use fdt32_ld_() here to slightly simplify this

I don't see what you suggest here. fixup_val is assigned to in
fdt_getprop_by_offset() which I need to get fixup_len and then fixup_val
is already around. If you still think this could be improved, please be
more explicit.

> > +
> > +			/*
> > +			 * phandles to fixup can be unaligned.
> > +			 *
> > +			 * Use a memcpy for the architectures that do
> > +			 * not support unaligned accesses.
> > +			 */
> > +			memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));
> 
> And here you can use fdt32_ld() which has unaligned access handling
> built in.
> 
> > +
> > +			if (fdt32_to_cpu(adj_val) == fdto_phandle) {
> > +
> > +				adj_val = cpu_to_fdt32(fdt_phandle);
> > +
> > +				ret = fdt_setprop_inplace_namelen_partial(fdto,
> > +									  tree_node,
> > +									  name,
> > +									  strlen(name),
> > +									  poffset,
> > +									  &adj_val,
> > +									  sizeof(adj_val));
> > +				if (ret == -FDT_ERR_NOSPACE)
> > +					return -FDT_ERR_BADOVERLAY;
> > +
> > +				if (ret)
> > +					return ret;
> 
> And here you can use fdt32_st() on (tree_val + poffset), avoiding
> quite some complexity.

There is some complexity because tree_val is a const char * (as this is
what fdt_getprop() returns) and fdt32_st() obviously doens't take a
const pointer.

If you want to play around with that, the code is mostly a copy of
overlay_update_local_node_references() which could benefit from your
suggestions in the same way.

Can I lure you in improving overlay_update_local_node_references()? Then
I could copy from the improved function for my patch. (Or maybe refactor
the function to take a function as parameter which is

	uint32_t delta;
	uint32_t increase_by_delta(uint32_t phandle)
	{
		return phandle + delta;
	}

for the overlay_update_local_node_references() case and

	uint32_t fdt_phandle;
	uint32_t fdto_phandle;
	uint32_t resolve_fdt_conflict(uint32_t phandle)
	{
		if (phandle == fdto_phandle)
			return fdt_phandle;
		else
			return phandle;
	}

for my function (maybe passing the data stored in global vars here in a
context parameter).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5157 bytes --]

On Thu, Feb 22, 2024 at 10:38:54AM +0100, Uwe Kleine-König wrote:
> On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote:
> > On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote:
> > > +static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
> > > +						      int fixup_node,
> > > +						      uint32_t fdt_phandle,
> > > +						      uint32_t fdto_phandle)
> > > +{
> > > +	int fixup_prop;
> > > +	int fixup_child;
> > > +	int ret;
> > > +
> > > +	fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
> > > +
> > > +		const fdt32_t *fixup_val;
> > > +                const char *tree_val;
> > > +                const char *name;
> > > +                int fixup_len;
> > > +                int tree_len;
> > > +                int i;
> > > +
> > > +                fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
> > > +                                                  &name, &fixup_len);
> > > +                if (!fixup_val)
> > > +                        return fixup_len;
> > > +
> > > +                if (fixup_len % sizeof(uint32_t))
> > > +                        return -FDT_ERR_BADOVERLAY;
> > > +                fixup_len /= sizeof(uint32_t);
> > > +
> > > +                tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
> > > +                if (!tree_val) {
> > > +                        if (tree_len == -FDT_ERR_NOTFOUND)
> > > +                                return -FDT_ERR_BADOVERLAY;
> > > +
> > > +                        return tree_len;
> > > +                }
> > > +
> > > +		for (i = 0; i < fixup_len; i++) {
> > > +			fdt32_t adj_val;
> > > +			uint32_t poffset;
> > > +
> > > +			poffset = fdt32_to_cpu(fixup_val[i]);
> > 
> > You can use fdt32_ld_() here to slightly simplify this
> 
> I don't see what you suggest here. fixup_val is assigned to in
> fdt_getprop_by_offset() which I need to get fixup_len and then fixup_val
> is already around. If you still think this could be improved, please be
> more explicit.

	poffset = fdt32_ld_(fixup_val + i);

Thus combining the memory read and the byteswap into one operation.  I
guess it doesn't really simplify the code, but correctly handling
reads and writes to memory within the dtb is what the fdt*_ld() and
fdt*_st() helpers are for.

> > > +
> > > +			/*
> > > +			 * phandles to fixup can be unaligned.
> > > +			 *
> > > +			 * Use a memcpy for the architectures that do
> > > +			 * not support unaligned accesses.
> > > +			 */
> > > +			memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));
> > 
> > And here you can use fdt32_ld() which has unaligned access handling
> > built in.

Specifically
	adj_val = fdt32_ld(tree_val + poffset)

And then replacing fdt32_to_cpu(adj_val) with just adj_val below.

> > 
> > > +
> > > +			if (fdt32_to_cpu(adj_val) == fdto_phandle) {
> > > +
> > > +				adj_val = cpu_to_fdt32(fdt_phandle);
> > > +
> > > +				ret = fdt_setprop_inplace_namelen_partial(fdto,
> > > +									  tree_node,
> > > +									  name,
> > > +									  strlen(name),
> > > +									  poffset,
> > > +									  &adj_val,
> > > +									  sizeof(adj_val));
> > > +				if (ret == -FDT_ERR_NOSPACE)
> > > +					return -FDT_ERR_BADOVERLAY;
> > > +
> > > +				if (ret)
> > > +					return ret;
> > 
> > And here you can use fdt32_st() on (tree_val + poffset), avoiding
> > quite some complexity.
> 
> There is some complexity because tree_val is a const char * (as this is
> what fdt_getprop() returns) and fdt32_st() obviously doens't take a
> const pointer.

Ah, right.  You can use fdt_getprop_w() to get a writable pointer
instead.  If there was a way to have fdt_getprop() return a const
pointer only if the fdt pointer it was given was const, I'd do that.

> If you want to play around with that, the code is mostly a copy of
> overlay_update_local_node_references() which could benefit from your
> suggestions in the same way.

That's quite plausible.

> Can I lure you in improving overlay_update_local_node_references()? Then

Fair point..

https://github.com/dgibson/dtc/commit/24f60011fd43683d8e3916435c4c726e9baac9c9

> I could copy from the improved function for my patch. (Or maybe refactor
> the function to take a function as parameter which is

Eh... I'd prefer to avoid higher order functions in something this low
level.

> 	uint32_t delta;
> 	uint32_t increase_by_delta(uint32_t phandle)
> 	{
> 		return phandle + delta;
> 	}
> 
> for the overlay_update_local_node_references() case and
> 
> 	uint32_t fdt_phandle;
> 	uint32_t fdto_phandle;
> 	uint32_t resolve_fdt_conflict(uint32_t phandle)
> 	{
> 		if (phandle == fdto_phandle)
> 			return fdt_phandle;
> 		else
> 			return phandle;
> 	}
> 
> for my function (maybe passing the data stored in global vars here in a
> context parameter).
> 
> Best regards
> Uwe
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On Fri, Feb 23, 2024 at 03:41:40PM +1100, David Gibson wrote:
> On Thu, Feb 22, 2024 at 10:38:54AM +0100, Uwe Kleine-König wrote:
> > On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote:
> > > And here you can use fdt32_st() on (tree_val + poffset), avoiding
> > > quite some complexity.
> > 
> > There is some complexity because tree_val is a const char * (as this is
> > what fdt_getprop() returns) and fdt32_st() obviously doens't take a
> > const pointer.
> 
> Ah, right.  You can use fdt_getprop_w() to get a writable pointer
> instead.  If there was a way to have fdt_getprop() return a const
> pointer only if the fdt pointer it was given was const, I'd do that.

Look at how container_of_const() is defined in the kernel[1], this
requires C11 though. See
https://en.cppreference.com/w/c/language/generic for some docs.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/container_of.h?h=v6.7#n32
 
> > If you want to play around with that, the code is mostly a copy of
> > overlay_update_local_node_references() which could benefit from your
> > suggestions in the same way.
> 
> That's quite plausible.
> 
> > Can I lure you in improving overlay_update_local_node_references()? Then
> 
> Fair point..
> 
> https://github.com/dgibson/dtc/commit/24f60011fd43683d8e3916435c4c726e9baac9c9

I gave feedback on that one. Looks good.

> > I could copy from the improved function for my patch. (Or maybe refactor
> > the function to take a function as parameter which is
> 
> Eh... I'd prefer to avoid higher order functions in something this low
> level.

OK.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] libfdt: overlay: ensure that existing phandles are not overwritten

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2461 bytes --]

On Fri, Feb 23, 2024 at 08:53:37AM +0100, Uwe Kleine-König wrote:
> On Fri, Feb 23, 2024 at 03:41:40PM +1100, David Gibson wrote:
> > On Thu, Feb 22, 2024 at 10:38:54AM +0100, Uwe Kleine-König wrote:
> > > On Thu, Feb 22, 2024 at 05:32:10PM +1100, David Gibson wrote:
> > > > And here you can use fdt32_st() on (tree_val + poffset), avoiding
> > > > quite some complexity.
> > > 
> > > There is some complexity because tree_val is a const char * (as this is
> > > what fdt_getprop() returns) and fdt32_st() obviously doens't take a
> > > const pointer.
> > 
> > Ah, right.  You can use fdt_getprop_w() to get a writable pointer
> > instead.  If there was a way to have fdt_getprop() return a const
> > pointer only if the fdt pointer it was given was const, I'd do that.
> 
> Look at how container_of_const() is defined in the kernel[1], this
> requires C11 though. See

Right.  Just like we avoid allocations to allow libfdt to be used in
constrained environments like bootloaders, we're also very
conservative with which compiler features we want to rely on.

> https://en.cppreference.com/w/c/language/generic for some docs.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/container_of.h?h=v6.7#n32
>  
> > > If you want to play around with that, the code is mostly a copy of
> > > overlay_update_local_node_references() which could benefit from your
> > > suggestions in the same way.
> > 
> > That's quite plausible.
> > 
> > > Can I lure you in improving overlay_update_local_node_references()? Then
> > 
> > Fair point..
> > 
> > https://github.com/dgibson/dtc/commit/24f60011fd43683d8e3916435c4c726e9baac9c9
> 
> I gave feedback on that one. Looks good.
> 
> > > I could copy from the improved function for my patch. (Or maybe refactor
> > > the function to take a function as parameter which is
> > 
> > Eh... I'd prefer to avoid higher order functions in something this low
> > level.
> 
> OK.

To elaborate here, I can also image certain platform + environment +
compiler combinations where indirect calls don't work properly in an
early boot environment.  They should, of course, but you'd be
astonished how many messed up things I've seen in firmwares.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]