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