[PATCH] dtc: When compiling to dts interpret /__symbols__ and /__local_fixups__

[PATCH] dtc: When compiling to dts interpret /__symbols__ and /__local_fixups__

[Uwe Kleine-König]

The data contained in these nodes can be used to restore labels for nodes
(from __symbols__ if -@ is given) and to identify which data values are
references (from __local_fixups__ if -L is given).

The resulting dts doesn't necessarily compiles back into the bitwise same
dtb, but the result is equivalent and easier to modify without breaking
references.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 dtc.c        | 16 +++++++---
 dtc.h        |  2 ++
 livetree.c   | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 treesource.c | 37 +++++++++++++++------
 4 files changed, 132 insertions(+), 13 deletions(-)

diff --git a/dtc.c b/dtc.c
index d2e4e2b55b5c..e36f0dfad313 100644
--- a/dtc.c
+++ b/dtc.c
@@ -335,12 +335,20 @@ int main(int argc, char *argv[])
 	if (auto_label_aliases)
 		generate_label_tree(dti, "aliases", false);
 
-	if (generate_symbols)
-		generate_label_tree(dti, "__symbols__", true);
+	if (generate_symbols) {
+		if (streq(inform, "fs") || streq(inform, "dtb"))
+			generate_labels_from_tree(dti, "__symbols__");
+		else
+			generate_label_tree(dti, "__symbols__", true);
+	}
 
 	if (generate_fixups) {
-		generate_fixups_tree(dti, "__fixups__");
-		generate_local_fixups_tree(dti, "__local_fixups__");
+		if (streq(inform, "fs") || streq(inform, "dtb")) {
+			fixup_local_phandles(dti, "__local_fixups__");
+		} else {
+			generate_fixups_tree(dti, "__fixups__");
+			generate_local_fixups_tree(dti, "__local_fixups__");
+		}
 	}
 
 	if (sort)
diff --git a/dtc.h b/dtc.h
index 4c4aaca1fc41..4c278ed30b26 100644
--- a/dtc.h
+++ b/dtc.h
@@ -337,8 +337,10 @@ struct dt_info *build_dt_info(unsigned int dtsflags,
 			      struct node *tree, uint32_t boot_cpuid_phys);
 void sort_tree(struct dt_info *dti);
 void generate_label_tree(struct dt_info *dti, const char *name, bool allocph);
+void generate_labels_from_tree(struct dt_info *dti, const char *name);
 void generate_fixups_tree(struct dt_info *dti, const char *name);
 void generate_local_fixups_tree(struct dt_info *dti, const char *name);
+void fixup_local_phandles(struct dt_info *dti, const char *name);
 
 /* Checks */
 
diff --git a/livetree.c b/livetree.c
index 0ec47ed41e2b..a4fa5278718e 100644
--- a/livetree.c
+++ b/livetree.c
@@ -1056,6 +1056,25 @@ void generate_label_tree(struct dt_info *dti, const char *name, bool allocph)
 				     dti->dt, allocph);
 }
 
+void generate_labels_from_tree(struct dt_info *dti, const char *name)
+{
+	struct node *an;
+	struct property *p;
+
+	an = get_subnode(dti->dt, name);
+	if (!an)
+		return;
+
+	for_each_property(an, p) {
+		struct node *labeled_node;
+
+		labeled_node = get_node_by_path(dti->dt, p->val.val);
+		add_label(&labeled_node->labels, p->name);
+	}
+
+	delete_node(an);
+}
+
 void generate_fixups_tree(struct dt_info *dti, const char *name)
 {
 	if (!any_fixup_tree(dti, dti->dt))
@@ -1071,3 +1090,74 @@ void generate_local_fixups_tree(struct dt_info *dti, const char *name)
 	generate_local_fixups_tree_internal(dti, build_root_node(dti->dt, name),
 					    dti->dt);
 }
