Re: [PATCH 4/4] fdtfactor: Add compiled FIT image factoring support

[Doug Anderson]

Hi Simon,

On Mon, Oct 26, 2026 at 8:31 AM Simon Glass <sjg@xxxxxxxxxxxx> wrote:
>
> > diff --git a/fdtfactor b/fdtfactor
> > @@ -1211,6 +1261,588 @@ def verify_recreated_dtb(orig_dtb, base_dtb_blob, dtbo_blob,
> > +    dtbo_comp_sizes = []
> > +    for entry, dtbo_blob in zip(entries, dtbo_blobs):
> > +        img_node = entry['node']
> > +        dtbo_comp_bytes = compress_blob(dtbo_blob, comp)
>
> One thing I'd like to check, since the README says this targets
> U-Boot. In U-Boot, boot_get_fdt_fit() loads each overlay with
> FIT_LOAD_IGNORED, and decomp_image() skips decompression in that case.
> So a compressed overlay goes to fdt_open_into() as-is and fails, which
> means the lz4 FIT that the test produces would not boot with U-Boot. I
> suspect U-Boot should be fixed. Until then it would be worth having an
> option to leave the overlays uncompressed (they are small anyway), or
> at least a note in the README.

Added a note in the README for this.


> > diff --git a/fdtfactor b/fdtfactor
> > @@ -1211,6 +1261,588 @@ def verify_recreated_dtb(orig_dtb, base_dtb_blob, dtbo_blob,
> > +            for sub_name in list(conf_node.children):
> > +                if sub_name.startswith(('signature', 'hash')):
> > +                    print(
> > +                        f'Warning: Stripping invalidated {sub_name} subnode '
>
> This only strips signatures from the configurations that are changed.
> But the whole FIT is rewritten with FdtSw, which lays out the strings
> block in a different order from dtc. A configuration signature also
> covers the strings block (via 'hashed-strings'), so I believe
> signatures on untouched configurations break too. The user has to
> re-sign with mkimage after factoring anyway, so it might be simpler to
> fail when any signature node is present and say so. What do you think?

_extract_fit_fdt_entries() now raises an error if any
/configurations/<conf> has a signature* or hash*(1) subnode or any
/images/<img> has a signature* subnode


> > diff --git a/fdtfactor b/fdtfactor
> > @@ -1211,6 +1261,588 @@ def verify_recreated_dtb(orig_dtb, base_dtb_blob, dtbo_blob,
> > +def _derive_base_dtb_description(entries, base_dtb_node_name, comp):
> > +    """Derive a `description` string for a factored base FDT image node.
>
> This is a lot of heuristics for a human-readable description, and the
> results are not obvious (e.g. ['sc7180-trogdor', 'sc7280-herobrine']
> gives 'sc-base.dtb'). It would be worth using something simple and
> predictable instead, such as 'fdt-base-1.dtb' or a description listing
> the images it serves. That would also simplify the tests.

Yeah, makes sense. Done.


> > diff --git a/fdtfactor b/fdtfactor
> > @@ -1211,6 +1261,588 @@ def verify_recreated_dtb(orig_dtb, base_dtb_blob, dtbo_blob,
> > +        placed = False
> > +        for grp in groups:
> > +            if grp['key'] != header_key:
> > +                continue
> > +            ref_nodes = grp['ref_nodes']
>
> 'ref_nodes' is only the node set of the first entry in each group, so
> the grouping depends on the order of the nodes in /images. For
> example, if the first board in a family has many extra nodes, its
> siblings can end up in a separate group. Comparing against the group's
> common node set (which is what ends up in the base) might be more
> robust. What do you think?

Sounds good. Done.


> For the argparse description, you really have two completely different
> cases...I suggest the FIT part of it should be worded differently so
> it is clear that the result is an update / new FIT.

Reworded in v2.


> mkimage uses -F to indicate an inplace FIT - with -f used for the
> source .its file. It might be better for your tool to use -F instead
> of -f, and then -O (confusing for dtc?) or --outfile to specify
> writing a separate file.

Done.