On Mon May 11, 2026 at 1:27 PM CDT, Simon Glass wrote:
> Hi Randolph,
>
> On 2026-05-08T22:29:09, Randolph Sapp <[email protected]> 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 <[email protected]>
>> Acked-by: Ilias Apalodimas <[email protected]>
>>
>> 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 <[email protected]>
>
> 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.
