[PATCH 0/2] selftests/alsa: Catch writes that silently change other controls

[PATCH 0/2] selftests/alsa: Catch writes that silently change other controls

[HyeongJun An]

Mark asked for this in the es8316 thread [1]: have mixer-test check
for other controls changing when one is written. Patch 2 adds that.

Patch 1 fixes a bug that testing patch 2 turned up. With a driver that
notifies a neighbour with a value event, wait_for_event() returns on
the neighbour's event and the next test reports a spurious one.

The cost is two extra writes per control and a read of every other
control on the card after each. On the two cards here, 39 controls in
all, the run time does not move. I have no card with hundreds of
controls to measure against the 45 second kselftest timeout.

[1] https://lore.kernel.org/all/0fefbfe1-021b-46b2-8945-15956e75e37f@sirena.org.uk/

HyeongJun An (2):
  selftests/alsa: Do not take another control's event as our own
  selftests/alsa: Check that a write does not silently change other
    controls

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

-- 
2.43.0

[PATCH 1/2] selftests/alsa: Do not take another control's event as our own

[HyeongJun An]

The wait_for_event() skips an event for a different control with a
continue inside its do-while loop. But continue goes straight to the
loop condition, and that tests the mask of the event just skipped. So
a value event for another control ends the wait as if it were ours.

A driver that notifies a second control from its put() queues that
event first. The core only sends ours once put() returns. So the wait
returns early and leaves ours in the queue. The next test then
reports it as spurious. And event_missing passes for a control whose
put() said nothing changed, as long as it notified a neighbour.

To fix this, clear the mask before skipping the event.

Fixes: b1446bda5645 ("kselftest: alsa: Check for event generation when we write to controls")
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
Assisted-by: Claude:claude-opus-5-5
---
 tools/testing/selftests/alsa/mixer-test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index 53a72753bb08..02136c436e16 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -267,6 +267,8 @@ static int wait_for_event(struct ctl_data *ctl, int timeout)
 		if (ev_id != snd_ctl_elem_info_get_numid(ctl->info)) {
 			ksft_print_msg("Event for unexpected ctl %s\n",
 				       snd_ctl_event_elem_get_name(event));
+			/* The loop condition must not see the other control's mask */
+			mask = 0;
 			continue;
 		}
 
-- 
2.43.0

[PATCH 2/2] selftests/alsa: Check that a write does not silently change other controls

[HyeongJun An]

A control whose register mask is wider than its field clobbers the
field next to it. No event is sent for the neighbour, so nothing
notices. The es8316 bug fixed in
commit 6c1bb4031729 ("ASoC: codec: es8316: "DAC Soft Ramp Rate" is just a 2 bit control")
was one.

Add a test that writes each control's minimum and then its maximum.
After each write it compares the other controls on the card with a
snapshot. The minimum clears the field and the maximum sets its top
bit, which is where a mask one bit too wide lands. Some drivers link
controls on purpose, so a change announced with a value event is only
logged. A silent one fails.

On an HDA HDMI codec and snd-dummy, 39 controls in all, it reports
nothing. A snd-dummy copy with an injected silent overlap fails, and
the same overlap announced with a value event passes.

Suggested-by: Mark Brown <broonie@kernel.org>
Link: https://lore.kernel.org/all/0fefbfe1-021b-46b2-8945-15956e75e37f@sirena.org.uk/
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
Assisted-by: Claude:claude-opus-5-5
---
 tools/testing/selftests/alsa/mixer-test.c | 233 +++++++++++++++++++++-
 1 file changed, 232 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index 02136c436e16..966ef83354b9 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -28,7 +28,7 @@
 #include "kselftest.h"
 #include "alsa-local.h"
 
-#define TESTS_PER_CONTROL 7
+#define TESTS_PER_CONTROL 8
 
 /* Suffixes of the SNDRV_CTL_NAME_IEC958() names, not exported to userspace */
 #define IEC958_DEFAULT		"Default"
@@ -52,9 +52,14 @@ struct ctl_data {
 	snd_ctl_elem_id_t *id;
 	snd_ctl_elem_info_t *info;
 	snd_ctl_elem_value_t *def_val;
+	snd_ctl_elem_value_t *snapshot;
+	bool snapshot_valid;
+	bool moved;
 	int elem;
 	int event_missing;
 	int event_spurious;
+	int side_effects;
+	unsigned int ev_mask;
 	struct card_data *card;
 	struct ctl_data *next;
 };
@@ -159,6 +164,10 @@ static void find_controls(void)
 			if (err < 0)
 				ksft_exit_fail_msg("Out of memory\n");
 
+			err = snd_ctl_elem_value_malloc(&ctl_data->snapshot);
+			if (err < 0)
+				ksft_exit_fail_msg("Out of memory\n");
+
 			snd_ctl_elem_list_get_id(card_data->ctls, ctl,
 						 ctl_data->id);
 			snd_ctl_elem_info_set_id(ctl_data->info, ctl_data->id);
