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

[PATCH] 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.

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>
---
Hello,

this fixes the fallout of commit 915daadbb62d ("Start with empty
__local_fixups__ and __fixups__ nodes") that was found while discussing
https://github.com/dgibson/dtc/pull/151 .

I'm a bit annoyed by the github workflow, so here comes a patch on the
mailing list :-)

Best regards
Uwe

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

diff --git a/livetree.c b/livetree.c
index d51d05830b18..24f7c561d77e 100644
--- a/livetree.c
+++ b/livetree.c
@@ -356,6 +356,60 @@ void append_to_property(struct node *node,
 	}
 }
 
+static void append_unique_str_to_property(struct node *node,
+					  char *name, const char *data, int len)
+{
+	struct data d;
+	struct property *p;
+
+	p = get_property(node, name);
+	if (p) {
+		const char *s;
+
+		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;
+		}
+
+		d = data_add_marker(p->val, TYPE_STRING, name);
+		d = data_append_data(d, data, len);
+		p->val = d;
+	} else {
+		d = data_add_marker(empty_data, TYPE_STRING, name);
+		d = data_append_data(d, data, len);
+		p = build_property(name, d, NULL);
+		add_property(node, p);
+	}
+
+}
+
+static void append_unique_u32_to_property(struct node *node, char *name, fdt32_t value)
+{
+	struct data d;
+	struct property *p;
+
+	p = get_property(node, name);
+	if (p) {
+		const fdt32_t *v;
+
+		for (v = (const void *)p->val.val; (const void *)v < (const void *)(p->val.val + p->val.len - 3); v++) {
+			if (*v == value)
+				/* value already contained */
+				return;
+		}
+
+		d = data_add_marker(p->val, TYPE_UINT32, name);
+		d = data_append_data(d, &value, 4);
+		p->val = d;
+	} else {
+		d = data_add_marker(empty_data, TYPE_UINT32, name);
+		d = data_append_data(d, &value, 4);
+		p = build_property(name, d, NULL);
+		add_property(node, p);
+	}
+}
+
 struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
 {
 	struct reserve_info *new = xmalloc(sizeof(*new));
@@ -939,7 +993,7 @@ 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);
+	append_unique_str_to_property(fn, m->ref, entry, strlen(entry) + 1);
 
 	free(entry);
 }
@@ -1020,7 +1074,7 @@ 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);
+	append_unique_u32_to_property(wn, prop->name, value_32);
 }
 
 static void generate_local_fixups_tree_internal(struct dt_info *dti,
@@ -1056,29 +1110,16 @@ 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),
+	generate_fixups_tree_internal(dti, build_root_node(dti->dt, name),
 				      dti->dt);
 }
 
 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),
+	generate_local_fixups_tree_internal(dti, build_root_node(dti->dt, name),
 					    dti->dt);
 }
diff --git a/tests/retain-fixups.dts b/tests/retain-fixups.dts
new file mode 100644
index 000000000000..7b7b52210659
--- /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

base-commit: 84d9dd2fcbc865a35d7f04d9b465b05ef286d281
-- 
2.50.1

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

[David Gibson]

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

On Fri, Aug 01, 2025 at 06:00:31PM +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.
> 
> 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>
> ---
> Hello,
> 
> this fixes the fallout of commit 915daadbb62d ("Start with empty
> __local_fixups__ and __fixups__ nodes") that was found while discussing
> https://github.com/dgibson/dtc/pull/151 .

Thanks.

> I'm a bit annoyed by the github workflow, so here comes a patch on the
> mailing list :-)

That's fine.

Sorry it's taken me so long to look at this.