+
+static void fixup_local_phandles_node(struct node *lf, struct node *n)
+{
+	struct property *lfp;
+	struct node *lfsubnode;
+
+	for_each_property(lf, lfp) {
+		struct property *p = get_property(n, lfp->name);
+		size_t i;
+
+		if (!p || p->val.len % sizeof(fdt32_t))
+			die("invalid length of property %s in node %s\n", lfp->name, lf->fullpath);
+
+		for (i = 0; i < lfp->val.len; i += sizeof(fdt32_t)) {
+			struct marker *m, **mi;
+
+			m = xmalloc(sizeof(*m));
+			m->offset = fdt32_to_cpu(*(fdt32_t *)(lfp->val.val + i));
+			m->type = REF_PHANDLE;
+			m->ref = NULL;
+
+			/* keep the list sorted in ascending ->offset order */
+			for (mi = &p->val.markers; *mi && (*mi)->offset < m->offset;)
+				mi = &(*mi)->next;
+
+			m->next = *mi;
+			*mi = m;
+		}
+	}
+
+	for_each_child(lf, lfsubnode) {
+		struct node *subnode = get_subnode(n, lfsubnode->name);
+
+		if (!subnode)
+			die("fixup for non-existent node in %s\n", lfsubnode->fullpath);
+
+		fixup_local_phandles_node(lfsubnode, subnode);
+	}
+}
+
+static void drop_phandles(struct node *n)
+{
+	struct property *p;
+	struct node *sn;
+
+	p = get_property(n, "phandle");
+	if (p)
+		delete_property(p);
+
+	p = get_property(n, "linux,phandle");
+	if (p)
+		delete_property(p);
+
+	for_each_child(n, sn)
+		drop_phandles(sn);
+}
+
+void fixup_local_phandles(struct dt_info *dti, const char *name)
+{
+	struct node *an;
+
+	an = get_subnode(dti->dt, name);
+	if (!an)
+		return;
+
+	fixup_local_phandles_node(an, dti->dt);
+
+	drop_phandles(dti->dt);
+
+	delete_node(an);
+}
diff --git a/treesource.c b/treesource.c
index de30188189fb..32f3171c14c3 100644
--- a/treesource.c
+++ b/treesource.c
@@ -172,7 +172,7 @@ static enum markertype guess_value_type(struct property *prop)
 	return TYPE_UINT8;
 }
 
-static void write_propval(FILE *f, struct property *prop)
+static void write_propval(FILE *f, struct node *root, struct property *prop)
 {
 	size_t len = prop->val.len;
 	struct marker *m = prop->val.markers;
@@ -219,6 +219,9 @@ static void write_propval(FILE *f, struct property *prop)
 		if (emit_type == TYPE_NONE || chunk_len == 0)
 			continue;
 
+		if (m->offset != 0)
+			fputc(' ', f);
+
 		switch(emit_type) {
 		case TYPE_UINT16:
 			write_propval_int(f, p, chunk_len, 2);
@@ -230,10 +233,26 @@ static void write_propval(FILE *f, struct property *prop)
 					break;
 
 			if (m_phandle) {
-				if (m_phandle->ref[0] == '/')
-					fprintf(f, "&{%s}", m_phandle->ref);
-				else
-					fprintf(f, "&%s", m_phandle->ref);
+				if (m_phandle->ref) {
+					if (m_phandle->ref[0] == '/')
+						fprintf(f, "&{%s}", m_phandle->ref);
+					else
+						fprintf(f, "&%s", m_phandle->ref);
+				} else {
+					cell_t phandle = fdt32_to_cpu(*(const fdt32_t *)p);
+					struct node *n = get_node_by_phandle(root, phandle);
+
+					if (!n) {
+						fprintf(f, "&??");
+					} else {
+						if (n->labels) {
+							fprintf(f, "&%s", n->labels->label);
+						} else {
+							fprintf(f, "&{%s}", n->fullpath);
+						}
+					}
+
+				}
 				if (chunk_len > 4) {
 					fputc(' ', f);
 					write_propval_int(f, p + 4, chunk_len - 4, 4);
@@ -270,7 +289,7 @@ static void write_propval(FILE *f, struct property *prop)
 	fprintf(f, "\n");
 }
 
-static void write_tree_source_node(FILE *f, struct node *tree, int level)
+static void write_tree_source_node(FILE *f, struct node *root, struct node *tree, int level)
 {
 	struct property *prop;
 	struct node *child;
@@ -299,11 +318,11 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
 		for_each_label(prop->labels, l)
 			fprintf(f, "%s: ", l->label);
 		fprintf(f, "%s", prop->name);
-		write_propval(f, prop);
+		write_propval(f, root, prop);
 	}
 	for_each_child(tree, child) {
 		fprintf(f, "\n");
-		write_tree_source_node(f, child, level+1);
+		write_tree_source_node(f, root, child, level+1);
 	}
 	write_prefix(f, level);
 	fprintf(f, "};");
@@ -333,5 +352,5 @@ void dt_to_source(FILE *f, struct dt_info *dti)
 			(unsigned long long)re->size);
 	}
 
-	write_tree_source_node(f, dti->dt, 0);
+	write_tree_source_node(f, dti->dt, dti->dt, 0);
 }
