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

[Randolph Sapp <rs@ti.com>]

On Mon May 11, 2026 at 1:27 PM CDT, Simon Glass wrote:
> Hi Randolph,
>
> On 2026-05-08T22:29:09, Randolph Sapp <rs@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@ti.com>
>> Acked-by: Ilias Apalodimas <ilias.apalodimas@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@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@9da00000 and memory@9db00000. Allocation and freeing of these regions
works without fault. Removing memory@9db00000 from the kernel dtb, but keeping
it in the u-boot dtb results in no warnings. Removing memory@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.