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

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

[Uwe Kleine-König]

Hello,

here comes v2 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.

To address the feedback I got from David in reply to (implicit) v1[1] I
added a cleanup patch to fix a concern in the original code of a
function that only I copied and adapted.

Best regards
Uwe

[1] https://lore.kernel.org/devicetree-compiler/20250801160031.624886-2-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              | 85 +++++++++++++++++++++++++++++------------
 tests/retain-fixups.dts | 29 ++++++++++++++
 tests/run_tests.sh      |  5 +++
 3 files changed, 95 insertions(+), 24 deletions(-)
 create mode 100644 tests/retain-fixups.dts

base-commit: 84d9dd2fcbc865a35d7f04d9b465b05ef286d281
-- 
2.50.1

[PATCH v2 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

[PATCH v2 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              | 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);
+		}
+	} 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);
+		}
+	} 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
-- 
2.50.1

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

[David Gibson]

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

On Fri, Aug 15, 2025 at 03:34:53PM +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.

The patch looks good, but I'll need a Signed-off-by line.

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

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

[David Gibson]

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

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

[Uwe Kleine-König]

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

On Sat, Aug 16, 2025 at 02:54:27PM +1000, David Gibson wrote:
> 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__.

I picked that option to somewhat keep the behaviour of dtc as it was
before (which appended irrespective of the pre-existing content).

But I don't feel strong and can rework it accordingly to your comments.

Best regards
Uwe

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

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

[David Gibson]

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

On Sun, Aug 17, 2025 at 12:40:59PM +0200, Uwe Kleine-König wrote:
> On Sat, Aug 16, 2025 at 02:54:27PM +1000, David Gibson wrote:
> > 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__.
> 
> I picked that option to somewhat keep the behaviour of dtc as it was
> before (which appended irrespective of the pre-existing content).

The rationale make sense, but I think just giving an error is a more
useful behaviour over all.

> But I don't feel strong and can rework it accordingly to your comments.
> 
> 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 --]