-- 
2.39.2

Re: [PATCH] dtc: When compiling to dts interpret /__symbols__ and /__local_fixups__

[David Gibson]

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

On Wed, Apr 26, 2023 at 08:25:58PM +0200, Uwe Kleine-König wrote:
> The data contained in these nodes can be used to restore labels for nodes
> (from __symbols__ if -@ is given) and to identify which data values are
> references (from __local_fixups__ if -L is given).

Oh, that's a neat idea.

> The resulting dts doesn't necessarily compiles back into the bitwise same
> dtb,

Hmm.. in exactly what ways can it differ?  Is that different from what
could happen before?  Generally a dtb -> dts -> dtb round trip should
be a no-op to a pretty high degree of fidelity.  That's not true of a
dts -> dtb -> dts round trip, since there are many more ways to
express things in source form.

> but the result is equivalent and easier to modify without breaking
> references.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  dtc.c        | 16 +++++++---
>  dtc.h        |  2 ++
>  livetree.c   | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  treesource.c | 37 +++++++++++++++------
>  4 files changed, 132 insertions(+), 13 deletions(-)
> 
> diff --git a/dtc.c b/dtc.c
> index d2e4e2b55b5c..e36f0dfad313 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -335,12 +335,20 @@ int main(int argc, char *argv[])
>  	if (auto_label_aliases)
>  		generate_label_tree(dti, "aliases", false);
>  
> -	if (generate_symbols)
> -		generate_label_tree(dti, "__symbols__", true);
> +	if (generate_symbols) {
> +		if (streq(inform, "fs") || streq(inform, "dtb"))
> +			generate_labels_from_tree(dti, "__symbols__");
> +		else
> +			generate_label_tree(dti, "__symbols__", true);

Changing behaviour of what we do to the tree based on the input format
smells a bit wrong to me.  Should we maybe be importing any existing
label information from __symbols__, then re-generating it all cases
(effectively merging any symbols from source parsing with ones given
in __symbols__).

> +	}
>  
>  	if (generate_fixups) {
> -		generate_fixups_tree(dti, "__fixups__");
> -		generate_local_fixups_tree(dti, "__local_fixups__");
> +		if (streq(inform, "fs") || streq(inform, "dtb")) {

Similar question here.

> +			fixup_local_phandles(dti, "__local_fixups__");
> +		} else {
> +			generate_fixups_tree(dti, "__fixups__");
> +			generate_local_fixups_tree(dti, "__local_fixups__");
> +		}
>  	}
>  
>  	if (sort)
> diff --git a/dtc.h b/dtc.h
> index 4c4aaca1fc41..4c278ed30b26 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -337,8 +337,10 @@ struct dt_info *build_dt_info(unsigned int dtsflags,
>  			      struct node *tree, uint32_t boot_cpuid_phys);
>  void sort_tree(struct dt_info *dti);
>  void generate_label_tree(struct dt_info *dti, const char *name, bool allocph);
> +void generate_labels_from_tree(struct dt_info *dti, const char *name);
>  void generate_fixups_tree(struct dt_info *dti, const char *name);
>  void generate_local_fixups_tree(struct dt_info *dti, const char *name);
> +void fixup_local_phandles(struct dt_info *dti, const char *name);
>  
>  /* Checks */
>  
> diff --git a/livetree.c b/livetree.c
> index 0ec47ed41e2b..a4fa5278718e 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -1056,6 +1056,25 @@ void generate_label_tree(struct dt_info *dti, const char *name, bool allocph)
>  				     dti->dt, allocph);
>  }
>  
> +void generate_labels_from_tree(struct dt_info *dti, const char *name)
> +{
> +	struct node *an;
> +	struct property *p;
> +
> +	an = get_subnode(dti->dt, name);
> +	if (!an)
> +		return;
> +
> +	for_each_property(an, p) {
> +		struct node *labeled_node;
> +
> +		labeled_node = get_node_by_path(dti->dt, p->val.val);

Need to handle failures here, if the given path doesn't exist.

> +		add_label(&labeled_node->labels, p->name);
> +	}
> +
> +	delete_node(an);
> +}
> +
>  void generate_fixups_tree(struct dt_info *dti, const char *name)
>  {
>  	if (!any_fixup_tree(dti, dti->dt))
> @@ -1071,3 +1090,74 @@ void generate_local_fixups_tree(struct dt_info *dti, const char *name)
>  	generate_local_fixups_tree_internal(dti, build_root_node(dti->dt, name),
>  					    dti->dt);
>  }
> +
> +static void fixup_local_phandles_node(struct node *lf, struct node *n)
> +{
> +	struct property *lfp;
> +	struct node *lfsubnode;
> +
> +	for_each_property(lf, lfp) {
> +		struct property *p = get_property(n, lfp->name);

You shouldn't need this, lfp should already be the property you want.

> +		size_t i;
> +
> +		if (!p || p->val.len % sizeof(fdt32_t))
> +			die("invalid length of property %s in node %s\n", lfp->name, lf->fullpath);

It would be really nice for this not to be a fatal error.

> +		for (i = 0; i < lfp->val.len; i += sizeof(fdt32_t)) {
> +			struct marker *m, **mi;
> +
> +			m = xmalloc(sizeof(*m));
> +			m->offset = fdt32_to_cpu(*(fdt32_t *)(lfp->val.val + i));

I think this would be cleaner if you case the whole property to an
array of integers.  See for example how 'cells' is handled in
check_pci_bridge().

> +			m->type = REF_PHANDLE;
> +			m->ref = NULL;

Hmm.. so this introduces a whole new state for a marker to be in (ref
== NULL), with new internal semantics.  Have you checked that every
single place that looks at the markers handles this case?


> +
> +			/* keep the list sorted in ascending ->offset order */
> +			for (mi = &p->val.markers; *mi && (*mi)->offset < m->offset;)
> +				mi = &(*mi)->next;
> +
> +			m->next = *mi;
> +			*mi = m;

Probably cleaner if you add a helper for this operation, say
'data_insert_marker()'.

> +		}
> +	}
> +
> +	for_each_child(lf, lfsubnode) {
> +		struct node *subnode = get_subnode(n, lfsubnode->name);
> +
> +		if (!subnode)
> +			die("fixup for non-existent node in %s\n", lfsubnode->fullpath);

Again, because this is, arguably, just adding cosmetic information it
would be much nicer if it wasn't a fatal error.

> +		fixup_local_phandles_node(lfsubnode, subnode);
> +	}
> +}
> +
> +static void drop_phandles(struct node *n)