@@ -207,6 +216,20 @@ static void find_controls(void)
 	snd_config_delete(config);
 }
 
+/* The control on the same card that an event's numid refers to */
+static struct ctl_data *find_ctl_by_numid(struct card_data *card,
+					  unsigned int numid)
+{
+	struct ctl_data *ctl;
+
+	for (ctl = ctl_list; ctl != NULL; ctl = ctl->next)
+		if (ctl->card == card &&
+		    snd_ctl_elem_info_get_numid(ctl->info) == numid)
+			return ctl;
+
+	return NULL;
+}
+
 /*
  * Block for up to timeout ms for an event, returns a negative value
  * on error, 0 for no event and 1 for an event.
@@ -265,6 +288,17 @@ static int wait_for_event(struct ctl_data *ctl, int timeout)
 		mask = snd_ctl_event_elem_get_mask(event);
 		ev_id = snd_ctl_event_elem_get_numid(event);
 		if (ev_id != snd_ctl_elem_info_get_numid(ctl->info)) {
+			struct ctl_data *other = find_ctl_by_numid(ctl->card,
+								   ev_id);
+
+			/*
+			 * Remember that the driver announced this one.
+			 * test_ctl_write_side_effects() uses that to tell a
+			 * deliberate link from a silent register collision.
+			 */
+			if (other)
+				other->ev_mask |= mask;
+
 			ksft_print_msg("Event for unexpected ctl %s\n",
 				       snd_ctl_event_elem_get_name(event));
 			/* The loop condition must not see the other control's mask */
@@ -1149,6 +1183,202 @@ static void test_ctl_write_valid(struct ctl_data *ctl)
 			 ctl->card->card_name, ctl->elem);
 }
 
