Re: [PATCH v2 2/2] 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: 7488 bytes --]

On Fri, Aug 15, 2025 at 03:34:54PM +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>
> ---
>  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 6127d604d528..f23988483b01 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -352,6 +352,60 @@ void append_to_property(struct node *node,
>  	p->val = data_append_data(p->val, data, len);
>  }
>  
> +static void 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) {
> +		if (p->val.val[p->val.len - 1] == '\0') {
> +			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;
> +			}
> +		} else {
> +			fprintf(stderr, "Warning: appending string to non-string property %s/%s\n",
> +				node->fullpath, name);

I don't think carrying on with the operation when the original
property is malformed is a good idea.  This message will also be
pretty confusing in the one place it will occur.  I think it would be
preferable to return an error code here.  The caller, which knows what
the purpose is can then report that the relevant property is malformed
- if that's the case, I think we can just give up on updating
__fixups__ / __local_fixups__.

> +		}
> +	} 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);
> +}
> +
> +static void append_unique_u32_to_property(struct node *node, char *name, fdt32_t value)
> +{
> +	struct property *p;
> +
> +	p = get_property(node, name);
> +	if (p) {
> +		if (p->val.len % 4 == 0) {
> +			const fdt32_t *v, *val_end = (const fdt32_t *)p->val.val + p->val.len / 4;
> +
> +			for (v = (const void *)p->val.val; v < val_end; v++) {
> +				if (*v == value)
> +					/* value already contained */
> +					return;
> +			}
> +		} else {
> +			fprintf(stderr, "Warning: appending u32 to property %s/%s with an unalign length\n",
> +				node->fullpath, name);

Similar comments here.

> +		}
> +	} 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);
> +}
> +
>  struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
>  {
>  	struct reserve_info *new = xmalloc(sizeof(*new));
> @@ -935,7 +989,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);
>  }
> @@ -1016,7 +1070,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,
> @@ -1052,29 +1106,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..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

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