alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Shuah Khan <skhan@linuxfoundation.org>
Cc: Takashi Iwai <tiwai@suse.de>,
	alsa-devel@alsa-project.org, Shuah Khan <shuah@kernel.org>,
	linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v2] kselftest: alsa: Add simplistic test for ALSA mixer controls kselftest
Date: Wed, 8 Dec 2021 18:39:52 +0000	[thread overview]
Message-ID: <YbD7+C74DFlZEokt@sirena.org.uk> (raw)
In-Reply-To: <de0c5677-c2cf-d1ab-68c5-2f410d17b66c@linuxfoundation.org>

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

On Wed, Dec 08, 2021 at 10:42:35AM -0700, Shuah Khan wrote:
> On 12/6/21 9:03 AM, Mark Brown wrote:

> > +SOUND - ALSA SELFTESTS
> > +M:	Mark Brown <broonie@kernel.org>
> > +L:	alsa-devel@alsa-project.org (moderated for non-subscribers)

> Please add linux-kselftest list as well here.

get_maintainers pulls it in from the wider entry (the mention of
alsa-devel is reudnant too).

> > +int num_cards = 0;
> > +int num_controls = 0;
> > +struct card_data *card_list = NULL;
> > +struct ctl_data *ctl_list = NULL;

> No need to initailize the above globals.

They're not declared static so the initial value is undefined.

> > +void find_controls(void)
> > +{
> > +	char name[32];

> Use SYSFS_PATH_MAX = 255 like other tools do?

This isn't a path, it's an ALSA limit for a name that is embedded in a
struct (snd_ctl_card_info->name).  There's no magic define for these
lengths.

> > +
> > +			ctl_data->next = ctl_list;
> > +			ctl_list = ctl_data;
> > +		}
> > +
> > +	next_card:

> No need to indent the label

No need but it looks wrong otherwise - it's certainly what I'd expect
for normal kernel code.

> > +	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);
> 
> The two messages could be combined?

The user facing control names almost always have spaces in them so while
it's useful to have them for diagnostic purposes I don't think it's a
good idea to put them in the KTAP test names, that's likely to confuse
things trying to work with the KTAP output.  The general pattern I'm
following for these tests is to print one or more diagnostic lines
explaining why a tests was skipped or failed with the human readable
control name so people can hopefully figure out what's going on and have
the KTAP facing name that tooling will deal with be a consistent
test.card.control format for parsers and databases dealing with test
results en masse.

> > +bool test_ctl_write_valid_boolean(struct ctl_data *ctl)
> > +{
> > +	int err, i, j;
> > +	bool fail = false;
> > +	snd_ctl_elem_value_t *val;
> 
> Add blank line after declarations.
> 
> > +	snd_ctl_elem_value_alloca(&val);

This is idiomatic for alsa-lib code.

> > +int main(void)
> > +{
> > +	struct ctl_data *ctl;
> > +
> > +	ksft_print_header();

> Add a check for root and skil the test.

There is no need for this test to run as root in most configurations,
it is common to provide direct access to the sound cards to some or all
users - for example with desktop distros the entire userspace audio
subsystem normally runs as the logged in user by default.  alsa-lib's
card enumeration should deal with any permissions problems accessing
cards in the system cleanly.  If the user running the test can't access
any cards or the cards that can be accessed don't have any controls to
test then we will find no controls during enumeration, report a plan to
do zero tests and then exit cleanly which seems reasonable to me.  If we
do need to explicitly bomb out rather than report zero tests we should
be doing it based on the number of controls we find rather than the user
we're running as.

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

  reply	other threads:[~2021-12-08 18:41 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-06 16:03 [PATCH v2] kselftest: alsa: Add simplistic test for ALSA mixer controls kselftest Mark Brown
2021-12-06 16:27 ` Pierre-Louis Bossart
2021-12-06 16:31   ` Pierre-Louis Bossart
2021-12-06 16:39   ` Mark Brown
2021-12-06 17:01     ` Pierre-Louis Bossart
2021-12-06 18:17       ` Mark Brown
2021-12-07  3:20 ` Takashi Sakamoto
2021-12-07  8:05   ` Jaroslav Kysela
2021-12-07 14:25   ` Mark Brown
2021-12-07 14:36     ` Takashi Iwai
2021-12-07 14:49       ` Mark Brown
2021-12-08 14:26     ` Takashi Sakamoto
2021-12-08 14:31       ` Takashi Sakamoto
2021-12-08 16:07       ` Mark Brown
2021-12-08 17:42 ` Shuah Khan
2021-12-08 18:39   ` Mark Brown [this message]
2021-12-08 18:59     ` Shuah Khan
2021-12-08 20:12       ` Mark Brown
2021-12-08 21:14         ` Shuah Khan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YbD7+C74DFlZEokt@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=shuah@kernel.org \
    --cc=skhan@linuxfoundation.org \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).