Re: [PATCH v2 06/10] libfdt: overlay: Update base /__local_fixups__ on apply

[David Gibson <david@gibson.dropbear.id.au>]

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

On Sun, Nov 01, 2026 at 02:05:09PM -0800, Doug Anderson wrote:
> Hi David,
> 
> On Fri, Oct 30, 2026 at 3:24 AM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > On Wed, Oct 28, 2026 at 02:55:56PM -0700, Douglas Anderson wrote:
> > > When `fdt_overlay_apply()` merges an overlay into a base device tree, it
> > > updates `/__symbols__` in the base tree if present, but previously left
> > > the base tree's `/__local_fixups__` node untouched.
> > >
> > > Add `overlay_local_fixups_update()` so that when the base tree contains
> > > a `/__local_fixups__` node, `fdt_overlay_apply()` updates it to reflect
> > > the applied overlay:
> > > - For each fragment in application order, any local fixup properties in
> > >   the base tree's `/__local_fixups__` that correspond to properties
> > >   overwritten by that fragment are cleared first.
> > > - Local phandle references from that fragment's subtree under the
> > >   overlay's `/__local_fixups__/<fragment>/__overlay__` are merged into
> > >   the corresponding target path under the base tree's
> > >   `/__local_fixups__`.
> > > - External phandle references for that fragment from the overlay's
> > >   `/__fixups__` (which are now resolved to local phandles in the merged
> > >   base tree) are added in sorted order to the base tree's
> > >   `/__local_fixups__`.
> > > - Any subnodes under the base tree's `/__local_fixups__` that become
> > >   empty after clearing overwritten properties are pruned.
> > >
> > > If the base tree does not have a `/__local_fixups__` node, nothing is
> > > added.
> >
> > So, this turned out to be substantially more involved than I realised.
> > I wonder if it might be better to make --strip-locals the default, and
> > only add support for preserving the local fixups as a later extension.
> 
> In v3 I went ahead and addressed all of your and Simon's review comments on
> this patch (which also trimmed ~80 lines of C from `fdt_overlay.c`).

Oh good :).

> To address Simon's point about code size in constrained environments like
> U-Boot SPL, I also gated `/__local_fixups__` handling in
> `fdt_overlay_apply()` behind a 3-state compile-time
> `FDT_OVERLAY_LOCAL_FIXUPS` option in `libfdt/libfdt_internal.h`:
> 
> - `0` (`FDT_OVERLAY_LOCAL_FIXUPS_IGNORE`): Leave `/__local_fixups__`
>   untouched. I made this the `#ifndef` fallback in `libfdt_internal.h` so
>   that embedded projects that vendor `libfdt` directly get zero code-size
>   increase and zero behavior change unless they opt in.
> - `1` (`FDT_OVERLAY_LOCAL_FIXUPS_STRIP`): Delete `/__local_fixups__` from
>   the base DTB if present (~50 bytes of code), so a stale/half-working
>   `/__local_fixups__` node isn't left behind in the merged tree.
> - `2` (`FDT_OVERLAY_LOCAL_FIXUPS_UPDATE`): Fully update `/__local_fixups__`
>   in the base DTB. This is enabled by default in `dtc`'s Meson
>   (`-Doverlay-local-fixups=2`) and Make (`OVERLAY_LOCAL_FIXUPS=2`) builds
>   for desktop tools and tests.
> 
> Even when `fdtfactor -S` is used, we actually do need `fdtoverlay` to
> update `/__local_fixups__` for two reasons:
> 
> 1. **Kernel composite DTBs (`foo-dtbs := base.dtb bar.dtbo`)**: In the
>    Linux kernel build (`scripts/Makefile.dtbs`), many upstream boards are
>    built as composite DTBs using `cmd_fdtoverlay` (`fdtoverlay -i base.dtb
>    -o foo.dtb bar.dtbo`) *before* `fdtfactor` runs on them. As Simon
>    pointed out on the kernel series [1], if `fdtoverlay` left
>    `/__local_fixups__` untouched (`IGNORE`), `foo.dtb` would still have
>    `base.dtb`'s `/__local_fixups__` node (missing all phandle references
>    from `bar.dtbo` and retaining stale offsets for any overwritten
>    properties), causing `fdtfactor` to silently miss or corrupt phandle
>    references when factoring `foo.dtb`. And if `fdtoverlay` stripped
>    `/__local_fixups__`, `foo.dtb` wouldn't have `/__local_fixups__` at all
>    and `fdtfactor` wouldn't be able to factor any composite DTB.

Ah, right.

> 2. **`fdtfactor` self-tests**: Since `fdtfactor` inputs are compiled with
>    `dtc -@ -L`, having `fdtoverlay` update `/__local_fixups__` lets the
>    test suite directly verify with `dtbs_equal_unordered` that recombining
>    `base.dtb` + `board.dtbo` produces an exact match for the original input
>    `.dtb` (including all phandle fixup metadata).
> 
> Let me know if you're happy with this approach (or if you'd prefer a
> different `FDT_OVERLAY_LOCAL_FIXUPS` default)!

I'll bear this in mind when I look at v3 and see what I think.

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