Re: [PATCHv7 2/3] test: boot: add a fdt reserved region check

[Randolph Sapp <rs@ti.com>]

On Wed May 13, 2026 at 11:34 AM CDT, Simon Glass wrote:
> Hi Randolph,
>
> On 2026-05-13T01:56:06, Randolph Sapp <rs@ti.com> 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 <rs@ti.com>
>>
>> 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.