[PATCH v7 7/7] test: dm: Add test for FW Loader feature

[Simon Glass]

Hi Christian,

On 2026-06-16T19:18:35, Christian Marangi <ansuelsmth at gmail.com> wrote:
> test: dm: Add test for FW Loader feature
>
> Add basic test for FW Loader feature for the Filesystem way.
>
> An additional MMC testing image is added that loads a dummy firmware.bin in
> a fat partition. Update all the affected test for this additional eMMC
> node.
>
> Signed-off-by: Christian Marangi <ansuelsmth at gmail.com>
>
> arch/sandbox/dts/test.dts          |  20 +++++++
>  configs/sandbox64_defconfig        |   1 +
>  configs/sandbox_defconfig          |   1 +
>  configs/sandbox_flattree_defconfig |   1 +
>  configs/sandbox_noinst_defconfig   |   1 +
>  configs/sandbox_spl_defconfig      |   1 +
>  configs/sandbox_vpl_defconfig      |   1 +
>  test/boot/bootdev.c                |  34 +++++++----
>  test/boot/bootflow.c               |  12 +++-
>  test/dm/Makefile                   |   1 +
>  test/dm/blk.c                      |  41 +++++++++-----
>  test/dm/fw_loader.c                | 112 +++++++++++++++++++++++++++++++++++++
>  test/py/tests/test_ut.py           |  18 ++++++
>  13 files changed, 216 insertions(+), 28 deletions(-)

> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> @@ -123,6 +123,7 @@ static int bootflow_cmd_label(struct unit_test_state *uts)
>        * 8   [   ]      OK  mmc       mmc2.bootdev
>        * 9   [ + ]      OK  mmc       mmc1.bootdev
>        * a   [   ]      OK  mmc       mmc0.bootdev
> +      * a   [   ]      OK  mmc       mmc11.bootdev
>        *

The label here should be b for the new bootdev, not a second a.

> diff --git a/test/dm/fw_loader.c b/test/dm/fw_loader.c
> @@ -0,0 +1,112 @@
> +static int dm_test_fw_loader_get_fw_size(struct unit_test_state *uts)
> +{
> +     struct udevice *mdio_dev, *fw_loader_dev = NULL;
> +     ofnode mdio_node, phy_node;
> +     int ret;
> +
> +     ut_assertok(uclass_get_device_by_name(UCLASS_MDIO, 'mdio-test',
> +                                           &mdio_dev));
> +
> +     mdio_node = dev_ofnode(mdio_dev);
> +     ut_assert(ofnode_valid(mdio_node));
> +
> +     phy_node = ofnode_find_subnode(mdio_node, 'ethernet-phy at 2');
> +     ut_assert(ofnode_valid(phy_node));
> +
> +     ret = get_fw_loader_from_node(phy_node, &fw_loader_dev);
> +     ut_assertok(ret);
> +     ut_assertnonnull(fw_loader_dev);

All three tests repeat the same six-line dance to look up the mdio
device, walk to ethernet-phy at 2 and resolve the loader. Please factor
this into a helper (or per-suite fixture).

> diff --git a/test/dm/fw_loader.c b/test/dm/fw_loader.c
> @@ -0,0 +1,112 @@
> +     ret = request_firmware_into_buf(fw_loader_dev, 'firmware.bin',
> +                                     buf, firmware_size, firmware_size);
> +     ut_asserteq(0, ret);
> +
> +     ret = request_firmware_into_buf(fw_loader_dev, 'firmware.bin',
> +                                     buf, firmware_size, 0);
> +     ut_asserteq(firmware_size, ret);

Please also exercise the past-the-end case (offset > firmware_size)
and a missing-file case (does-not-exist.bin). Also free(buf) before
returning.

> diff --git a/test/dm/fw_loader.c b/test/dm/fw_loader.c
> @@ -0,0 +1,112 @@
> +DM_TEST(dm_test_fw_loader_get_fw_in_buf, UTF_SCAN_FDT);

The FIP loader is the headline feature of the series but has no
coverage here - only the FS loader path is exercised. Given that the
FIP parser does UUID lookups, header validation and offset accounting,
please add at least one test (a small canned FIP blob in an image, or
a sandbox helper to fabricate one). Otherwise regressions in
fip_loader.c will go unnoticed.

> diff --git a/test/dm/blk.c b/test/dm/blk.c
> @@ -208,14 +212,15 @@ static int dm_test_blk_iter(struct unit_test_state *uts)
>       i = 0;
>       blk_foreach_probe(BLKF_REMOVABLE, dev)
> -             ut_asserteq_str(i++ ? 'mmc0.blk' : 'mmc1.blk', dev->name);
> -     ut_asserteq(2, i);
> +             ut_asserteq_str((++i == 1 ? 'mmc1.blk' : i == 2 ?
> +                     'mmc0.blk' : 'mmc11.blk'), dev->name);
> +     ut_asserteq(3, i);

The nested ternary is hard to read; with three entries a small const
char *expected[] = { ... } indexed by i would be clearer, matching the
BLKF_BOTH case below.

Regards,
Simon