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@xxxxxxxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
