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

[Simon Glass]

Hi Doug,

On 2026-10-28T21:46:49, Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
> libfdt: overlay: Update base /__local_fixups__ on apply
>
> When a base device tree is compiled with `dtc -L` (`--local-fixups`), it
> contains a root `/__local_fixups__` node recording the byte offsets of
> all phandle references in its properties.
>
> Previously, `fdt_overlay_apply()` updated the base tree's `/__symbols__`
> via `overlay_symbol_update()`, but left the base tree's
> `/__local_fixups__` untouched. As a result, applying an overlay to a

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.

> base tree with `/__local_fixups__` left `/__local_fixups__` missing the
> phandle references introduced by the overlay (both from the overlay's
> `/__local_fixups__` and from its resolved `/__fixups__`), and left stale
> offsets if the overlay overwrote a base property that previously had
> phandle references.
>
> Add `overlay_local_fixups_update()` to update the base tree's
> `/__local_fixups__` whenever `/__local_fixups__` is present in the base
> tree:
>
> - Remove pre-existing `/__local_fixups__` properties for any base
> [...]
>
>  libfdt/fdt_overlay.c | 621 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  tests/run_tests.sh   |  19 ++
>  2 files changed, 636 insertions(+), 4 deletions(-)

> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> @@ -1122,6 +1121,616 @@ static int overlay_symbol_update(void *fdt, void *fdto)
> +		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;

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?

> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> @@ -1122,6 +1121,616 @@ static int overlay_symbol_update(void *fdt, void *fdto)
> +		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.

> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> @@ -1122,6 +1121,616 @@ static int overlay_symbol_update(void *fdt, void *fdto)
> +static int overlay_validate_local_fixups(const void *fdto, int ov_lf_root)
> +{
> +	int lf_frag;

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.

> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> @@ -1162,6 +1771,10 @@ int fdt_overlay_apply(void *fdt, void *fdto)
> +	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?

Regards,
Simon