Re: [PATCH v2] selftests/alsa: Test writes to IEC958 controls

[Mark Brown]

On Fri, Sep 04, 2026 at 09:33:34PM +0900, HyeongJun An wrote:

> Nothing checks that the put() callback of an IEC958 control reports a
> change, because write_valid skips these controls.

> Toggle one channel status bit and look for the event. The bit comes
> from the Con Mask, Pro Mask or plain Mask sibling where there is one,
> non-audio by preference because it means the same thing in both
> layouts. Bit 0 of the first byte picks the layout, so leave it alone.

> Write the value once without grading it first, since a device need not
> implement the bit we picked. The graded write then compares against
> what the device gave back, so it can only fail on the notification.

You're using "grading" a bunch, I think you mean "check"?  It makes
things hard to follow.

> That probe and the restore after it are not graded, so nothing consumes
> the events they make and the graded write would find one of those
> instead of its own. Drop them first. The other write_valid tests write
> only through write_and_verify(), so they have no ungraded write of their
> own to drop after.
> 
> On an HDA card with four HDMI PCMs:
> 
>   -# Totals: pass:212 fail:5 xfail:0 xpass:0 skip:56 error:0
>   +# Totals: pass:216 fail:5 xfail:0 xpass:0 skip:52 error:0
> 
> A driver whose put() never reports a change now shows up as
> event_missing rather than as a skip. hdmi-codec, img-spdif-out and
> uniperif_player store the value and return 0, so boards using them
> should report a new failure.
> 
> write_invalid stays a skip. Every bit pattern is a valid channel
> status.

> +/*
> + * Find the read only mask control for an IEC958 value control.  The two
> + * share device, subdevice and index but not always the interface.
> + */
> +static struct ctl_data *find_iec958_mask_ctl(struct ctl_data *ctl,
> +					     const char *suffix)
> +{
> +	char name[64];
> +	int stem;
> +	struct ctl_data *mask;
> +
> +	stem = strlen(ctl->name) - strlen(IEC958_DEFAULT);
> +	if (snprintf(name, sizeof(name), "%.*s%s", stem, ctl->name, suffix) >=
> +	    (int)sizeof(name))
> +		return NULL;
> +
> +	for (mask = ctl_list; mask != NULL; mask = mask->next) {
> +		if (mask->card != ctl->card)
> +			continue;
> +		if (snd_ctl_elem_info_get_type(mask->info) !=
> +		    SND_CTL_ELEM_TYPE_IEC958)
> +			continue;
> +		if (snd_ctl_elem_info_is_inactive(mask->info))
> +			continue;
> +		if (!snd_ctl_elem_info_is_readable(mask->info))
> +			continue;
> +		if (snd_ctl_elem_id_get_device(mask->id) !=
> +		    snd_ctl_elem_id_get_device(ctl->id))
> +			continue;
> +		if (snd_ctl_elem_id_get_subdevice(mask->id) !=
> +		    snd_ctl_elem_id_get_subdevice(ctl->id))
> +			continue;
> +		if (snd_ctl_elem_id_get_index(mask->id) !=
> +		    snd_ctl_elem_id_get_index(ctl->id))
> +			continue;
> +		if (strcmp(mask->name, name) == 0)
> +			return mask;
> +	}
> +
> +	return NULL;
> +}

It seems like we should have a test that we can actually find this
control, right now it looks like we silently ignore it being missing.
Are all IEC958 controls supposed to have one of these? 

> +/*
> + * Toggle a single channel status bit and check that userspace is told about
> + * it.  A device need not implement the bit we picked so write the value
> + * without grading it first, then grade against what it gave back.
> + */
> +static bool test_ctl_write_valid_iec958(struct ctl_data *ctl)
> +{

> +	if (drop_events(ctl) < 0)
> +		return false;

This will fail without reporting a diagnostic which isn't helpful.

> +	if (snd_ctl_elem_value_compare(val, read_val)) {
> +		/* Grade against what came back, the event check still runs */
> +		ksft_print_msg("%s does not implement status[%u] 0x%02x\n",
> +			       ctl->name, byte, bit);
> +		return write_and_verify(ctl, val, read_val) == 0;
> +	}

Aren't there two cases here?  There's the case where the device
advertised a mask where we presumably know that the control ought to be
writable, and there's the case where we just guessed a bit to write
where it's more legitimate that the write got ignored.

In theory we could also probe every single bit advertised rather than
just the first one, that's more what the tests for other control types
do.

Attachment: signature.asc
Description: PGP signature