Re: [PATCH v2] kselftest: alsa: Add simplistic test for ALSA mixer controls kselftest

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Hi Mark,

On Mon, Dec 06, 2021 at 04:03:05PM +0000, Mark Brown wrote:
> Add a basic test for the mixer control interface. For every control on
> every sound card in the system it checks that it can read and write the
> default value where the control supports that and for writeable controls
> attempts to write all valid values, restoring the default values after
> each test to minimise disruption for users.
> 
> There are quite a few areas for improvement - currently no coverage of the
> generation of notifications, several of the control types don't have any
> coverage for the values and we don't have any testing of error handling
> when we attempt to write out of range values - but this provides some basic
> coverage.
> 
> This is added as a kselftest since unlike other ALSA test programs it does
> not require either physical setup of the device or interactive monitoring
> by users and kselftest is one of the test suites that is frequently run by
> people doing general automated testing so should increase coverage. It is
> written in terms of alsa-lib since tinyalsa is not generally packaged for
> distributions which makes things harder for general users interested in
> kselftest as a whole but it will be a barrier to people with Android.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> 
> v2: Use pkg-config to get CFLAGS and LDLIBS for alsa-lib.
> 
>  MAINTAINERS                               |   7 +
>  tools/testing/selftests/Makefile          |   3 +-
>  tools/testing/selftests/alsa/.gitignore   |   1 +
>  tools/testing/selftests/alsa/Makefile     |   9 +
>  tools/testing/selftests/alsa/mixer-test.c | 616 ++++++++++++++++++++++
>  5 files changed, 635 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/alsa/.gitignore
>  create mode 100644 tools/testing/selftests/alsa/Makefile
>  create mode 100644 tools/testing/selftests/alsa/mixer-test.c

I think it safer to take care of volatile attribute when comparing read
value to written value. I'm glad if you review below patch.

As another topic, the runtime of alsa-lib application largely differs
between process user due to the result of parsing text files for
configuration space. I can easily imagine that developers unfamiliar to
alsa-lib carelessly adds invalid or inadequate configurations to files
under target path of alsa-lib configuration space, and they are puzzled
since they are unaware of the fact that the kselftest is affected by
userspace stuffs for the runtime.

If we respect the basic theory of test (idempotence), we can use ioctl(2)
with requests for ALSA control interface since it's not so complicated
(at least it is easier than ALSA PCM interface). The purpose of
kselftest is to test kernel stuffs, not to test userspace stuffs
including alsa-lib implementation and variety of plugins.

======== 8< --------

From 0052f48a931d93b993e406ffaf4c8fbecac15e84 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Tue, 7 Dec 2021 11:43:56 +0900
Subject: [PATCH] kselftest: alsa: optimization for
 SNDRV_CTL_ELEM_ACCESS_VOLATILE

The volatile attribute of control element means that the hardware can
voluntarily change the state of control element independent of any
operation by software. ALSA control core necessarily sends notification
to userspace subscribers for any change from userspace application, while
it doesn't for the hardware's voluntary change.

This commit adds optimization for the attribute. Even if read value is
different from written value, the test reports success as long as the
target control element has the attribute. On the other hand, the
difference is itself reported for developers' convenience.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 tools/testing/selftests/alsa/mixer-test.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index 6082efa0b426..b87475fb7372 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -317,9 +317,13 @@ bool show_mismatch(struct ctl_data *ctl, int index,
 	}
 
 	if (expected_int != read_int) {
-		ksft_print_msg("%s.%d expected %lld but read %lld\n",
-			       ctl->name, index, expected_int, read_int);
-		return true;
+		// NOTE: The volatile attribute means that the hardware can voluntarily change the
+		// state of control element independent of any operation by software.
+		bool is_volatile = snd_ctl_elem_info_is_volatile(ctl->info);
+
+		ksft_print_msg("%s.%d expected %lld but read %lld, is_volatile %d\n",
+			       ctl->name, index, expected_int, read_int, is_volatile);
+		return !is_volatile;
 	} else {
 		return false;
 	}
-- 
2.32.0

======== 8< --------


Cheers

Takashi Sakamoto