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

[Randolph Sapp]

On Mon May 11, 2026 at 1:27 PM CDT, Simon Glass wrote:
> 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)

Fair point. Moved the walking logic to a static boot_fdt_handle_mem_rsv_regions
and made boot_fdt_add_mem_rsv_regions call into 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.

Nah, coalesced regions can be freed without issue. The region is simply split if
needed in _lmb_free.

If the free or allocation doesn't succeed for any reason the user must be *at
least* be informed. This hidden warning stuff was the start of all of this. I
don't like keeping the users in the dark.

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

To humor this, the pocketbeagle2 actually has 2 regions back to back.
memory at 9da00000 and memory at 9db00000. Allocation and freeing of these regions
works without fault. Removing memory at 9db00000 from the kernel dtb, but keeping
it in the u-boot dtb results in no warnings. Removing memory at 9db00000 in u-boot
and keeping it in the kernel fdt also resulted in no warnings.

The only thing that would produce logs would be:
1. If someone actually switched gd->fdt_blob before calling this function
2. If someone called this function without switching gd->fdt_blob afterward

Both of these *should* produce warnings, considering they would permanently and
incorrectly change the LMB regions. That would have been unusual in the old
path, we're just making it explicitly bad now.

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

Fair point, I just switched boot_fdt_add_mem_rsv_regions to use const.