I don't really understand why you want to remove the phandle properties.

> +{
> +	struct property *p;
> +	struct node *sn;
> +
> +	p = get_property(n, "phandle");
> +	if (p)
> +		delete_property(p);
> +
> +	p = get_property(n, "linux,phandle");
> +	if (p)
> +		delete_property(p);
> +
> +	for_each_child(n, sn)
> +		drop_phandles(sn);
> +}
> +
> +void fixup_local_phandles(struct dt_info *dti, const char *name)
> +{
> +	struct node *an;
> +
> +	an = get_subnode(dti->dt, name);
> +	if (!an)
> +		return;
> +
> +	fixup_local_phandles_node(an, dti->dt);
> +
> +	drop_phandles(dti->dt);
> +
> +	delete_node(an);
> +}
> diff --git a/treesource.c b/treesource.c
> index de30188189fb..32f3171c14c3 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -172,7 +172,7 @@ static enum markertype guess_value_type(struct property *prop)
>  	return TYPE_UINT8;
>  }
>  
> -static void write_propval(FILE *f, struct property *prop)
> +static void write_propval(FILE *f, struct node *root, struct property *prop)
>  {
>  	size_t len = prop->val.len;
>  	struct marker *m = prop->val.markers;
> @@ -219,6 +219,9 @@ static void write_propval(FILE *f, struct property *prop)
>  		if (emit_type == TYPE_NONE || chunk_len == 0)
>  			continue;
>  
> +		if (m->offset != 0)
> +			fputc(' ', f);

I'm not sure how this change is related to anything else.

>  		switch(emit_type) {
>  		case TYPE_UINT16:
>  			write_propval_int(f, p, chunk_len, 2);
> @@ -230,10 +233,26 @@ static void write_propval(FILE *f, struct property *prop)
>  					break;
>  
>  			if (m_phandle) {
> -				if (m_phandle->ref[0] == '/')
> -					fprintf(f, "&{%s}", m_phandle->ref);
> -				else
> -					fprintf(f, "&%s", m_phandle->ref);
> +				if (m_phandle->ref) {
> +					if (m_phandle->ref[0] == '/')
> +						fprintf(f, "&{%s}", m_phandle->ref);
> +					else
> +						fprintf(f, "&%s", m_phandle->ref);
> +				} else {
> +					cell_t phandle = fdt32_to_cpu(*(const fdt32_t *)p);
> +					struct node *n = get_node_by_phandle(root, phandle);
> +
> +					if (!n) {
> +						fprintf(f, "&??");

In this case you will present strictly less information than before
"&??" instead of whatever integer value was there.  That doesn't seem
ideal.

> +					} else {
> +						if (n->labels) {
> +							fprintf(f, "&%s", n->labels->label);
> +						} else {
> +							fprintf(f, "&{%s}", n->fullpath);

DT paths can have some slightly odd characters in them, have you
verified that you don't need any escaping here?

> +						}
> +					}
> +
> +				}
>  				if (chunk_len > 4) {
>  					fputc(' ', f);
>  					write_propval_int(f, p + 4, chunk_len - 4, 4);
> @@ -270,7 +289,7 @@ static void write_propval(FILE *f, struct property *prop)
>  	fprintf(f, "\n");
>  }
>  
> -static void write_tree_source_node(FILE *f, struct node *tree, int level)
> +static void write_tree_source_node(FILE *f, struct node *root, struct node *tree, int level)
>  {
>  	struct property *prop;
>  	struct node *child;
> @@ -299,11 +318,11 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
>  		for_each_label(prop->labels, l)
>  			fprintf(f, "%s: ", l->label);
>  		fprintf(f, "%s", prop->name);
> -		write_propval(f, prop);
> +		write_propval(f, root, prop);
>  	}
>  	for_each_child(tree, child) {
>  		fprintf(f, "\n");
> -		write_tree_source_node(f, child, level+1);
> +		write_tree_source_node(f, root, child, level+1);
>  	}
>  	write_prefix(f, level);
>  	fprintf(f, "};");
> @@ -333,5 +352,5 @@ void dt_to_source(FILE *f, struct dt_info *dti)
>  			(unsigned long long)re->size);
>  	}
>  
> -	write_tree_source_node(f, dti->dt, 0);
> +	write_tree_source_node(f, dti->dt, dti->dt, 0);
>  }

