Re: [PATCH] kselftest: alsa: Check for event generation when we write to controls

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

Hi Mark,

On Tue, Feb 01, 2022 at 03:15:51PM +0000, Mark Brown wrote:
> Add some coverage of event generation to mixer-test. Rather than doing a
> separate set of writes designed to trigger events we add a step to the
> existing write_and_verify() which checks to see if the value we read back
> from non-volatile controls matches the value before writing and that an
> event is or isn't generated as appropriate. The "tests" for events then
> simply check that no spurious or missing events were detected. This avoids
> needing further logic to generate appropriate values for each control type
> and maximises coverage.
> 
> When checking for events we use a timeout of 0. This relies on the kernel
> generating any event prior to returning to userspace when setting a control.
> That is currently the case and it is difficult to see it changing, if it
> does the test will need to be updated. Using a delay of 0 means that we
> don't slow things down unduly when checking for no event or when events
> fail to be generated.
> 
> We don't check behaviour for volatile controls since we can't tell what
> the behaviour is supposed to be for any given control.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  tools/testing/selftests/alsa/mixer-test.c | 148 +++++++++++++++++++++-
>  1 file changed, 145 insertions(+), 3 deletions(-)
 
I'm still under reviewing your patch, while I have a slight concern
about the evaluation of 'struct snd_ctl_event.data.elem.id.numid'.

> diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
> index 0e88f4f3d802..42cf3b724586 100644
> --- a/tools/testing/selftests/alsa/mixer-test.c
> +++ b/tools/testing/selftests/alsa/mixer-test.c
> ...
> +/*
> + * Block for up to timeout ms for an event, returns a negative value
> + * on error, 0 for no event and 1 for an event.
> + */
> +int wait_for_event(struct ctl_data *ctl, int timeout)
> +{
> ...
> +		/* The ID returned from the event is 1 less than numid */
> +		mask = snd_ctl_event_elem_get_mask(event);
> +		ev_id = snd_ctl_event_elem_get_numid(event);
> +		if (ev_id != ctl->elem + 1) {
> +			ksft_print_msg("Event for unexpected ctl %s\n",
> +				       snd_ctl_event_elem_get_name(event));
> +			continue;
> +		}
> +	} while ((mask & SND_CTL_EVENT_MASK_VALUE) != SND_CTL_EVENT_MASK_VALUE);
> +
> +	return 1;
> +}

As long as I know, the design of ALSA control core just exposes the
numeric identification of a control element issued for notification in
'snd_ctl_event' structure. On the other hand, the above evaluation
expects decremented value against position of queried list structure
has come.

I note that current design of ALSA control core is:
 * 1 is selected for numeric identification of the first element in the
   first element set added to sound card.
 * at removal of element set, the series of assigned numeric identification
   becomes blank (coded as hole).
 * Userspace application can always add/remove element set to the card.
 * the position of element in queried list structure does not
   necessarily corresponds to numeric identification even if decremented
   by 1 due to the hole.

Of cource, I can see the decremented-by-1 comparison covers the most cases
in which developer requires the test (excluding the case of user-defined
control element set), while we can use numid value got from 'id' field of
'ctl_data' structure for the comparison. I think the alternative way has
fewer problems than the decremented-by-1 comparison based on the rough
assumption against the design.


Regards

Takashi Sakamoto