[PATCH v2] kselftest: alsa: Add simplistic test for ALSA mixer controls kselftest
Add a basic test for the mixer control interface. For every control on every sound card in the system it checks that it can read and write the default value where the control supports that and for writeable controls attempts to write all valid values, restoring the default values after each test to minimise disruption for users.
There are quite a few areas for improvement - currently no coverage of the generation of notifications, several of the control types don't have any coverage for the values and we don't have any testing of error handling when we attempt to write out of range values - but this provides some basic coverage.
This is added as a kselftest since unlike other ALSA test programs it does not require either physical setup of the device or interactive monitoring by users and kselftest is one of the test suites that is frequently run by people doing general automated testing so should increase coverage. It is written in terms of alsa-lib since tinyalsa is not generally packaged for distributions which makes things harder for general users interested in kselftest as a whole but it will be a barrier to people with Android.
Signed-off-by: Mark Brown broonie@kernel.org ---
v2: Use pkg-config to get CFLAGS and LDLIBS for alsa-lib.
MAINTAINERS | 7 + tools/testing/selftests/Makefile | 3 +- tools/testing/selftests/alsa/.gitignore | 1 + tools/testing/selftests/alsa/Makefile | 9 + tools/testing/selftests/alsa/mixer-test.c | 616 ++++++++++++++++++++++ 5 files changed, 635 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/alsa/.gitignore create mode 100644 tools/testing/selftests/alsa/Makefile create mode 100644 tools/testing/selftests/alsa/mixer-test.c
diff --git a/MAINTAINERS b/MAINTAINERS index 79ef55bf2ca7..ba25b33e2f96 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17946,6 +17946,7 @@ F: Documentation/sound/ F: include/sound/ F: include/uapi/sound/ F: sound/ +F: tools/testing/selftests/alsa
SOUND - COMPRESSED AUDIO M: Vinod Koul vkoul@kernel.org @@ -17965,6 +17966,12 @@ F: include/sound/dmaengine_pcm.h F: sound/core/pcm_dmaengine.c F: sound/soc/soc-generic-dmaengine-pcm.c
+SOUND - ALSA SELFTESTS +M: Mark Brown broonie@kernel.org +L: alsa-devel@alsa-project.org (moderated for non-subscribers) +S: Supported +F: tools/testing/selftests/alsa + SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEMENT (ASoC) M: Liam Girdwood lgirdwood@gmail.com M: Mark Brown broonie@kernel.org diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index c852eb40c4f7..d08fe4cfe811 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 -TARGETS = arm64 +TARGETS += alsa +TARGETS += arm64 TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities diff --git a/tools/testing/selftests/alsa/.gitignore b/tools/testing/selftests/alsa/.gitignore new file mode 100644 index 000000000000..3bb7c41266a8 --- /dev/null +++ b/tools/testing/selftests/alsa/.gitignore @@ -0,0 +1 @@ +mixer-test diff --git a/tools/testing/selftests/alsa/Makefile b/tools/testing/selftests/alsa/Makefile new file mode 100644 index 000000000000..f64d9090426d --- /dev/null +++ b/tools/testing/selftests/alsa/Makefile @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0 +# + +CFLAGS += $(shell pkg-config --cflags alsa) +LDLIBS += $(shell pkg-config --libs alsa) + +TEST_GEN_PROGS := mixer-test + +include ../lib.mk diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c new file mode 100644 index 000000000000..6082efa0b426 --- /dev/null +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -0,0 +1,616 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// kselftest for the ALSA mixer API +// +// Original author: Mark Brown broonie@kernel.org +// Copyright (c) 2021 Arm Limited + +// This test will iterate over all cards detected in the system, exercising +// every mixer control it can find. This may conflict with other system +// software if there is audio activity so is best run on a system with a +// minimal active userspace. + +#include <stdio.h> +#include <stdlib.h> +#include <stdbool.h> +#include <string.h> +#include <getopt.h> +#include <stdarg.h> +#include <ctype.h> +#include <math.h> +#include <errno.h> +#include <assert.h> +#include <alsa/asoundlib.h> +#include <poll.h> +#include <stdint.h> + +#include "../kselftest.h" + +#define TESTS_PER_CONTROL 3 + +struct card_data { + snd_ctl_t *handle; + int card; + int num_ctls; + snd_ctl_elem_list_t *ctls; + struct card_data *next; +}; + +struct ctl_data { + const char *name; + snd_ctl_elem_id_t *id; + snd_ctl_elem_info_t *info; + snd_ctl_elem_value_t *def_val; + int elem; + struct card_data *card; + struct ctl_data *next; +}; + +int num_cards = 0; +int num_controls = 0; +struct card_data *card_list = NULL; +struct ctl_data *ctl_list = NULL; + +void find_controls(void) +{ + char name[32]; + int card, ctl, err; + struct card_data *card_data; + struct ctl_data *ctl_data; + + card = -1; + if (snd_card_next(&card) < 0 || card < 0) + return; + + while (card >= 0) { + sprintf(name, "hw:%d", card); + + card_data = malloc(sizeof(*card_data)); + if (!card_data) { + ksft_print_msg("Out of memory\n"); + ksft_exit_fail(); + } + + err = snd_ctl_open(&card_data->handle, name, 0); + if (err < 0) { + ksft_print_msg("Failed to get hctl for card %d: %s\n", + card, snd_strerror(err)); + goto next_card; + } + + /* Count controls */ + snd_ctl_elem_list_malloc(&card_data->ctls); + snd_ctl_elem_list(card_data->handle, card_data->ctls); + card_data->num_ctls = snd_ctl_elem_list_get_count(card_data->ctls); + + /* Enumerate control information */ + snd_ctl_elem_list_alloc_space(card_data->ctls, card_data->num_ctls); + snd_ctl_elem_list(card_data->handle, card_data->ctls); + + card_data->card = num_cards++; + card_data->next = card_list; + card_list = card_data; + + num_controls += card_data->num_ctls; + + for (ctl = 0; ctl < card_data->num_ctls; ctl++) { + ctl_data = malloc(sizeof(*ctl_data)); + if (!ctl_data) { + ksft_print_msg("Out of memory\n"); + ksft_exit_fail(); + } + + ctl_data->card = card_data; + ctl_data->elem = ctl; + ctl_data->name = snd_ctl_elem_list_get_name(card_data->ctls, + ctl); + + err = snd_ctl_elem_id_malloc(&ctl_data->id); + if (err < 0) { + ksft_print_msg("Out of memory\n"); + ksft_exit_fail(); + } + + err = snd_ctl_elem_info_malloc(&ctl_data->info); + if (err < 0) { + ksft_print_msg("Out of memory\n"); + ksft_exit_fail(); + } + + err = snd_ctl_elem_value_malloc(&ctl_data->def_val); + if (err < 0) { + ksft_print_msg("Out of memory\n"); + ksft_exit_fail(); + } + + snd_ctl_elem_list_get_id(card_data->ctls, ctl, + ctl_data->id); + snd_ctl_elem_info_set_id(ctl_data->info, ctl_data->id); + err = snd_ctl_elem_info(card_data->handle, + ctl_data->info); + if (err < 0) { + ksft_print_msg("%s getting info for %d\n", + snd_strerror(err), + ctl_data->name); + } + + snd_ctl_elem_value_set_id(ctl_data->def_val, + ctl_data->id); + + ctl_data->next = ctl_list; + ctl_list = ctl_data; + } + + next_card: + if (snd_card_next(&card) < 0) { + ksft_print_msg("snd_card_next"); + break; + } + } +} + +/* + * Check that we can read the default value and it is valid. Write + * tests use the read value to restore the default. + */ +void test_ctl_get_value(struct ctl_data *ctl) +{ + int err; + long int_val; + long long int64_val; + + /* If the control is turned off let's be polite */ + if (snd_ctl_elem_info_is_inactive(ctl->info)) { + ksft_print_msg("%s is inactive\n", ctl->name); + ksft_test_result_skip("get_value.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + + /* Can't test reading on an unreadable control */ + if (!snd_ctl_elem_info_is_readable(ctl->info)) { + ksft_print_msg("%s is not readable\n", ctl->name); + ksft_test_result_skip("get_value.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + + err = snd_ctl_elem_read(ctl->card->handle, ctl->def_val); + if (err < 0) { + ksft_print_msg("snd_ctl_elem_read() failed: %s\n", + snd_strerror(err)); + goto out; + } + + switch (snd_ctl_elem_info_get_type(ctl->info)) { + case SND_CTL_ELEM_TYPE_NONE: + ksft_print_msg("%s Invalid control type NONE\n", ctl->name); + err = -1; + break; + + case SND_CTL_ELEM_TYPE_BOOLEAN: + int_val = snd_ctl_elem_value_get_boolean(ctl->def_val, 0); + switch (int_val) { + case 0: + case 1: + break; + default: + ksft_print_msg("%s Invalid boolean value %ld\n", + ctl->name, int_val); + err = -1; + break; + } + break; + + case SND_CTL_ELEM_TYPE_INTEGER: + int_val = snd_ctl_elem_value_get_integer(ctl->def_val, 0); + + if (int_val < snd_ctl_elem_info_get_min(ctl->info)) { + ksft_print_msg("%s value %ld less than minimum %ld\n", + ctl->name, int_val, + snd_ctl_elem_info_get_min(ctl->info)); + err = -1; + } + + if (int_val > snd_ctl_elem_info_get_max(ctl->info)) { + ksft_print_msg("%s value %ld more than maximum %ld\n", + ctl->name, int_val, + snd_ctl_elem_info_get_max(ctl->info)); + err = -1; + } + + /* Only check step size if there is one and we're in bounds */ + if (err >= 0 && snd_ctl_elem_info_get_step(ctl->info) && + (int_val - snd_ctl_elem_info_get_min(ctl->info) % + snd_ctl_elem_info_get_step(ctl->info))) { + ksft_print_msg("%s value %ld invalid for step %ld minimum %ld\n", + ctl->name, int_val, + snd_ctl_elem_info_get_step(ctl->info), + snd_ctl_elem_info_get_min(ctl->info)); + err = -1; + } + break; + + case SND_CTL_ELEM_TYPE_INTEGER64: + int64_val = snd_ctl_elem_value_get_integer64(ctl->def_val, 0); + + if (int64_val < snd_ctl_elem_info_get_min64(ctl->info)) { + ksft_print_msg("%s value %lld less than minimum %lld\n", + ctl->name, int64_val, + snd_ctl_elem_info_get_min64(ctl->info)); + err = -1; + } + + if (int64_val > snd_ctl_elem_info_get_max64(ctl->info)) { + ksft_print_msg("%s value %lld more than maximum %lld\n", + ctl->name, int64_val, + snd_ctl_elem_info_get_max(ctl->info)); + err = -1; + } + + /* Only check step size if there is one and we're in bounds */ + if (err >= 0 && snd_ctl_elem_info_get_step64(ctl->info) && + (int64_val - snd_ctl_elem_info_get_min64(ctl->info)) % + snd_ctl_elem_info_get_step64(ctl->info)) { + ksft_print_msg("%s value %lld invalid for step %lld minimum %lld\n", + ctl->name, int64_val, + snd_ctl_elem_info_get_step64(ctl->info), + snd_ctl_elem_info_get_min64(ctl->info)); + err = -1; + } + break; + + default: + /* No tests for other types */ + ksft_test_result_skip("get_value.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + +out: + ksft_test_result(err >= 0, "get_value.%d.%d\n", + ctl->card->card, ctl->elem); +} + +bool show_mismatch(struct ctl_data *ctl, int index, + snd_ctl_elem_value_t *read_val, + snd_ctl_elem_value_t *expected_val) +{ + long long expected_int, read_int; + + /* + * We factor out the code to compare values representable as + * integers, ensure that check doesn't log otherwise. + */ + expected_int = 0; + read_int = 0; + + switch (snd_ctl_elem_info_get_type(ctl->info)) { + case SND_CTL_ELEM_TYPE_BOOLEAN: + expected_int = snd_ctl_elem_value_get_boolean(expected_val, + index); + read_int = snd_ctl_elem_value_get_boolean(read_val, index); + break; + + case SND_CTL_ELEM_TYPE_INTEGER: + expected_int = snd_ctl_elem_value_get_integer(expected_val, + index); + read_int = snd_ctl_elem_value_get_integer(read_val, index); + break; + + case SND_CTL_ELEM_TYPE_INTEGER64: + expected_int = snd_ctl_elem_value_get_integer64(expected_val, + index); + read_int = snd_ctl_elem_value_get_integer64(read_val, + index); + break; + + case SND_CTL_ELEM_TYPE_ENUMERATED: + expected_int = snd_ctl_elem_value_get_enumerated(expected_val, + index); + read_int = snd_ctl_elem_value_get_enumerated(read_val, + index); + break; + + default: + break; + } + + if (expected_int != read_int) { + ksft_print_msg("%s.%d expected %lld but read %lld\n", + ctl->name, index, expected_int, read_int); + return true; + } else { + return false; + } +} + +/* + * Write a value then if possible verify that we get the expected + * result. An optional expected value can be provided if we expect + * the write to fail, for verifying that invalid writes don't corrupt + * anything. + */ +int write_and_verify(struct ctl_data *ctl, + snd_ctl_elem_value_t *write_val, + snd_ctl_elem_value_t *expected_val) +{ + int err, i; + bool error_expected, mismatch_shown; + snd_ctl_elem_value_t *read_val, *w_val; + snd_ctl_elem_value_alloca(&read_val); + snd_ctl_elem_value_alloca(&w_val); + + /* + * We need to copy the write value since writing can modify + * the value which causes surprises, and allocate an expected + * value if we expect to read back what we wrote. + */ + snd_ctl_elem_value_copy(w_val, write_val); + if (expected_val) { + error_expected = true; + } else { + error_expected = false; + snd_ctl_elem_value_alloca(&expected_val); + snd_ctl_elem_value_copy(expected_val, write_val); + } + + /* + * Do the write, if we have an expected value ignore the error + * and carry on to validate the expected value. + */ + err = snd_ctl_elem_write(ctl->card->handle, w_val); + if (err < 0 && !error_expected) { + ksft_print_msg("snd_ctl_elem_write() failed: %s\n", + snd_strerror(err)); + return err; + } + + /* Can we do the verification part? */ + if (!snd_ctl_elem_info_is_readable(ctl->info)) + return err; + + snd_ctl_elem_value_set_id(read_val, ctl->id); + + err = snd_ctl_elem_read(ctl->card->handle, read_val); + if (err < 0) { + ksft_print_msg("snd_ctl_elem_read() failed: %s\n", + snd_strerror(err)); + return err; + } + + /* + * Use the libray to compare values, if there's a mismatch + * carry on and try to provide a more useful diagnostic than + * just "mismatch". + */ + if (!snd_ctl_elem_value_compare(expected_val, read_val)) + return 0; + + mismatch_shown = false; + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) + if (show_mismatch(ctl, i, read_val, expected_val)) + mismatch_shown = true; + + if (!mismatch_shown) + ksft_print_msg("%s read and written values differ\n", + ctl->name); + + return -1; +} + +/* + * Make sure we can write the default value back to the control, this + * should validate that at least some write works. + */ +void test_ctl_write_default(struct ctl_data *ctl) +{ + int err; + + /* If the control is turned off let's be polite */ + if (snd_ctl_elem_info_is_inactive(ctl->info)) { + ksft_print_msg("%s is inactive\n", ctl->name); + ksft_test_result_skip("write_default.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + + if (!snd_ctl_elem_info_is_writable(ctl->info)) { + ksft_print_msg("%s is not writeable\n", ctl->name); + ksft_test_result_skip("write_default.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + + /* No idea what the default was for unreadable controls */ + if (!snd_ctl_elem_info_is_readable(ctl->info)) { + ksft_print_msg("%s couldn't read default\n", ctl->name); + ksft_test_result_skip("write_default.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + + err = write_and_verify(ctl, ctl->def_val, NULL); + + ksft_test_result(err >= 0, "write_default.%d.%d\n", + ctl->card->card, ctl->elem); +} + +bool test_ctl_write_valid_boolean(struct ctl_data *ctl) +{ + int err, i, j; + bool fail = false; + snd_ctl_elem_value_t *val; + snd_ctl_elem_value_alloca(&val); + + snd_ctl_elem_value_set_id(val, ctl->id); + + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) { + for (j = 0; j < 2; j++) { + snd_ctl_elem_value_set_boolean(val, i, j); + err = write_and_verify(ctl, val, NULL); + if (err != 0) + fail = true; + } + } + + return !fail; +} + +bool test_ctl_write_valid_integer(struct ctl_data *ctl) +{ + int err; + int i; + long j, step; + bool fail = false; + snd_ctl_elem_value_t *val; + snd_ctl_elem_value_alloca(&val); + + snd_ctl_elem_value_set_id(val, ctl->id); + + step = snd_ctl_elem_info_get_step(ctl->info); + if (!step) + step = 1; + + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) { + for (j = snd_ctl_elem_info_get_min(ctl->info); + j <= snd_ctl_elem_info_get_max(ctl->info); j += step) { + + snd_ctl_elem_value_set_integer(val, i, j); + err = write_and_verify(ctl, val, NULL); + if (err != 0) + fail = true; + } + } + + + return !fail; +} + +bool test_ctl_write_valid_integer64(struct ctl_data *ctl) +{ + int err, i; + long long j, step; + bool fail = false; + snd_ctl_elem_value_t *val; + snd_ctl_elem_value_alloca(&val); + + snd_ctl_elem_value_set_id(val, ctl->id); + + step = snd_ctl_elem_info_get_step64(ctl->info); + if (!step) + step = 1; + + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) { + for (j = snd_ctl_elem_info_get_min64(ctl->info); + j <= snd_ctl_elem_info_get_max64(ctl->info); j += step) { + + snd_ctl_elem_value_set_integer64(val, i, j); + err = write_and_verify(ctl, val, NULL); + if (err != 0) + fail = true; + } + } + + + return !fail; +} + +bool test_ctl_write_valid_enumerated(struct ctl_data *ctl) +{ + int err, i, j; + bool fail = false; + snd_ctl_elem_value_t *val; + snd_ctl_elem_value_alloca(&val); + + snd_ctl_elem_value_set_id(val, ctl->id); + + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) { + for (j = 0; j < snd_ctl_elem_info_get_items(ctl->info); j++) { + snd_ctl_elem_value_set_enumerated(val, i, j); + err = write_and_verify(ctl, val, NULL); + if (err != 0) + fail = true; + } + } + + return !fail; +} + +void test_ctl_write_valid(struct ctl_data *ctl) +{ + bool pass; + int err; + + /* If the control is turned off let's be polite */ + if (snd_ctl_elem_info_is_inactive(ctl->info)) { + ksft_print_msg("%s is inactive\n", ctl->name); + ksft_test_result_skip("write_valid.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + + if (!snd_ctl_elem_info_is_writable(ctl->info)) { + ksft_print_msg("%s is not writeable\n", ctl->name); + ksft_test_result_skip("write_valid.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + + switch (snd_ctl_elem_info_get_type(ctl->info)) { + case SND_CTL_ELEM_TYPE_BOOLEAN: + pass = test_ctl_write_valid_boolean(ctl); + break; + + case SND_CTL_ELEM_TYPE_INTEGER: + pass = test_ctl_write_valid_integer(ctl); + break; + + case SND_CTL_ELEM_TYPE_INTEGER64: + pass = test_ctl_write_valid_integer64(ctl); + break; + + case SND_CTL_ELEM_TYPE_ENUMERATED: + pass = test_ctl_write_valid_enumerated(ctl); + break; + + default: + /* No tests for this yet */ + ksft_test_result_skip("write_valid.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + + /* Restore the default value to minimise disruption */ + err = write_and_verify(ctl, ctl->def_val, NULL); + if (err < 0) + pass = false; + + ksft_test_result(pass, "write_valid.%d.%d\n", + ctl->card->card, ctl->elem); +} + +int main(void) +{ + struct ctl_data *ctl; + + ksft_print_header(); + + find_controls(); + + ksft_set_plan(num_controls * TESTS_PER_CONTROL); + + for (ctl = ctl_list; ctl != NULL; ctl = ctl->next) { + /* + * Must test get_value() before we write anything, the + * test stores the default value for later cleanup. + */ + test_ctl_get_value(ctl); + test_ctl_write_default(ctl); + test_ctl_write_valid(ctl); + } + + ksft_exit_pass(); + + return 0; +}
This is added as a kselftest since unlike other ALSA test programs it does not require either physical setup of the device or interactive monitoring
what did you mean by 'not require physical setup of the device'?
diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c new file mode 100644 index 000000000000..6082efa0b426 --- /dev/null +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -0,0 +1,616 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// kselftest for the ALSA mixer API +// +// Original author: Mark Brown broonie@kernel.org +// Copyright (c) 2021 Arm Limited
+// This test will iterate over all cards detected in the system, exercising
would it make sense to test only specific cards? People doing automated tests might have a USB device for capture of analog loopbacks, or injection of specific streams for capture, and usually care about testing such devices - which do need manual setups and wiring btw.
- switch (snd_ctl_elem_info_get_type(ctl->info)) {
- case SND_CTL_ELEM_TYPE_NONE:
ksft_print_msg("%s Invalid control type NONE\n", ctl->name);err = -1;break;- case SND_CTL_ELEM_TYPE_BOOLEAN:
int_val = snd_ctl_elem_value_get_boolean(ctl->def_val, 0);switch (int_val) {case 0:case 1:break;default:ksft_print_msg("%s Invalid boolean value %ld\n",ctl->name, int_val);err = -1;break;}break;- case SND_CTL_ELEM_TYPE_INTEGER:
int_val = snd_ctl_elem_value_get_integer(ctl->def_val, 0);if (int_val < snd_ctl_elem_info_get_min(ctl->info)) {ksft_print_msg("%s value %ld less than minimum %ld\n",ctl->name, int_val,snd_ctl_elem_info_get_min(ctl->info));err = -1;}if (int_val > snd_ctl_elem_info_get_max(ctl->info)) {ksft_print_msg("%s value %ld more than maximum %ld\n",ctl->name, int_val,snd_ctl_elem_info_get_max(ctl->info));err = -1;}/* Only check step size if there is one and we're in bounds */if (err >= 0 && snd_ctl_elem_info_get_step(ctl->info) &&(int_val - snd_ctl_elem_info_get_min(ctl->info) %snd_ctl_elem_info_get_step(ctl->info))) {ksft_print_msg("%s value %ld invalid for step %ld minimum %ld\n",ctl->name, int_val,snd_ctl_elem_info_get_step(ctl->info),snd_ctl_elem_info_get_min(ctl->info));err = -1;}break;- case SND_CTL_ELEM_TYPE_INTEGER64:
int64_val = snd_ctl_elem_value_get_integer64(ctl->def_val, 0);if (int64_val < snd_ctl_elem_info_get_min64(ctl->info)) {ksft_print_msg("%s value %lld less than minimum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_min64(ctl->info));err = -1;}if (int64_val > snd_ctl_elem_info_get_max64(ctl->info)) {ksft_print_msg("%s value %lld more than maximum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_max(ctl->info));err = -1;}/* Only check step size if there is one and we're in bounds */if (err >= 0 && snd_ctl_elem_info_get_step64(ctl->info) &&(int64_val - snd_ctl_elem_info_get_min64(ctl->info)) %snd_ctl_elem_info_get_step64(ctl->info)) {ksft_print_msg("%s value %lld invalid for step %lld minimum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_step64(ctl->info),snd_ctl_elem_info_get_min64(ctl->info));err = -1;}break;- default:
/* No tests for other types */
these types include ENUMERATED, BYTES and IEC958, but see below for ENUMERATED...
ksft_test_result_skip("get_value.%d.%d\n",ctl->card->card, ctl->elem);return;- }
+out:
- ksft_test_result(err >= 0, "get_value.%d.%d\n",
ctl->card->card, ctl->elem);+}
+bool show_mismatch(struct ctl_data *ctl, int index,
snd_ctl_elem_value_t *read_val,snd_ctl_elem_value_t *expected_val)+{
- long long expected_int, read_int;
- /*
* We factor out the code to compare values representable as* integers, ensure that check doesn't log otherwise.*/- expected_int = 0;
- read_int = 0;
- switch (snd_ctl_elem_info_get_type(ctl->info)) {
- case SND_CTL_ELEM_TYPE_BOOLEAN:
expected_int = snd_ctl_elem_value_get_boolean(expected_val,index);read_int = snd_ctl_elem_value_get_boolean(read_val, index);break;- case SND_CTL_ELEM_TYPE_INTEGER:
expected_int = snd_ctl_elem_value_get_integer(expected_val,index);read_int = snd_ctl_elem_value_get_integer(read_val, index);break;- case SND_CTL_ELEM_TYPE_INTEGER64:
expected_int = snd_ctl_elem_value_get_integer64(expected_val,index);read_int = snd_ctl_elem_value_get_integer64(read_val,index);break;- case SND_CTL_ELEM_TYPE_ENUMERATED:
... here you are handling ENUMERATED types?
expected_int = snd_ctl_elem_value_get_enumerated(expected_val,index);read_int = snd_ctl_elem_value_get_enumerated(read_val,index);break;- default:
break;- }
- if (expected_int != read_int) {
ksft_print_msg("%s.%d expected %lld but read %lld\n",ctl->name, index, expected_int, read_int);return true;- } else {
return false;- }
+}
+// This test will iterate over all cards detected in the system, exercising
would it make sense to test only specific cards? People doing automated tests might have a USB device for capture of analog loopbacks, or injection of specific streams for capture, and usually care about
typo: Usually don't care about testing such devices.
'testing the tester' is a separate endeavor.
testing such devices - which do need manual setups and wiring btw.
- switch (snd_ctl_elem_info_get_type(ctl->info)) {
- case SND_CTL_ELEM_TYPE_NONE:
ksft_print_msg("%s Invalid control type NONE\n", ctl->name);err = -1;break;- case SND_CTL_ELEM_TYPE_BOOLEAN:
int_val = snd_ctl_elem_value_get_boolean(ctl->def_val, 0);switch (int_val) {case 0:case 1:break;default:ksft_print_msg("%s Invalid boolean value %ld\n",ctl->name, int_val);err = -1;break;}break;- case SND_CTL_ELEM_TYPE_INTEGER:
int_val = snd_ctl_elem_value_get_integer(ctl->def_val, 0);if (int_val < snd_ctl_elem_info_get_min(ctl->info)) {ksft_print_msg("%s value %ld less than minimum %ld\n",ctl->name, int_val,snd_ctl_elem_info_get_min(ctl->info));err = -1;}if (int_val > snd_ctl_elem_info_get_max(ctl->info)) {ksft_print_msg("%s value %ld more than maximum %ld\n",ctl->name, int_val,snd_ctl_elem_info_get_max(ctl->info));err = -1;}/* Only check step size if there is one and we're in bounds */if (err >= 0 && snd_ctl_elem_info_get_step(ctl->info) &&(int_val - snd_ctl_elem_info_get_min(ctl->info) %snd_ctl_elem_info_get_step(ctl->info))) {ksft_print_msg("%s value %ld invalid for step %ld minimum %ld\n",ctl->name, int_val,snd_ctl_elem_info_get_step(ctl->info),snd_ctl_elem_info_get_min(ctl->info));err = -1;}break;- case SND_CTL_ELEM_TYPE_INTEGER64:
int64_val = snd_ctl_elem_value_get_integer64(ctl->def_val, 0);if (int64_val < snd_ctl_elem_info_get_min64(ctl->info)) {ksft_print_msg("%s value %lld less than minimum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_min64(ctl->info));err = -1;}if (int64_val > snd_ctl_elem_info_get_max64(ctl->info)) {ksft_print_msg("%s value %lld more than maximum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_max(ctl->info));err = -1;}/* Only check step size if there is one and we're in bounds */if (err >= 0 && snd_ctl_elem_info_get_step64(ctl->info) &&(int64_val - snd_ctl_elem_info_get_min64(ctl->info)) %snd_ctl_elem_info_get_step64(ctl->info)) {ksft_print_msg("%s value %lld invalid for step %lld minimum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_step64(ctl->info),snd_ctl_elem_info_get_min64(ctl->info));err = -1;}break;- default:
/* No tests for other types */these types include ENUMERATED, BYTES and IEC958, but see below for ENUMERATED...
ksft_test_result_skip("get_value.%d.%d\n",ctl->card->card, ctl->elem);return;- }
+out:
- ksft_test_result(err >= 0, "get_value.%d.%d\n",
ctl->card->card, ctl->elem);+}
+bool show_mismatch(struct ctl_data *ctl, int index,
snd_ctl_elem_value_t *read_val,snd_ctl_elem_value_t *expected_val)+{
- long long expected_int, read_int;
- /*
* We factor out the code to compare values representable as* integers, ensure that check doesn't log otherwise.*/- expected_int = 0;
- read_int = 0;
- switch (snd_ctl_elem_info_get_type(ctl->info)) {
- case SND_CTL_ELEM_TYPE_BOOLEAN:
expected_int = snd_ctl_elem_value_get_boolean(expected_val,index);read_int = snd_ctl_elem_value_get_boolean(read_val, index);break;- case SND_CTL_ELEM_TYPE_INTEGER:
expected_int = snd_ctl_elem_value_get_integer(expected_val,index);read_int = snd_ctl_elem_value_get_integer(read_val, index);break;- case SND_CTL_ELEM_TYPE_INTEGER64:
expected_int = snd_ctl_elem_value_get_integer64(expected_val,index);read_int = snd_ctl_elem_value_get_integer64(read_val,index);break;- case SND_CTL_ELEM_TYPE_ENUMERATED:
... here you are handling ENUMERATED types?
expected_int = snd_ctl_elem_value_get_enumerated(expected_val,index);read_int = snd_ctl_elem_value_get_enumerated(read_val,index);break;- default:
break;- }
- if (expected_int != read_int) {
ksft_print_msg("%s.%d expected %lld but read %lld\n",ctl->name, index, expected_int, read_int);return true;- } else {
return false;- }
+}
On Mon, Dec 06, 2021 at 10:27:26AM -0600, Pierre-Louis Bossart wrote:
This is added as a kselftest since unlike other ALSA test programs it does not require either physical setup of the device or interactive monitoring
what did you mean by 'not require physical setup of the device'?
You don't need for example a loopback cable plugging in.
+// This test will iterate over all cards detected in the system, exercising
would it make sense to test only specific cards? People doing automated tests might have a USB device for capture of analog loopbacks, or injection of specific streams for capture, and usually care about testing such devices - which do need manual setups and wiring btw.
It's not really idiomatic for kselftest to require any per system configuration by default - half the thing is that you can just run it and it should do as much as it can sensibly on the system. You could definitely add some command line options for development or manual usage though.
snd_ctl_elem_info_get_step64(ctl->info)) {ksft_print_msg("%s value %lld invalid for step %lld minimum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_step64(ctl->info),snd_ctl_elem_info_get_min64(ctl->info));err = -1;}break;- default:
/* No tests for other types */
these types include ENUMERATED, BYTES and IEC958, but see below for ENUMERATED...
These are tests that the information returned when we query the card status is coherent, we're not doing any validation at present that the enumeration's return value was sensible.
- case SND_CTL_ELEM_TYPE_ENUMERATED:
... here you are handling ENUMERATED types?
This is a different test, that we can write all valid values.
+// This test will iterate over all cards detected in the system, exercising
would it make sense to test only specific cards? People doing automated tests might have a USB device for capture of analog loopbacks, or injection of specific streams for capture, and usually care about testing such devices - which do need manual setups and wiring btw.
It's not really idiomatic for kselftest to require any per system configuration by default - half the thing is that you can just run it and it should do as much as it can sensibly on the system. You could definitely add some command line options for development or manual usage though.
I was thinking of adding this test to our CI, it's a bit orthogonal to self-tests indeed. IIRC we check that we can modify all the PGA settings for volume control but this test is a lot more generic.
On Mon, Dec 06, 2021 at 11:01:25AM -0600, Pierre-Louis Bossart wrote:
It's not really idiomatic for kselftest to require any per system configuration by default - half the thing is that you can just run it and it should do as much as it can sensibly on the system. You could definitely add some command line options for development or manual usage though.
I was thinking of adding this test to our CI, it's a bit orthogonal to self-tests indeed. IIRC we check that we can modify all the PGA settings
I do think it's useful to run there, it'd give coverage for all the CODEC drivers in your systems and of course for controls added for the DSPs.
for volume control but this test is a lot more generic.
It should be fine to run in your CI as it is - assuming there's no bugs it finds in the USB cards anyway. It tries to leave the card in the state it found it in so providing we can read and write whatever the current settings are it should leave things configured as they were. I'm not against adding an option to run on specific cards, you'd have to run it outside of the kselftest harness for that but that's more of a thing for your CI anyway.
Hi Mark,
On Mon, Dec 06, 2021 at 04:03:05PM +0000, Mark Brown wrote:
Add a basic test for the mixer control interface. For every control on every sound card in the system it checks that it can read and write the default value where the control supports that and for writeable controls attempts to write all valid values, restoring the default values after each test to minimise disruption for users.
There are quite a few areas for improvement - currently no coverage of the generation of notifications, several of the control types don't have any coverage for the values and we don't have any testing of error handling when we attempt to write out of range values - but this provides some basic coverage.
This is added as a kselftest since unlike other ALSA test programs it does not require either physical setup of the device or interactive monitoring by users and kselftest is one of the test suites that is frequently run by people doing general automated testing so should increase coverage. It is written in terms of alsa-lib since tinyalsa is not generally packaged for distributions which makes things harder for general users interested in kselftest as a whole but it will be a barrier to people with Android.
Signed-off-by: Mark Brown broonie@kernel.org
v2: Use pkg-config to get CFLAGS and LDLIBS for alsa-lib.
MAINTAINERS | 7 + tools/testing/selftests/Makefile | 3 +- tools/testing/selftests/alsa/.gitignore | 1 + tools/testing/selftests/alsa/Makefile | 9 + tools/testing/selftests/alsa/mixer-test.c | 616 ++++++++++++++++++++++ 5 files changed, 635 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/alsa/.gitignore create mode 100644 tools/testing/selftests/alsa/Makefile create mode 100644 tools/testing/selftests/alsa/mixer-test.c
I think it safer to take care of volatile attribute when comparing read value to written value. I'm glad if you review below patch.
As another topic, the runtime of alsa-lib application largely differs between process user due to the result of parsing text files for configuration space. I can easily imagine that developers unfamiliar to alsa-lib carelessly adds invalid or inadequate configurations to files under target path of alsa-lib configuration space, and they are puzzled since they are unaware of the fact that the kselftest is affected by userspace stuffs for the runtime.
If we respect the basic theory of test (idempotence), we can use ioctl(2) with requests for ALSA control interface since it's not so complicated (at least it is easier than ALSA PCM interface). The purpose of kselftest is to test kernel stuffs, not to test userspace stuffs including alsa-lib implementation and variety of plugins.
======== 8< --------
From 0052f48a931d93b993e406ffaf4c8fbecac15e84 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto o-takashi@sakamocchi.jp Date: Tue, 7 Dec 2021 11:43:56 +0900 Subject: [PATCH] kselftest: alsa: optimization for SNDRV_CTL_ELEM_ACCESS_VOLATILE
The volatile attribute of control element means that the hardware can voluntarily change the state of control element independent of any operation by software. ALSA control core necessarily sends notification to userspace subscribers for any change from userspace application, while it doesn't for the hardware's voluntary change.
This commit adds optimization for the attribute. Even if read value is different from written value, the test reports success as long as the target control element has the attribute. On the other hand, the difference is itself reported for developers' convenience.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- tools/testing/selftests/alsa/mixer-test.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c index 6082efa0b426..b87475fb7372 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -317,9 +317,13 @@ bool show_mismatch(struct ctl_data *ctl, int index, }
if (expected_int != read_int) { - ksft_print_msg("%s.%d expected %lld but read %lld\n", - ctl->name, index, expected_int, read_int); - return true; + // NOTE: The volatile attribute means that the hardware can voluntarily change the + // state of control element independent of any operation by software. + bool is_volatile = snd_ctl_elem_info_is_volatile(ctl->info); + + ksft_print_msg("%s.%d expected %lld but read %lld, is_volatile %d\n", + ctl->name, index, expected_int, read_int, is_volatile); + return !is_volatile; } else { return false; }
On 07. 12. 21 4:20, Takashi Sakamoto wrote:
As another topic, the runtime of alsa-lib application largely differs between process user due to the result of parsing text files for configuration space. I can easily imagine that developers unfamiliar to alsa-lib carelessly adds invalid or inadequate configurations to files under target path of alsa-lib configuration space, and they are puzzled since they are unaware of the fact that the kselftest is affected by userspace stuffs for the runtime.
The alsa-lib configuration can be restricted. I would suggest to use snd_ctl_open_lconf() function with a simple configuration which defines only ctl.hw device. In this case, the global configuration is not used, so the user errors are eliminated. Another way is to use the environment variable for the global alsa-lib configuration - ALSA_CONFIG_DIR.
I will try prepare a patch for snd_ctl_open_lconf().
Jaroslav
BTW: Thank you Mark for this test.
On Tue, Dec 07, 2021 at 12:20:32PM +0900, Takashi Sakamoto wrote:
On Mon, Dec 06, 2021 at 04:03:05PM +0000, Mark Brown wrote:
I think it safer to take care of volatile attribute when comparing read value to written value. I'm glad if you review below patch.
Yes, that's a good spot, it was an oversight to not take care of volatile controls - I'll roll that in if I send a new version or I guess Takashi could apply on top of my v2? If people are mostly happy and at Jaroslav is also preparing patches on top of this it might make sense to get it into git sooner.
Reviewed-by: Mark Brown broonie@kernel.org
As another topic, the runtime of alsa-lib application largely differs between process user due to the result of parsing text files for configuration space. I can easily imagine that developers unfamiliar to alsa-lib carelessly adds invalid or inadequate configurations to files under target path of alsa-lib configuration space, and they are puzzled since they are unaware of the fact that the kselftest is affected by userspace stuffs for the runtime.
If we respect the basic theory of test (idempotence), we can use ioctl(2) with requests for ALSA control interface since it's not so complicated (at least it is easier than ALSA PCM interface). The purpose of kselftest is to test kernel stuffs, not to test userspace stuffs including alsa-lib implementation and variety of plugins.
Right, I was originally thinking of implementing this in terms of tinyalsa which is much more direct (though I was amused to see that's gained userspace plugins at some point!) partly for this reason but the lack of widespread packaging for it was a bit of a blocker and it didn't feel like a great idea to essentially do yet another userspace ALSA library even if as you say it can be pretty trivial. Jaroslav's suggestion of using a custom configuration to override the default seems like it addresses everything though.
I do think there's an advantage for test comprehensibility in having the test written in terms of similar APIs to a normal userspace application - it makes it easier to relate what the test is doing to normal usage which is helpful when trying to understand what the test is trying to tell you.
On Tue, 07 Dec 2021 15:25:00 +0100, Mark Brown wrote:
On Tue, Dec 07, 2021 at 12:20:32PM +0900, Takashi Sakamoto wrote:
On Mon, Dec 06, 2021 at 04:03:05PM +0000, Mark Brown wrote:
I think it safer to take care of volatile attribute when comparing read value to written value. I'm glad if you review below patch.
Yes, that's a good spot, it was an oversight to not take care of volatile controls - I'll roll that in if I send a new version or I guess Takashi could apply on top of my v2? If people are mostly happy and at Jaroslav is also preparing patches on top of this it might make sense to get it into git sooner.
Reviewed-by: Mark Brown broonie@kernel.org
I'd like to get a comment from kselftest people about this addition with the external alsa-lib. Shua, what do you think?
If it's acceptable, I'd happily take the v2 patch and Sakamoto-san's additional fix to sound.git tree.
As another topic, the runtime of alsa-lib application largely differs between process user due to the result of parsing text files for configuration space. I can easily imagine that developers unfamiliar to alsa-lib carelessly adds invalid or inadequate configurations to files under target path of alsa-lib configuration space, and they are puzzled since they are unaware of the fact that the kselftest is affected by userspace stuffs for the runtime.
If we respect the basic theory of test (idempotence), we can use ioctl(2) with requests for ALSA control interface since it's not so complicated (at least it is easier than ALSA PCM interface). The purpose of kselftest is to test kernel stuffs, not to test userspace stuffs including alsa-lib implementation and variety of plugins.
Right, I was originally thinking of implementing this in terms of tinyalsa which is much more direct (though I was amused to see that's gained userspace plugins at some point!) partly for this reason but the lack of widespread packaging for it was a bit of a blocker and it didn't feel like a great idea to essentially do yet another userspace ALSA library even if as you say it can be pretty trivial. Jaroslav's suggestion of using a custom configuration to override the default seems like it addresses everything though.
I do think there's an advantage for test comprehensibility in having the test written in terms of similar APIs to a normal userspace application
- it makes it easier to relate what the test is doing to normal usage
which is helpful when trying to understand what the test is trying to tell you.
This comes to the question again to the test with external libraries, IMO. If a self-contained test code is much preferred, we can go for implementing open-code with raw kernel ABI. The control API is relatively simple and fairly solid over years, hence it's likely feasible. OTOH, using alsa-lib is also showing the test with the actual use case, and in that sense, it's not bad at all as more practical tests.
thanks,
Takashi
On Tue, Dec 07, 2021 at 03:36:12PM +0100, Takashi Iwai wrote:
I'd like to get a comment from kselftest people about this addition with the external alsa-lib. Shua, what do you think?
Probably worth pointing out that there's quite a few selftests linking with external libraries already (grep for -l in the kselftest Makefiles) - eg, memfd wants libfuse, netfilter wants libmnl, clone3 wants libcap, capabilities wants libcap-ng and there's others, plus the ambitious requirements that the bpf selftests have.
On Tue, Dec 7, 2021, at 23:25, Mark Brown wrote:
On Tue, Dec 07, 2021 at 12:20:32PM +0900, Takashi Sakamoto wrote:
On Mon, Dec 06, 2021 at 04:03:05PM +0000, Mark Brown wrote:
I think it safer to take care of volatile attribute when comparing read value to written value. I'm glad if you review below patch.
Yes, that's a good spot, it was an oversight to not take care of volatile controls - I'll roll that in if I send a new version or I guess Takashi could apply on top of my v2? If people are mostly happy and at Jaroslav is also preparing patches on top of this it might make sense to get it into git sooner.
Reviewed-by: Mark Brown broonie@kernel.org
Feel free to append it for your new version with or without my sign-off. If keeping it, I could review your respun one by receiving according to To or Cc.
As another topic, the runtime of alsa-lib application largely differs between process user due to the result of parsing text files for configuration space. I can easily imagine that developers unfamiliar to alsa-lib carelessly adds invalid or inadequate configurations to files under target path of alsa-lib configuration space, and they are puzzled since they are unaware of the fact that the kselftest is affected by userspace stuffs for the runtime.
If we respect the basic theory of test (idempotence), we can use ioctl(2) with requests for ALSA control interface since it's not so complicated (at least it is easier than ALSA PCM interface). The purpose of kselftest is to test kernel stuffs, not to test userspace stuffs including alsa-lib implementation and variety of plugins.
Right, I was originally thinking of implementing this in terms of tinyalsa which is much more direct (though I was amused to see that's gained userspace plugins at some point!) partly for this reason but the lack of widespread packaging for it was a bit of a blocker and it didn't feel like a great idea to essentially do yet another userspace ALSA library even if as you say it can be pretty trivial. Jaroslav's suggestion of using a custom configuration to override the default seems like it addresses everything though.
I do think there's an advantage for test comprehensibility in having the test written in terms of similar APIs to a normal userspace application
- it makes it easier to relate what the test is doing to normal usage
which is helpful when trying to understand what the test is trying to tell you.
In my opinion, test is merely test. It's not a sample program.
What important is what is tested. and how to assist developers if failed. If more suitable for the direction, we should do it, even if using raw ioctl in the case.
For your information, `check_event()` in `test/user-ctl-element-set.c`, my rough implementation of test for event triggered by tlv operation, might be helpful to you or start point t to discuss about event check.
Regards
Takashi Sakamoto
On Wed, Dec 8, 2021, at 23:26, Takashi Sakamoto wrote:
For your information, `check_event()` in `test/user-ctl-element-set.c`, my rough implementation of test for event triggered by tlv operation, might be helpful to you or start point t to discuss about event check.
Oops. It's in source code of alsa-lib.
* https://github.com/alsa-project/alsa-lib/blob/master/test/user-ctl-element-s...
Regards
Takashi Sakamoto
On Wed, Dec 08, 2021 at 11:26:59PM +0900, Takashi Sakamoto wrote:
On Tue, Dec 7, 2021, at 23:25, Mark Brown wrote:
I do think there's an advantage for test comprehensibility in having the test written in terms of similar APIs to a normal userspace application
- it makes it easier to relate what the test is doing to normal usage
which is helpful when trying to understand what the test is trying to tell you.
In my opinion, test is merely test. It's not a sample program.
What important is what is tested. and how to assist developers if failed. If more suitable for the direction, we should do it, even if using raw ioctl in the case.
Sure, but it's also important that if someone sees a test failure they can figure out what was being tested and if the test makes sense - there are plenty of testsuites out there with tests where what the problems are in the test, or it's hard to tell what the test is even supposed to be verifying. Putting barriers in the way of understanding the test means it's that bit harder to get people to put in the work required to figure out what the test is trying to tell them and pay attention to it.
For your information, `check_event()` in `test/user-ctl-element-set.c`, my rough implementation of test for event triggered by tlv operation, might be helpful to you or start point t to discuss about event check.
Thanks. The main issue here is finding time to look at stuff rather than figuring out the APIs, they're reasonably easy to work with fortunately.
On 12/6/21 9:03 AM, Mark Brown wrote:
Add a basic test for the mixer control interface. For every control on every sound card in the system it checks that it can read and write the default value where the control supports that and for writeable controls attempts to write all valid values, restoring the default values after each test to minimise disruption for users.
There are quite a few areas for improvement - currently no coverage of the generation of notifications, several of the control types don't have any coverage for the values and we don't have any testing of error handling when we attempt to write out of range values - but this provides some basic coverage.
This is added as a kselftest since unlike other ALSA test programs it does not require either physical setup of the device or interactive monitoring by users and kselftest is one of the test suites that is frequently run by people doing general automated testing so should increase coverage. It is written in terms of alsa-lib since tinyalsa is not generally packaged for distributions which makes things harder for general users interested in kselftest as a whole but it will be a barrier to people with Android.
Thank you for the test. This is a much needed addition.
Ran the test - output looks good - looks like need to be run as root. Let's add a root user test in main and skip test.
There is a blank like at the end - checkpatch complains. Please run checkpatch --strict and fix any problems you see.
Signed-off-by: Mark Brown broonie@kernel.org
v2: Use pkg-config to get CFLAGS and LDLIBS for alsa-lib.
MAINTAINERS | 7 + tools/testing/selftests/Makefile | 3 +- tools/testing/selftests/alsa/.gitignore | 1 + tools/testing/selftests/alsa/Makefile | 9 + tools/testing/selftests/alsa/mixer-test.c | 616 ++++++++++++++++++++++ 5 files changed, 635 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/alsa/.gitignore create mode 100644 tools/testing/selftests/alsa/Makefile create mode 100644 tools/testing/selftests/alsa/mixer-test.c
diff --git a/MAINTAINERS b/MAINTAINERS index 79ef55bf2ca7..ba25b33e2f96 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17946,6 +17946,7 @@ F: Documentation/sound/ F: include/sound/ F: include/uapi/sound/ F: sound/ +F: tools/testing/selftests/alsa
SOUND - COMPRESSED AUDIO M: Vinod Koul vkoul@kernel.org @@ -17965,6 +17966,12 @@ F: include/sound/dmaengine_pcm.h F: sound/core/pcm_dmaengine.c F: sound/soc/soc-generic-dmaengine-pcm.c
+SOUND - ALSA SELFTESTS +M: Mark Brown broonie@kernel.org +L: alsa-devel@alsa-project.org (moderated for non-subscribers)
Please add linux-kselftest list as well here.
+S: Supported +F: tools/testing/selftests/alsa
- SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEMENT (ASoC) M: Liam Girdwood lgirdwood@gmail.com M: Mark Brown broonie@kernel.org
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index c852eb40c4f7..d08fe4cfe811 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 -TARGETS = arm64 +TARGETS += alsa
Assuming it can run as part of default run?
+TARGETS += arm64 TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities diff --git a/tools/testing/selftests/alsa/.gitignore b/tools/testing/selftests/alsa/.gitignore new file mode 100644 index 000000000000..3bb7c41266a8 --- /dev/null +++ b/tools/testing/selftests/alsa/.gitignore @@ -0,0 +1 @@ +mixer-test diff --git a/tools/testing/selftests/alsa/Makefile b/tools/testing/selftests/alsa/Makefile new file mode 100644 index 000000000000..f64d9090426d --- /dev/null +++ b/tools/testing/selftests/alsa/Makefile @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0 +#
+CFLAGS += $(shell pkg-config --cflags alsa) +LDLIBS += $(shell pkg-config --libs alsa)
+TEST_GEN_PROGS := mixer-test
+include ../lib.mk diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c new file mode 100644 index 000000000000..6082efa0b426 --- /dev/null +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -0,0 +1,616 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// kselftest for the ALSA mixer API +// +// Original author: Mark Brown broonie@kernel.org +// Copyright (c) 2021 Arm Limited
+// This test will iterate over all cards detected in the system, exercising +// every mixer control it can find. This may conflict with other system +// software if there is audio activity so is best run on a system with a +// minimal active userspace.
+#include <stdio.h> +#include <stdlib.h> +#include <stdbool.h> +#include <string.h> +#include <getopt.h> +#include <stdarg.h> +#include <ctype.h> +#include <math.h> +#include <errno.h> +#include <assert.h> +#include <alsa/asoundlib.h> +#include <poll.h> +#include <stdint.h>
+#include "../kselftest.h"
+#define TESTS_PER_CONTROL 3
+struct card_data {
- snd_ctl_t *handle;
- int card;
- int num_ctls;
- snd_ctl_elem_list_t *ctls;
- struct card_data *next;
+};
+struct ctl_data {
- const char *name;
- snd_ctl_elem_id_t *id;
- snd_ctl_elem_info_t *info;
- snd_ctl_elem_value_t *def_val;
- int elem;
- struct card_data *card;
- struct ctl_data *next;
+};
+int num_cards = 0; +int num_controls = 0; +struct card_data *card_list = NULL; +struct ctl_data *ctl_list = NULL;
No need to initailize the above globals.
+void find_controls(void) +{
- char name[32];
Use SYSFS_PATH_MAX = 255 like other tools do?
- int card, ctl, err;
- struct card_data *card_data;
- struct ctl_data *ctl_data;
- card = -1;
- if (snd_card_next(&card) < 0 || card < 0)
return;- while (card >= 0) {
sprintf(name, "hw:%d", card);
Let's use snprintf with sizeof(name)
card_data = malloc(sizeof(*card_data));if (!card_data) {ksft_print_msg("Out of memory\n");ksft_exit_fail();
You can replace these two with ksft_exit_fail_msg() which will take a message and exit with fail code.
}err = snd_ctl_open(&card_data->handle, name, 0);if (err < 0) {ksft_print_msg("Failed to get hctl for card %d: %s\n",card, snd_strerror(err));goto next_card;}/* Count controls */snd_ctl_elem_list_malloc(&card_data->ctls);snd_ctl_elem_list(card_data->handle, card_data->ctls);card_data->num_ctls = snd_ctl_elem_list_get_count(card_data->ctls);/* Enumerate control information */snd_ctl_elem_list_alloc_space(card_data->ctls, card_data->num_ctls);snd_ctl_elem_list(card_data->handle, card_data->ctls);card_data->card = num_cards++;card_data->next = card_list;card_list = card_data;num_controls += card_data->num_ctls;for (ctl = 0; ctl < card_data->num_ctls; ctl++) {ctl_data = malloc(sizeof(*ctl_data));if (!ctl_data) {ksft_print_msg("Out of memory\n");ksft_exit_fail();
Same here - can be simplified using ksft_exit_fail_msg()
}ctl_data->card = card_data;ctl_data->elem = ctl;ctl_data->name = snd_ctl_elem_list_get_name(card_data->ctls,ctl);err = snd_ctl_elem_id_malloc(&ctl_data->id);if (err < 0) {ksft_print_msg("Out of memory\n");ksft_exit_fail();
Same here - can be simplified using ksft_exit_fail_msg()
}err = snd_ctl_elem_info_malloc(&ctl_data->info);if (err < 0) {ksft_print_msg("Out of memory\n");ksft_exit_fail();
Same here - can be simplified using ksft_exit_fail_msg()
}err = snd_ctl_elem_value_malloc(&ctl_data->def_val);if (err < 0) {ksft_print_msg("Out of memory\n");ksft_exit_fail();
Same here - can be simplified using ksft_exit_fail_msg()
}snd_ctl_elem_list_get_id(card_data->ctls, ctl,ctl_data->id);snd_ctl_elem_info_set_id(ctl_data->info, ctl_data->id);err = snd_ctl_elem_info(card_data->handle,ctl_data->info);if (err < 0) {ksft_print_msg("%s getting info for %d\n",snd_strerror(err),ctl_data->name);}snd_ctl_elem_value_set_id(ctl_data->def_val,ctl_data->id);ctl_data->next = ctl_list;ctl_list = ctl_data;}- next_card:
No need to indent the label
if (snd_card_next(&card) < 0) {ksft_print_msg("snd_card_next");break;}- }
+}
+/*
- Check that we can read the default value and it is valid. Write
- tests use the read value to restore the default.
- */
+void test_ctl_get_value(struct ctl_data *ctl) +{
- int err;
- long int_val;
- long long int64_val;
- /* If the control is turned off let's be polite */
- if (snd_ctl_elem_info_is_inactive(ctl->info)) {
ksft_print_msg("%s is inactive\n", ctl->name);ksft_test_result_skip("get_value.%d.%d\n",ctl->card->card, ctl->elem);
The two messages could be combined?
return;- }
- /* Can't test reading on an unreadable control */
- if (!snd_ctl_elem_info_is_readable(ctl->info)) {
ksft_print_msg("%s is not readable\n", ctl->name);ksft_test_result_skip("get_value.%d.%d\n",ctl->card->card, ctl->elem);
The two messages could be combined?
return;- }
- err = snd_ctl_elem_read(ctl->card->handle, ctl->def_val);
- if (err < 0) {
ksft_print_msg("snd_ctl_elem_read() failed: %s\n",snd_strerror(err));goto out;- }
- switch (snd_ctl_elem_info_get_type(ctl->info)) {
- case SND_CTL_ELEM_TYPE_NONE:
ksft_print_msg("%s Invalid control type NONE\n", ctl->name);err = -1;break;- case SND_CTL_ELEM_TYPE_BOOLEAN:
int_val = snd_ctl_elem_value_get_boolean(ctl->def_val, 0);switch (int_val) {case 0:case 1:break;default:ksft_print_msg("%s Invalid boolean value %ld\n",ctl->name, int_val);err = -1;break;}break;- case SND_CTL_ELEM_TYPE_INTEGER:
int_val = snd_ctl_elem_value_get_integer(ctl->def_val, 0);if (int_val < snd_ctl_elem_info_get_min(ctl->info)) {ksft_print_msg("%s value %ld less than minimum %ld\n",ctl->name, int_val,snd_ctl_elem_info_get_min(ctl->info));err = -1;}if (int_val > snd_ctl_elem_info_get_max(ctl->info)) {ksft_print_msg("%s value %ld more than maximum %ld\n",ctl->name, int_val,snd_ctl_elem_info_get_max(ctl->info));err = -1;}/* Only check step size if there is one and we're in bounds */if (err >= 0 && snd_ctl_elem_info_get_step(ctl->info) &&(int_val - snd_ctl_elem_info_get_min(ctl->info) %snd_ctl_elem_info_get_step(ctl->info))) {ksft_print_msg("%s value %ld invalid for step %ld minimum %ld\n",ctl->name, int_val,snd_ctl_elem_info_get_step(ctl->info),snd_ctl_elem_info_get_min(ctl->info));err = -1;}break;- case SND_CTL_ELEM_TYPE_INTEGER64:
int64_val = snd_ctl_elem_value_get_integer64(ctl->def_val, 0);if (int64_val < snd_ctl_elem_info_get_min64(ctl->info)) {ksft_print_msg("%s value %lld less than minimum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_min64(ctl->info));err = -1;}if (int64_val > snd_ctl_elem_info_get_max64(ctl->info)) {ksft_print_msg("%s value %lld more than maximum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_max(ctl->info));err = -1;}/* Only check step size if there is one and we're in bounds */if (err >= 0 && snd_ctl_elem_info_get_step64(ctl->info) &&(int64_val - snd_ctl_elem_info_get_min64(ctl->info)) %snd_ctl_elem_info_get_step64(ctl->info)) {ksft_print_msg("%s value %lld invalid for step %lld minimum %lld\n",ctl->name, int64_val,snd_ctl_elem_info_get_step64(ctl->info),snd_ctl_elem_info_get_min64(ctl->info));err = -1;}break;- default:
/* No tests for other types */ksft_test_result_skip("get_value.%d.%d\n",ctl->card->card, ctl->elem);return;- }
+out:
- ksft_test_result(err >= 0, "get_value.%d.%d\n",
ctl->card->card, ctl->elem);+}
+bool show_mismatch(struct ctl_data *ctl, int index,
snd_ctl_elem_value_t *read_val,snd_ctl_elem_value_t *expected_val)+{
- long long expected_int, read_int;
- /*
* We factor out the code to compare values representable as* integers, ensure that check doesn't log otherwise.*/- expected_int = 0;
- read_int = 0;
- switch (snd_ctl_elem_info_get_type(ctl->info)) {
- case SND_CTL_ELEM_TYPE_BOOLEAN:
expected_int = snd_ctl_elem_value_get_boolean(expected_val,index);read_int = snd_ctl_elem_value_get_boolean(read_val, index);break;- case SND_CTL_ELEM_TYPE_INTEGER:
expected_int = snd_ctl_elem_value_get_integer(expected_val,index);read_int = snd_ctl_elem_value_get_integer(read_val, index);break;- case SND_CTL_ELEM_TYPE_INTEGER64:
expected_int = snd_ctl_elem_value_get_integer64(expected_val,index);read_int = snd_ctl_elem_value_get_integer64(read_val,index);break;- case SND_CTL_ELEM_TYPE_ENUMERATED:
expected_int = snd_ctl_elem_value_get_enumerated(expected_val,index);read_int = snd_ctl_elem_value_get_enumerated(read_val,index);break;- default:
break;- }
- if (expected_int != read_int) {
ksft_print_msg("%s.%d expected %lld but read %lld\n",ctl->name, index, expected_int, read_int);return true;- } else {
return false;- }
+}
+/*
- Write a value then if possible verify that we get the expected
- result. An optional expected value can be provided if we expect
- the write to fail, for verifying that invalid writes don't corrupt
- anything.
- */
+int write_and_verify(struct ctl_data *ctl,
snd_ctl_elem_value_t *write_val,snd_ctl_elem_value_t *expected_val)+{
- int err, i;
- bool error_expected, mismatch_shown;
- snd_ctl_elem_value_t *read_val, *w_val;
- snd_ctl_elem_value_alloca(&read_val);
- snd_ctl_elem_value_alloca(&w_val);
- /*
* We need to copy the write value since writing can modify* the value which causes surprises, and allocate an expected* value if we expect to read back what we wrote.*/- snd_ctl_elem_value_copy(w_val, write_val);
- if (expected_val) {
error_expected = true;- } else {
error_expected = false;snd_ctl_elem_value_alloca(&expected_val);snd_ctl_elem_value_copy(expected_val, write_val);- }
- /*
* Do the write, if we have an expected value ignore the error* and carry on to validate the expected value.*/- err = snd_ctl_elem_write(ctl->card->handle, w_val);
- if (err < 0 && !error_expected) {
ksft_print_msg("snd_ctl_elem_write() failed: %s\n",snd_strerror(err));return err;- }
- /* Can we do the verification part? */
- if (!snd_ctl_elem_info_is_readable(ctl->info))
return err;- snd_ctl_elem_value_set_id(read_val, ctl->id);
- err = snd_ctl_elem_read(ctl->card->handle, read_val);
- if (err < 0) {
ksft_print_msg("snd_ctl_elem_read() failed: %s\n",snd_strerror(err));return err;- }
- /*
* Use the libray to compare values, if there's a mismatch* carry on and try to provide a more useful diagnostic than* just "mismatch".*/- if (!snd_ctl_elem_value_compare(expected_val, read_val))
return 0;- mismatch_shown = false;
- for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++)
if (show_mismatch(ctl, i, read_val, expected_val))mismatch_shown = true;- if (!mismatch_shown)
ksft_print_msg("%s read and written values differ\n",ctl->name);- return -1;
+}
+/*
- Make sure we can write the default value back to the control, this
- should validate that at least some write works.
- */
+void test_ctl_write_default(struct ctl_data *ctl) +{
- int err;
- /* If the control is turned off let's be polite */
- if (snd_ctl_elem_info_is_inactive(ctl->info)) {
ksft_print_msg("%s is inactive\n", ctl->name);ksft_test_result_skip("write_default.%d.%d\n",ctl->card->card, ctl->elem);return;- }
- if (!snd_ctl_elem_info_is_writable(ctl->info)) {
ksft_print_msg("%s is not writeable\n", ctl->name);ksft_test_result_skip("write_default.%d.%d\n",ctl->card->card, ctl->elem);return;- }
- /* No idea what the default was for unreadable controls */
- if (!snd_ctl_elem_info_is_readable(ctl->info)) {
ksft_print_msg("%s couldn't read default\n", ctl->name);ksft_test_result_skip("write_default.%d.%d\n",ctl->card->card, ctl->elem);return;- }
- err = write_and_verify(ctl, ctl->def_val, NULL);
- ksft_test_result(err >= 0, "write_default.%d.%d\n",
ctl->card->card, ctl->elem);+}
+bool test_ctl_write_valid_boolean(struct ctl_data *ctl) +{
- int err, i, j;
- bool fail = false;
- snd_ctl_elem_value_t *val;
Add blank line after declarations.
- snd_ctl_elem_value_alloca(&val);
- snd_ctl_elem_value_set_id(val, ctl->id);
- for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) {
for (j = 0; j < 2; j++) {snd_ctl_elem_value_set_boolean(val, i, j);err = write_and_verify(ctl, val, NULL);if (err != 0)fail = true;}- }
- return !fail;
+}
+bool test_ctl_write_valid_integer(struct ctl_data *ctl) +{
- int err;
- int i;
- long j, step;
- bool fail = false;
- snd_ctl_elem_value_t *val;
Add blank line after declarations.
- snd_ctl_elem_value_alloca(&val);
- snd_ctl_elem_value_set_id(val, ctl->id);
- step = snd_ctl_elem_info_get_step(ctl->info);
- if (!step)
step = 1;- for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) {
for (j = snd_ctl_elem_info_get_min(ctl->info);j <= snd_ctl_elem_info_get_max(ctl->info); j += step) {snd_ctl_elem_value_set_integer(val, i, j);err = write_and_verify(ctl, val, NULL);if (err != 0)fail = true;}- }
- return !fail;
+}
+bool test_ctl_write_valid_integer64(struct ctl_data *ctl) +{
- int err, i;
- long long j, step;
- bool fail = false;
- snd_ctl_elem_value_t *val;
Add blank line after declarations.
- snd_ctl_elem_value_alloca(&val);
- snd_ctl_elem_value_set_id(val, ctl->id);
- step = snd_ctl_elem_info_get_step64(ctl->info);
- if (!step)
step = 1;- for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) {
for (j = snd_ctl_elem_info_get_min64(ctl->info);j <= snd_ctl_elem_info_get_max64(ctl->info); j += step) {snd_ctl_elem_value_set_integer64(val, i, j);err = write_and_verify(ctl, val, NULL);if (err != 0)fail = true;}- }
- return !fail;
+}
+bool test_ctl_write_valid_enumerated(struct ctl_data *ctl) +{
- int err, i, j;
- bool fail = false;
- snd_ctl_elem_value_t *val;
Add blank line after declarations.
- snd_ctl_elem_value_alloca(&val);
- snd_ctl_elem_value_set_id(val, ctl->id);
- for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) {
for (j = 0; j < snd_ctl_elem_info_get_items(ctl->info); j++) {snd_ctl_elem_value_set_enumerated(val, i, j);err = write_and_verify(ctl, val, NULL);if (err != 0)fail = true;}- }
- return !fail;
+}
+void test_ctl_write_valid(struct ctl_data *ctl) +{
- bool pass;
- int err;
- /* If the control is turned off let's be polite */
- if (snd_ctl_elem_info_is_inactive(ctl->info)) {
ksft_print_msg("%s is inactive\n", ctl->name);ksft_test_result_skip("write_valid.%d.%d\n",ctl->card->card, ctl->elem);return;- }
- if (!snd_ctl_elem_info_is_writable(ctl->info)) {
ksft_print_msg("%s is not writeable\n", ctl->name);ksft_test_result_skip("write_valid.%d.%d\n",ctl->card->card, ctl->elem);return;- }
- switch (snd_ctl_elem_info_get_type(ctl->info)) {
- case SND_CTL_ELEM_TYPE_BOOLEAN:
pass = test_ctl_write_valid_boolean(ctl);break;- case SND_CTL_ELEM_TYPE_INTEGER:
pass = test_ctl_write_valid_integer(ctl);break;- case SND_CTL_ELEM_TYPE_INTEGER64:
pass = test_ctl_write_valid_integer64(ctl);break;- case SND_CTL_ELEM_TYPE_ENUMERATED:
pass = test_ctl_write_valid_enumerated(ctl);break;- default:
/* No tests for this yet */ksft_test_result_skip("write_valid.%d.%d\n",ctl->card->card, ctl->elem);return;- }
- /* Restore the default value to minimise disruption */
- err = write_and_verify(ctl, ctl->def_val, NULL);
- if (err < 0)
pass = false;- ksft_test_result(pass, "write_valid.%d.%d\n",
ctl->card->card, ctl->elem);+}
+int main(void) +{
- struct ctl_data *ctl;
- ksft_print_header();
Add a check for root and skil the test.
- find_controls();
- ksft_set_plan(num_controls * TESTS_PER_CONTROL);
- for (ctl = ctl_list; ctl != NULL; ctl = ctl->next) {
/** Must test get_value() before we write anything, the* test stores the default value for later cleanup.*/test_ctl_get_value(ctl);test_ctl_write_default(ctl);test_ctl_write_valid(ctl);- }
- ksft_exit_pass();
- return 0;
+}
thanks, -- Shuah
On Wed, Dec 08, 2021 at 10:42:35AM -0700, Shuah Khan wrote:
On 12/6/21 9:03 AM, Mark Brown wrote:
+SOUND - ALSA SELFTESTS +M: Mark Brown broonie@kernel.org +L: alsa-devel@alsa-project.org (moderated for non-subscribers)
Please add linux-kselftest list as well here.
get_maintainers pulls it in from the wider entry (the mention of alsa-devel is reudnant too).
+int num_cards = 0; +int num_controls = 0; +struct card_data *card_list = NULL; +struct ctl_data *ctl_list = NULL;
No need to initailize the above globals.
They're not declared static so the initial value is undefined.
+void find_controls(void) +{
- char name[32];
Use SYSFS_PATH_MAX = 255 like other tools do?
This isn't a path, it's an ALSA limit for a name that is embedded in a struct (snd_ctl_card_info->name). There's no magic define for these lengths.
ctl_data->next = ctl_list;ctl_list = ctl_data;}- next_card:
No need to indent the label
No need but it looks wrong otherwise - it's certainly what I'd expect for normal kernel code.
- if (snd_ctl_elem_info_is_inactive(ctl->info)) {
ksft_print_msg("%s is inactive\n", ctl->name);ksft_test_result_skip("get_value.%d.%d\n",ctl->card->card, ctl->elem);The two messages could be combined?
The user facing control names almost always have spaces in them so while it's useful to have them for diagnostic purposes I don't think it's a good idea to put them in the KTAP test names, that's likely to confuse things trying to work with the KTAP output. The general pattern I'm following for these tests is to print one or more diagnostic lines explaining why a tests was skipped or failed with the human readable control name so people can hopefully figure out what's going on and have the KTAP facing name that tooling will deal with be a consistent test.card.control format for parsers and databases dealing with test results en masse.
+bool test_ctl_write_valid_boolean(struct ctl_data *ctl) +{
- int err, i, j;
- bool fail = false;
- snd_ctl_elem_value_t *val;
Add blank line after declarations.
- snd_ctl_elem_value_alloca(&val);
This is idiomatic for alsa-lib code.
+int main(void) +{
- struct ctl_data *ctl;
- ksft_print_header();
Add a check for root and skil the test.
There is no need for this test to run as root in most configurations, it is common to provide direct access to the sound cards to some or all users - for example with desktop distros the entire userspace audio subsystem normally runs as the logged in user by default. alsa-lib's card enumeration should deal with any permissions problems accessing cards in the system cleanly. If the user running the test can't access any cards or the cards that can be accessed don't have any controls to test then we will find no controls during enumeration, report a plan to do zero tests and then exit cleanly which seems reasonable to me. If we do need to explicitly bomb out rather than report zero tests we should be doing it based on the number of controls we find rather than the user we're running as.
On 12/8/21 11:39 AM, Mark Brown wrote:
On Wed, Dec 08, 2021 at 10:42:35AM -0700, Shuah Khan wrote:
On 12/6/21 9:03 AM, Mark Brown wrote:
+SOUND - ALSA SELFTESTS +M: Mark Brown broonie@kernel.org +L: alsa-devel@alsa-project.org (moderated for non-subscribers)
Please add linux-kselftest list as well here.
get_maintainers pulls it in from the wider entry (the mention of alsa-devel is reudnant too).
+int num_cards = 0; +int num_controls = 0; +struct card_data *card_list = NULL; +struct ctl_data *ctl_list = NULL;
No need to initailize the above globals.
They're not declared static so the initial value is undefined.
+void find_controls(void) +{
- char name[32];
Use SYSFS_PATH_MAX = 255 like other tools do?
This isn't a path, it's an ALSA limit for a name that is embedded in a struct (snd_ctl_card_info->name). There's no magic define for these lengths.
ctl_data->next = ctl_list;ctl_list = ctl_data;}- next_card:
No need to indent the label
No need but it looks wrong otherwise - it's certainly what I'd expect for normal kernel code.
- if (snd_ctl_elem_info_is_inactive(ctl->info)) {
ksft_print_msg("%s is inactive\n", ctl->name);ksft_test_result_skip("get_value.%d.%d\n",ctl->card->card, ctl->elem);The two messages could be combined?
The user facing control names almost always have spaces in them so while it's useful to have them for diagnostic purposes I don't think it's a good idea to put them in the KTAP test names, that's likely to confuse things trying to work with the KTAP output. The general pattern I'm following for these tests is to print one or more diagnostic lines explaining why a tests was skipped or failed with the human readable control name so people can hopefully figure out what's going on and have the KTAP facing name that tooling will deal with be a consistent test.card.control format for parsers and databases dealing with test results en masse.
Fine with me.
+bool test_ctl_write_valid_boolean(struct ctl_data *ctl) +{
- int err, i, j;
- bool fail = false;
- snd_ctl_elem_value_t *val;
Add blank line after declarations.
- snd_ctl_elem_value_alloca(&val);
This is idiomatic for alsa-lib code.
This is kernel code that is going into kernel sources. Why follow alsa-lib convention?
+int main(void) +{
- struct ctl_data *ctl;
- ksft_print_header();
Add a check for root and skil the test.
There is no need for this test to run as root in most configurations, it is common to provide direct access to the sound cards to some or all users - for example with desktop distros the entire userspace audio subsystem normally runs as the logged in user by default. alsa-lib's card enumeration should deal with any permissions problems accessing cards in the system cleanly. If the user running the test can't access any cards or the cards that can be accessed don't have any controls to test then we will find no controls during enumeration, report a plan to do zero tests and then exit cleanly which seems reasonable to me. If we do need to explicitly bomb out rather than report zero tests we should be doing it based on the number of controls we find rather than the user we're running as.
On my system, I don't see any output if run as root. Are there some tests that work as non-root?
thanks, -- Shuah
On Wed, Dec 08, 2021 at 11:59:18AM -0700, Shuah Khan wrote:
On 12/8/21 11:39 AM, Mark Brown wrote:
On Wed, Dec 08, 2021 at 10:42:35AM -0700, Shuah Khan wrote:
- snd_ctl_elem_value_alloca(&val);
This is idiomatic for alsa-lib code.
This is kernel code that is going into kernel sources. Why follow alsa-lib convention?
Well, the kernel doesn't generally use alloca() as a pattern given the relatively small stack sizes we have and doesn't define helpers like these for it... it's a toss up here between the conventions for use of the library we're using and the conventions of the kernel.
- ksft_print_header();
Add a check for root and skil the test.
There is no need for this test to run as root in most configurations, it is common to provide direct access to the sound cards to some or all users - for example with desktop distros the entire userspace audio subsystem normally runs as the logged in user by default. alsa-lib's
On my system, I don't see any output if run as root. Are there some tests that work as non-root?
All of them work as non-root if the user they're running as has access to a card, if they do or not is system dependent - there may not be any cards at all in a given system to find. Running as root will punch through most permission problems but it's not a requirement and a system could use a security module like SELinux to restrict what root can do. The sound devices are usually in /dev/snd, though userspace can place them where it wants - if run as a user that can access the relevant devices for the mixer interface (usually /dev/snd/controlC* though again userspace can rename them) then the tests will run on those devices.
On 12/8/21 1:12 PM, Mark Brown wrote:
On Wed, Dec 08, 2021 at 11:59:18AM -0700, Shuah Khan wrote:
On 12/8/21 11:39 AM, Mark Brown wrote:
On Wed, Dec 08, 2021 at 10:42:35AM -0700, Shuah Khan wrote:
- snd_ctl_elem_value_alloca(&val);
This is idiomatic for alsa-lib code.
This is kernel code that is going into kernel sources. Why follow alsa-lib convention?
Well, the kernel doesn't generally use alloca() as a pattern given the relatively small stack sizes we have and doesn't define helpers like these for it... it's a toss up here between the conventions for use of the library we're using and the conventions of the kernel.
- ksft_print_header();
Add a check for root and skil the test.
There is no need for this test to run as root in most configurations, it is common to provide direct access to the sound cards to some or all users - for example with desktop distros the entire userspace audio subsystem normally runs as the logged in user by default. alsa-lib's
On my system, I don't see any output if run as root. Are there some tests that work as non-root?
All of them work as non-root if the user they're running as has access to a card, if they do or not is system dependent - there may not be any cards at all in a given system to find. Running as root will punch through most permission problems but it's not a requirement and a system could use a security module like SELinux to restrict what root can do. The sound devices are usually in /dev/snd, though userspace can place them where it wants - if run as a user that can access the relevant devices for the mixer interface (usually /dev/snd/controlC* though again userspace can rename them) then the tests will run on those devices.
Sounds good to me.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
participants (6)
-
Jaroslav Kysela -
Mark Brown -
Pierre-Louis Bossart -
Shuah Khan -
Takashi Iwai -
Takashi Sakamoto