diff mbox series

[v3,2/3] kselftest: alsa: optimization for SNDRV_CTL_ELEM_ACCESS_VOLATILE

Message ID 20211208211745.533603-3-broonie@kernel.org (mailing list archive)
State Superseded
Headers show
Series kselftest: alsa: Add basic mixer selftest | expand

Commit Message

Mark Brown Dec. 8, 2021, 9:17 p.m. UTC 
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>

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>
Link: https://lore.kernel.org/r/Ya7TAHdMe9i41bsC@workstation
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/alsa/mixer-test.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Shuah Khan Dec. 8, 2021, 9:25 p.m. UTC | #1 
On 12/8/21 2:17 PM, Mark Brown wrote:
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> 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>
> Link: https://lore.kernel.org/r/Ya7TAHdMe9i41bsC@workstation
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>   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 ab51cf7b9e03..171d33692c7b 100644
> --- a/tools/testing/selftests/alsa/mixer-test.c
> +++ b/tools/testing/selftests/alsa/mixer-test.c
> @@ -307,9 +307,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.

Let's stick to kernel comment format :)

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

With that change:

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah
Takashi Sakamoto Dec. 10, 2021, 3:10 a.m. UTC | #2 
Dear Shuah Khan,

On Wed, Dec 08, 2021 at 02:25:36PM -0700, Shuah Khan wrote:
> On 12/8/21 2:17 PM, Mark Brown wrote:
> > From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > 
> > 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>
> > Link: https://lore.kernel.org/r/Ya7TAHdMe9i41bsC@workstation
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> > ---
> >   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 ab51cf7b9e03..171d33692c7b 100644
> > --- a/tools/testing/selftests/alsa/mixer-test.c
> > +++ b/tools/testing/selftests/alsa/mixer-test.c
> > @@ -307,9 +307,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.
> 
> Let's stick to kernel comment format :)
> 
> > +		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;
> >   	}
> > 
> 
> With that change:
> 
> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

Thanks for your review. Indeed, when following to existent guideline of
coding style, the comment should follow to C89/C90 style. I have no
objection to your advice itself, while for the guideline itself I'd like
to ask your opinion (and your help if possible).


In section '8) Commenting' in 'Documentation/process/coding-style.rst',
we can see no example for comment prefixed with double slashes; '//'. On
the other hand, we can see tons of actual usage in whole tree. We have the
inconsistency between the guideline and what developers have done.

I think that the decision to allow double-slashes comment or not is left
to subsystem maintainers, while I know that it's not allowed in UAPI
header since they are built with --std=C90 compiler option (see head of
'usr/include/Makefile'). I can not find such restriction in the other
parts of kernel code.

In my reference book about C language, double-slashes comment was
officially introduced in C99 (ISO/IEC 9899:1999) therefore it's not
specific to C++ nowadays. It's merely out of specification called as
'standard C' or 'ANSI C' (C89/C90, ISO/IEC 9899:1990).


Linux Torvalds appeared as his acceptance of double-slashes comment in the
context about his intolerance of multi-line comment such that the
introduction of comment, '/*', is just followed by content of comment
without line break:

 * Re: [patch] crypto: sha256-mb - cleanup a || vs | typo
     * https://lore.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@mail.gmail.com/

His preference is not necessarily equivalent to collective opinion in
kernel development community when seeing the patch applied later:

 * commit c4ff1b5f8bf0 ("CodingStyle: add networking specific block comment style")
     * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c4ff1b5f8bf0

His opinion does not necessarily have complete clout in the community,
but overall there is less reason to reject the double-slashes comment.


In my opinion, it's time to modify the coding style documentation in the
point of comment so that:

 * accept double-slashes comment from C99 in whole tree
 * except for UAPI header (to keep backward compatibility of userspace
   applications still built for C89/C90)

...But the discussion about official acceptance of C99 code can itself
evolve many developers since it's equivalent to loss of backward
compatibility to the environment built just for C89/C90. It's the reason I
never work for it since I have limited resources to join in the
discussion (I'm unpaid hobbyist with language barrier. My task in sound
subsystem is development and maintenance of in-kernel protocol
implementation of IEC 61883-1/6 and application drivers, including heavy
load for reverse engineering).

I'm glad if getting your assistance somehow for the issue.


Best regards

Takashi Sakamoto
diff mbox series

Patch

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index ab51cf7b9e03..171d33692c7b 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -307,9 +307,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;
 	}