On Wed May 13, 2026 at 11:34 AM CDT, Simon Glass wrote: > Hi Randolph, > > On 2026-05-13T01:56:06, Randolph Sapp <[email protected]> wrote: >> test: boot: add a fdt reserved region check >> >> Add a image_fdt suite and a check for boot_fdt_add_mem_rsv_regions. This >> will ensure the user is properly informed of any reservation failures. >> It will also validate that reservations are cleaned up correctly when >> switching FDTs. >> >> Signed-off-by: Randolph Sapp <[email protected]> >> >> test/boot/Makefile | 1 + >> test/boot/image_fdt.c | 53 >> +++++++++++++++++++++++++++++++++++++++++++++ >> test/cmd_ut.c | 2 ++ >> test/py/tests/test_suite.py | 2 +- >> 4 files changed, 57 insertions(+), 1 deletion(-) > > Change log?
No change log for a patch that didn't exist in the series previously. >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c >> @@ -0,0 +1,53 @@ >> +#include <fdt_support.h> >> +#include <asm/global_data.h> >> +#include <config.h> >> +#include <image.h> >> +#include <lmb.h> >> +#include <malloc.h> >> +#include <test/test.h> >> +#include <test/ut.h> > > Please sort these: config.h (if needed) first, then the plain > top-level headers alphabetically (fdt_support.h, image.h, lmb.h, > malloc.h), then asm/global_data.h, then test/*. > >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c >> @@ -0,0 +1,53 @@ >> +#define FDTDEC_MAX_SIZE (2 * 1024 * 1024) >> + >> +#define IMAGE_FDT_TEST(_name, _flags) UNIT_TEST(_name, _flags, image_fdt) > > FDTDEC_MAX_SIZE is never referenced - looks copied from > test/dm/fdtdec.c - please drop it. > >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c >> @@ -0,0 +1,53 @@ >> + /* Loading a new device tree should be allowed */ >> + blob = malloc(fdt_sz); >> + ut_assertok_ptr(blob); >> + memcpy(blob, gd->fdt_blob, fdt_sz); >> + >> + nodeoffset = fdt_path_offset(blob, '/reserved-memory'); >> + ut_assertok(fdt_del_node(blob, nodeoffset)); > > If /reserved-memory is missing, fdt_path_offset() returns a negative > libfdt error and fdt_del_node() then fails with a confusing message. > Please ut_assert(nodeoffset >= 0) first. > > Also, blob is malloc()'d but never freed. > >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c >> @@ -0,0 +1,53 @@ >> + boot_fdt_add_mem_rsv_regions(blob); >> + ut_assert_console_end(); >> + boot_relocate_fdt((char **)&blob, &fdt_sz); >> + >> + /* Reservation should not exist now */ >> + ut_asserteq(0, lmb_is_reserved_flags(start, LMB_NOMAP)); > > What is the boot_relocate_fdt() call testing - could you add a > comment? The reservation at start is already gone after > boot_fdt_add_mem_rsv_regions(blob), so the final ut_asserteq() does > not depend on it. It also allocates from LMB for the relocated copy, > which then leaks. I'd drop it, or split it into a separate test with > its own assertions. The (char **)&blob cast is a hint that the call > doesn't belong here. > >> diff --git a/test/boot/image_fdt.c b/test/boot/image_fdt.c >> @@ -0,0 +1,53 @@ >> +IMAGE_FDT_TEST(test_boot_fdt_add_mem_rsv_regions, UTF_LIVE_TREE | >> UTF_CONSOLE); > > Just to check: the test only touches gd->fdt_blob (the flat tree), so > why UTF_LIVE_TREE? If you want it to run regardless of OF_LIVE, drop > the flag (or use UTF_FLAT_TREE). If the intent was to avoid running > twice, please mention that in a comment. > > Regards, > Simon As far as I'm aware, UTF_LIVE_TREE is required for this test. I've tried using UTF_FLAT_TREE and no UTF.*TREE options and for some reason this restricts part of the parsing that's required for boot_fdt_add_mem_rsv_regions. Operations that should report errors simply do not without UTF_LIVE_TREE.
