[PATCH] selftests: alsa: Start validating control names

[PATCH] selftests: alsa: Start validating control names

[Mark Brown]

Not much of a test but we keep on getting problems with boolean controls
not being called Switches so let's add a few basic checks to help people
spot problems.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/alsa/mixer-test.c | 41 ++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index eb2213540fe3..b747dbc023ab 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -27,7 +27,7 @@
 
 #include "../kselftest.h"
 
-#define TESTS_PER_CONTROL 6
+#define TESTS_PER_CONTROL 7
 
 struct card_data {
 	snd_ctl_t *handle;
@@ -456,6 +456,44 @@ static void test_ctl_get_value(struct ctl_data *ctl)
 			 ctl->card->card, ctl->elem);
 }
 
+bool strend(const char *haystack, const char *needle)
+{
+	size_t haystack_len = strlen(haystack);
+	size_t needle_len = strlen(needle);
+
+	if (needle_len > haystack_len)
+		return false;
+	return strcmp(haystack + haystack_len - needle_len, needle) == 0;
+}
+
+static void test_ctl_name(struct ctl_data *ctl)
+{
+	bool name_ok = true;
+	bool check;
+
+	/* Only boolean controls should end in Switch */
+	if (strend(ctl->name, "Switch")) {
+		if (snd_ctl_elem_info_get_type(ctl->info) != SND_CTL_ELEM_TYPE_BOOLEAN) {
+			ksft_print_msg("%d.%d %s ends in Switch but is not boolean\n",
+				       ctl->card->card, ctl->elem, ctl->name);
+			name_ok = false;
+		}
+	}
+
+	/* Writeable boolean controls should end in Switch */
+	if (snd_ctl_elem_info_get_type(ctl->info) == SND_CTL_ELEM_TYPE_BOOLEAN &&
+	    snd_ctl_elem_info_is_writable(ctl->info)) {
+		if (!strend(ctl->name, "Switch")) {
+			ksft_print_msg("%d.%d %s is a writeable boolean but not a Switch\n",
+				       ctl->card->card, ctl->elem, ctl->name);
+			name_ok = false;
+		}
+	}
+
+	ksft_test_result(name_ok, "name.%d.%d\n",
+			 ctl->card->card, ctl->elem);
+}
+
 static bool show_mismatch(struct ctl_data *ctl, int index,
 			  snd_ctl_elem_value_t *read_val,
 			  snd_ctl_elem_value_t *expected_val)
@@ -1062,6 +1100,7 @@ int main(void)
 		 * test stores the default value for later cleanup.
 		 */
 		test_ctl_get_value(ctl);
+		test_ctl_name(ctl);
 		test_ctl_write_default(ctl);
 		test_ctl_write_valid(ctl);
 		test_ctl_write_invalid(ctl);
-- 
2.30.2

[PATCH] selftests: alsa: Start validating control names

[Mark Brown]

Not much of a test but we keep on getting problems with boolean controls
not being called Switches so let's add a few basic checks to help people
spot problems.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/alsa/mixer-test.c | 41 ++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index eb2213540fe3..b747dbc023ab 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -27,7 +27,7 @@
 
 #include "../kselftest.h"
 
-#define TESTS_PER_CONTROL 6
+#define TESTS_PER_CONTROL 7
 
 struct card_data {
 	snd_ctl_t *handle;
@@ -456,6 +456,44 @@ static void test_ctl_get_value(struct ctl_data *ctl)
 			 ctl->card->card, ctl->elem);
 }
 
+bool strend(const char *haystack, const char *needle)
+{
+	size_t haystack_len = strlen(haystack);
+	size_t needle_len = strlen(needle);
+
+	if (needle_len > haystack_len)
+		return false;
+	return strcmp(haystack + haystack_len - needle_len, needle) == 0;
+}
+
+static void test_ctl_name(struct ctl_data *ctl)
+{
+	bool name_ok = true;
+	bool check;
+
+	/* Only boolean controls should end in Switch */
+	if (strend(ctl->name, "Switch")) {
+		if (snd_ctl_elem_info_get_type(ctl->info) != SND_CTL_ELEM_TYPE_BOOLEAN) {
+			ksft_print_msg("%d.%d %s ends in Switch but is not boolean\n",
+				       ctl->card->card, ctl->elem, ctl->name);
+			name_ok = false;
+		}
+	}
+
+	/* Writeable boolean controls should end in Switch */
+	if (snd_ctl_elem_info_get_type(ctl->info) == SND_CTL_ELEM_TYPE_BOOLEAN &&
+	    snd_ctl_elem_info_is_writable(ctl->info)) {
+		if (!strend(ctl->name, "Switch")) {
+			ksft_print_msg("%d.%d %s is a writeable boolean but not a Switch\n",
+				       ctl->card->card, ctl->elem, ctl->name);
+			name_ok = false;
+		}
+	}
+
+	ksft_test_result(name_ok, "name.%d.%d\n",
+			 ctl->card->card, ctl->elem);
+}
+
 static bool show_mismatch(struct ctl_data *ctl, int index,
 			  snd_ctl_elem_value_t *read_val,
 			  snd_ctl_elem_value_t *expected_val)
@@ -1062,6 +1100,7 @@ int main(void)
 		 * test stores the default value for later cleanup.
 		 */
 		test_ctl_get_value(ctl);
+		test_ctl_name(ctl);
 		test_ctl_write_default(ctl);
 		test_ctl_write_valid(ctl);
 		test_ctl_write_invalid(ctl);
-- 
2.30.2

Re: [PATCH] selftests: alsa: Start validating control names

