Re: [PATCH 7/8] test: dm: reboot-mode: test trigger dispatch and mode enumeration

Hi Balaji,

On 2026-08-11T05:18:35, Balaji Selvanathan
<[email protected]> wrote:
> test: dm: reboot-mode: test trigger dispatch and mode enumeration
>
> Add a trigger-only reboot-mode backend and two device tree modes to the
> sandbox test tree, then cover the new trigger path:
>
>   - reboot_mode_request() fires the owning device's trigger op with the
>     correct magic cells for both a 2-cell and a 1-cell mode;
>   - an unknown mode, and a backing-store-only mode (gpio/rtc), both return
>     -ENOENT because they are not triggerable;
>   - reboot_mode_list() enumerates only the triggerable modes and hides the
>     backing-store modes.
>
> Signed-off-by: Balaji Selvanathan <[email protected]>
>
> arch/sandbox/dts/test.dts |  6 ++++
>  test/dm/reboot-mode.c     | 87 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)

> diff --git a/test/dm/reboot-mode.c b/test/dm/reboot-mode.c
> @@ -5,6 +5,7 @@
>
>  #include <dm.h>
>  #include <reboot-mode/reboot-mode.h>
> +#include <console.h>
>  #include <env.h>
>  #include <log.h>

Please sort alphabetically - console.h belongs above dm.h (or at least
above env.h), not tucked in after the subdirectory header (which
should be at the end).

> diff --git a/test/dm/reboot-mode.c b/test/dm/reboot-mode.c
> @@ -16,6 +17,42 @@
> +static int test_trigger(struct udevice *dev, const u32 *magic, int count)
> +{
> +     int i;
> +
> +     test_trigger_count = count;
> +     for (i = 0; i < count && i < REBOOT_MODE_MAX_MAGIC; i++)
> +             test_trigger_magic[i] = magic[i];
> +
> +     /* A real backend does not return here; the test one does. */
> +     return -EINPROGRESS;
> +}

Since the framework's contract is that trigger() does not return on
success, returning -EINPROGRESS here is a bit of a smell - a future
change that treats any negative return as failure would break this
test silently. Consider returning 0 and asserting 0, or add a comment
near the ops declaration reminding readers this is a deliberate
test-only convention.

> diff --git a/test/dm/reboot-mode.c b/test/dm/reboot-mode.c
> @@ -66,3 +103,53 @@ static int dm_test_reboot_mode_rtc(struct unit_test_state *uts)
> +     test_trigger_count = 0;
> +     test_trigger_magic[0] = 0;
> +     test_trigger_magic[1] = 0;

memset(test_trigger_magic, '\0', sizeof(test_trigger_magic)) is safer
- with REBOOT_MODE_MAX_MAGIC == 3, magic[2] is left uninitialised, and
if a future mode grows to 3 cells this partial clear will hide state
leaked across ut runs.

> diff --git a/test/dm/reboot-mode.c b/test/dm/reboot-mode.c
> @@ -66,3 +103,53 @@ static int dm_test_reboot_mode_rtc(struct unit_test_state *uts)
> +     ut_assert_nextline("Available reset modes:");
> +     ut_assert_nextline("  obelisk");
> +     ut_assert_nextline("  sarcophagus");
> +     /* Backing-store modes (test/download) have no trigger and are hidden */
> +     ut_assert_console_end();

This bakes in the property iteration order of dev_for_each_property()
and the uclass probe order. That happens to be deterministic on
sandbox today, but please add a brief comment explaining why the order
is guaranteed, otherwise a future dts reshuffle will look like a test
regression.

Regards,
Simon