Re: [PATCH 5/5] spl: fit: Add test to check loadables

Hi Nikita,

On 2026-08-07T08:06:41, Nikita Shubin <[email protected]> wrote:
> spl: fit: Add test to check loadables
>
> Add spl_test_firmware_image() to check lodables correctly provided by
> spl_load_fit_image().

Same 'lodables' typo as patch 4; also please can you reword to "check
that loadables are correctly provided". BTW I applied the series and
ran this test on sandbox_spl - it passes here, so thanks for adding
sandbox coverage.

>
> We create special image with U-Boot, firmware and empty dtb. Then invoke
> spl_load_fit_image(), check everything (including dtb) loaded correctly
> and then check if loaded dtb contains "/fit-images/u-boot".
>
> Signed-off-by: Nikita Shubin <[email protected]>
>
> test/image/spl_load.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 195 insertions(+)

> diff --git a/test/image/spl_load.c b/test/image/spl_load.c
> @@ -275,6 +275,201 @@ size_t create_image(void *dst, enum spl_test_image type,
> +     char *data_uboot, *data_firmware;
> +     const char *uname;
> +     ulong check_addr;
> +     char str[100];

'uname' and 'str' are unused, giving build warnings:

    spl_load.c:439:14: warning: unused variable 'str' [-Wunused-variable]
    spl_load.c:437:21: warning: unused variable 'uname' [-Wunused-variable]

Please drop them.

> diff --git a/test/image/spl_load.c b/test/image/spl_load.c
> @@ -275,6 +275,201 @@ size_t create_image(void *dst, enum spl_test_image type,
> +     if (fdt_end_node(dst)) /* root */
> +             return 0;
> +     if (fdt_finish(dst))
> +             return 0;
> +
> +out:
> +     return total_size;

This is fragile: fdt_finish() shrinks totalsize to the actual
serialised size, and fit_image_get_data() locates external data
relative to the aligned totalsize, not to prop_size - so the data
offsets only line up with the data at 'img + 1168' because 1168
happens to be exactly the serialised size. Any change to a node or
property will silently shift the external-data base and the test will
fail with a confusing memory-compare error. Please can you do what
create_fit() does for the external case: check that fdt_totalsize(dst)
does not exceed 'size', then call fdt_set_totalsize(dst, size), so the
layout no longer depends on the magic number being exact.

> diff --git a/test/image/spl_load.c b/test/image/spl_load.c
> @@ -275,6 +275,201 @@ size_t create_image(void *dst, enum spl_test_image type,
> +     if (fdt_property_u32(dst, "#address-cells", 2))
> +             return 0;

The file already has ADDRESS_CELLS and fdt_property_addr() for this -
please use them here and for the entry/load properties below, rather
than hard-coding 2 cells and fdt_property_u64(), for consistency with
create_fit()

> diff --git a/test/image/spl_load.c b/test/image/spl_load.c
> @@ -275,6 +275,201 @@ size_t create_image(void *dst, enum spl_test_image type,
> +     /* opensbi image node */
> +     if (fdt_begin_node(dst, "opensbi"))
> +             return 0;

The u-boot and opensbi nodes are written with two near-identical
20-line sequences. Please can you factor this into a small helper
which writes one image node given a name, type string, data offset and
struct spl_image_info? That would remove most of the duplication and
make it easy to add further images later

> diff --git a/test/image/spl_load.c b/test/image/spl_load.c
> @@ -275,6 +275,201 @@ size_t create_image(void *dst, enum spl_test_image type,
> +     /* check loadable and load address recorded successfully in empty dtb */
> +     noffset = fdt_path_offset(phys_to_virt(fdt_load_addr), "/fit-images/u-boot");
> +     ut_assert(noffset);

Not quite: fdt_path_offset() returns a negative libfdt error when the
path is missing, which is non-zero, so this assertion passes even when
the loadable is not recorded. Please use ut_assert(noffset > 0) so a
missing node fails at the right place.

Regards,
Simon