Hi Caleb, Thank you for the patch.
On mar., mars 19, 2024 at 13:24, Caleb Connolly <caleb.conno...@linaro.org> wrote: > Add a test for the button_cmd feature. This validates that commands can > be mapped to two buttons, that the correct command runs based on which > button is pressed, that only 1 command is run, and that no command runs > if button_cmd_0_name is wrong or unset. > > Additionally, fix a potential uninitialised variable use caught by these > tests, the btn variable in get_button_cmd() is assumed to be null if > button_get_by_label() fails, but it's actually used uninitialised in > that case. > > CONFIG_BUTTON is now enabled automatically and was removed when running > save_defconfig. > > Fixes: e761035b6423 ("boot: add support for button commands") > Signed-off-by: Caleb Connolly <caleb.conno...@linaro.org> Reviewed-by: Mattijs Korpershoek <mkorpersh...@baylibre.com> > --- > Pipeline: > https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/pipelines/19995 > Changes in v3: > - Enable CONFIG_BUTTON_CMD for sandbox_flattree as well. > - Link to v2: > https://lore.kernel.org/u-boot/20240305145111.1391645-1-caleb.conno...@linaro.org > > Changes in v2: > - Explicitly assign btn as NULL in get_button_cmd(). This fixes a > bug where if the undefined variable is non-zero the > button_get_by_label() check would fail and result in invalid memory > being accessed. > - Enable CONFIG_BUTTON_CMD for sandbox64 as well as sandbox. > - Link to v1: > https://lore.kernel.org/u-boot/20240214170357.4091708-1-caleb.conno...@linaro.org/ > --- > common/button_cmd.c | 2 +- > configs/sandbox64_defconfig | 1 + > configs/sandbox_defconfig | 1 + > configs/sandbox_flattree_defconfig | 1 + > test/dm/button.c | 96 ++++++++++++++++++++++++++++++ > 5 files changed, 100 insertions(+), 1 deletion(-) > > diff --git a/common/button_cmd.c b/common/button_cmd.c > index b6a8434d6f29..8642c26735cc 100644 > --- a/common/button_cmd.c > +++ b/common/button_cmd.c > @@ -32,9 +32,9 @@ struct button_cmd { > */ > static int get_button_cmd(int n, struct button_cmd *cmd) > { > const char *cmd_str; > - struct udevice *btn; > + struct udevice *btn = NULL; > char buf[24]; > > snprintf(buf, sizeof(buf), "button_cmd_%d_name", n); > cmd->btn_name = env_get(buf); > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig > index 3be9a00a8575..a62faf772482 100644 > --- a/configs/sandbox64_defconfig > +++ b/configs/sandbox64_defconfig > @@ -10,8 +10,9 @@ CONFIG_PCI=y > CONFIG_SANDBOX64=y > CONFIG_DEBUG_UART=y > CONFIG_SYS_MEMTEST_START=0x00100000 > CONFIG_SYS_MEMTEST_END=0x00101000 > +CONFIG_BUTTON_CMD=y > CONFIG_FIT=y > CONFIG_FIT_SIGNATURE=y > CONFIG_FIT_VERBOSE=y > CONFIG_LEGACY_IMAGE_FORMAT=y > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig > index 4ad10363e91b..93b52f2de5cf 100644 > --- a/configs/sandbox_defconfig > +++ b/configs/sandbox_defconfig > @@ -9,8 +9,9 @@ CONFIG_SYS_LOAD_ADDR=0x0 > CONFIG_PCI=y > CONFIG_DEBUG_UART=y > CONFIG_SYS_MEMTEST_START=0x00100000 > CONFIG_SYS_MEMTEST_END=0x00101000 > +CONFIG_BUTTON_CMD=y > CONFIG_FIT=y > CONFIG_FIT_RSASSA_PSS=y > CONFIG_FIT_CIPHER=y > CONFIG_FIT_VERBOSE=y > diff --git a/configs/sandbox_flattree_defconfig > b/configs/sandbox_flattree_defconfig > index 039018627527..6bf8874e722e 100644 > --- a/configs/sandbox_flattree_defconfig > +++ b/configs/sandbox_flattree_defconfig > @@ -7,8 +7,9 @@ CONFIG_SYS_LOAD_ADDR=0x0 > CONFIG_PCI=y > CONFIG_DEBUG_UART=y > CONFIG_SYS_MEMTEST_START=0x00100000 > CONFIG_SYS_MEMTEST_END=0x00101000 > +CONFIG_BUTTON_CMD=y > CONFIG_FIT=y > CONFIG_FIT_SIGNATURE=y > CONFIG_FIT_VERBOSE=y > CONFIG_LEGACY_IMAGE_FORMAT=y > diff --git a/test/dm/button.c b/test/dm/button.c > index 3318668df25a..830d96fbef34 100644 > --- a/test/dm/button.c > +++ b/test/dm/button.c > @@ -130,4 +130,100 @@ static int dm_test_button_keys_adc(struct > unit_test_state *uts) > > return 0; > } > DM_TEST(dm_test_button_keys_adc, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); > + > +/* Test of the button uclass using the button_gpio driver */ > +static int dm_test_button_cmd(struct unit_test_state *uts) > +{ > + struct udevice *btn1_dev, *btn2_dev, *gpio; > + const char *envstr; > + > +#define BTN1_GPIO 3 > +#define BTN2_GPIO 4 > +#define BTN1_PASS_VAR "test_button_cmds_0" > +#define BTN2_PASS_VAR "test_button_cmds_1" > + > + /* > + * Buttons 1 and 2 are connected to gpio_a gpios 3 and 4 respectively. > + * set the GPIOs to known values and then check that the appropriate > + * commands are run when invoking process_button_cmds(). > + */ > + ut_assertok(uclass_get_device(UCLASS_BUTTON, 1, &btn1_dev)); > + ut_assertok(uclass_get_device(UCLASS_BUTTON, 2, &btn2_dev)); > + ut_assertok(uclass_get_device(UCLASS_GPIO, 1, &gpio)); > + > + /* > + * Map a command to button 1 and check that it process_button_cmds() > + * runs it if called with button 1 pressed. > + */ > + ut_assertok(env_set("button_cmd_0_name", "button1")); > + ut_assertok(env_set("button_cmd_0", "env set " BTN1_PASS_VAR " PASS")); > + ut_assertok(sandbox_gpio_set_value(gpio, BTN1_GPIO, 1)); > + /* Sanity check that the button is actually pressed */ > + ut_asserteq(BUTTON_ON, button_get_state(btn1_dev)); > + process_button_cmds(); > + ut_assertnonnull((envstr = env_get(BTN1_PASS_VAR))); > + ut_asserteq_str(envstr, "PASS"); > + > + /* Clear result */ > + ut_assertok(env_set(BTN1_PASS_VAR, NULL)); > + > + /* > + * Map a command for button 2, press it, check that only the command > + * for button 1 runs because it comes first and is also pressed. > + */ > + ut_assertok(env_set("button_cmd_1_name", "button2")); > + ut_assertok(env_set("button_cmd_1", "env set " BTN2_PASS_VAR " PASS")); > + ut_assertok(sandbox_gpio_set_value(gpio, BTN2_GPIO, 1)); > + ut_asserteq(BUTTON_ON, button_get_state(btn2_dev)); > + process_button_cmds(); > + /* Check that button 1 triggered again */ > + ut_assertnonnull((envstr = env_get(BTN1_PASS_VAR))); > + ut_asserteq_str(envstr, "PASS"); > + /* And button 2 didn't */ > + ut_assertnull(env_get(BTN2_PASS_VAR)); > + > + /* Clear result */ > + ut_assertok(env_set(BTN1_PASS_VAR, NULL)); > + > + /* > + * Release button 1 and check that the command for button 2 is run > + */ > + ut_assertok(sandbox_gpio_set_value(gpio, BTN1_GPIO, 0)); > + process_button_cmds(); > + ut_assertnull(env_get(BTN1_PASS_VAR)); > + /* Check that the command for button 2 ran */ > + ut_assertnonnull((envstr = env_get(BTN2_PASS_VAR))); > + ut_asserteq_str(envstr, "PASS"); > + > + /* Clear result */ > + ut_assertok(env_set(BTN2_PASS_VAR, NULL)); > + > + /* > + * Unset "button_cmd_0_name" and check that no commands run even > + * with both buttons pressed. > + */ > + ut_assertok(env_set("button_cmd_0_name", NULL)); > + /* Press button 1 (button 2 is already pressed )*/ > + ut_assertok(sandbox_gpio_set_value(gpio, BTN1_GPIO, 1)); > + ut_asserteq(BUTTON_ON, button_get_state(btn1_dev)); > + process_button_cmds(); > + ut_assertnull(env_get(BTN1_PASS_VAR)); > + ut_assertnull(env_get(BTN2_PASS_VAR)); > + > + /* > + * Check that no command is run if the button name is wrong. > + */ > + ut_assertok(env_set("button_cmd_0_name", "invalid_button")); > + process_button_cmds(); > + ut_assertnull(env_get(BTN1_PASS_VAR)); > + ut_assertnull(env_get(BTN2_PASS_VAR)); > + > +#undef BTN1_PASS_VAR > +#undef BTN2_PASS_VAR > +#undef BTN1_GPIO > +#undef BTN2_GPIO > + > + return 0; > +} > +DM_TEST(dm_test_button_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); > -- > 2.44.0