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

[Shuah Khan <skhan@linuxfoundation.org>]

On 12/8/21 11:39 AM, Mark Brown wrote:
> 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.
> 

Fine with me.

>>> +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.

This is kernel code that is going into kernel sources. Why follow
alsa-lib convention?

> 
>>> +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.
> 

On my system, I don't see any output if run as root. Are there some tests
that work as non-root?

thanks,
-- Shuah