Re: [RFC PATCH 13/16] ALSA: mixer_audient: Add enumerated controls

[Will Porter <mrwillporter@gmail.com>]

Hi Christian,

The bot found a real problem in my patch 13:

> Does advertising ctype->enumeration->items + 1 items in
> snd_ctl_audient_info() while rejecting item index e->items with
> -EINVAL in snd_ctl_audient_enum_set() break the ALSA control UAPI
> contract for writable enumerated controls?

Yes. mixer-test writes every advertised item, and alsactl restore
fails on a value that store saved as the extra item.

The diff below drops the extra item from every enum:

- A device value outside the list makes get return -EIO. I chose an
  error over a fallback item: an unknown routing value shown as Main
  Mix could hide a route that bypasses the speaker volume, cut and
  dim. The device is not known to report such a value.
- A write-only value that no transfer has confirmed (only after a
  failed probe write) reads as its init value, as a write-only switch
  reads off.

Tested on the iD14 (stable 7.2.15) with the diff applied, with a
script that does the enum writes of mixer-test on this card only:
get returned every advertised item after its put (from the cache for
the write-only controls), and the item past the end was rejected.
alsactl store and restore of the card succeeded. I did not run the
full mixer-test, because it writes the controls of every card.

Does -EIO from get look right to you here? If the diff looks good,
I'll fold it into patches 13 and 15 in the next version.

Thanks,
Will

-- >8 --
From: Will Porter <mrwillporter@gmail.com>
Subject: [RFC PATCH] ALSA: mixer_audient: Advertise only selectable enum items

Every enumerated control advertised one item more than a put accepts.
mixer-test writes every advertised item, and alsactl restore fails on
a stored value of that item. Advertise only the real items.

A get of a device value outside the list now returns -EIO: an unknown
routing value shown as Main Mix could hide a route that bypasses the
speaker volume, cut and dim. After a failed probe write, a write-only
value reads as its init value; this does not confirm the device
state.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Will Porter <mrwillporter@gmail.com>
---
Proposed change to patches 13 and 15.

 sound/usb/mixer_audient.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/sound/usb/mixer_audient.c b/sound/usb/mixer_audient.c
