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

[David Gibson <david@gibson.dropbear.id.au>]

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