* [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test @ 2021-12-17 13:02 ` Mark Brown 0 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2021-12-17 13:02 UTC (permalink / raw) To: Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, linux-kselftest, Mark Brown These two patches improve the mixer test, checking that the default values for enums are valid and extending them to cover all the values for multi-value controls, not just the first one. It also factors out the validation that values are OK for controls for future reuse. Mark Brown (2): kselftest: alsa: Factor out check that values meet constraints kselftest: alsa: Validate values read from enumerations tools/testing/selftests/alsa/mixer-test.c | 158 ++++++++++++++-------- 1 file changed, 99 insertions(+), 59 deletions(-) base-commit: b73dad806533cad55df41a9c0349969b56d4ff7f -- 2.30.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test @ 2021-12-17 13:02 ` Mark Brown 0 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2021-12-17 13:02 UTC (permalink / raw) To: Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, Mark Brown, linux-kselftest These two patches improve the mixer test, checking that the default values for enums are valid and extending them to cover all the values for multi-value controls, not just the first one. It also factors out the validation that values are OK for controls for future reuse. Mark Brown (2): kselftest: alsa: Factor out check that values meet constraints kselftest: alsa: Validate values read from enumerations tools/testing/selftests/alsa/mixer-test.c | 158 ++++++++++++++-------- 1 file changed, 99 insertions(+), 59 deletions(-) base-commit: b73dad806533cad55df41a9c0349969b56d4ff7f -- 2.30.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints 2021-12-17 13:02 ` Mark Brown @ 2021-12-17 13:02 ` Mark Brown -1 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2021-12-17 13:02 UTC (permalink / raw) To: Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, linux-kselftest, Mark Brown To simplify the code a bit and allow future reuse factor the checks that values we read are valid out of test_ctl_get_value() into a separate function which can be reused later. As part of this extend the test to check all the values for the control, not just the first one. Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/alsa/mixer-test.c | 141 +++++++++++++--------- 1 file changed, 82 insertions(+), 59 deletions(-) diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c index b798a76f6825..b009fc5df605 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -193,124 +193,147 @@ void find_controls(void) 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) +bool ctl_value_index_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val, + int index) { - 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; + ksft_print_msg("%s.%d Invalid control type NONE\n", + ctl->name, index); + return false; case SND_CTL_ELEM_TYPE_BOOLEAN: - int_val = snd_ctl_elem_value_get_boolean(ctl->def_val, 0); + int_val = snd_ctl_elem_value_get_boolean(val, index); 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; + ksft_print_msg("%s.%d Invalid boolean value %ld\n", + ctl->name, index, int_val); + return false; } break; case SND_CTL_ELEM_TYPE_INTEGER: - int_val = snd_ctl_elem_value_get_integer(ctl->def_val, 0); + int_val = snd_ctl_elem_value_get_integer(val, index); 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, + ksft_print_msg("%s.%d value %ld less than minimum %ld\n", + ctl->name, index, int_val, snd_ctl_elem_info_get_min(ctl->info)); - err = -1; + return false; } 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, + ksft_print_msg("%s.%d value %ld more than maximum %ld\n", + ctl->name, index, int_val, snd_ctl_elem_info_get_max(ctl->info)); - err = -1; + return false; } /* Only check step size if there is one and we're in bounds */ - if (err >= 0 && snd_ctl_elem_info_get_step(ctl->info) && + if (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, + ksft_print_msg("%s.%d value %ld invalid for step %ld minimum %ld\n", + ctl->name, index, int_val, snd_ctl_elem_info_get_step(ctl->info), snd_ctl_elem_info_get_min(ctl->info)); - err = -1; + return false; } break; case SND_CTL_ELEM_TYPE_INTEGER64: - int64_val = snd_ctl_elem_value_get_integer64(ctl->def_val, 0); + int64_val = snd_ctl_elem_value_get_integer64(val, index); 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, + ksft_print_msg("%s.%d value %lld less than minimum %lld\n", + ctl->name, index, int64_val, snd_ctl_elem_info_get_min64(ctl->info)); - err = -1; + return false; } 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, + ksft_print_msg("%s.%d value %lld more than maximum %lld\n", + ctl->name, index, int64_val, snd_ctl_elem_info_get_max(ctl->info)); - err = -1; + return false; } /* Only check step size if there is one and we're in bounds */ - if (err >= 0 && snd_ctl_elem_info_get_step64(ctl->info) && + if (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, + ksft_print_msg("%s.%d value %lld invalid for step %lld minimum %lld\n", + ctl->name, index, int64_val, snd_ctl_elem_info_get_step64(ctl->info), snd_ctl_elem_info_get_min64(ctl->info)); - err = -1; + return false; } break; default: /* No tests for other types */ + break; + } + + return true; +} + +/* + * Check that the provided value meets the constraints for the + * provided control. + */ +bool ctl_value_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val) +{ + int i; + bool valid = true; + + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) + if (!ctl_value_index_valid(ctl, val, i)) + valid = false; + + return valid; +} + +/* + * 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; + + /* 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; + } + + if (!ctl_value_valid(ctl, ctl->def_val)) + err = -EINVAL; + out: ksft_test_result(err >= 0, "get_value.%d.%d\n", ctl->card->card, ctl->elem); -- 2.30.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints @ 2021-12-17 13:02 ` Mark Brown 0 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2021-12-17 13:02 UTC (permalink / raw) To: Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, Mark Brown, linux-kselftest To simplify the code a bit and allow future reuse factor the checks that values we read are valid out of test_ctl_get_value() into a separate function which can be reused later. As part of this extend the test to check all the values for the control, not just the first one. Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/alsa/mixer-test.c | 141 +++++++++++++--------- 1 file changed, 82 insertions(+), 59 deletions(-) diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c index b798a76f6825..b009fc5df605 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -193,124 +193,147 @@ void find_controls(void) 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) +bool ctl_value_index_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val, + int index) { - 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; + ksft_print_msg("%s.%d Invalid control type NONE\n", + ctl->name, index); + return false; case SND_CTL_ELEM_TYPE_BOOLEAN: - int_val = snd_ctl_elem_value_get_boolean(ctl->def_val, 0); + int_val = snd_ctl_elem_value_get_boolean(val, index); 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; + ksft_print_msg("%s.%d Invalid boolean value %ld\n", + ctl->name, index, int_val); + return false; } break; case SND_CTL_ELEM_TYPE_INTEGER: - int_val = snd_ctl_elem_value_get_integer(ctl->def_val, 0); + int_val = snd_ctl_elem_value_get_integer(val, index); 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, + ksft_print_msg("%s.%d value %ld less than minimum %ld\n", + ctl->name, index, int_val, snd_ctl_elem_info_get_min(ctl->info)); - err = -1; + return false; } 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, + ksft_print_msg("%s.%d value %ld more than maximum %ld\n", + ctl->name, index, int_val, snd_ctl_elem_info_get_max(ctl->info)); - err = -1; + return false; } /* Only check step size if there is one and we're in bounds */ - if (err >= 0 && snd_ctl_elem_info_get_step(ctl->info) && + if (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, + ksft_print_msg("%s.%d value %ld invalid for step %ld minimum %ld\n", + ctl->name, index, int_val, snd_ctl_elem_info_get_step(ctl->info), snd_ctl_elem_info_get_min(ctl->info)); - err = -1; + return false; } break; case SND_CTL_ELEM_TYPE_INTEGER64: - int64_val = snd_ctl_elem_value_get_integer64(ctl->def_val, 0); + int64_val = snd_ctl_elem_value_get_integer64(val, index); 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, + ksft_print_msg("%s.%d value %lld less than minimum %lld\n", + ctl->name, index, int64_val, snd_ctl_elem_info_get_min64(ctl->info)); - err = -1; + return false; } 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, + ksft_print_msg("%s.%d value %lld more than maximum %lld\n", + ctl->name, index, int64_val, snd_ctl_elem_info_get_max(ctl->info)); - err = -1; + return false; } /* Only check step size if there is one and we're in bounds */ - if (err >= 0 && snd_ctl_elem_info_get_step64(ctl->info) && + if (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, + ksft_print_msg("%s.%d value %lld invalid for step %lld minimum %lld\n", + ctl->name, index, int64_val, snd_ctl_elem_info_get_step64(ctl->info), snd_ctl_elem_info_get_min64(ctl->info)); - err = -1; + return false; } break; default: /* No tests for other types */ + break; + } + + return true; +} + +/* + * Check that the provided value meets the constraints for the + * provided control. + */ +bool ctl_value_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val) +{ + int i; + bool valid = true; + + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) + if (!ctl_value_index_valid(ctl, val, i)) + valid = false; + + return valid; +} + +/* + * 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; + + /* 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; + } + + if (!ctl_value_valid(ctl, ctl->def_val)) + err = -EINVAL; + out: ksft_test_result(err >= 0, "get_value.%d.%d\n", ctl->card->card, ctl->elem); -- 2.30.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints 2021-12-17 13:02 ` Mark Brown @ 2021-12-17 13:32 ` Cezary Rojewski -1 siblings, 0 replies; 16+ messages in thread From: Cezary Rojewski @ 2021-12-17 13:32 UTC (permalink / raw) To: Mark Brown, Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, linux-kselftest On 2021-12-17 2:02 PM, Mark Brown wrote: > To simplify the code a bit and allow future reuse factor the checks that > values we read are valid out of test_ctl_get_value() into a separate > function which can be reused later. As part of this extend the test to > check all the values for the control, not just the first one. > > Signed-off-by: Mark Brown <broonie@kernel.org> ... > +/* > + * Check that the provided value meets the constraints for the > + * provided control. > + */ > +bool ctl_value_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val) > +{ > + int i; > + bool valid = true; > + > + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) > + if (!ctl_value_index_valid(ctl, val, i)) > + valid = false; Correct me I'm wrong, but it seems a 'return false' would suffice. Is the continuation of looping still needed once a single check found above evaluates to true? Regards, Czarek > + > + return valid; > +} ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints @ 2021-12-17 13:32 ` Cezary Rojewski 0 siblings, 0 replies; 16+ messages in thread From: Cezary Rojewski @ 2021-12-17 13:32 UTC (permalink / raw) To: Mark Brown, Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, linux-kselftest On 2021-12-17 2:02 PM, Mark Brown wrote: > To simplify the code a bit and allow future reuse factor the checks that > values we read are valid out of test_ctl_get_value() into a separate > function which can be reused later. As part of this extend the test to > check all the values for the control, not just the first one. > > Signed-off-by: Mark Brown <broonie@kernel.org> ... > +/* > + * Check that the provided value meets the constraints for the > + * provided control. > + */ > +bool ctl_value_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val) > +{ > + int i; > + bool valid = true; > + > + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) > + if (!ctl_value_index_valid(ctl, val, i)) > + valid = false; Correct me I'm wrong, but it seems a 'return false' would suffice. Is the continuation of looping still needed once a single check found above evaluates to true? Regards, Czarek > + > + return valid; > +} ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints 2021-12-17 13:32 ` Cezary Rojewski @ 2021-12-17 14:38 ` Mark Brown -1 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2021-12-17 14:38 UTC (permalink / raw) To: Cezary Rojewski Cc: Takashi Iwai, Jaroslav Kysela, Shuah Khan, alsa-devel, linux-kselftest [-- Attachment #1: Type: text/plain, Size: 731 bytes --] On Fri, Dec 17, 2021 at 02:32:24PM +0100, Cezary Rojewski wrote: > On 2021-12-17 2:02 PM, Mark Brown wrote: > > + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) > > + if (!ctl_value_index_valid(ctl, val, i)) > > + valid = false; > Correct me I'm wrong, but it seems a 'return false' would suffice. Is the > continuation of looping still needed once a single check found above > evaluates to true? It doesn't affect the result of the test but it will cause us to print a diagnostic message for each invalid value rather than just the first one we see (eg, if both channels in a stereo control have an invalid value) which seems like it's more helpful to people working with the output than just the first error. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints @ 2021-12-17 14:38 ` Mark Brown 0 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2021-12-17 14:38 UTC (permalink / raw) To: Cezary Rojewski; +Cc: Takashi Iwai, alsa-devel, Shuah Khan, linux-kselftest [-- Attachment #1: Type: text/plain, Size: 731 bytes --] On Fri, Dec 17, 2021 at 02:32:24PM +0100, Cezary Rojewski wrote: > On 2021-12-17 2:02 PM, Mark Brown wrote: > > + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) > > + if (!ctl_value_index_valid(ctl, val, i)) > > + valid = false; > Correct me I'm wrong, but it seems a 'return false' would suffice. Is the > continuation of looping still needed once a single check found above > evaluates to true? It doesn't affect the result of the test but it will cause us to print a diagnostic message for each invalid value rather than just the first one we see (eg, if both channels in a stereo control have an invalid value) which seems like it's more helpful to people working with the output than just the first error. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints 2021-12-17 14:38 ` Mark Brown @ 2021-12-17 14:54 ` Cezary Rojewski -1 siblings, 0 replies; 16+ messages in thread From: Cezary Rojewski @ 2021-12-17 14:54 UTC (permalink / raw) To: Mark Brown Cc: Takashi Iwai, Jaroslav Kysela, Shuah Khan, alsa-devel, linux-kselftest On 2021-12-17 3:38 PM, Mark Brown wrote: > On Fri, Dec 17, 2021 at 02:32:24PM +0100, Cezary Rojewski wrote: >> On 2021-12-17 2:02 PM, Mark Brown wrote: > >>> + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) >>> + if (!ctl_value_index_valid(ctl, val, i)) >>> + valid = false; > >> Correct me I'm wrong, but it seems a 'return false' would suffice. Is the >> continuation of looping still needed once a single check found above >> evaluates to true? Just read my initial message and clearly an 'if' : (. Sorry for the confusion. > It doesn't affect the result of the test but it will cause us to print a > diagnostic message for each invalid value rather than just the first one > we see (eg, if both channels in a stereo control have an invalid value) > which seems like it's more helpful to people working with the output > than just the first error. So there is a good reason to do so. And I agree with such approach. Thanks for explaining, Mark! Regards, Czarek ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints @ 2021-12-17 14:54 ` Cezary Rojewski 0 siblings, 0 replies; 16+ messages in thread From: Cezary Rojewski @ 2021-12-17 14:54 UTC (permalink / raw) To: Mark Brown; +Cc: Takashi Iwai, alsa-devel, Shuah Khan, linux-kselftest On 2021-12-17 3:38 PM, Mark Brown wrote: > On Fri, Dec 17, 2021 at 02:32:24PM +0100, Cezary Rojewski wrote: >> On 2021-12-17 2:02 PM, Mark Brown wrote: > >>> + for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) >>> + if (!ctl_value_index_valid(ctl, val, i)) >>> + valid = false; > >> Correct me I'm wrong, but it seems a 'return false' would suffice. Is the >> continuation of looping still needed once a single check found above >> evaluates to true? Just read my initial message and clearly an 'if' : (. Sorry for the confusion. > It doesn't affect the result of the test but it will cause us to print a > diagnostic message for each invalid value rather than just the first one > we see (eg, if both channels in a stereo control have an invalid value) > which seems like it's more helpful to people working with the output > than just the first error. So there is a good reason to do so. And I agree with such approach. Thanks for explaining, Mark! Regards, Czarek ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 2/2] kselftest: alsa: Validate values read from enumerations 2021-12-17 13:02 ` Mark Brown @ 2021-12-17 13:02 ` Mark Brown -1 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2021-12-17 13:02 UTC (permalink / raw) To: Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, linux-kselftest, Mark Brown Enumerations should return a value between 0 and items-1, check that this is the case. Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/alsa/mixer-test.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c index b009fc5df605..17f158d7a767 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -276,6 +276,23 @@ bool ctl_value_index_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val, } break; + case SND_CTL_ELEM_TYPE_ENUMERATED: + int_val = snd_ctl_elem_value_get_enumerated(val, index); + + if (int_val < 0) { + ksft_print_msg("%s.%d negative value %ld for enumeration\n", + ctl->name, index, int_val); + return false; + } + + if (int_val >= snd_ctl_elem_info_get_items(ctl->info)) { + ksft_print_msg("%s.%d value %ld more than item count %ld\n", + ctl->name, index, int_val, + snd_ctl_elem_info_get_items(ctl->info)); + return false; + } + break; + default: /* No tests for other types */ break; -- 2.30.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 2/2] kselftest: alsa: Validate values read from enumerations @ 2021-12-17 13:02 ` Mark Brown 0 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2021-12-17 13:02 UTC (permalink / raw) To: Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, Mark Brown, linux-kselftest Enumerations should return a value between 0 and items-1, check that this is the case. Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/alsa/mixer-test.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c index b009fc5df605..17f158d7a767 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -276,6 +276,23 @@ bool ctl_value_index_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val, } break; + case SND_CTL_ELEM_TYPE_ENUMERATED: + int_val = snd_ctl_elem_value_get_enumerated(val, index); + + if (int_val < 0) { + ksft_print_msg("%s.%d negative value %ld for enumeration\n", + ctl->name, index, int_val); + return false; + } + + if (int_val >= snd_ctl_elem_info_get_items(ctl->info)) { + ksft_print_msg("%s.%d value %ld more than item count %ld\n", + ctl->name, index, int_val, + snd_ctl_elem_info_get_items(ctl->info)); + return false; + } + break; + default: /* No tests for other types */ break; -- 2.30.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test 2021-12-17 13:02 ` Mark Brown @ 2021-12-17 15:01 ` Cezary Rojewski -1 siblings, 0 replies; 16+ messages in thread From: Cezary Rojewski @ 2021-12-17 15:01 UTC (permalink / raw) To: Mark Brown, Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, linux-kselftest On 2021-12-17 2:02 PM, Mark Brown wrote: > These two patches improve the mixer test, checking that the default > values for enums are valid and extending them to cover all the values > for multi-value controls, not just the first one. It also factors out > the validation that values are OK for controls for future reuse. > > Mark Brown (2): > kselftest: alsa: Factor out check that values meet constraints > kselftest: alsa: Validate values read from enumerations > > tools/testing/selftests/alsa/mixer-test.c | 158 ++++++++++++++-------- > 1 file changed, 99 insertions(+), 59 deletions(-) > > > base-commit: b73dad806533cad55df41a9c0349969b56d4ff7f > Apart from my previous question regarding patch 1/2 (which has been already answered), I have not found any issues with the series. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Regards, Czarek ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test @ 2021-12-17 15:01 ` Cezary Rojewski 0 siblings, 0 replies; 16+ messages in thread From: Cezary Rojewski @ 2021-12-17 15:01 UTC (permalink / raw) To: Mark Brown, Takashi Iwai, Jaroslav Kysela, Shuah Khan Cc: alsa-devel, linux-kselftest On 2021-12-17 2:02 PM, Mark Brown wrote: > These two patches improve the mixer test, checking that the default > values for enums are valid and extending them to cover all the values > for multi-value controls, not just the first one. It also factors out > the validation that values are OK for controls for future reuse. > > Mark Brown (2): > kselftest: alsa: Factor out check that values meet constraints > kselftest: alsa: Validate values read from enumerations > > tools/testing/selftests/alsa/mixer-test.c | 158 ++++++++++++++-------- > 1 file changed, 99 insertions(+), 59 deletions(-) > > > base-commit: b73dad806533cad55df41a9c0349969b56d4ff7f > Apart from my previous question regarding patch 1/2 (which has been already answered), I have not found any issues with the series. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Regards, Czarek ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test 2021-12-17 13:02 ` Mark Brown @ 2021-12-25 8:13 ` Takashi Iwai -1 siblings, 0 replies; 16+ messages in thread From: Takashi Iwai @ 2021-12-25 8:13 UTC (permalink / raw) To: Mark Brown; +Cc: Jaroslav Kysela, Shuah Khan, alsa-devel, linux-kselftest On Fri, 17 Dec 2021 14:02:11 +0100, Mark Brown wrote: > > These two patches improve the mixer test, checking that the default > values for enums are valid and extending them to cover all the values > for multi-value controls, not just the first one. It also factors out > the validation that values are OK for controls for future reuse. > > Mark Brown (2): > kselftest: alsa: Factor out check that values meet constraints > kselftest: alsa: Validate values read from enumerations Applied both patches now. Thanks. Takashi ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test @ 2021-12-25 8:13 ` Takashi Iwai 0 siblings, 0 replies; 16+ messages in thread From: Takashi Iwai @ 2021-12-25 8:13 UTC (permalink / raw) To: Mark Brown; +Cc: alsa-devel, Shuah Khan, linux-kselftest On Fri, 17 Dec 2021 14:02:11 +0100, Mark Brown wrote: > > These two patches improve the mixer test, checking that the default > values for enums are valid and extending them to cover all the values > for multi-value controls, not just the first one. It also factors out > the validation that values are OK for controls for future reuse. > > Mark Brown (2): > kselftest: alsa: Factor out check that values meet constraints > kselftest: alsa: Validate values read from enumerations Applied both patches now. Thanks. Takashi ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-12-25 8:14 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-17 13:02 [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test Mark Brown 2021-12-17 13:02 ` Mark Brown 2021-12-17 13:02 ` [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints Mark Brown 2021-12-17 13:02 ` Mark Brown 2021-12-17 13:32 ` Cezary Rojewski 2021-12-17 13:32 ` Cezary Rojewski 2021-12-17 14:38 ` Mark Brown 2021-12-17 14:38 ` Mark Brown 2021-12-17 14:54 ` Cezary Rojewski 2021-12-17 14:54 ` Cezary Rojewski 2021-12-17 13:02 ` [PATCH v1 2/2] kselftest: alsa: Validate values read from enumerations Mark Brown 2021-12-17 13:02 ` Mark Brown 2021-12-17 15:01 ` [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test Cezary Rojewski 2021-12-17 15:01 ` Cezary Rojewski 2021-12-25 8:13 ` Takashi Iwai 2021-12-25 8:13 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.