* [PATCH v3] test: dm: add button_cmd test
@ 2024-03-19 13:24 Caleb Connolly
2024-03-21 10:08 ` Mattijs Korpershoek
2024-03-21 12:15 ` Tom Rini
0 siblings, 2 replies; 3+ messages in thread
From: Caleb Connolly @ 2024-03-19 13:24 UTC (permalink / raw)
To: Abdellatif El Khlifi, AKASHI Takahiro, Alexander Gendin,
Caleb Connolly, Ehsan Mohandesi, Heinrich Schuchardt,
Ilias Apalodimas, Joshua Watt, Marek Vasut, Mario Six,
Sean Anderson, Sergei Antonov, Simon Glass, Sughosh Ganu,
Tom Rini
Cc: Mattijs Korpershoek, u-boot
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.connolly@linaro.org>
---
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.connolly@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.connolly@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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] test: dm: add button_cmd test
2024-03-19 13:24 [PATCH v3] test: dm: add button_cmd test Caleb Connolly
@ 2024-03-21 10:08 ` Mattijs Korpershoek
2024-03-21 12:15 ` Tom Rini
1 sibling, 0 replies; 3+ messages in thread
From: Mattijs Korpershoek @ 2024-03-21 10:08 UTC (permalink / raw)
To: Caleb Connolly, Abdellatif El Khlifi, AKASHI Takahiro,
Alexander Gendin, Caleb Connolly, Ehsan Mohandesi,
Heinrich Schuchardt, Ilias Apalodimas, Joshua Watt, Marek Vasut,
Mario Six, Sean Anderson, Sergei Antonov, Simon Glass,
Sughosh Ganu, Tom Rini
Cc: u-boot
Hi Caleb,
Thank you for the patch.
On mar., mars 19, 2024 at 13:24, Caleb Connolly <caleb.connolly@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.connolly@linaro.org>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@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.connolly@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.connolly@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] test: dm: add button_cmd test
2024-03-19 13:24 [PATCH v3] test: dm: add button_cmd test Caleb Connolly
2024-03-21 10:08 ` Mattijs Korpershoek
@ 2024-03-21 12:15 ` Tom Rini
1 sibling, 0 replies; 3+ messages in thread
From: Tom Rini @ 2024-03-21 12:15 UTC (permalink / raw)
To: Abdellatif El Khlifi, AKASHI Takahiro, Alexander Gendin,
Ehsan Mohandesi, Heinrich Schuchardt, Ilias Apalodimas,
Joshua Watt, Marek Vasut, Mario Six, Sean Anderson,
Sergei Antonov, Simon Glass, Sughosh Ganu, Caleb Connolly
Cc: Mattijs Korpershoek, u-boot
On Tue, 19 Mar 2024 13:24:42 +0000, Caleb Connolly 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.
>
> [...]
Applied to u-boot/next, thanks!
--
Tom
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-03-21 12:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-19 13:24 [PATCH v3] test: dm: add button_cmd test Caleb Connolly
2024-03-21 10:08 ` Mattijs Korpershoek
2024-03-21 12:15 ` Tom Rini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox