[PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test

[PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test

[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

[PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test

[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

[PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints

[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

[PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints

[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

[PATCH v1 2/2] kselftest: alsa: Validate values read from enumerations

[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

[PATCH v1 2/2] kselftest: alsa: Validate values read from enumerations

[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

Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints

[Cezary Rojewski]

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;
> +}

Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints

[Cezary Rojewski]

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;
> +}

Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints

[Mark Brown]

[-- 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 --]

Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints

[Mark Brown]

[-- 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 --]

Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints

[Cezary Rojewski]

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

Re: [PATCH v1 1/2] kselftest: alsa: Factor out check that values meet constraints

[Cezary Rojewski]

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

Re: [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test

[Cezary Rojewski]

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

Re: [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test

[Cezary Rojewski]

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

Re: [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test

[Takashi Iwai]

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

Re: [PATCH v1 0/2] kselftest: alsa: Small enhancements to mixer-test

[Takashi Iwai]

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