>  livetree.c              | 75 +++++++++++++++++++++++++++++++----------
>  tests/retain-fixups.dts | 29 ++++++++++++++++
>  tests/run_tests.sh      |  5 +++
>  3 files changed, 92 insertions(+), 17 deletions(-)
>  create mode 100644 tests/retain-fixups.dts
> 
> diff --git a/livetree.c b/livetree.c
> index d51d05830b18..24f7c561d77e 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -356,6 +356,60 @@ void append_to_property(struct node *node,
>  	}
>  }
>  
> +static void append_unique_str_to_property(struct node *node,
> +					  char *name, const char *data, int len)
> +{
> +	struct data d;
> +	struct property *p;
> +
> +	p = get_property(node, name);
> +	if (p) {
> +		const char *s;
> +
> +		for (s = p->val.val; s < p->val.val + p->val.len; s = strchr(s, '\0') + 1) {

This isn't quite safe.  You check s is within bounds on each
iteration, but if the property is malformed and doesn't end with a \0,
the strchr() itself could read beyond the property's bounds.

strnchr() could work, but is awkward: it would return NULL if there
are no further \0 within the property, and on most systems NULL <
p->val.val + p->val.len would return true, so you'd have to check for
that case separately.  You could use strnlen() instead, but that's
also a bit awkward - it doesn't include the \0, so you'd need to add
+1 - but in the malformed case that would put you one beyond the end
of the buffer.  That would probably work in practice, but creating
pointers beyond the buffer they're within is technically UB.

> +			if (strcmp(data, s) == 0)

This also relies on the \0 being there.  Any fix for the above, will
probably result in being able to get the length of each string segment
fairly naturally, so it would make sense to use memcmp() instead.

> +				/* data already contained in node.name */
> +				return;
> +		}
> +
> +		d = data_add_marker(p->val, TYPE_STRING, name);
> +		d = data_append_data(d, data, len);
> +		p->val = d;
> +	} else {
> +		d = data_add_marker(empty_data, TYPE_STRING, name);
> +		d = data_append_data(d, data, len);
> +		p = build_property(name, d, NULL);
> +		add_property(node, p);

You can add the property first, then share the data_append_data()
logic with the previous case.

> +	}
> +
> +}
> +
> +static void append_unique_u32_to_property(struct node *node, char *name, fdt32_t value)
> +{
> +	struct data d;
> +	struct property *p;
> +
> +	p = get_property(node, name);
> +	if (p) {
> +		const fdt32_t *v;
> +
> +		for (v = (const void *)p->val.val; (const void *)v < (const void *)(p->val.val + p->val.len - 3); v++) {
> +			if (*v == value)

Probably more elegant to get the length of the property in words
first.

> +				/* value already contained */
> +				return;
> +		}
> +
> +		d = data_add_marker(p->val, TYPE_UINT32, name);
> +		d = data_append_data(d, &value, 4);
> +		p->val = d;
> +	} else {
> +		d = data_add_marker(empty_data, TYPE_UINT32, name);
> +		d = data_append_data(d, &value, 4);
> +		p = build_property(name, d, NULL);
> +		add_property(node, p);
> +	}
> +}
> +
>  struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
>  {
>  	struct reserve_info *new = xmalloc(sizeof(*new));
> @@ -939,7 +993,7 @@ 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);
> +	append_unique_str_to_property(fn, m->ref, entry, strlen(entry) + 1);
>  
>  	free(entry);
>  }
> @@ -1020,7 +1074,7 @@ 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);
> +	append_unique_u32_to_property(wn, prop->name, value_32);
>  }
>  
>  static void generate_local_fixups_tree_internal(struct dt_info *dti,
> @@ -1056,29 +1110,16 @@ 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),
> +	generate_fixups_tree_internal(dti, build_root_node(dti->dt, name),
>  				      dti->dt);

It's not obvious to me why this change follows from the rest.

>  }
>  
>  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),
> +	generate_local_fixups_tree_internal(dti, build_root_node(dti->dt, name),
>  					    dti->dt);
>  }
> diff --git a/tests/retain-fixups.dts b/tests/retain-fixups.dts
> new file mode 100644
> index 000000000000..7b7b52210659
> --- /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
> 
> base-commit: 84d9dd2fcbc865a35d7f04d9b465b05ef286d281

-- 
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] livetree: Add only new data to fixup nodes instead of complete regeneration

[Uwe Kleine-König]

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

Hello David,

