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

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

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