[PATCH v3 0/2] livetree: Add only new data to fixup nodes instead of complete regeneration

[PATCH v3 0/2] livetree: Add only new data to fixup nodes instead of complete regeneration

[Uwe Kleine-König]

Hello,

here comes v3 of my effort to fix the fallout of commit 915daadbb62d
("Start with empty __local_fixups__ and __fixups__ nodes") before trying
to restore phandle information from __fixups__ and __local_fixups__ when
compiling from dtb to dts.

Changes since v2[1]:

 - Don't add data to malformed properties
 - Make check for for property being a string in
   append_unique_str_to_property() more robust.

I didn't add a specific error message pointing out malformed
properties because it's hard to only emit the first problem and also to
only emit each property once. (Well, it's not really hard, but needs
considerable restructuring of the affected functions that makes them
less intuitive.)

Best regards
Uwe

[1] https://lore.kernel.org/devicetree-compiler/cover.1755264521.git.u.kleine-koenig@baylibre.com

Uwe Kleine-König (2):
  livetree: Simplify append_to_property()
  livetree: Add only new data to fixup nodes instead of complete
    regeneration

 livetree.c              | 137 ++++++++++++++++++++++++++++------------
 tests/retain-fixups.dts |  29 +++++++++
 tests/run_tests.sh      |   5 ++
 3 files changed, 131 insertions(+), 40 deletions(-)
 create mode 100644 tests/retain-fixups.dts


base-commit: 84d9dd2fcbc865a35d7f04d9b465b05ef286d281
-- 
2.50.1

[PATCH v3 1/2] livetree: Simplify append_to_property()

[Uwe Kleine-König]