+/*
+ * Build the smallest or the largest value the control offers.  The smallest
+ * clears the control's register field and the largest sets its top bit, which
+ * is the bit a mask one bit too wide puts in its neighbour.
+ */
+static bool set_limit_value(struct ctl_data *ctl, snd_ctl_elem_value_t *val,
+			    bool max)
+{
+	int i, count = snd_ctl_elem_info_get_count(ctl->info);
+
+	snd_ctl_elem_value_set_id(val, ctl->id);
+
+	switch (snd_ctl_elem_info_get_type(ctl->info)) {
+	case SND_CTL_ELEM_TYPE_BOOLEAN:
+		for (i = 0; i < count; i++)
+			snd_ctl_elem_value_set_boolean(val, i, max);
+		return true;
+
+	case SND_CTL_ELEM_TYPE_INTEGER:
+		for (i = 0; i < count; i++)
+			snd_ctl_elem_value_set_integer(val, i, max ?
+				snd_ctl_elem_info_get_max(ctl->info) :
+				snd_ctl_elem_info_get_min(ctl->info));
+		return true;
+
+	case SND_CTL_ELEM_TYPE_INTEGER64:
+		for (i = 0; i < count; i++)
+			snd_ctl_elem_value_set_integer64(val, i, max ?
+				snd_ctl_elem_info_get_max64(ctl->info) :
+				snd_ctl_elem_info_get_min64(ctl->info));
+		return true;
+
+	case SND_CTL_ELEM_TYPE_ENUMERATED:
+		for (i = 0; i < count; i++)
+			snd_ctl_elem_value_set_enumerated(val, i, max ?
+				snd_ctl_elem_info_get_items(ctl->info) - 1 : 0);
+		return true;
+
+	default:
+		/* Nothing sensible to write for the rest */
+		return false;
+	}
+}
+
+/* Note every control on the card that no longer reads as it did */
+static void find_moved_ctls(struct ctl_data *ctl, snd_ctl_elem_value_t *val)
+{
+	struct ctl_data *other;
+	int err;
+
+	for (other = ctl_list; other != NULL; other = other->next) {
+		if (!other->snapshot_valid)
+			continue;
+
+		/*
+		 * The buffer is shared and compare() looks at all of it, so
+		 * clear what the last control left in the slots this one
+		 * does not use.
+		 */
+		snd_ctl_elem_value_clear(val);
+		snd_ctl_elem_value_set_id(val, other->id);
+		err = snd_ctl_elem_read(ctl->card->handle, val);
+		if (err < 0) {
+			ksft_print_msg("snd_ctl_elem_read() failed for %s: %s\n",
+				       other->name, snd_strerror(err));
+			continue;
+		}
+
+		if (snd_ctl_elem_value_compare(other->snapshot, val))
+			other->moved = true;
+	}
+}
+
+/*
+ * Write one control and look for others on the same card that moved with it.
+ * A driver that links two controls on purpose tells userspace about both, so
+ * only an unannounced change is counted.  That is what a control whose
+ * register mask covers bits belonging to its neighbour looks like from here.
+ */
+static void test_ctl_write_side_effects(struct ctl_data *ctl)
+{
+	struct ctl_data *other;
+	snd_ctl_elem_value_t *min_val, *max_val, *read_val;
+	int err;
+
+	snd_ctl_elem_value_alloca(&min_val);
+	snd_ctl_elem_value_alloca(&max_val);
+	snd_ctl_elem_value_alloca(&read_val);
+
+	/* Without a readable default there is nothing to put back */
+	if (snd_ctl_elem_info_is_inactive(ctl->info) ||
+	    !snd_ctl_elem_info_is_writable(ctl->info) ||
+	    !snd_ctl_elem_info_is_readable(ctl->info) ||
+	    !set_limit_value(ctl, min_val, false) ||
+	    !set_limit_value(ctl, max_val, true)) {
+		ksft_test_result_skip("write_side_effects.%s.%d\n",
+				      ctl->card->card_name, ctl->elem);
+		return;
+	}
+
+	/* Drain first, a stale event would look like the driver announced it */
+	drop_events(ctl);
+
+	/*
+	 * Record what the rest of the card reads as.  A volatile control can
+	 * move on its own so there is nothing to compare it against.
+	 */
+	for (other = ctl_list; other != NULL; other = other->next) {
+		other->snapshot_valid = false;
+		other->moved = false;
+		other->ev_mask = 0;
+
+		if (other == ctl || other->card != ctl->card)
+			continue;
+		if (!snd_ctl_elem_info_is_readable(other->info) ||
+		    snd_ctl_elem_info_is_volatile(other->info))
+			continue;
+
+		snd_ctl_elem_value_clear(other->snapshot);
+		snd_ctl_elem_value_set_id(other->snapshot, other->id);
+		err = snd_ctl_elem_read(ctl->card->handle, other->snapshot);
+		if (err < 0) {
+			ksft_print_msg("snd_ctl_elem_read() failed for %s: %s\n",
+				       other->name, snd_strerror(err));
+			continue;
+		}
+
+		other->snapshot_valid = true;
+	}
+
+	/*
+	 * Compare against the snapshot after each write, before anything is
+	 * put back.  Restoring the control we wrote goes through the same
+	 * mask, so doing it first would hide the change we are looking for.
+	 */
+	err = snd_ctl_elem_write(ctl->card->handle, min_val);
+	if (err >= 0) {
+		drop_events(ctl);
+		find_moved_ctls(ctl, read_val);
+		err = snd_ctl_elem_write(ctl->card->handle, max_val);
+	}
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_write() failed for %s: %s\n",
+			       ctl->name, snd_strerror(err));
+	} else {
+		drop_events(ctl);
+		find_moved_ctls(ctl, read_val);
+	}
+
+	for (other = ctl_list; other != NULL; other = other->next) {
+		if (!other->moved)
+			continue;
+
+		if (other->ev_mask & SND_CTL_EVENT_MASK_VALUE) {
+			ksft_print_msg("Writing %s changed %s, the driver said so\n",
+				       ctl->name, other->name);
+		} else {
+			ksft_print_msg("Writing %s silently changed %s\n",
+				       ctl->name, other->name);
+			ctl->side_effects++;
+		}
+	}
+
+	/*
+	 * The control we wrote goes back first so its mask stops moving the
+	 * rest.  A plain write keeps this out of the event counters, they
+	 * belong to the tests that check them.
+	 */
+	snd_ctl_elem_write(ctl->card->handle, ctl->def_val);
+
+	for (other = ctl_list; other != NULL; other = other->next) {
+		if (!other->snapshot_valid ||
+		    !snd_ctl_elem_info_is_writable(other->info))
+			continue;
+
+		snd_ctl_elem_value_clear(read_val);
+		snd_ctl_elem_value_set_id(read_val, other->id);
+		if (snd_ctl_elem_read(ctl->card->handle, read_val) < 0)
+			continue;
+
+		if (snd_ctl_elem_value_compare(other->snapshot, read_val))
+			snd_ctl_elem_write(ctl->card->handle, other->snapshot);
+	}
+
+	/* Our own restores queue events, the next test must not see them */
+	drop_events(ctl);
+
+	if (err < 0)
+		ksft_test_result_skip("write_side_effects.%s.%d\n",
+				      ctl->card->card_name, ctl->elem);
+	else
+		ksft_test_result(!ctl->side_effects,
+				 "write_side_effects.%s.%d\n",
+				 ctl->card->card_name, ctl->elem);
+}
+
 static bool test_ctl_write_invalid_value(struct ctl_data *ctl,
 					 snd_ctl_elem_value_t *val)
 {
@@ -1392,6 +1622,7 @@ int main(void)
 		test_ctl_name(ctl);
 		test_ctl_write_default(ctl);
 		test_ctl_write_valid(ctl);
+		test_ctl_write_side_effects(ctl);
 		test_ctl_write_invalid(ctl);
 		test_ctl_event_missing(ctl);
 		test_ctl_event_spurious(ctl);
-- 
2.43.0