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

[Doug Anderson]

Hi David,

On Fri, Oct 30, 2026 at 3:24 AM David Gibson
<david@xxxxxxxxxxxxxxxxxxxxx> 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`).

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.
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)!

[1] https://lore.kernel.org/all/CAFLszTiVvuaMUsRHjhRTFxFf8Q2hbN1a1K-G=QFv4DyXNpWY6w@xxxxxxxxxxxxxx/


> > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> > index a19eb1bc4bd8..bf1b9044ff2a 100644
> > --- a/libfdt/fdt_overlay.c
> > +++ b/libfdt/fdt_overlay.c
> > @@ -804,7 +804,7 @@ static int overlay_prevent_phandle_overwrite(void *fdt, void *fdto)
> >   *      Negative error code on failure
> >   */
> >  static int overlay_apply_node(void *fdt, int target,
> > -                           void *fdto, int node)
> > +                           const void *fdto, int node)
> >  {
> >       int property;
> >       int subnode;
>
> This change is unrelated to the rest of the patch.

Split into its own standalone preparatory patch ("libfdt: overlay: Make
fdto const in overlay_apply_node()").


> > +static int overlay_create_path(void *fdt, int parent, const char *path,
> > +                            int path_len)
> > +{
> > +     const char *sep;
> > +     int node = parent;
> > +
> > +     while (path_len > 0 && *path == '/') {
> > +             path++;
> > +             path_len--;
> > +     }
>
> In Python, I'm pretty happy to have the code to optionally allow extra
> slashes to make the functions more widely useful.  In C - especially
> since this is code that may be run in limited environments - I'd
> prefer to pin down exactly what the expected format is and require
> that.  So, I'd suggest this function only take a relative path, and
> make sure that's the case from the callers.
>
> > +     while (path_len > 0) {
> > +             int comp_len;
> > +             int next;
> > +
> > +             sep = memchr(path, '/', path_len);
> > +             comp_len = sep ? (sep - path) : path_len;
> > +
> > +             if (comp_len > 0) {
>
> Similarly, in C I'd treat comp_len == 0 as an error condition, rather
> than trying to handle it.
>
> > +                     next = fdt_subnode_offset_namelen(fdt, node, path,
> > +                                                       comp_len);
> > +                     if (next == -FDT_ERR_NOTFOUND)
> > +                             next = fdt_add_subnode_namelen(fdt, node, path,
> > +                                                            comp_len);
> > +                     if (next < 0)
> > +                             return next;
> > +                     node = next;
> > +             }
> > +
> > +             path += comp_len;
> > +             path_len -= comp_len;
> > +             while (path_len > 0 && *path == '/') {
>
> Same thoughts here.

Done. `overlay_create_path()` now requires a strict relative path and
returns `-FDT_ERR_BADOVERLAY` if `comp_len == 0` (leading `/`, repeated
`//`) or if `@path` has a trailing `/`.


-Doug