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

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

[HyeongJun An]

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.

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.

Suggested-by: Mark Brown <broonie@kernel.org>
Link: https://lore.kernel.org/all/317fc80e-ec80-477d-80f6-53c395de4b4a@sirena.org.uk/
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
Assisted-by: Claude:claude-opus-5
---
 tools/testing/selftests/alsa/mixer-test.c | 225 ++++++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index a329f901c5ed..0f5750edcd98 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -30,6 +30,12 @@
 
 #define TESTS_PER_CONTROL 7
 
+/* Suffixes of the SNDRV_CTL_NAME_IEC958() names, not exported to userspace */
+#define IEC958_DEFAULT		"Default"
+#define IEC958_CON_MASK		"Con Mask"
+#define IEC958_PRO_MASK		"Pro Mask"
+#define IEC958_MASK		"Mask"
+
 struct card_data {
 	snd_ctl_t *handle;
 	int card;
@@ -842,6 +848,221 @@ static bool test_ctl_write_valid_enumerated(struct ctl_data *ctl)
 	return !fail;
 }
 
+/*
+ * 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;
+}
+
+/*
+ * Read the bits the device says it implements.  Bit 0 of the first status
+ * byte picks which mask applies, some devices publish only a plain Mask.
+ */
+static bool read_iec958_mask(struct ctl_data *ctl,
+			     const snd_aes_iec958_t *cur,
+			     snd_aes_iec958_t *mask)
+{
+	int err;
+	struct ctl_data *mask_ctl;
+	snd_ctl_elem_value_t *val;
+
+	if (!strend(ctl->name, IEC958_DEFAULT))
+		return false;
+
+	if (cur->status[0] & IEC958_AES0_PROFESSIONAL)
+		mask_ctl = find_iec958_mask_ctl(ctl, IEC958_PRO_MASK);
+	else
+		mask_ctl = find_iec958_mask_ctl(ctl, IEC958_CON_MASK);
+	if (!mask_ctl)
+		mask_ctl = find_iec958_mask_ctl(ctl, IEC958_MASK);
+	if (!mask_ctl)
+		return false;
+
+	snd_ctl_elem_value_alloca(&val);
+	snd_ctl_elem_value_set_id(val, mask_ctl->id);
+
+	err = snd_ctl_elem_read(mask_ctl->card->handle, val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed for %s: %s\n",
+			       mask_ctl->name, snd_strerror(err));
+		return false;
+	}
+
+	snd_ctl_elem_value_get_iec958(val, mask);
+
+	return true;
+}
+
+/*
+ * Pick one bit to toggle.  Bit 0 of the first status byte selects the
+ * layout the others are read in so leave it alone, prefer non audio.
+ */
+static bool pick_iec958_bit(const snd_aes_iec958_t *mask, unsigned int *byte,
+			    unsigned char *bit)
+{
+	unsigned int i;
+	int j;
+
+	if (mask->status[0] & IEC958_AES0_NONAUDIO) {
+		*byte = 0;
+		*bit = IEC958_AES0_NONAUDIO;
+		return true;
+	}
+
+	for (i = 0; i < sizeof(mask->status); i++) {
+		for (j = 0; j < 8; j++) {
+			if (i == 0 && (1 << j) == IEC958_AES0_PROFESSIONAL)
+				continue;
+			if (mask->status[i] & (1 << j)) {
+				*byte = i;
+				*bit = 1 << j;
+				return true;
+			}
+		}
+	}
+
+	return false;
+}
+
+/*
+ * Throw away the events from a write we are not grading, one left behind
+ * would make a missing notification look like a notification we got.
+ */
+static int drop_events(struct ctl_data *ctl)
+{
+	int err;
+
+	do {
+		err = wait_for_event(ctl, 0);
+	} while (err > 0);
+
+	return err;
+}
+
+/*
+ * 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)
+{
+	int err;
+	unsigned int byte;
+	unsigned char bit;
+	snd_aes_iec958_t iec958, mask;
+	snd_ctl_elem_value_t *orig_val, *val, *read_val, *w_val;
+	snd_ctl_elem_value_alloca(&orig_val);
+	snd_ctl_elem_value_alloca(&val);
+	snd_ctl_elem_value_alloca(&read_val);
+	snd_ctl_elem_value_alloca(&w_val);
+
+	/*
+	 * The bytes past the ones a driver implements are compared too,
+	 * so start from a read rather than building a value here.
+	 */
+	snd_ctl_elem_value_set_id(orig_val, ctl->id);
+	err = snd_ctl_elem_read(ctl->card->handle, orig_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	snd_ctl_elem_value_get_iec958(orig_val, &iec958);
+
+	/* With no mask to go on all we can do is try non audio */
+	memset(&mask, 0, sizeof(mask));
+	if (!read_iec958_mask(ctl, &iec958, &mask))
+		mask.status[0] = IEC958_AES0_NONAUDIO;
+
+	if (!pick_iec958_bit(&mask, &byte, &bit)) {
+		ksft_print_msg("%s implements no settable status bits\n",
+			       ctl->name);
+		return true;
+	}
+
+	iec958.status[byte] ^= bit;
+	snd_ctl_elem_value_copy(val, orig_val);
+	snd_ctl_elem_value_set_iec958(val, &iec958);
+
+	/* Writing can modify the value so keep a copy to write from */
+	snd_ctl_elem_value_copy(w_val, val);
+	err = snd_ctl_elem_write(ctl->card->handle, w_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_write() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	snd_ctl_elem_value_set_id(read_val, ctl->id);
+	err = snd_ctl_elem_read(ctl->card->handle, read_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	/* Put it back where we found it, then forget both writes */
+	snd_ctl_elem_value_copy(w_val, orig_val);
+	err = snd_ctl_elem_write(ctl->card->handle, w_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_write() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	if (drop_events(ctl) < 0)
+		return false;
+
+	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;
+	}
+
+	ksft_print_msg("%s toggling status[%u] 0x%02x\n", ctl->name, byte,
+		       bit);
+
+	return write_and_verify(ctl, val, NULL) == 0;
+}
+
 static void test_ctl_write_valid(struct ctl_data *ctl)
 {
 	bool pass;
@@ -878,6 +1099,10 @@ static void test_ctl_write_valid(struct ctl_data *ctl)
 		pass = test_ctl_write_valid_enumerated(ctl);
 		break;
 
+	case SND_CTL_ELEM_TYPE_IEC958:
+		pass = test_ctl_write_valid_iec958(ctl);
+		break;
+
 	default:
 		/* No tests for this yet */
 		ksft_test_result_skip("write_valid.%s.%d\n",
-- 
2.43.0

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

[Mark Brown]

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

On Fri, Sep 04, 2026 at 11:19:39AM +0900, HyeongJun An wrote:
> Nothing checks that the put() callback of an IEC958 control reports a
> change, because write_valid skips these controls.

> +/*
> + * Throw away the events from a write we are not grading, one left behind
> + * would make a missing notification look like a notification we got.
> + */
> +static int drop_events(struct ctl_data *ctl)
> +{
> +	int err;
> +
> +	do {
> +		err = wait_for_event(ctl, 0);
> +	} while (err > 0);
> +
> +	return err;
> +}

This seems like an unrelated change that wasn't noted in the changelog,
and one that should be made for all control types if we're going to make
it.

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

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

[HyeongJun An]

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.

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.

Suggested-by: Mark Brown <broonie@kernel.org>
Link: https://lore.kernel.org/all/317fc80e-ec80-477d-80f6-53c395de4b4a@sirena.org.uk/
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
Assisted-by: Claude:claude-opus-5
---
v2: No code change.  The changelog now says why the test drops the events
    from the two writes it does not grade.  It is bookkeeping for this
    test rather than a general change - the other write_valid tests write
    only through write_and_verify(), so they have no ungraded write of
    their own to drop after.

Link to v1: https://lore.kernel.org/all/20260904021939.149468-1-sammiee5311@gmail.com/

 tools/testing/selftests/alsa/mixer-test.c | 225 ++++++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index a329f901c5ed..0f5750edcd98 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -30,6 +30,12 @@
 
 #define TESTS_PER_CONTROL 7
 
+/* Suffixes of the SNDRV_CTL_NAME_IEC958() names, not exported to userspace */
+#define IEC958_DEFAULT		"Default"
+#define IEC958_CON_MASK		"Con Mask"
+#define IEC958_PRO_MASK		"Pro Mask"
+#define IEC958_MASK		"Mask"
+
 struct card_data {
 	snd_ctl_t *handle;
 	int card;
@@ -842,6 +848,221 @@ static bool test_ctl_write_valid_enumerated(struct ctl_data *ctl)
 	return !fail;
 }
 
+/*
+ * 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;
+}
+
+/*
+ * Read the bits the device says it implements.  Bit 0 of the first status
+ * byte picks which mask applies, some devices publish only a plain Mask.
+ */
+static bool read_iec958_mask(struct ctl_data *ctl,
+			     const snd_aes_iec958_t *cur,
+			     snd_aes_iec958_t *mask)
+{
+	int err;
+	struct ctl_data *mask_ctl;
+	snd_ctl_elem_value_t *val;
+
+	if (!strend(ctl->name, IEC958_DEFAULT))
+		return false;
+
+	if (cur->status[0] & IEC958_AES0_PROFESSIONAL)
+		mask_ctl = find_iec958_mask_ctl(ctl, IEC958_PRO_MASK);
+	else
+		mask_ctl = find_iec958_mask_ctl(ctl, IEC958_CON_MASK);
+	if (!mask_ctl)
+		mask_ctl = find_iec958_mask_ctl(ctl, IEC958_MASK);
+	if (!mask_ctl)
+		return false;
+
+	snd_ctl_elem_value_alloca(&val);
+	snd_ctl_elem_value_set_id(val, mask_ctl->id);
+
+	err = snd_ctl_elem_read(mask_ctl->card->handle, val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed for %s: %s\n",
+			       mask_ctl->name, snd_strerror(err));
+		return false;
+	}
+
+	snd_ctl_elem_value_get_iec958(val, mask);
+
+	return true;
+}
+
+/*
+ * Pick one bit to toggle.  Bit 0 of the first status byte selects the
+ * layout the others are read in so leave it alone, prefer non audio.
+ */
+static bool pick_iec958_bit(const snd_aes_iec958_t *mask, unsigned int *byte,
+			    unsigned char *bit)
+{
+	unsigned int i;
+	int j;
+
+	if (mask->status[0] & IEC958_AES0_NONAUDIO) {
+		*byte = 0;
+		*bit = IEC958_AES0_NONAUDIO;
+		return true;
+	}
+
+	for (i = 0; i < sizeof(mask->status); i++) {
+		for (j = 0; j < 8; j++) {
+			if (i == 0 && (1 << j) == IEC958_AES0_PROFESSIONAL)
+				continue;
+			if (mask->status[i] & (1 << j)) {
+				*byte = i;
+				*bit = 1 << j;
+				return true;
+			}
+		}
+	}
+
+	return false;
+}
+
+/*
+ * Throw away the events from a write we are not grading, one left behind
+ * would make a missing notification look like a notification we got.
+ */
+static int drop_events(struct ctl_data *ctl)
+{
+	int err;
+
+	do {
+		err = wait_for_event(ctl, 0);
+	} while (err > 0);
+
+	return err;
+}
+
+/*
+ * 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)
+{
+	int err;
+	unsigned int byte;
+	unsigned char bit;
+	snd_aes_iec958_t iec958, mask;
+	snd_ctl_elem_value_t *orig_val, *val, *read_val, *w_val;
+	snd_ctl_elem_value_alloca(&orig_val);
+	snd_ctl_elem_value_alloca(&val);
+	snd_ctl_elem_value_alloca(&read_val);
+	snd_ctl_elem_value_alloca(&w_val);
+
+	/*
+	 * The bytes past the ones a driver implements are compared too,
+	 * so start from a read rather than building a value here.
+	 */
+	snd_ctl_elem_value_set_id(orig_val, ctl->id);
+	err = snd_ctl_elem_read(ctl->card->handle, orig_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	snd_ctl_elem_value_get_iec958(orig_val, &iec958);
+
+	/* With no mask to go on all we can do is try non audio */
+	memset(&mask, 0, sizeof(mask));
+	if (!read_iec958_mask(ctl, &iec958, &mask))
+		mask.status[0] = IEC958_AES0_NONAUDIO;
+
+	if (!pick_iec958_bit(&mask, &byte, &bit)) {
+		ksft_print_msg("%s implements no settable status bits\n",
+			       ctl->name);
+		return true;
+	}
+
+	iec958.status[byte] ^= bit;
+	snd_ctl_elem_value_copy(val, orig_val);
+	snd_ctl_elem_value_set_iec958(val, &iec958);
+
+	/* Writing can modify the value so keep a copy to write from */
+	snd_ctl_elem_value_copy(w_val, val);
+	err = snd_ctl_elem_write(ctl->card->handle, w_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_write() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	snd_ctl_elem_value_set_id(read_val, ctl->id);
+	err = snd_ctl_elem_read(ctl->card->handle, read_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	/* Put it back where we found it, then forget both writes */
+	snd_ctl_elem_value_copy(w_val, orig_val);
+	err = snd_ctl_elem_write(ctl->card->handle, w_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_write() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	if (drop_events(ctl) < 0)
+		return false;
+
+	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;
+	}
+
+	ksft_print_msg("%s toggling status[%u] 0x%02x\n", ctl->name, byte,
+		       bit);
+
+	return write_and_verify(ctl, val, NULL) == 0;
+}
+
 static void test_ctl_write_valid(struct ctl_data *ctl)
 {
 	bool pass;
@@ -878,6 +1099,10 @@ static void test_ctl_write_valid(struct ctl_data *ctl)
 		pass = test_ctl_write_valid_enumerated(ctl);
 		break;
 
+	case SND_CTL_ELEM_TYPE_IEC958:
+		pass = test_ctl_write_valid_iec958(ctl);
+		break;
+
 	default:
 		/* No tests for this yet */
 		ksft_test_result_skip("write_valid.%s.%d\n",
-- 
2.43.0

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

[Mark Brown]

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[HyeongJun An]

On Tue, Sep 08, 2026 at 05:23:19PM +0100, Mark Brown wrote:

Thanks for the detailed review.

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

Yes, that's what I meant. Will change it.

> 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?

I couldn't find anything saying they must. 32 of the 36 writable
Playback Defaults in the tree have a Con Mask, Pro Mask or Mask sibling,
4 don't (fsl_spdif, fsl_xcvr, uniperif_player, stm32_sai_sub). The
driver docs only ask that mask and default share an iface, and
sound/core doesn't check. So unless there's a rule I'm missing I'll keep
the non audio guess for those and print that no mask was found, rather
than fail them.

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

Right, thanks. Will add one. I'll print the raw return value rather
than snd_strerror() since three of the wait_for_event() paths hand back
a bare -1.

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

Yes, good point. I'll split on whether a mask was found. With one,
walk every advertised bit except the layout bit through
write_and_verify() and fail if a bit doesn't stick, same as the integer
loop does. Without one, keep the v2 guess and only check the event.
That also means drop_events() only runs on the guess path.

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

That falls out of the split. 11 writes per control on the HDA card
here.

One thing I'd like to check with you first. Failing on an advertised
bit that's dropped turns three drivers that pass today into failures:
au88x0 (mask says 0xff, put() reads only the FS bits), ice1712 on Delta
1010 and Mediastation (mask 0xff, no put callback), and rme32 in
professional layout (mask advertises the whole emphasis field, the
converter reads one value). I think those are the mask not matching the
driver, which is the kind of thing the test is for, but I don't have
any of that hardware. I'll go with strict for v3 unless you'd rather
those were reported without failing. Happy to do either.

Thanks,
HyeongJun

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

[Mark Brown]

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

On Fri, Sep 11, 2026 at 10:44:16PM +0900, HyeongJun An wrote:
> On Tue, Sep 08, 2026 at 05:23:19PM +0100, Mark Brown wrote:

> > 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?

> I couldn't find anything saying they must. 32 of the 36 writable
> Playback Defaults in the tree have a Con Mask, Pro Mask or Mask sibling,
> 4 don't (fsl_spdif, fsl_xcvr, uniperif_player, stm32_sai_sub). The
> driver docs only ask that mask and default share an iface, and
> sound/core doesn't check. So unless there's a rule I'm missing I'll keep
> the non audio guess for those and print that no mask was found, rather
> than fail them.

I dunno, if 32 of 36 have a mask that sounds more like there is a
standard for all practical purposes but we've got a few drivers that
could be improved to better implement things.  This is quite common with
mixer-test - we generally enforce things relatively laxly compared to
the spec.  It's probably more helpful all round to add the missing mixer
controls.

> One thing I'd like to check with you first. Failing on an advertised
> bit that's dropped turns three drivers that pass today into failures:
> au88x0 (mask says 0xff, put() reads only the FS bits), ice1712 on Delta
> 1010 and Mediastation (mask 0xff, no put callback), and rme32 in
> professional layout (mask advertises the whole emphasis field, the
> converter reads one value). I think those are the mask not matching the
> driver, which is the kind of thing the test is for, but I don't have
> any of that hardware. I'll go with strict for v3 unless you'd rather
> those were reported without failing. Happy to do either.

Yes, I think this is a reasonable thing to add - like I was saying above
it's realtively common given our lack of enforcement and general driver
quality for us to see existing bugs.  Hopefully someone with the
hardware will notice and take a look, or if they don't perhaps it's not
actually causing them problems anyway.

Thanks for working on this!

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

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

[HyeongJun An]

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

Where the device publishes a mask, toggle every bit in it and check each
write.  A bit the mask advertises that does not stick fails the test, the
way a rejected value fails for the other control types.

Where none is found, keep the old single guess at the non audio bit,
which the device is free to ignore, so only the notification can be
counted against it.  That path writes without checking so drop_events()
clears what those writes leave behind.

On an HDA card with four HDMI PCMs the Con Mask is 0f ff, so each control
walks eleven bits instead of one and all eleven stick:

  -# 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 still shows up as
event_missing.  hdmi-codec and img-spdif-out store every byte and return
0, so boards using them report one event_missing per walked bit, and
hdmi-codec advertises the whole block so that is 191 of them.  A driver
whose mask claims more than its put() keeps now fails write_valid
outright: au88x0 and ice1712 on Delta 1010 and Mediastation advertise
bytes they never store, cs46xx, oxygen and ice1724 advertise bits their
put() drops, and the RME cards regenerate the whole emphasis field from
one bit so the rest of it never reads back.  None of that hardware was
to hand, the paths were read rather than run.

write_invalid stays a skip.  Every bit pattern is a valid channel status.

Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
Assisted-by: Claude:claude-opus-5
---
Changes in v3:
- Say check rather than grading.
- Where a mask is found, walk every bit it advertises instead of one and
  fail a bit that does not stick.
- Split the two cases into test_ctl_write_valid_iec958_mask() and
  test_ctl_write_valid_iec958_guess(), so drop_events() now runs only on
  the guess path.
- Keep the guess for the four drivers with no mask.  Adding the mask
  controls to fsl_spdif, fsl_xcvr, uniperif_player and stm32_sai_sub is a
  separate change for each of them.
- Report when the mask lookup falls back to the non audio guess, and
  report a drop_events() failure with the raw return value since three
  of the wait_for_event() paths hand back a bare -1.
- Name the drivers whose mask claims more than their put() keeps, which
  a full walk widens beyond the three v2 mentioned.

v2: https://lore.kernel.org/all/20260904123334.1505003-1-sammiee5311@gmail.com/
v1: https://lore.kernel.org/all/20260904021939.149468-1-sammiee5311@gmail.com/

 tools/testing/selftests/alsa/mixer-test.c | 255 ++++++++++++++++++++++
 1 file changed, 255 insertions(+)

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index 0857d64c322a..53a72753bb08 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -30,6 +30,12 @@
 
 #define TESTS_PER_CONTROL 7
 
+/* Suffixes of the SNDRV_CTL_NAME_IEC958() names, not exported to userspace */
+#define IEC958_DEFAULT		"Default"
+#define IEC958_CON_MASK		"Con Mask"
+#define IEC958_PRO_MASK		"Pro Mask"
+#define IEC958_MASK		"Mask"
+
 struct card_data {
 	snd_ctl_t *handle;
 	int card;
@@ -842,6 +848,251 @@ static bool test_ctl_write_valid_enumerated(struct ctl_data *ctl)
 	return !fail;
 }
 
+/*
+ * 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;
+}
+
+/*
+ * Read the bits the device says it implements.  Bit 0 of the first status
+ * byte picks which mask applies, some devices publish only a plain Mask.
+ */
+static bool read_iec958_mask(struct ctl_data *ctl,
+			     const snd_aes_iec958_t *cur,
+			     snd_aes_iec958_t *mask)
+{
+	int err;
+	struct ctl_data *mask_ctl;
+	snd_ctl_elem_value_t *val;
+
+	if (!strend(ctl->name, IEC958_DEFAULT))
+		return false;
+
+	if (cur->status[0] & IEC958_AES0_PROFESSIONAL)
+		mask_ctl = find_iec958_mask_ctl(ctl, IEC958_PRO_MASK);
+	else
+		mask_ctl = find_iec958_mask_ctl(ctl, IEC958_CON_MASK);
+	if (!mask_ctl)
+		mask_ctl = find_iec958_mask_ctl(ctl, IEC958_MASK);
+	if (!mask_ctl)
+		return false;
+
+	snd_ctl_elem_value_alloca(&val);
+	snd_ctl_elem_value_set_id(val, mask_ctl->id);
+
+	err = snd_ctl_elem_read(mask_ctl->card->handle, val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed for %s: %s\n",
+			       mask_ctl->name, snd_strerror(err));
+		return false;
+	}
+
+	snd_ctl_elem_value_get_iec958(val, mask);
+
+	return true;
+}
+
+/*
+ * Throw away the events from a write we are not checking, one left behind
+ * would make a missing notification look like a notification we got.
+ */
+static int drop_events(struct ctl_data *ctl)
+{
+	int err;
+
+	do {
+		err = wait_for_event(ctl, 0);
+	} while (err > 0);
+
+	return err;
+}
+
+/*
+ * Toggle every bit the device advertises, one at a time.  Each one starts
+ * from the value we read since a driver can look at the rest of the block
+ * when it stores a bit, and bit 0 of the first status byte selects the
+ * layout the others are read in so leave that one alone.
+ */
+static bool test_ctl_write_valid_iec958_mask(struct ctl_data *ctl,
+					     snd_ctl_elem_value_t *orig_val,
+					     const snd_aes_iec958_t *mask)
+{
+	int err, j;
+	unsigned int byte;
+	unsigned char bit;
+	bool fail = false, found = false;
+	snd_aes_iec958_t iec958;
+	snd_ctl_elem_value_t *val;
+	snd_ctl_elem_value_alloca(&val);
+
+	snd_ctl_elem_value_copy(val, orig_val);
+
+	for (byte = 0; byte < sizeof(mask->status); byte++) {
+		for (j = 0; j < 8; j++) {
+			bit = 1 << j;
+
+			if (byte == 0 && bit == IEC958_AES0_PROFESSIONAL)
+				continue;
+			if (!(mask->status[byte] & bit))
+				continue;
+			found = true;
+
+			snd_ctl_elem_value_get_iec958(orig_val, &iec958);
+			iec958.status[byte] ^= bit;
+			snd_ctl_elem_value_set_iec958(val, &iec958);
+
+			err = write_and_verify(ctl, val, NULL);
+			if (err != 0) {
+				ksft_print_msg("%s failed to set advertised status[%u] 0x%02x\n",
+					       ctl->name, byte, bit);
+				fail = true;
+			}
+		}
+	}
+
+	if (!found)
+		ksft_print_msg("%s implements no settable status bits\n",
+			       ctl->name);
+
+	return !fail;
+}
+
+/*
+ * With nothing advertised all we can do is try non audio, and the device
+ * need not implement even that.  Write it once and ignore the result, then
+ * compare against what came back so the value cannot fail, which leaves the
+ * notification counted by event_missing.
+ */
+static bool test_ctl_write_valid_iec958_guess(struct ctl_data *ctl,
+					      snd_ctl_elem_value_t *orig_val)
+{
+	int err;
+	snd_aes_iec958_t iec958;
+	snd_ctl_elem_value_t *val, *read_val, *w_val;
+	snd_ctl_elem_value_alloca(&val);
+	snd_ctl_elem_value_alloca(&read_val);
+	snd_ctl_elem_value_alloca(&w_val);
+
+	snd_ctl_elem_value_get_iec958(orig_val, &iec958);
+	iec958.status[0] ^= IEC958_AES0_NONAUDIO;
+	snd_ctl_elem_value_copy(val, orig_val);
+	snd_ctl_elem_value_set_iec958(val, &iec958);
+
+	/* Writing can modify the value so keep a copy to write from */
+	snd_ctl_elem_value_copy(w_val, val);
+	err = snd_ctl_elem_write(ctl->card->handle, w_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_write() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	snd_ctl_elem_value_set_id(read_val, ctl->id);
+	err = snd_ctl_elem_read(ctl->card->handle, read_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	/* Put it back where we found it, then drop the events from both */
+	snd_ctl_elem_value_copy(w_val, orig_val);
+	err = snd_ctl_elem_write(ctl->card->handle, w_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_write() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	err = drop_events(ctl);
+	if (err < 0) {
+		ksft_print_msg("drop_events() failed for %s: %d\n",
+			       ctl->name, err);
+		return false;
+	}
+
+	if (snd_ctl_elem_value_compare(val, read_val)) {
+		ksft_print_msg("%s does not implement status[0] 0x%02x\n",
+			       ctl->name, IEC958_AES0_NONAUDIO);
+
+		/* Expect what came back, the notification is still checked */
+		return write_and_verify(ctl, val, read_val) == 0;
+	}
+
+	return write_and_verify(ctl, val, NULL) == 0;
+}
+
+/*
+ * Write the channel status bits and check that userspace is told about it.
+ * Where a mask was found every bit in it has to stick, otherwise we fall
+ * back to one guessed bit the device is free to ignore.
+ */
+static bool test_ctl_write_valid_iec958(struct ctl_data *ctl)
+{
+	int err;
+	snd_aes_iec958_t iec958, mask;
+	snd_ctl_elem_value_t *orig_val;
+	snd_ctl_elem_value_alloca(&orig_val);
+
+	/*
+	 * The bytes past the ones a driver implements are compared too,
+	 * so start from a read rather than building a value here.
+	 */
+	snd_ctl_elem_value_set_id(orig_val, ctl->id);
+	err = snd_ctl_elem_read(ctl->card->handle, orig_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed: %s\n",
+			       snd_strerror(err));
+		return false;
+	}
+
+	snd_ctl_elem_value_get_iec958(orig_val, &iec958);
+
+	if (!read_iec958_mask(ctl, &iec958, &mask)) {
+		ksft_print_msg("%s falling back to non audio\n", ctl->name);
+		return test_ctl_write_valid_iec958_guess(ctl, orig_val);
+	}
+
+	return test_ctl_write_valid_iec958_mask(ctl, orig_val, &mask);
+}
+
 static void test_ctl_write_valid(struct ctl_data *ctl)
 {
 	bool pass;
@@ -878,6 +1129,10 @@ static void test_ctl_write_valid(struct ctl_data *ctl)
 		pass = test_ctl_write_valid_enumerated(ctl);
 		break;
 
+	case SND_CTL_ELEM_TYPE_IEC958:
+		pass = test_ctl_write_valid_iec958(ctl);
+		break;
+
 	default:
 		/* No tests for this yet */
 		ksft_test_result_skip("write_valid.%s.%d\n",
-- 
2.43.0

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

[Mark Brown]

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

On Sat, Sep 12, 2026 at 11:54:30AM +0900, HyeongJun An wrote:
> Nothing checks that the put() callback of an IEC958 control reports a
> change, because write_valid skips these controls.

Reviewed-by: Mark Brown <broonie@kernel.org>

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

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

[Takashi Iwai]

On Sat, 12 Sep 2026 04:54:30 +0200,
HyeongJun An wrote:
> 
> Nothing checks that the put() callback of an IEC958 control reports a
> change, because write_valid skips these controls.
> 
> Where the device publishes a mask, toggle every bit in it and check each
> write.  A bit the mask advertises that does not stick fails the test, the
> way a rejected value fails for the other control types.
> 
> Where none is found, keep the old single guess at the non audio bit,
> which the device is free to ignore, so only the notification can be
> counted against it.  That path writes without checking so drop_events()
> clears what those writes leave behind.
> 
> On an HDA card with four HDMI PCMs the Con Mask is 0f ff, so each control
> walks eleven bits instead of one and all eleven stick:
> 
>   -# 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 still shows up as
> event_missing.  hdmi-codec and img-spdif-out store every byte and return
> 0, so boards using them report one event_missing per walked bit, and
> hdmi-codec advertises the whole block so that is 191 of them.  A driver
> whose mask claims more than its put() keeps now fails write_valid
> outright: au88x0 and ice1712 on Delta 1010 and Mediastation advertise
> bytes they never store, cs46xx, oxygen and ice1724 advertise bits their
> put() drops, and the RME cards regenerate the whole emphasis field from
> one bit so the rest of it never reads back.  None of that hardware was
> to hand, the paths were read rather than run.
> 
> write_invalid stays a skip.  Every bit pattern is a valid channel status.
> 
> Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
> Assisted-by: Claude:claude-opus-5

Applied to for-next branch now.  Thanks.


Takashi