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/ |