The two if branches are quite similar. Build the property first (in case
it doesn't exist) and then the common parts can be done outside the if
block.
---
 livetree.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/livetree.c b/livetree.c
index d51d05830b18..6127d604d528 100644
--- a/livetree.c
+++ b/livetree.c
@@ -340,20 +340,16 @@ void append_to_property(struct node *node,
 			char *name, const void *data, int len,
 			enum markertype type)
 {
-	struct data d;
 	struct property *p;
 
 	p = get_property(node, name);
-	if (p) {
-		d = data_add_marker(p->val, type, name);
-		d = data_append_data(d, data, len);
-		p->val = d;
-	} else {
-		d = data_add_marker(empty_data, type, name);
-		d = data_append_data(d, data, len);
-		p = build_property(name, d, NULL);
+	if (!p) {
+		p = build_property(name, empty_data, NULL);
 		add_property(node, p);
 	}
+
+	p->val = data_add_marker(p->val, type, name);
+	p->val = data_append_data(p->val, data, len);
 }
 
 struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
-- 
2.50.1

Re: [PATCH v3 1/2] livetree: Simplify append_to_property()

[Uwe Kleine-König]

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

Hello,

On Mon, Aug 18, 2025 at 12:35:45PM +0200, Uwe Kleine-König wrote:
> The two if branches are quite similar. Build the property first (in case
> it doesn't exist) and then the common parts can be done outside the if
> block.

Argh, I failed to add my s-o-b line here. Maybe b4 picks it up when I
write:

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

here. It's fixed in my working copy know, so if there will be a v4, this
isn't an issue any more.

Best regards
Uwe

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

Re: [PATCH v3 1/2] livetree: Simplify append_to_property()

[David Gibson]

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

On Mon, Aug 18, 2025 at 12:38:57PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Aug 18, 2025 at 12:35:45PM +0200, Uwe Kleine-König wrote:
> > The two if branches are quite similar. Build the property first (in case
> > it doesn't exist) and then the common parts can be done outside the if
> > block.
> 
> Argh, I failed to add my s-o-b line here. Maybe b4 picks it up when I
> write:
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Heh, thanks for adding this after.  I'll take it from here and paste
it in.

> here. It's fixed in my working copy know, so if there will be a v4, this
> isn't an issue any more.
> 
> Best regards
> Uwe



-- 
David Gibson (he or they)	| 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 v3 1/2] livetree: Simplify append_to_property()

[Uwe Kleine-König]

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

Hey David,

On Tue, Aug 19, 2025 at 09:25:35AM +1000, David Gibson wrote:
> On Mon, Aug 18, 2025 at 12:38:57PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Aug 18, 2025 at 12:35:45PM +0200, Uwe Kleine-König wrote:
> > > The two if branches are quite similar. Build the property first (in case
> > > it doesn't exist) and then the common parts can be done outside the if
> > > block.
> > 
> > Argh, I failed to add my s-o-b line here. Maybe b4 picks it up when I
> > write:
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> 
> Heh, thanks for adding this after.  I'll take it from here and paste
> it in.

Does that mean you're not using b4, or that b4 doesn't pick it up?

Best regards
Uwe

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

Re: [PATCH v3 1/2] livetree: Simplify append_to_property()

[David Gibson]

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

On Tue, Aug 19, 2025 at 07:35:39AM +0200, Uwe Kleine-König wrote:
> Hey David,
> 
> On Tue, Aug 19, 2025 at 09:25:35AM +1000, David Gibson wrote:
> > On Mon, Aug 18, 2025 at 12:38:57PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > On Mon, Aug 18, 2025 at 12:35:45PM +0200, Uwe Kleine-König wrote:
> > > > The two if branches are quite similar. Build the property first (in case
> > > > it doesn't exist) and then the common parts can be done outside the if
> > > > block.
> > > 
> > > Argh, I failed to add my s-o-b line here. Maybe b4 picks it up when I
> > > write:
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > 
> > Heh, thanks for adding this after.  I'll take it from here and paste
> > it in.
> 
> Does that mean you're not using b4, or that b4 doesn't pick it up?

I'm not using b4 - never had enough impetus to figure it out.

-- 
David Gibson (he or they)	| 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 v3 1/2] livetree: Simplify append_to_property()

[Uwe Kleine-König]

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

Hello David,

On Tue, Aug 19, 2025 at 04:17:43PM +1000, David Gibson wrote:
> On Tue, Aug 19, 2025 at 07:35:39AM +0200, Uwe Kleine-König wrote:
> > On Tue, Aug 19, 2025 at 09:25:35AM +1000, David Gibson wrote:
> > > On Mon, Aug 18, 2025 at 12:38:57PM +0200, Uwe Kleine-König wrote:
> > > > Argh, I failed to add my s-o-b line here. Maybe b4 picks it up when I
> > > > write:
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > > 
> > > Heh, thanks for adding this after.  I'll take it from here and paste
> > > it in.
> > 
> > Does that mean you're not using b4, or that b4 doesn't pick it up?
> 
> I'm not using b4 - never had enough impetus to figure it out.

In this case it would be just:

	b4 am -s -l -t -3 aKQXB5PuW-vt9zKJ@zatzit

and then execute the git-am at the end of its output.
("aKQXB5PuW-vt9zKJ@zatzit" is just a message id from one mail in the
thread you want to apply.)

I think it doesn't need any preparatory configuration to make that work.

The output for me (after changing my email address in .git/config to
test if my late S-o-b is picked up) is:

	$ b4 am -s -l -t -3 aKQXB5PuW-vt9zKJ@zatzit
	Analyzing 7 messages in the thread
	Analyzing 7 code-review messages
	Checking attestation on all messages, may take a moment...
	---
	  ✓ [PATCH v3 1/2] livetree: Simplify append_to_property()
	    + Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> (✓ DKIM/baylibre-com.20230601.gappssmtp.com)
	    + Link: https://lore.kernel.org/r/eef88e559f5b9818c4c2311fa8a75ab6fd4508d5.1755512759.git.u.kleine-koenig@baylibre.com
	    + Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
	  ✓ [PATCH v3 2/2] livetree: Add only new data to fixup nodes instead of complete regeneration
	    + Link: https://lore.kernel.org/r/b061ee57157fafbb9d5b9b0b86af760d13a04eda.1755512759.git.u.kleine-koenig@baylibre.com
	    + Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
	  ---
	  ✓ Signed: openpgp/u.kleine-koenig@baylibre.com
	  ✓ Signed: DKIM/baylibre-com.20230601.gappssmtp.com (From: u.kleine-koenig@baylibre.com)
	---
	Total patches: 2
	Preared a fake commit range for 3-way merge (62461e41cde3..3dab71c00c24)
	---
	Cover: ./v3_20250818_u_kleine_koenig_livetree_add_only_new_data_to_fixup_nodes_instead_of_complete_regenerati.cover
	 Link: https://lore.kernel.org/r/cover.1755512759.git.u.kleine-koenig@baylibre.com
	 Base: using specified base-commit 84d9dd2fcbc865a35d7f04d9b465b05ef286d281
	       git checkout -b v3_20250818_u_kleine_koenig_baylibre_com 84d9dd2fcbc865a35d7f04d9b465b05ef286d281
	       git am -3 ./v3_20250818_u_kleine_koenig_livetree_add_only_new_data_to_fixup_nodes_instead_of_complete_regenerati.mbx

So the added value is:

 - adds your Signed-off-by line (-s)
 - adds a Link: trailer (-l)
 - collects tags from replies to patches (also my late S-o-b)
 - collects tags from replies on the cover message and applies it to all
   patches (-t, though doesn't apply here)
 - apply some magic to make it more likely that git can apply the patch
   if you have a different base than the submitter (-3)
 - checks openpgp, DKIM etc.

Best regards
Uwe

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

[PATCH v3 2/2] livetree: Add only new data to fixup nodes instead of complete regeneration

[Uwe Kleine-König]

Removing the complete __fixups__ and __local_fixups__ tree might delete
data that should better be retained. See the added test for a situation
that was broken before.

Note that without removing /__fixups__ and /__local_fixups__ in
generate_fixups_tree() and generate_local_fixups_tree() respectively
calling build_and_name_child_node() isn't safe as the nodes might
already exist and then a duplicate would be added. So build_root_node()
has to be used which copes correctly here.

Fixes: 915daadbb62d ("Start with empty __local_fixups__ and __fixups__ nodes")
Closes: https://github.com/dgibson/dtc/issues/170
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 livetree.c              | 127 +++++++++++++++++++++++++++++-----------
 tests/retain-fixups.dts |  29 +++++++++
 tests/run_tests.sh      |   5 ++
 3 files changed, 128 insertions(+), 33 deletions(-)
 create mode 100644 tests/retain-fixups.dts

diff --git a/livetree.c b/livetree.c
index 6127d604d528..2241ec276044 100644
--- a/livetree.c
+++ b/livetree.c
@@ -352,6 +352,63 @@ void append_to_property(struct node *node,
 	p->val = data_append_data(p->val, data, len);
 }
 
+static int append_unique_str_to_property(struct node *node,
+					 char *name, const char *data, int len)
+{
+	struct property *p;
+
+	p = get_property(node, name);
+	if (p) {
+		const char *s;
+
+		if (p->val.len && p->val.val[p->val.len - 1] != '\0')
+			/* The current content doesn't look like a string */
+			return 1;
+
+		for (s = p->val.val; s < p->val.val + p->val.len; s = strchr(s, '\0') + 1) {
+			if (strcmp(data, s) == 0)
+				/* data already contained in node.name */
+				return 0;
+		}
+	} else {
+		p = build_property(name, empty_data, NULL);
+		add_property(node, p);
+	}
+
+	p->val = data_add_marker(p->val, TYPE_STRING, name);
+	p->val = data_append_data(p->val, data, len);
+
+	return 0;
+}
+
+static int append_unique_u32_to_property(struct node *node, char *name, fdt32_t value)
+{
+	struct property *p;
+
+	p = get_property(node, name);
+	if (p) {
+		const fdt32_t *v, *val_end = (const fdt32_t *)p->val.val + p->val.len / 4;
+
+		if (p->val.len % 4 != 0)
+			/* The current content doesn't look like a u32 array */
+			return 1;
+
+		for (v = (const void *)p->val.val; v < val_end; v++) {
+			if (*v == value)
+				/* value already contained */
+				return 0;
+		}
+	} else {
+		p = build_property(name, empty_data, NULL);
+		add_property(node, p);
+	}
+
+	p->val = data_add_marker(p->val, TYPE_UINT32, name);
+	p->val = data_append_data(p->val, &value, 4);
+
+	return 0;
+}
+
 struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
 {
 	struct reserve_info *new = xmalloc(sizeof(*new));
@@ -914,11 +971,12 @@ static bool any_fixup_tree(struct dt_info *dti, struct node *node)
 	return false;
 }
 
-static void add_fixup_entry(struct dt_info *dti, struct node *fn,
-			    struct node *node, struct property *prop,
-			    struct marker *m)
+static int add_fixup_entry(struct dt_info *dti, struct node *fn,
+			   struct node *node, struct property *prop,
+			   struct marker *m)
 {
 	char *entry;
+	int ret;
 
 	/* m->ref can only be a REF_PHANDLE, but check anyway */
 	assert(m->type == REF_PHANDLE);
@@ -935,32 +993,39 @@ static void add_fixup_entry(struct dt_info *dti, struct node *fn,
 
 	xasprintf(&entry, "%s:%s:%u",
 			node->fullpath, prop->name, m->offset);
-	append_to_property(fn, m->ref, entry, strlen(entry) + 1, TYPE_STRING);
+	ret = append_unique_str_to_property(fn, m->ref, entry, strlen(entry) + 1);
 
 	free(entry);
+
+	return ret;
 }
 
-static void generate_fixups_tree_internal(struct dt_info *dti,
-					  struct node *fn,
-					  struct node *node)
+static int generate_fixups_tree_internal(struct dt_info *dti,
+					 struct node *fn,
+					 struct node *node)
 {
 	struct node *dt = dti->dt;
 	struct node *c;
 	struct property *prop;
 	struct marker *m;
 	struct node *refnode;
+	int ret = 0;
 
 	for_each_property(node, prop) {
 		m = prop->val.markers;
 		for_each_marker_of_type(m, REF_PHANDLE) {
 			refnode = get_node_by_ref(dt, m->ref);
 			if (!refnode)
-				add_fixup_entry(dti, fn, node, prop, m);
+				if (add_fixup_entry(dti, fn, node, prop, m))
+					ret = 1;
 		}
 	}
 
 	for_each_child(node, c)
-		generate_fixups_tree_internal(dti, fn, c);
+		if (generate_fixups_tree_internal(dti, fn, c))
+			ret = 1;
+
+	return ret;
 }
 
 static bool any_local_fixup_tree(struct dt_info *dti, struct node *node)
@@ -985,7 +1050,7 @@ static bool any_local_fixup_tree(struct dt_info *dti, struct node *node)
 	return false;
 }
 
-static void add_local_fixup_entry(struct dt_info *dti,
+static int add_local_fixup_entry(struct dt_info *dti,
 		struct node *lfn, struct node *node,
 		struct property *prop, struct marker *m,
 		struct node *refnode)
@@ -1016,30 +1081,35 @@ static void add_local_fixup_entry(struct dt_info *dti,
 	free(compp);
 
 	value_32 = cpu_to_fdt32(m->offset);
-	append_to_property(wn, prop->name, &value_32, sizeof(value_32), TYPE_UINT32);
+	return append_unique_u32_to_property(wn, prop->name, value_32);
 }
 
-static void generate_local_fixups_tree_internal(struct dt_info *dti,
-						struct node *lfn,
-						struct node *node)
+static int generate_local_fixups_tree_internal(struct dt_info *dti,
+					       struct node *lfn,
+					       struct node *node)
 {
 	struct node *dt = dti->dt;
 	struct node *c;
 	struct property *prop;
 	struct marker *m;
 	struct node *refnode;
+	int ret = 0;
 
 	for_each_property(node, prop) {
 		m = prop->val.markers;
 		for_each_marker_of_type(m, REF_PHANDLE) {
 			refnode = get_node_by_ref(dt, m->ref);
 			if (refnode)
-				add_local_fixup_entry(dti, lfn, node, prop, m, refnode);
+				if (add_local_fixup_entry(dti, lfn, node, prop, m, refnode))
+					ret = 1;
 		}
 	}
 
 	for_each_child(node, c)
-		generate_local_fixups_tree_internal(dti, lfn, c);
+		if (generate_local_fixups_tree_internal(dti, lfn, c))
+			ret = 1;
+
+	return ret;
 }
 
 void generate_label_tree(struct dt_info *dti, const char *name, bool allocph)
@@ -1052,29 +1122,20 @@ void generate_label_tree(struct dt_info *dti, const char *name, bool allocph)
 
 void generate_fixups_tree(struct dt_info *dti, const char *name)
 {
-	struct node *n = get_subnode(dti->dt, name);
-
-	/* Start with an empty __fixups__ node to not get duplicates */
-	if (n)
-		n->deleted = true;
-
 	if (!any_fixup_tree(dti, dti->dt))
 		return;
-	generate_fixups_tree_internal(dti,
-				      build_and_name_child_node(dti->dt, name),
-				      dti->dt);
+	if (generate_fixups_tree_internal(dti, build_root_node(dti->dt, name), dti->dt))
+		fprintf(stderr,
+			"Warning: Preexisting data in %s malformed, some content could not be added.\n",
+			name);
 }
 
 void generate_local_fixups_tree(struct dt_info *dti, const char *name)
 {
-	struct node *n = get_subnode(dti->dt, name);
-
-	/* Start with an empty __local_fixups__ node to not get duplicates */
-	if (n)
-		n->deleted = true;
 	if (!any_local_fixup_tree(dti, dti->dt))
 		return;
-	generate_local_fixups_tree_internal(dti,
-					    build_and_name_child_node(dti->dt, name),
-					    dti->dt);
+	if (generate_local_fixups_tree_internal(dti, build_root_node(dti->dt, name), dti->dt))
+		fprintf(stderr,
+			"Warning: Preexisting data in %s malformed, some content could not be added.\n",
+			name);
 }
diff --git a/tests/retain-fixups.dts b/tests/retain-fixups.dts
new file mode 100644
index 000000000000..ec09db8b441c
--- /dev/null
+++ b/tests/retain-fixups.dts
@@ -0,0 +1,29 @@
+/dts-v1/;
+/plugin/;
+
+/ {
+	fixup-node {
+		property = <0xffffffff>;
+		property-with-fixup = <0xffffffff>;
+		property-with-label = <&somenode>;
+		property-with-label-and-fixup = <&somenode>;
+	};
+
+	label: local-fixup-node {
+		property = <0xffffffff>;
+		property-with-local-fixup = <0xffffffff>;
+		property-with-local-label = <&label>;
+		property-with-local-label-and-fixup = <&label>;
+	};
+
+	__fixups__ {
+		somenode = "/fixup-node:property-with-fixup:0", "/fixup-node:property-with-label-and-fixup:0";
+	};
+
+	__local_fixups__ {
+		local-fixup-node {
+			property-with-local-fixup = <0x00>;
+			property-with-local-label-and-fixup = <0x00>;
+		};
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index fecfe7cc09b3..1c9278d93689 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -666,6 +666,11 @@ dtc_tests () {
         run_test dtbs_equal_ordered $tree.test.dtb $tree.test.dts.test.dtb
     done
 
+    # Check preservation of __fixups__ and __local_fixups__
+    run_dtc_test -I dts -O dtb -o retain-fixups.test.dtb "$SRCDIR/retain-fixups.dts"
+    run_fdtget_test "/fixup-node:property-with-fixup:0 /fixup-node:property-with-label-and-fixup:0 /fixup-node:property-with-label:0" retain-fixups.test.dtb /__fixups__ somenode
+    run_fdtget_test "property-with-local-fixup\nproperty-with-local-label-and-fixup\nproperty-with-local-label" -p retain-fixups.test.dtb /__local_fixups__/local-fixup-node
+
     # Check -Oyaml output
     if ! $no_yaml; then
             for tree in type-preservation; do
-- 
2.50.1

Re: [PATCH v3 2/2] livetree: Add only new data to fixup nodes instead of complete regeneration

[David Gibson]

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

On Mon, Aug 18, 2025 at 12:35:46PM +0200, Uwe Kleine-König wrote:
> Removing the complete __fixups__ and __local_fixups__ tree might delete
> data that should better be retained. See the added test for a situation
> that was broken before.
> 
> Note that without removing /__fixups__ and /__local_fixups__ in
> generate_fixups_tree() and generate_local_fixups_tree() respectively
> calling build_and_name_child_node() isn't safe as the nodes might
> already exist and then a duplicate would be added. So build_root_node()
> has to be used which copes correctly here.
> 
> Fixes: 915daadbb62d ("Start with empty __local_fixups__ and __fixups__ nodes")
> Closes: https://github.com/dgibson/dtc/issues/170
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Merged. thanks.

Note that I generally prefer negative values for error codes (-1
instead of 1).  I altered that on merge.

-- 
David Gibson (he or they)	| 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 --]