On Thu, Aug 14, 2025 at 05:36:08PM +1000, David Gibson wrote:
> On Fri, Aug 01, 2025 at 06:00:31PM +0200, Uwe Kleine-König wrote:
> >  livetree.c              | 75 +++++++++++++++++++++++++++++++----------
> >  tests/retain-fixups.dts | 29 ++++++++++++++++
> >  tests/run_tests.sh      |  5 +++
> >  3 files changed, 92 insertions(+), 17 deletions(-)
> >  create mode 100644 tests/retain-fixups.dts
> > 
> > diff --git a/livetree.c b/livetree.c
> > index d51d05830b18..24f7c561d77e 100644
> > --- a/livetree.c
> > +++ b/livetree.c
> > @@ -356,6 +356,60 @@ void append_to_property(struct node *node,
> >  	}
> >  }
> >  
> > +static void append_unique_str_to_property(struct node *node,
> > +					  char *name, const char *data, int len)
> > +{
> > +	struct data d;
> > +	struct property *p;
> > +
> > +	p = get_property(node, name);
> > +	if (p) {
> > +		const char *s;
> > +
> > +		for (s = p->val.val; s < p->val.val + p->val.len; s = strchr(s, '\0') + 1) {
> 
> This isn't quite safe.  You check s is within bounds on each
> iteration, but if the property is malformed and doesn't end with a \0,
> the strchr() itself could read beyond the property's bounds.
> 
> strnchr() could work, but is awkward: it would return NULL if there
> are no further \0 within the property, and on most systems NULL <
> p->val.val + p->val.len would return true, so you'd have to check for
> that case separately.  You could use strnlen() instead, but that's
> also a bit awkward - it doesn't include the \0, so you'd need to add
> +1 - but in the malformed case that would put you one beyond the end
> of the buffer.  That would probably work in practice, but creating
> pointers beyond the buffer they're within is technically UB.

fair, my approach would be to check p->val.val[p->val.len - 1] == '\0'
once before the loop.

> > +			if (strcmp(data, s) == 0)
> 
> This also relies on the \0 being there.  Any fix for the above, will
> probably result in being able to get the length of each string segment
> fairly naturally, so it would make sense to use memcmp() instead.
> 
> > +				/* data already contained in node.name */
> > +				return;
> > +		}
> > +
> > +		d = data_add_marker(p->val, TYPE_STRING, name);
> > +		d = data_append_data(d, data, len);
> > +		p->val = d;
> > +	} else {
> > +		d = data_add_marker(empty_data, TYPE_STRING, name);
> > +		d = data_append_data(d, data, len);
> > +		p = build_property(name, d, NULL);
> > +		add_property(node, p);
> 
> You can add the property first, then share the data_append_data()
> logic with the previous case.

This is mostly taken from append_to_property(), I will check if your
suggested improvement will apply to that one, too.
 
> > @@ -1056,29 +1110,16 @@ 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),
> > +	generate_fixups_tree_internal(dti, build_root_node(dti->dt, name),
> >  				      dti->dt);
> 
> It's not obvious to me why this change follows from the rest.

The relevant difference here is that now it's unknown if the __fixups__
node already exists. build_and_name_child_node() only works if it
doesn't exist which isn't ensured now any more with the lines deleted
above.

This is a revert of 915daadbb62d.

Best regards
Uwe

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

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

[Uwe Kleine-König]

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

On Thu, Aug 14, 2025 at 11:05:02AM +0200, Uwe Kleine-König wrote:
> Hello David,
> 
> On Thu, Aug 14, 2025 at 05:36:08PM +1000, David Gibson wrote:
> > On Fri, Aug 01, 2025 at 06:00:31PM +0200, Uwe Kleine-König wrote:
> > >  livetree.c              | 75 +++++++++++++++++++++++++++++++----------
> > >  tests/retain-fixups.dts | 29 ++++++++++++++++
> > >  tests/run_tests.sh      |  5 +++
> > >  3 files changed, 92 insertions(+), 17 deletions(-)
> > >  create mode 100644 tests/retain-fixups.dts
> > > 
> > > diff --git a/livetree.c b/livetree.c
> > > index d51d05830b18..24f7c561d77e 100644
> > > --- a/livetree.c
> > > +++ b/livetree.c
> > > @@ -356,6 +356,60 @@ void append_to_property(struct node *node,
> > >  	}
> > >  }
> > >  
> > > +static void append_unique_str_to_property(struct node *node,
> > > +					  char *name, const char *data, int len)
> > > +{
> > > +	struct data d;
> > > +	struct property *p;
> > > +
> > > +	p = get_property(node, name);
> > > +	if (p) {
> > > +		const char *s;
> > > +
> > > +		for (s = p->val.val; s < p->val.val + p->val.len; s = strchr(s, '\0') + 1) {
> > 
> > This isn't quite safe.  You check s is within bounds on each
> > iteration, but if the property is malformed and doesn't end with a \0,
> > the strchr() itself could read beyond the property's bounds.
> > 
> > strnchr() could work, but is awkward: it would return NULL if there
> > are no further \0 within the property, and on most systems NULL <
> > p->val.val + p->val.len would return true, so you'd have to check for
> > that case separately.  You could use strnlen() instead, but that's
> > also a bit awkward - it doesn't include the \0, so you'd need to add
> > +1 - but in the malformed case that would put you one beyond the end
> > of the buffer.  That would probably work in practice, but creating
> > pointers beyond the buffer they're within is technically UB.
> 
> fair, my approach would be to check p->val.val[p->val.len - 1] == '\0'
> once before the loop.
> 
> > > +			if (strcmp(data, s) == 0)
> > 
> > This also relies on the \0 being there.  Any fix for the above, will
> > probably result in being able to get the length of each string segment
> > fairly naturally, so it would make sense to use memcmp() instead.
> > 
> > > +				/* data already contained in node.name */
> > > +				return;
> > > +		}
> > > +
> > > +		d = data_add_marker(p->val, TYPE_STRING, name);
> > > +		d = data_append_data(d, data, len);
> > > +		p->val = d;
> > > +	} else {
> > > +		d = data_add_marker(empty_data, TYPE_STRING, name);
> > > +		d = data_append_data(d, data, len);
> > > +		p = build_property(name, d, NULL);
> > > +		add_property(node, p);
> > 
> > You can add the property first, then share the data_append_data()
> > logic with the previous case.
> 
> This is mostly taken from append_to_property(), I will check if your
> suggested improvement will apply to that one, too.

I think that would be:

diff --git a/livetree.c b/livetree.c
index d51d05830b18..2ccdc9d9c2a7 100644
--- a/livetree.c
+++ b/livetree.c
@@ -344,16 +344,14 @@ void append_to_property(struct node *node,
 	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);
 	}
+
+	d = data_add_marker(p->val, type, name);
+	d = data_append_data(d, data, len);
+	p->val = d;
 }
 
 struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)