[Takashi Iwai]

On Wed, 20 Apr 2022 22:33:20 +0200,
Mark Brown wrote:
> 
> +bool strend(const char *haystack, const char *needle)

Missing static?

> +{
> +	size_t haystack_len = strlen(haystack);
> +	size_t needle_len = strlen(needle);
> +
> +	if (needle_len > haystack_len)
> +		return false;
> +	return strcmp(haystack + haystack_len - needle_len, needle) == 0;
> +}
> +
> +static void test_ctl_name(struct ctl_data *ctl)
> +{
> +	bool name_ok = true;
> +	bool check;
> +
> +	/* Only boolean controls should end in Switch */
> +	if (strend(ctl->name, "Switch")) {

This should be with " Switch" so that it won't check a concatenated
word.

> +		if (snd_ctl_elem_info_get_type(ctl->info) != SND_CTL_ELEM_TYPE_BOOLEAN) {
> +			ksft_print_msg("%d.%d %s ends in Switch but is not boolean\n",
> +				       ctl->card->card, ctl->elem, ctl->name);
> +			name_ok = false;
> +		}
> +	}
> +
> +	/* Writeable boolean controls should end in Switch */
> +	if (snd_ctl_elem_info_get_type(ctl->info) == SND_CTL_ELEM_TYPE_BOOLEAN &&
> +	    snd_ctl_elem_info_is_writable(ctl->info)) {
> +		if (!strend(ctl->name, "Switch")) {
> +			ksft_print_msg("%d.%d %s is a writeable boolean but not a Switch\n",
> +				       ctl->card->card, ctl->elem, ctl->name);
> +			name_ok = false;

I'm afraid that this would hit too many when applying to the existing
code; although the control name should be indeed with Switch suffix,
we tend to allow without suffix for casual non-standard elements.

But having the check would help for avoiding such a mistake for the
future code, so it's fine to add this strict check, IMO.


thanks,

Takashi

Re: [PATCH] selftests: alsa: Start validating control names

[Takashi Iwai]

On Wed, 20 Apr 2022 22:33:20 +0200,
Mark Brown wrote:
> 
> +bool strend(const char *haystack, const char *needle)

Missing static?

> +{
> +	size_t haystack_len = strlen(haystack);
> +	size_t needle_len = strlen(needle);
> +
> +	if (needle_len > haystack_len)
> +		return false;
> +	return strcmp(haystack + haystack_len - needle_len, needle) == 0;
> +}
> +
> +static void test_ctl_name(struct ctl_data *ctl)
> +{
> +	bool name_ok = true;
> +	bool check;
> +
> +	/* Only boolean controls should end in Switch */
> +	if (strend(ctl->name, "Switch")) {

This should be with " Switch" so that it won't check a concatenated
word.

> +		if (snd_ctl_elem_info_get_type(ctl->info) != SND_CTL_ELEM_TYPE_BOOLEAN) {
> +			ksft_print_msg("%d.%d %s ends in Switch but is not boolean\n",
> +				       ctl->card->card, ctl->elem, ctl->name);
> +			name_ok = false;
> +		}
> +	}
> +
> +	/* Writeable boolean controls should end in Switch */
> +	if (snd_ctl_elem_info_get_type(ctl->info) == SND_CTL_ELEM_TYPE_BOOLEAN &&
> +	    snd_ctl_elem_info_is_writable(ctl->info)) {
> +		if (!strend(ctl->name, "Switch")) {
> +			ksft_print_msg("%d.%d %s is a writeable boolean but not a Switch\n",
> +				       ctl->card->card, ctl->elem, ctl->name);
> +			name_ok = false;

I'm afraid that this would hit too many when applying to the existing
code; although the control name should be indeed with Switch suffix,
we tend to allow without suffix for casual non-standard elements.

But having the check would help for avoiding such a mistake for the
future code, so it's fine to add this strict check, IMO.


thanks,

Takashi

Re: [PATCH] selftests: alsa: Start validating control names

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On Thu, Apr 21, 2022 at 09:50:33AM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > +		if (!strend(ctl->name, "Switch")) {
> > +			ksft_print_msg("%d.%d %s is a writeable boolean but not a Switch\n",
> > +				       ctl->card->card, ctl->elem, ctl->name);
> > +			name_ok = false;

> I'm afraid that this would hit too many when applying to the existing
> code; although the control name should be indeed with Switch suffix,
> we tend to allow without suffix for casual non-standard elements.

It wasn't looking too bad in my survey of cards I had to hand - the
writable check is there because of jacks, which does make sense since
you can't actually switch them.  Some of that is due to ASoC handling
this in the core though.

> But having the check would help for avoiding such a mistake for the
> future code, so it's fine to add this strict check, IMO.

Yeah, that's more the target here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] selftests: alsa: Start validating control names

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On Thu, Apr 21, 2022 at 09:50:33AM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > +		if (!strend(ctl->name, "Switch")) {
> > +			ksft_print_msg("%d.%d %s is a writeable boolean but not a Switch\n",
> > +				       ctl->card->card, ctl->elem, ctl->name);
> > +			name_ok = false;

> I'm afraid that this would hit too many when applying to the existing
> code; although the control name should be indeed with Switch suffix,
> we tend to allow without suffix for casual non-standard elements.

It wasn't looking too bad in my survey of cards I had to hand - the
writable check is there because of jacks, which does make sense since
you can't actually switch them.  Some of that is due to ASoC handling
this in the core though.

> But having the check would help for avoiding such a mistake for the
> future code, so it's fine to add this strict check, IMO.

Yeah, that's more the target here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]