[PATCHv6 2/3] boot_fdt_add_mem_rsv_regions: free old dtb reservations

[Simon Glass]

Hi Randolph,

On 2026-05-08T22:29:09, Randolph Sapp <rs at ti.com> wrote:
> boot_fdt_add_mem_rsv_regions: free old dtb reservations
>
> Add a free flag and an initial call to free allocations covered by the
> global FDT. This assumes that all calls to boot_fdt_add_mem_rsv_regions
> occur before the transition to the new device tree, thus we can access
> the currently active device tree through the global data pointer.
>
> This allows us to clearly indicate to the user when a device tree
> reservation fails. How we handle this can still use some improvement.
> Right now we'll keep the default behavior and try to boot anyway.
>
> This functionality was broken in:
> 5a6aa7d ("boot: fdt: Handle already reserved memory in boot_fdt_reserve_region()")
>
> Signed-off-by: Randolph Sapp <rs at ti.com>
> Acked-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>
> arch/mips/lib/bootm.c |  2 +-
>  boot/bootm.c          |  2 +-
>  boot/bootm_os.c       |  2 +-
>  boot/image-board.c    |  2 +-
>  boot/image-fdt.c      | 55 +++++++++++++++++++++++++++++++++++----------------
>  include/image.h       |  2 +-
>  lib/lmb.c             |  2 +-
>  7 files changed, 44 insertions(+), 23 deletions(-)

Reviewed-by: Simon Glass <sjg at chromium.org>

with some thoughts below

> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> @@ -108,12 +124,16 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
> +     /* Remove old regions */
> +     if (gd->fdt_blob != fdt_blob)
> +             boot_fdt_add_mem_rsv_regions((void *)gd->fdt_blob, true);
> +

Recursing through the public entry point with a flipped bool is hard
to follow, and it relies on the implicit invariant that the recursive
call will not itself recurse. I'd find this clearer as two named
helpers. For example you could have boot_fdt_reserve_regions() and
boot_fdt_free_regions() sharing a static walker. Then the intent at
the call site would be obvious.

Please also add a comment explaining the assumption from the commit
message: this must run before gd->fdt_blob is swapped to the new tree
(since nothing in the code itself enforces it)

> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> @@ -69,35 +69,51 @@ static const struct legacy_img_hdr *image_get_fdt(ulong fdt_addr)
> -     } else if (ret != -EEXIST && ret != -EINVAL) {
> -             puts("ERROR: reserving fdt memory region failed ");
> -             printf("(addr=%llx size=%llx flags=%x)\n",
> -                    (unsigned long long)addr,
> -                    (unsigned long long)size, flags);
> +     } else {
> +             printf("ERROR: %s fdt memory region failed (addr=%llx size=%llx flags=%x): %ld\n",
> +                    free ? 'freeing' : 'reserving', (unsigned long long)addr,
> +                    (unsigned long long)size, flags, ret);
>       }

Dropping the -EEXIST/-EINVAL filter only works on the reserve path
once the free path always succeeds. On the free side, anything in the
old fdt's reserved-memory that wasn't actually picked up by
lmb_reserve_common() (a non-enabled subnode, or a region since
split/coalesced inside LMB) will now produce a spurious "ERROR:
freeing fdt memory region failed" line.

I'm not sure if this actually happens? You could try it on a board
whose memreserve/reserved-memory layout differs between the U-Boot dtb
and the kernel dtb? Perhaps for now, best to tolerate  -EINVAL on the
free path at least.

> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> @@ -108,12 +124,16 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
> +     /* Remove old regions */
> +     if (gd->fdt_blob != fdt_blob)
> +             boot_fdt_add_mem_rsv_regions((void *)gd->fdt_blob, true);

Just to check - the cast drops the const from gd->fdt_blob and we then
walk it read-only, which is fine, but boot_fdt_add_mem_rsv_regions()
takes a non-const void * for no good reason that I can see? Worth a
brief comment so a future reader doesn't try to 'fix' the cast.

Regards,
Simon