[PATCH v3 0/3] kselftest: alsa: Add basic mixer selftest
This series adds a basic selftest for the mixer interface, as discussed in the cover letter for the main patch there's plenty of additional coverage that could be added but this is a good starting point.
v3: - Pull in incremental updates adding a fixed library configuration from Jaroslav and support for volatile controls from Takashi Sakamoto. - Stylistic updates suggested by Shuah. v2: - Use pkg-config to get CFLAGS and LDLIBS for alsa-lib.
Jaroslav Kysela (1): kselftest: alsa: Use private alsa-lib configuration in mixer test
Mark Brown (1): kselftest: alsa: Add simplistic test for ALSA mixer controls kselftest
Takashi Sakamoto (1): kselftest: alsa: optimization for SNDRV_CTL_ELEM_ACCESS_VOLATILE
MAINTAINERS | 8 + tools/testing/selftests/Makefile | 3 +- tools/testing/selftests/alsa/.gitignore | 1 + tools/testing/selftests/alsa/Makefile | 9 + tools/testing/selftests/alsa/mixer-test.c | 663 ++++++++++++++++++++++ 5 files changed, 683 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
base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
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 Reviewed-by: Shuah Khan skhan@linuxfoundation.org --- MAINTAINERS | 8 + tools/testing/selftests/Makefile | 3 +- tools/testing/selftests/alsa/.gitignore | 1 + tools/testing/selftests/alsa/Makefile | 9 + tools/testing/selftests/alsa/mixer-test.c | 605 ++++++++++++++++++++++ 5 files changed, 625 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 7a2345ce8521..8ded9f48b432 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17805,6 +17805,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 @@ -17824,6 +17825,13 @@ 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) +L: linux-kselftest@vger.kernel.org +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..ab51cf7b9e03 --- /dev/null +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -0,0 +1,605 @@ +// 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_exit_fail_msg("Out of memory\n"); + + 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_exit_fail_msg("Out of memory\n"); + + 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_exit_fail_msg("Out of memory\n"); + + err = snd_ctl_elem_info_malloc(&ctl_data->info); + if (err < 0) + ksft_exit_fail_msg("Out of memory\n"); + + err = snd_ctl_elem_value_malloc(&ctl_data->def_val); + if (err < 0) + ksft_exit_fail_msg("Out of memory\n"); + + 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; +}
From: Takashi Sakamoto o-takashi@sakamocchi.jp
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 Link: https://lore.kernel.org/r/Ya7TAHdMe9i41bsC@workstation Signed-off-by: Mark Brown broonie@kernel.org --- 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 ab51cf7b9e03..171d33692c7b 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -307,9 +307,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 12/8/21 2:17 PM, Mark Brown wrote:
From: Takashi Sakamoto o-takashi@sakamocchi.jp
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 Link: https://lore.kernel.org/r/Ya7TAHdMe9i41bsC@workstation Signed-off-by: Mark Brown broonie@kernel.org
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 ab51cf7b9e03..171d33692c7b 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -307,9 +307,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.
Let's stick to kernel comment format :)
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);
} else { return false; }return !is_volatile;
With that change:
Reviewed-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
Dear Shuah Khan,
On Wed, Dec 08, 2021 at 02:25:36PM -0700, Shuah Khan wrote:
On 12/8/21 2:17 PM, Mark Brown wrote:
From: Takashi Sakamoto o-takashi@sakamocchi.jp
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 Link: https://lore.kernel.org/r/Ya7TAHdMe9i41bsC@workstation Signed-off-by: Mark Brown broonie@kernel.org
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 ab51cf7b9e03..171d33692c7b 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -307,9 +307,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.
Let's stick to kernel comment format :)
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);
} else { return false; }return !is_volatile;
With that change:
Reviewed-by: Shuah Khan skhan@linuxfoundation.org
Thanks for your review. Indeed, when following to existent guideline of coding style, the comment should follow to C89/C90 style. I have no objection to your advice itself, while for the guideline itself I'd like to ask your opinion (and your help if possible).
In section '8) Commenting' in 'Documentation/process/coding-style.rst', we can see no example for comment prefixed with double slashes; '//'. On the other hand, we can see tons of actual usage in whole tree. We have the inconsistency between the guideline and what developers have done.
I think that the decision to allow double-slashes comment or not is left to subsystem maintainers, while I know that it's not allowed in UAPI header since they are built with --std=C90 compiler option (see head of 'usr/include/Makefile'). I can not find such restriction in the other parts of kernel code.
In my reference book about C language, double-slashes comment was officially introduced in C99 (ISO/IEC 9899:1999) therefore it's not specific to C++ nowadays. It's merely out of specification called as 'standard C' or 'ANSI C' (C89/C90, ISO/IEC 9899:1990).
Linux Torvalds appeared as his acceptance of double-slashes comment in the context about his intolerance of multi-line comment such that the introduction of comment, '/*', is just followed by content of comment without line break:
* Re: [patch] crypto: sha256-mb - cleanup a || vs | typo * https://lore.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_...
His preference is not necessarily equivalent to collective opinion in kernel development community when seeing the patch applied later:
* commit c4ff1b5f8bf0 ("CodingStyle: add networking specific block comment style") * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
His opinion does not necessarily have complete clout in the community, but overall there is less reason to reject the double-slashes comment.
In my opinion, it's time to modify the coding style documentation in the point of comment so that:
* accept double-slashes comment from C99 in whole tree * except for UAPI header (to keep backward compatibility of userspace applications still built for C89/C90)
...But the discussion about official acceptance of C99 code can itself evolve many developers since it's equivalent to loss of backward compatibility to the environment built just for C89/C90. It's the reason I never work for it since I have limited resources to join in the discussion (I'm unpaid hobbyist with language barrier. My task in sound subsystem is development and maintenance of in-kernel protocol implementation of IEC 61883-1/6 and application drivers, including heavy load for reverse engineering).
I'm glad if getting your assistance somehow for the issue.
Best regards
Takashi Sakamoto
From: Jaroslav Kysela perex@perex.cz
As mentined by Takashi Sakamoto, the system-wide alsa-lib configuration may override the standard device declarations. This patch use the private alsa-lib configuration to set the predictable environment.
Signed-off-by: Jaroslav Kysela perex@perex.cz Link: https://lore.kernel.org/r/20211208095209.1772296-1-perex@perex.cz [Restructure version test to keep the preprocessor happy -- broonie] Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/alsa/mixer-test.c | 56 ++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c index 171d33692c7b..a177676c530e 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -46,22 +46,74 @@ struct ctl_data { struct ctl_data *next; };
+static const char *alsa_config = +"ctl.hw {\n" +" @args [ CARD ]\n" +" @args.CARD.type string\n" +" type hw\n" +" card $CARD\n" +"}\n" +; + int num_cards = 0; int num_controls = 0; struct card_data *card_list = NULL; struct ctl_data *ctl_list = NULL;
+#ifdef SND_LIB_VER +#if SND_LIB_VERSION >= SND_LIB_VER(1, 2, 6) +#define LIB_HAS_LOAD_STRING +#endif +#endif + +#ifndef LIB_HAS_LOAD_STRING +int snd_config_load_string(snd_config_t **config, const char *s, size_t size) +{ + snd_input_t *input; + snd_config_t *dst; + int err; + + assert(config && s); + if (size == 0) + size = strlen(s); + err = snd_input_buffer_open(&input, s, size); + if (err < 0) + return err; + err = snd_config_top(&dst); + if (err < 0) { + snd_input_close(input); + return err; + } + err = snd_config_load(dst, input); + snd_input_close(input); + if (err < 0) { + snd_config_delete(dst); + return err; + } + *config = dst; + return 0; +} +#endif + void find_controls(void) { char name[32]; int card, ctl, err; struct card_data *card_data; struct ctl_data *ctl_data; + snd_config_t *config;
card = -1; if (snd_card_next(&card) < 0 || card < 0) return;
+ err = snd_config_load_string(&config, alsa_config, strlen(alsa_config)); + if (err < 0) { + ksft_print_msg("Unable to parse custom alsa-lib configuration: %s\n", + snd_strerror(err)); + ksft_exit_fail(); + } + while (card >= 0) { sprintf(name, "hw:%d", card);
@@ -69,7 +121,7 @@ void find_controls(void) if (!card_data) ksft_exit_fail_msg("Out of memory\n");
- err = snd_ctl_open(&card_data->handle, name, 0); + err = snd_ctl_open_lconf(&card_data->handle, name, 0, config); if (err < 0) { ksft_print_msg("Failed to get hctl for card %d: %s\n", card, snd_strerror(err)); @@ -137,6 +189,8 @@ void find_controls(void) break; } } + + snd_config_delete(config); }
/*
On 12/8/21 2:17 PM, Mark Brown wrote:
From: Jaroslav Kysela perex@perex.cz
As mentined by Takashi Sakamoto, the system-wide alsa-lib configuration may override the standard device declarations. This patch use the private alsa-lib configuration to set the predictable environment.
Signed-off-by: Jaroslav Kysela perex@perex.cz Link: https://lore.kernel.org/r/20211208095209.1772296-1-perex@perex.cz [Restructure version test to keep the preprocessor happy -- broonie] Signed-off-by: Mark Brown broonie@kernel.org
tools/testing/selftests/alsa/mixer-test.c | 56 ++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c index 171d33692c7b..a177676c530e 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -46,22 +46,74 @@ struct ctl_data { struct ctl_data *next; };
+static const char *alsa_config = +"ctl.hw {\n" +" @args [ CARD ]\n" +" @args.CARD.type string\n" +" type hw\n" +" card $CARD\n" +"}\n" +;
- int num_cards = 0; int num_controls = 0; struct card_data *card_list = NULL; struct ctl_data *ctl_list = NULL;
+#ifdef SND_LIB_VER +#if SND_LIB_VERSION >= SND_LIB_VER(1, 2, 6) +#define LIB_HAS_LOAD_STRING +#endif +#endif
+#ifndef LIB_HAS_LOAD_STRING +int snd_config_load_string(snd_config_t **config, const char *s, size_t size) +{
- snd_input_t *input;
- snd_config_t *dst;
- int err;
- assert(config && s);
- if (size == 0)
size = strlen(s);
- err = snd_input_buffer_open(&input, s, size);
- if (err < 0)
return err;
- err = snd_config_top(&dst);
- if (err < 0) {
snd_input_close(input);
return err;
- }
- err = snd_config_load(dst, input);
- snd_input_close(input);
- if (err < 0) {
snd_config_delete(dst);
return err;
- }
- *config = dst;
- return 0;
Why not consolidate the error path code?
+} +#endif
void find_controls(void) { char name[32]; int card, ctl, err; struct card_data *card_data; struct ctl_data *ctl_data;
snd_config_t *config;
card = -1; if (snd_card_next(&card) < 0 || card < 0) return;
err = snd_config_load_string(&config, alsa_config, strlen(alsa_config));
if (err < 0) {
ksft_print_msg("Unable to parse custom alsa-lib configuration: %s\n",
snd_strerror(err));
ksft_exit_fail();
}
while (card >= 0) { sprintf(name, "hw:%d", card);
@@ -69,7 +121,7 @@ void find_controls(void) if (!card_data) ksft_exit_fail_msg("Out of memory\n");
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));err = snd_ctl_open_lconf(&card_data->handle, name, 0, config);
@@ -137,6 +189,8 @@ void find_controls(void) break; } }
snd_config_delete(config); }
/*
This open comment at the end of the patch looks odd. Does this compile?
thanks -- Shuah
On Wed, Dec 08, 2021 at 02:27:34PM -0700, Shuah Khan wrote:
}
}
- snd_config_delete(config); } /*
This open comment at the end of the patch looks odd. Does this compile?
Yes, it's the start of the comment describing the next function, more complete context is:
}
snd_config_delete(config); }
/* * 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) {
On 12/8/21 2:44 PM, Mark Brown wrote:
On Wed, Dec 08, 2021 at 02:27:34PM -0700, Shuah Khan wrote:
} }
- snd_config_delete(config); } /*
This open comment at the end of the patch looks odd. Does this compile?
Yes, it's the start of the comment describing the next function, more complete context is:
} snd_config_delete(config);
}
/* * 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) {
Yes my bad :)
Reviewed-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
participants (3)
-
Mark Brown
-
Shuah Khan
-
Takashi Sakamoto