The test suite is happy with it. I will include that with the next
revision of my fix patch.

Best regards
Uwe

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

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

[David Gibson]

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

On Thu, Aug 14, 2025 at 11:28:05AM +0200, Uwe Kleine-König wrote:
> On Thu, Aug 14, 2025 at 11:05:02AM +0200, Uwe Kleine-König wrote:
> > Hello David,
> > 
> > On Thu, Aug 14, 2025 at 05:36:08PM +1000, David Gibson wrote:
> > > On Fri, Aug 01, 2025 at 06:00:31PM +0200, Uwe Kleine-König wrote:
> > > >  livetree.c              | 75 +++++++++++++++++++++++++++++++----------
> > > >  tests/retain-fixups.dts | 29 ++++++++++++++++
> > > >  tests/run_tests.sh      |  5 +++
> > > >  3 files changed, 92 insertions(+), 17 deletions(-)
> > > >  create mode 100644 tests/retain-fixups.dts
> > > > 
> > > > diff --git a/livetree.c b/livetree.c
> > > > index d51d05830b18..24f7c561d77e 100644
> > > > --- a/livetree.c
> > > > +++ b/livetree.c
> > > > @@ -356,6 +356,60 @@ void append_to_property(struct node *node,
> > > >  	}
> > > >  }
> > > >  
> > > > +static void append_unique_str_to_property(struct node *node,
> > > > +					  char *name, const char *data, int len)
> > > > +{
> > > > +	struct data d;
> > > > +	struct property *p;
> > > > +
> > > > +	p = get_property(node, name);
> > > > +	if (p) {
> > > > +		const char *s;
> > > > +
> > > > +		for (s = p->val.val; s < p->val.val + p->val.len; s = strchr(s, '\0') + 1) {
> > > 
> > > This isn't quite safe.  You check s is within bounds on each
> > > iteration, but if the property is malformed and doesn't end with a \0,
> > > the strchr() itself could read beyond the property's bounds.
> > > 
> > > strnchr() could work, but is awkward: it would return NULL if there
> > > are no further \0 within the property, and on most systems NULL <
> > > p->val.val + p->val.len would return true, so you'd have to check for
> > > that case separately.  You could use strnlen() instead, but that's
> > > also a bit awkward - it doesn't include the \0, so you'd need to add
> > > +1 - but in the malformed case that would put you one beyond the end
> > > of the buffer.  That would probably work in practice, but creating
> > > pointers beyond the buffer they're within is technically UB.
> > 
> > fair, my approach would be to check p->val.val[p->val.len - 1] == '\0'
> > once before the loop.
> > 
> > > > +			if (strcmp(data, s) == 0)
> > > 
> > > This also relies on the \0 being there.  Any fix for the above, will
> > > probably result in being able to get the length of each string segment
> > > fairly naturally, so it would make sense to use memcmp() instead.
> > > 
> > > > +				/* data already contained in node.name */
> > > > +				return;
> > > > +		}
> > > > +
> > > > +		d = data_add_marker(p->val, TYPE_STRING, name);
> > > > +		d = data_append_data(d, data, len);
> > > > +		p->val = d;
> > > > +	} else {
> > > > +		d = data_add_marker(empty_data, TYPE_STRING, name);
> > > > +		d = data_append_data(d, data, len);
> > > > +		p = build_property(name, d, NULL);
> > > > +		add_property(node, p);
> > > 
> > > You can add the property first, then share the data_append_data()
> > > logic with the previous case.
> > 
> > This is mostly taken from append_to_property(), I will check if your
> > suggested improvement will apply to that one, too.
> 
> I think that would be:

Looks good.

> diff --git a/livetree.c b/livetree.c
> index d51d05830b18..2ccdc9d9c2a7 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -344,16 +344,14 @@ void append_to_property(struct node *node,
>  	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);
>  	}
> +
> +	d = data_add_marker(p->val, type, name);
> +	d = data_append_data(d, data, len);
> +	p->val = d;

You could even do this in place in p->val, avoiding the temporary.

>  }
>  
>  struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
> 
> The test suite is happy with it. I will include that with the next
> revision of my fix patch.
> 
> 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] livetree: Add only new data to fixup nodes instead of complete regeneration

