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
