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

[Doug Anderson]

Hi Simon,

On Fri, Oct 30, 2026 at 5:22 AM Simon Glass <sjg@xxxxxxxxxxxx> 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.
>
> Please can you put the motivation first and use the present tense
> ('fdt_overlay_apply() updates /__symbols__ but leaves
> /__local_fixups__ untouched...')? It would also help to say why anyone
> needs an accurate /__local_fixups__ in the merged tree. Patch 8 gives
> the reason (an OS that wants to know which cells hold phandles, and
> the merged tree matching what 'dtc -L' produces), and that belongs
> here, since this patch changes behaviour for every libfdt user.

Done. Rewrote the commit message in the present tense with the motivation
up front.


> > +             next_lf = fdt_subnode_offset_namelen(fdt, node, name, namelen);
> > +             if (next_lf == -FDT_ERR_NOTFOUND && create) {
> > +                     char namebuf[256];
> > +                     int old_struct_size = fdt_size_dt_struct(fdt);
> > +
> > +                     if ((size_t)namelen >= sizeof(namebuf))
> > +                             return -FDT_ERR_INTERNAL;
> > +                     memcpy(namebuf, name, namelen);
> > +                     next_lf = fdt_add_subnode_namelen(fdt, node, namebuf,
> > +                                                       namelen);
> > +                     if (next_lf < 0)
> > +                             return next_lf;
> > +                     if (next_lf <= ancestor) {
> > +                             int delta = fdt_size_dt_struct(fdt) -
> > +                                         old_struct_size;
> > +                             target += delta;
> > +                             ancestor += delta;
> > +                     }
>
> I can see why the copy is needed, since the name is in the same blob
> and fdt_add_subnode_namelen() may move it. But libfdt has no limit on
> node-name length, so a valid tree with a long name makes
> fdt_overlay_apply() fail with -FDT_ERR_INTERNAL, which is meant for
> libfdt bugs. It also puts 256 bytes on the stack in code that runs in
> bootloaders. Please can you either avoid the copy (e.g. by re-reading
> the name after the splice, adjusted by the delta) or at least return a
> proper error code and document the limit?

Done! Removed `namebuf[256]` completely and instead re-read `name` from
`ancestor` (adjusted by `delta`) after `fdt_add_subnode_namelen()` when
`next_lf <= ancestor`.


> > +static int overlay_add_local_fixup_offset(void *fdt, int lf_node,
> > +                                       const char *name, int name_len,
> > +                                       uint32_t poffset)
> > +{
> ...
> > +     } else {
> > +             if (old_len % sizeof(uint32_t))
> > +                     return -FDT_ERR_BADSTRUCTURE;
>
> -FDT_ERR_BADSTRUCTURE means the structure block is corrupt. Elsewhere
> in this file (e.g. overlay_symbol_update()) a badly sized property
> value gives -FDT_ERR_BADVALUE, and if the property came from the
> overlay's /__local_fixups__, -FDT_ERR_BADOVERLAY fits better.

Done. Changed this to return `-FDT_ERR_BADVALUE`.


> > +static int overlay_validate_local_fixups(const void *fdto, int ov_lf_root)
> > +{
> > +     int lf_frag;
> > +
> > +     fdt_for_each_subnode(lf_frag, fdto, ov_lf_root) {
> > +             const char *frag_name;
> > +             int frag_name_len, lf_overlay, fragment;
> > +
> > +             frag_name = fdt_get_name(fdto, lf_frag, &frag_name_len);
> > +             if (!frag_name)
> > +                     return frag_name_len;
> > +
> > +             lf_overlay = fdt_subnode_offset(fdto, lf_frag, "__overlay__");
> > +             if (lf_overlay == -FDT_ERR_NOTFOUND)
> > +                     continue;
> > +             if (lf_overlay < 0)
> > +                     return lf_overlay;
> > +
> > +             fragment = fdt_subnode_offset_namelen(fdto, 0, frag_name,
> > +                                                   frag_name_len);
> > +             if (fragment < 0)
> > +                     return -FDT_ERR_BADOVERLAY;
> > +
> > +             if (fdt_subnode_offset(fdto, fragment, "__overlay__") < 0)
> > +                     return -FDT_ERR_BADOVERLAY;
> > +     }
> > +
> > +     return 0;
> > +}
>
> Just to check, is this needed? overlay_update_local_references()
> already walks the overlay's /__local_fixups__ against the overlay tree
> from the root, returning -FDT_ERR_BADOVERLAY if a node is missing. So
> by this point each /__local_fixups__/<frag>/__overlay__ is known to
> have a matching /<frag>/__overlay__ in fdto. If I have that right,
> this function and its call can go, which also helps with the size
> issue below.

You're right, `overlay_update_local_references()` has already walked and
validated `/__local_fixups__` against `fdto` before
`overlay_local_fixups_update()`
is called. I've dropped `overlay_validate_local_fixups()` (and also inlined
the single-caller `overlay_fragment_local_fixups_src()` and
`overlay_merge_fixup_into_local_fixups()` helpers).


> > @@ -1771,6 +1771,10 @@ int fdt_overlay_apply(void *fdt, void *fdto)
> >         ret = overlay_symbol_update(fdt, fdto);
> >         if (ret)
> >                 goto err;
> >
> > +       ret = overlay_local_fixups_update(fdt, fdto);
> > +       if (ret)
> > +               goto err;
> > +
>
> One thing I'd like to check before this goes in.. this changes what an
> exported function does. A base with /__local_fixups__ now needs more
> free space (so it can hit -FDT_ERR_NOSPACE where it did not before),
> and unrelated empty nodes under /__local_fixups__ get pruned. Please
> can you update the fdt_overlay_apply() comment in libfdt.h to describe
> the new behaviour?
>
> Code size is also worth a look. With -Os, the fdt_overlay.c text grows
> from 3040 to 4904 bytes on Thumb-2 and from 4770 to 7983 on x86_64,
> about 60%. That code goes into things like U-Boot SPL, mostly for
> boards whose base has no /__local_fixups__ at all. Could this be made
> optional for size-sensitive users, or at least slimmed down? What do
> you think?

I've updated the `fdt_overlay_apply()` doc comment in `libfdt/libfdt.h`,
slimmed down the implementation as noted above, and 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 (like U-Boot SPL) that compile `libfdt/*.c`
  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), avoiding leaving a stale or
  incomplete `/__local_fixups__` node in the merged DTB.
- `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.

Let me know if that works well for you or if you'd prefer a different
default!


-Doug