Re: [PATCH v3 12/15] fdtfactor: Add tool to split DTBs into a base DTB and overlays

[Doug Anderson]

Hi David,

On Tue, Nov 10, 2026 at 8:16 PM David Gibson
<david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Sun, Nov 01, 2026 at 02:01:19PM -0800, Douglas Anderson wrote:
> > diff --git a/README.md b/README.md
> > index fcfc2e55d804..61fefc4424fe 100644
> > --- a/README.md
> > +++ b/README.md
> > @@ -112,6 +112,49 @@ $ meson setup -Dpython=disabled build
> >  More work remains to support all of libfdt, including access to numeric
> >  values.
> >
> > +## fdtfactor
>
> Pre-existing, but it would be nice to have blurbs for dtdiff,
> fdt{get,put} and the other anciliary tools we already include.

Agreed -- in my draft v4 I've added a preparatory patch adding brief
`README.md` sections for `dtc` and the ancillary tools (`dtdiff`,
`fdtget`, `fdtput`, `fdtdump`, `convert-dtsv0`, and `fdtoverlay`).


> > +        self.filename = filename
> > +        with open(filename, 'rb') as f:
> > +            raw_bytes = f.read()
> > +        self.raw_bytes = raw_bytes
>
> AFAICT, self.raw_bytes is never used again (unlike the raw_bytes
> local).

`_print_dtb_stats()` actually used `len(d.raw_bytes)` further down to
compute `orig_uncomp` when `--stats` is passed, though nothing ever
reads the buffer contents again after `__init__()`. In my draft v4 I've
replaced `self.raw_bytes` with `self.uncompressed_size = len(raw_bytes)`
(pairing with `self.compressed_size` in the compression patch) so we
don't keep all the uncompressed DTB buffers in memory just for their
lengths.


> > +            if 'phandle' in node.props:
> > +                node.phandle = node.get_u32('phandle')
> > +                del node.props['phandle']
> > +
> > +            if node.phandle is not None:
>
> Changing this to:
> if node.phandle is None:
> continue
> reduces nesting and IMO, would make this a little more readable.

Done in my draft v4 (and did the same for a couple of other `for` loops
in `fdtfactor`).


> > +                offsets = sorted(set(lf_node.get_u32_list(prop_name)))
>
> I don't think there's any reason to convert the list to a set before sorting.

Done in my draft v4 (`offsets = sorted(lf_prop.as_uint32_list())`).
Neither `dtc` nor `libfdt` emits duplicate offsets in
`/__local_fixups__`, and dropping `set()` also means any duplicate
offset in a malformed blob will be caught by the overlapping-offset
check right below.


> > +    base_root = FactoredTree()
> > +    next_ph = 1
> > +    for path, _ in dtbs[0].root.walk():
> > +        if path not in common_paths:
> > +            continue
>
> I think this makes the loop equivalent to:
> for path in common_paths:
>    ....
>
> > +        if path and base_root.get_node(path.rpartition('/')[0]) is None:
> > +            continue

We can't iterate `common_paths` directly here because `common_paths` is
an unordered `set`:

1. The `base_root.get_node(path.rpartition('/')[0]) is None` check right
   below relies on parents always being visited before their children so
   that if an ancestor was excluded, its descendants are skipped too.
   Iterating a `set` can visit `/a/b` before `/a`, causing `/a/b` to be
   wrongly skipped.
2. Walking `dtbs[0].root.walk()` preserves `dtbs[0]`'s subnode ordering
   in `base_root` and keeps sequential `next_ph` assignment
   deterministic across `PYTHONHASHSEED` values.

I've added a comment above the loop in my draft v4 explaining why it
iterates `dtbs[0]`'s nodes in pre-order instead of iterating
`common_paths`.


> > +        has_phandle = any(
> > +            d.root.get_node(path).phandle is not None for d in dtbs
> > +        )
>
> I expect this will work in practice, but arguably the condition you
> really need here is whether the node is referred to by any fixup in
> any non-common property.
>
> > +        if has_phandle and path and path not in common_sym_targets:
> > +            continue
> > +        base_node = base_root.create_path(path)
> > +        if has_phandle:
> > +            base_node.phandle = next_ph
> > +            next_ph += 1

For the `common_sym_targets` exclusion (`if has_phandle and path and
path not in common_sym_targets: continue`), that's true in principle
with respect to `/__fixups__`: an overlay only needs a symbol in
`base.dtb`'s `/__symbols__` if a non-common property ends up referencing
`path`. There are a couple of catches if we tried to relax this, though:

1. Which properties are common vs. non-common isn't known yet here --
   `_build_base_tree()` decides that next, and its candidate eligibility
   depends on which fixup target nodes are in `base_root` (so resolving
   both together would need an iterative pass).
2. Even when no non-common property references `path`, `base_node` still
   needs a `phandle` in `base.dtb` if `path` is referenced by a *common*
   property in `base.dtb`, or if `path` has a symbol in `/__symbols__`
   (from `dtc -@`) so the reconstructed DTB retains its `phandle`
   property and remains valid as a base for any further runtime overlays
   targeting that symbol. Conversely, if one board had a `phandle` on
   `path` (e.g. a board-specific label) and another board didn't,
   putting `path` in `base.dtb` with a `phandle` would add a `phandle`
   property to the second board's reconstructed DTB that wasn't in its
   original DTB.

Since in `dtc -@` inputs almost every node with a `phandle` also has a
label in `/__symbols__`, checking `has_phandle` up front keeps this
single-pass and exact. I've expanded the docstring on
`_find_common_nodes_and_phandles()` in my draft v4 to note this.

-Doug