* [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
* [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 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
* 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.