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.