-- 
David Gibson			| 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] dtc: When compiling to dts interpret /__symbols__ and /__local_fixups__

[Uwe Kleine-König]

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

Hello,

On Fri, Apr 28, 2023 at 04:11:45PM +1000, David Gibson wrote:
> On Wed, Apr 26, 2023 at 08:25:58PM +0200, Uwe Kleine-König wrote:
> > The data contained in these nodes can be used to restore labels for nodes
> > (from __symbols__ if -@ is given) and to identify which data values are
> > references (from __local_fixups__ if -L is given).
> 
> Oh, that's a neat idea.

:-)

> > The resulting dts doesn't necessarily compiles back into the bitwise same
> > dtb,
> 
> Hmm.. in exactly what ways can it differ?  Is that different from what
> could happen before?  Generally a dtb -> dts -> dtb round trip should
> be a no-op to a pretty high degree of fidelity.  That's not true of a
> dts -> dtb -> dts round trip, since there are many more ways to
> express things in source form.

The actual values of the phandle properties might differ, and the order
of the contents of the __fixup__, __local_fixup__ and __symbol__ nodes.

> 
> > but the result is equivalent and easier to modify without breaking
> > references.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > ---
> >  dtc.c        | 16 +++++++---
> >  dtc.h        |  2 ++
> >  livetree.c   | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  treesource.c | 37 +++++++++++++++------
> >  4 files changed, 132 insertions(+), 13 deletions(-)
> > 
> > diff --git a/dtc.c b/dtc.c
> > index d2e4e2b55b5c..e36f0dfad313 100644
> > --- a/dtc.c
> > +++ b/dtc.c
> > @@ -335,12 +335,20 @@ int main(int argc, char *argv[])
> >  	if (auto_label_aliases)
> >  		generate_label_tree(dti, "aliases", false);
> >  
> > -	if (generate_symbols)
> > -		generate_label_tree(dti, "__symbols__", true);
> > +	if (generate_symbols) {
> > +		if (streq(inform, "fs") || streq(inform, "dtb"))
> > +			generate_labels_from_tree(dti, "__symbols__");
> > +		else
> > +			generate_label_tree(dti, "__symbols__", true);
> 
> Changing behaviour of what we do to the tree based on the input format
> smells a bit wrong to me.  Should we maybe be importing any existing
> label information from __symbols__, then re-generating it all cases
> (effectively merging any symbols from source parsing with ones given
> in __symbols__).

Yeah, that sounds sensible. (Note however that with inform "fs" and
"dtb" there are no labels in dti, so there is nothing lost here.)

> > +	}
> >  
> >  	if (generate_fixups) {
> > -		generate_fixups_tree(dti, "__fixups__");
> > -		generate_local_fixups_tree(dti, "__local_fixups__");
> > +		if (streq(inform, "fs") || streq(inform, "dtb")) {
> 
> Similar question here.

Similar answer here. :-)

I'll rework the patch.

> > [...]
> > +static void drop_phandles(struct node *n)
> 
> I don't really understand why you want to remove the phandle properties.

As the phandle references are restored the phandle properties hold only
duplicate information and they might make it harder to further work with
the resulting dts.

> > @@ -219,6 +219,9 @@ static void write_propval(FILE *f, struct property *prop)
> >  		if (emit_type == TYPE_NONE || chunk_len == 0)
> >  			continue;
> >  
> > +		if (m->offset != 0)
> > +			fputc(' ', f);
> 
> I'm not sure how this change is related to anything else.

Without this, the resulting dts might have:

	clocks = <&clk 17&clk 19>;

I think before my patch this never happend in practise because an array
never had more than one marker?!

> >  		switch(emit_type) {
> >  		case TYPE_UINT16:
> >  			write_propval_int(f, p, chunk_len, 2);
> > @@ -230,10 +233,26 @@ static void write_propval(FILE *f, struct property *prop)
> >  					break;
> >  
> >  			if (m_phandle) {
> > -				if (m_phandle->ref[0] == '/')
> > -					fprintf(f, "&{%s}", m_phandle->ref);
> > -				else
> > -					fprintf(f, "&%s", m_phandle->ref);
> > +				if (m_phandle->ref) {
> > +					if (m_phandle->ref[0] == '/')
> > +						fprintf(f, "&{%s}", m_phandle->ref);
> > +					else
> > +						fprintf(f, "&%s", m_phandle->ref);
> > +				} else {
> > +					cell_t phandle = fdt32_to_cpu(*(const fdt32_t *)p);
> > +					struct node *n = get_node_by_phandle(root, phandle);
> > +
> > +					if (!n) {
> > +						fprintf(f, "&??");
> 
> In this case you will present strictly less information than before
> "&??" instead of whatever integer value was there.  That doesn't seem
> ideal.

I don't think that keeping the previous integer value is beneficial. The
phandle properties generated when compiling to dtb again will likely
differ on recompilation and so the previous value might lead to silent
inconsistencies. So this should result in some human attention.
Generating a broken dts is one option (the one I chose), I think the
only other half-way sane option is to abort with an error.

> > +					} else {
> > +						if (n->labels) {
> > +							fprintf(f, "&%s", n->labels->label);
> > +						} else {
> > +							fprintf(f, "&{%s}", n->fullpath);
> 
> DT paths can have some slightly odd characters in them, have you
> verified that you don't need any escaping here?

what would need escaping? A '}'? Looking at the grammar files I don't
spot a way to quote things here. Am I missing anything?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH] dtc: When compiling to dts interpret /__symbols__ and /__local_fixups__

[David Gibson]

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

On Fri, Apr 28, 2023 at 09:11:30PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Apr 28, 2023 at 04:11:45PM +1000, David Gibson wrote:
> > On Wed, Apr 26, 2023 at 08:25:58PM +0200, Uwe Kleine-König wrote:
> > > The data contained in these nodes can be used to restore labels for nodes
> > > (from __symbols__ if -@ is given) and to identify which data values are
> > > references (from __local_fixups__ if -L is given).
> > 
> > Oh, that's a neat idea.
> 
> :-)
> 
> > > The resulting dts doesn't necessarily compiles back into the bitwise same
> > > dtb,
> > 
> > Hmm.. in exactly what ways can it differ?  Is that different from what
> > could happen before?  Generally a dtb -> dts -> dtb round trip should
> > be a no-op to a pretty high degree of fidelity.  That's not true of a
> > dts -> dtb -> dts round trip, since there are many more ways to
> > express things in source form.
> 
> The actual values of the phandle properties might differ,

Oof.. sorry, I don't think that's an acceptable change for a round
trip of this type.  Couldn't you avoid that if you don't strip out the
'phandle' properties?

> and the order
> of the contents of the __fixup__, __local_fixup__ and __symbol__ nodes.

This.. isn't ideal, but might be tolerable.

> > > but the result is equivalent and easier to modify without breaking
> > > references.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > ---
> > >  dtc.c        | 16 +++++++---
> > >  dtc.h        |  2 ++
> > >  livetree.c   | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  treesource.c | 37 +++++++++++++++------
> > >  4 files changed, 132 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/dtc.c b/dtc.c
> > > index d2e4e2b55b5c..e36f0dfad313 100644
> > > --- a/dtc.c
> > > +++ b/dtc.c
> > > @@ -335,12 +335,20 @@ int main(int argc, char *argv[])
> > >  	if (auto_label_aliases)
> > >  		generate_label_tree(dti, "aliases", false);
> > >  
> > > -	if (generate_symbols)
> > > -		generate_label_tree(dti, "__symbols__", true);
> > > +	if (generate_symbols) {
> > > +		if (streq(inform, "fs") || streq(inform, "dtb"))
> > > +			generate_labels_from_tree(dti, "__symbols__");
> > > +		else
> > > +			generate_label_tree(dti, "__symbols__", true);
> > 
> > Changing behaviour of what we do to the tree based on the input format
> > smells a bit wrong to me.  Should we maybe be importing any existing
> > label information from __symbols__, then re-generating it all cases
> > (effectively merging any symbols from source parsing with ones given
> > in __symbols__).
> 
> Yeah, that sounds sensible. (Note however that with inform "fs" and
> "dtb" there are no labels in dti, so there is nothing lost here.)
> > > +	}
> > >  
> > >  	if (generate_fixups) {
> > > -		generate_fixups_tree(dti, "__fixups__");
> > > -		generate_local_fixups_tree(dti, "__local_fixups__");
> > > +		if (streq(inform, "fs") || streq(inform, "dtb")) {
> > 
> > Similar question here.
> 
> Similar answer here. :-)
> 
> I'll rework the patch.

Thanks.

> > > [...]
> > > +static void drop_phandles(struct node *n)
> > 
> > I don't really understand why you want to remove the phandle properties.
> 
> As the phandle references are restored the phandle properties hold only
> duplicate information and they might make it harder to further work with
> the resulting dts.

Hrm.. that's not really true, though, since without those you lose the
exact numerical values of the phandles.  Usually that's unimportant of
course, but in certain debugging situations it might matter.

> > > @@ -219,6 +219,9 @@ static void write_propval(FILE *f, struct property *prop)
> > >  		if (emit_type == TYPE_NONE || chunk_len == 0)
> > >  			continue;
> > >  
> > > +		if (m->offset != 0)
> > > +			fputc(' ', f);
> > 
> > I'm not sure how this change is related to anything else.
> 
> Without this, the resulting dts might have:
> 
> 	clocks = <&clk 17&clk 19>;
> 
> I think before my patch this never happend in practise because an array
> never had more than one marker?!

Huh, right.  I'd prefer to see this bugfix separated out into a
preliminary patch.

> > >  		switch(emit_type) {
> > >  		case TYPE_UINT16:
> > >  			write_propval_int(f, p, chunk_len, 2);
> > > @@ -230,10 +233,26 @@ static void write_propval(FILE *f, struct property *prop)
> > >  					break;
> > >  
> > >  			if (m_phandle) {
> > > -				if (m_phandle->ref[0] == '/')
> > > -					fprintf(f, "&{%s}", m_phandle->ref);
> > > -				else
> > > -					fprintf(f, "&%s", m_phandle->ref);
> > > +				if (m_phandle->ref) {
> > > +					if (m_phandle->ref[0] == '/')
> > > +						fprintf(f, "&{%s}", m_phandle->ref);
> > > +					else
> > > +						fprintf(f, "&%s", m_phandle->ref);
> > > +				} else {
> > > +					cell_t phandle = fdt32_to_cpu(*(const fdt32_t *)p);
> > > +					struct node *n = get_node_by_phandle(root, phandle);
> > > +
> > > +					if (!n) {
> > > +						fprintf(f, "&??");
> > 
> > In this case you will present strictly less information than before
> > "&??" instead of whatever integer value was there.  That doesn't seem
> > ideal.
> 
> I don't think that keeping the previous integer value is beneficial. The
> phandle properties generated when compiling to dtb again will likely
> differ on recompilation and so the previous value might lead to silent
> inconsistencies. So this should result in some human attention.
> Generating a broken dts is one option (the one I chose), I think the
> only other half-way sane option is to abort with an error.
> 
> > > +					} else {
> > > +						if (n->labels) {
> > > +							fprintf(f, "&%s", n->labels->label);
> > > +						} else {
> > > +							fprintf(f, "&{%s}", n->fullpath);
> > 
> > DT paths can have some slightly odd characters in them, have you
> > verified that you don't need any escaping here?
> 
> what would need escaping? A '}'? Looking at the grammar files I don't
> spot a way to quote things here. Am I missing anything?

Ah.. yes, I think you're right.  Actually, it more or less has to be
ok, because we don't do any de-escaping when parsing references like
that.

-- 
David Gibson			| 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] dtc: When compiling to dts interpret /__symbols__ and /__local_fixups__

[Uwe Kleine-König]

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

Hello,

On Fri, Apr 28, 2023 at 09:11:45PM +0200, Uwe Kleine-König wrote:
> > > @@ -219,6 +219,9 @@ static void write_propval(FILE *f, struct property *prop)
> > >  		if (emit_type == TYPE_NONE || chunk_len == 0)
> > >  			continue;
> > >  
> > > +		if (m->offset != 0)
> > > +			fputc(' ', f);
> > 
> > I'm not sure how this change is related to anything else.
> 
> Without this, the resulting dts might have:
> 
> 	clocks = <&clk 17&clk 19>;
> 
> I think before my patch this never happend in practise because an array
> never had more than one marker?!

That hunk is wrong, but I don't see the right fix. A reproducer is:

diff --git a/tests/type-preservation.dts b/tests/type-preservation.dts
index 921ea21172d1..38e97cee6353 100644
--- a/tests/type-preservation.dts
+++ b/tests/type-preservation.dts
@@ -18,6 +18,7 @@
 		a-string-with-nulls = "foo\0bar", "baz";
 		a-phandle = <&subsub1>;
 		a-phandle-with-args = <&subsub1 0x00 0x01>, <&subsub1 0x02 0x03>;
+		another-phandle-with-args = <&subsub1 0x00 0x01 &subsub1 0x02 0x03>;
 
 		subsub1: subsubnode {
 			compatible = "subsubnode1", "subsubnode";

Maybe you see the right thing to do?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH] dtc: When compiling to dts interpret /__symbols__ and /__local_fixups__

[David Gibson]

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

On Wed, May 03, 2023 at 11:05:35PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Apr 28, 2023 at 09:11:45PM +0200, Uwe Kleine-König wrote:
> > > > @@ -219,6 +219,9 @@ static void write_propval(FILE *f, struct property *prop)
> > > >  		if (emit_type == TYPE_NONE || chunk_len == 0)
> > > >  			continue;
> > > >  
> > > > +		if (m->offset != 0)
> > > > +			fputc(' ', f);
> > > 
> > > I'm not sure how this change is related to anything else.
> > 
> > Without this, the resulting dts might have:
> > 
> > 	clocks = <&clk 17&clk 19>;
> > 
> > I think before my patch this never happend in practise because an array
> > never had more than one marker?!
> 
> That hunk is wrong, but I don't see the right fix. A reproducer is:
> 
> diff --git a/tests/type-preservation.dts b/tests/type-preservation.dts
> index 921ea21172d1..38e97cee6353 100644
> --- a/tests/type-preservation.dts
> +++ b/tests/type-preservation.dts
> @@ -18,6 +18,7 @@
>  		a-string-with-nulls = "foo\0bar", "baz";
>  		a-phandle = <&subsub1>;
>  		a-phandle-with-args = <&subsub1 0x00 0x01>, <&subsub1 0x02 0x03>;
> +		another-phandle-with-args = <&subsub1 0x00 0x01 &subsub1 0x02 0x03>;
>  
>  		subsub1: subsubnode {
>  			compatible = "subsubnode1", "subsubnode";
> 
> Maybe you see the right thing to do?

Oof, finally got a chance to look at this.  Thanks for the simple
reproducer.  write_propval() is an even worse mess than I suspected
:(.  I think I figured out a fix, which I've just committed as
3b02a94b486f998aa22d898b427820a805d0904f.

All that type preservation and yaml stuff is really ghastly though.
If you had a chance to submit poatches which tore it out, I'd be
pretty grateful.

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