[David Gibson]

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

On Thu, Aug 14, 2025 at 11:05:00AM +0200, Uwe Kleine-König wrote:
> Hello David,
> 
> On Thu, Aug 14, 2025 at 05:36:08PM +1000, David Gibson wrote:
> > On Fri, Aug 01, 2025 at 06:00:31PM +0200, Uwe Kleine-König wrote:
> > >  livetree.c              | 75 +++++++++++++++++++++++++++++++----------
> > >  tests/retain-fixups.dts | 29 ++++++++++++++++
> > >  tests/run_tests.sh      |  5 +++
> > >  3 files changed, 92 insertions(+), 17 deletions(-)
> > >  create mode 100644 tests/retain-fixups.dts
> > > 
> > > diff --git a/livetree.c b/livetree.c
> > > index d51d05830b18..24f7c561d77e 100644
> > > --- a/livetree.c
> > > +++ b/livetree.c
> > > @@ -356,6 +356,60 @@ void append_to_property(struct node *node,
> > >  	}
> > >  }
> > >  
> > > +static void append_unique_str_to_property(struct node *node,
> > > +					  char *name, const char *data, int len)
> > > +{
> > > +	struct data d;
> > > +	struct property *p;
> > > +
> > > +	p = get_property(node, name);
> > > +	if (p) {
> > > +		const char *s;
> > > +
> > > +		for (s = p->val.val; s < p->val.val + p->val.len; s = strchr(s, '\0') + 1) {
> > 
> > This isn't quite safe.  You check s is within bounds on each
> > iteration, but if the property is malformed and doesn't end with a \0,
> > the strchr() itself could read beyond the property's bounds.
> > 
> > strnchr() could work, but is awkward: it would return NULL if there
> > are no further \0 within the property, and on most systems NULL <
> > p->val.val + p->val.len would return true, so you'd have to check for
> > that case separately.  You could use strnlen() instead, but that's
> > also a bit awkward - it doesn't include the \0, so you'd need to add
> > +1 - but in the malformed case that would put you one beyond the end
> > of the buffer.  That would probably work in practice, but creating
> > pointers beyond the buffer they're within is technically UB.
> 
> fair, my approach would be to check p->val.val[p->val.len - 1] == '\0'
> once before the loop.

Ah, yes, that's an easier way to do it.

> > > +			if (strcmp(data, s) == 0)
> > 
> > This also relies on the \0 being there.  Any fix for the above, will
> > probably result in being able to get the length of each string segment
> > fairly naturally, so it would make sense to use memcmp() instead.
> > 
> > > +				/* data already contained in node.name */
> > > +				return;
> > > +		}
> > > +
> > > +		d = data_add_marker(p->val, TYPE_STRING, name);
> > > +		d = data_append_data(d, data, len);
> > > +		p->val = d;
> > > +	} else {
> > > +		d = data_add_marker(empty_data, TYPE_STRING, name);
> > > +		d = data_append_data(d, data, len);
> > > +		p = build_property(name, d, NULL);
> > > +		add_property(node, p);
> > 
> > You can add the property first, then share the data_append_data()
> > logic with the previous case.
> 
> This is mostly taken from append_to_property(), I will check if your
> suggested improvement will apply to that one, too.

Ok, thanks.

> > > @@ -1056,29 +1110,16 @@ 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),
> > > +	generate_fixups_tree_internal(dti, build_root_node(dti->dt, name),
> > >  				      dti->dt);
> > 
> > It's not obvious to me why this change follows from the rest.
> 
> The relevant difference here is that now it's unknown if the __fixups__
> node already exists. build_and_name_child_node() only works if it
> doesn't exist which isn't ensured now any more with the lines deleted
> above.
> 
> This is a revert of 915daadbb62d.

Ah, yes I see.  Thanks.

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