index e65c62e1eac4a..4180d7d8b325c 100644
--- a/sound/usb/mixer_audient.c
+++ b/sound/usb/mixer_audient.c
@@ -67,10 +67,8 @@ static const DECLARE_TLV_DB_MINMAX_MUTE(snd_audient_tlv_mixer, -12800, 600);
 #define SND_AUDIENT_CTYPE_FLAGS_CACHE		BIT(3)
 
 /*
- * An enumerated control: item i sends values[i]. The texts have one item
- * more than the values. A put cannot select that last item: a get shows it
- * for a device value outside the list ("Other"), or for a write-only value
- * that no write has confirmed ("Unknown").
+ * An enumerated control: item i sends values[i]. A get of a device value
+ * outside the list returns -EIO; it does not show a wrong item.
  */
 struct snd_audient_enum {
 	const char *const *texts;
@@ -85,7 +83,7 @@ struct snd_audient_enum {
 		.values = name##_values,				\
 		.items = ARRAY_SIZE(name##_values),			\
 	};								\
-	static_assert(ARRAY_SIZE(texts_) == ARRAY_SIZE(name##_values) + 1)
+	static_assert(ARRAY_SIZE(texts_) == ARRAY_SIZE(name##_values))
 
 /*
  * iD14 MkI units. The monitor controller, output routing, headphone level,
@@ -126,7 +124,7 @@ static const DECLARE_TLV_DB_SCALE(snd_audient_tlv_id14_vol, -12800, 100, 0);
 
 /* Mono mode: one-speaker sum (Left, Right) or phantom centre */
 static const char *const snd_audient_id14_mono_mode_texts[] = {
-	"Left", "Centre", "Right", "Other",
+	"Left", "Centre", "Right",
 };
 SND_AUDIENT_ENUM(snd_audient_id14_mono_mode,
 		 snd_audient_id14_mono_mode_texts, 1, 0, 2);
@@ -139,10 +137,10 @@ SND_AUDIENT_ENUM(snd_audient_id14_mono_mode,
  * mute still act on every headphone source.
  */
 static const char *const snd_audient_id14_route_main_texts[] = {
-	"Main Mix", "Cue Mix", "DAW Thru (full scale)", "Other",
+	"Main Mix", "Cue Mix", "DAW Thru (full scale)",
 };
 static const char *const snd_audient_id14_route_hp_texts[] = {
-	"Main Mix", "Cue Mix", "DAW Thru", "Other",
+	"Main Mix", "Cue Mix", "DAW Thru",
 };
 SND_AUDIENT_ENUM(snd_audient_id14_route_main_l,
 		 snd_audient_id14_route_main_texts, 0x14, 0x10, 0x00);
@@ -156,12 +154,10 @@ SND_AUDIENT_ENUM(snd_audient_id14_route_hp_r,
 /*
  * iD key function: the value is the monitor controller selector the key
  * toggles. ScrollControl (0xffff) toggles nothing: the knob then scrolls
- * through the HID interface. The last item of a write-only enum shows a
- * value that is not known.
+ * through the HID interface.
  */
 static const char *const snd_audient_id14_id_key_texts[] = {
 	"ScrollControl", "Mono", "Mono + Polarity", "Dim", "Talkback",
-	"Unknown",
 };
 SND_AUDIENT_ENUM(snd_audient_id14_id_key, snd_audient_id14_id_key_texts,
 		 0xffff, ID14_MON_MONO, ID14_MON_POLARITY, ID14_MON_DIM,
@@ -170,13 +166,13 @@ SND_AUDIENT_ENUM(snd_audient_id14_id_key, snd_audient_id14_id_key_texts,
 /* Talkback source: the routing source number of the input; none = 0xff */
 static const char *const snd_audient_id14_talkback_texts[] = {
 	"None", "Mic 1", "Mic 2", "Digi 1", "Digi 2", "Digi 3", "Digi 4",
-	"Digi 5", "Digi 6", "Digi 7", "Digi 8", "Unknown",
+	"Digi 5", "Digi 6", "Digi 7", "Digi 8",
 };
 SND_AUDIENT_ENUM(snd_audient_id14_talkback, snd_audient_id14_talkback_texts,
 		 0xff, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13);
 
 static const char *const snd_audient_id14_optical_texts[] = {
-	"ADAT", "S/PDIF", "Unknown",
+	"ADAT", "S/PDIF",
 };
 SND_AUDIENT_ENUM(snd_audient_id14_optical, snd_audient_id14_optical_texts,
 		 0, 1);
@@ -933,22 +929,23 @@ static int snd_ctl_audient_enum_get(struct snd_kcontrol *kctl,
 	guard(mutex)(&mixer->lock);
 
 	for (i = 0; i < hwchan; i++) {
+		unsigned int item;
 		u32 val;
 		int ret;
 
 		ret = snd_audient_raw_get_one(mixer, ctype,
 					      snd_audient_cache_slot(elem, i),
 					      base + i, &val);
-		if (ret == -ENODATA) {	/* unknown write-only value */
-			ctl_val->value.enumerated.item[i] =
-				ctype->enumeration->items;
-			continue;
-		} else if (ret < 0) {
+		if (ret == -ENODATA)	/* unknown write-only value: init */
+			val = ctype->init;
+		else if (ret < 0)
 			return ret;
-		}
 
-		ctl_val->value.enumerated.item[i] =
-			snd_audient_enum_item(ctype->enumeration, val);
+		item = snd_audient_enum_item(ctype->enumeration, val);
+		if (item >= ctype->enumeration->items)
+			return -EIO;
+
+		ctl_val->value.enumerated.item[i] = item;
 	}
 
 	for (; i < nchan; i++)
@@ -1007,7 +1004,7 @@ static int snd_ctl_audient_info(struct snd_kcontrol *kctl,
 
 	if (ctype->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
 		return snd_ctl_enum_info(uinfo, snd_audient_kctl_channels(kctl),
-					 ctype->enumeration->items + 1,
+					 ctype->enumeration->items,
 					 ctype->enumeration->texts);
 
 	uinfo->type = ctype->type;