[alsa-devel] focusrite scarlett 18i20 : Mixer controls with corrupted names for

[Takashi Iwai]

On Wed, 26 Jun 2019 22:06:32 +0200,
Stefan Sauer wrote:
> 
> Am 25.06.19 um 09:54 schrieb Takashi Iwai:
> > On Sat, 22 Jun 2019 22:55:25 +0200,
> > Stefan Sauer wrote:
> >>
> >> So the first 8 controls are added somewhere else. Looks like this is from
> >> mixer.c and after
> >> echo -n 'file sound/usb/mixer.c +p' >/sys/kernel/debug/dynamic_debug/control
> >> I get
> >> [ 4405.855432] usb 1-2: [51] PU [KKKKKKKKKKKKKÃÃÃÃÃÃÃÃÃÃÃ Switch] ch = 1, val = 0/1
> >> [ 4405.856423] usb 1-2: [52] PU [ Switch] ch = 1, val = 0/1
> > 
> > This indicates that these weird names come from the two extension
> > units id 51 and 52.
> > Could you put some debug print in build_audio_procunit() like below?
> 
> I traced it down to this part from sound/usb/mixer.c:
> nameid = uac_processing_unit_iProcessing(desc, state->mixer->protocol);
> usb_audio_dbg(state->chip, "desc->bUnitID=%d, proto=%d, nameid=%d\n",
>  	      desc->bUnitID, state->mixer->protocol, nameid);
> len = 0;
> if (nameid) {
> 	len = snd_usb_copy_string_desc(state->chip,
> 				       nameid,
> 				       kctl->id.name,
> 				       sizeof(kctl->id.name));
> 	usb_audio_dbg(state->chip, "nameid=%d, len=%d, fallback name='%s'\n",
>  		      nameid, len, name);
> }
> 
> [ 6241.045734] usb 1-2: desc->bUnitID=51, proto=32, nameid=90
> [ 6241.045861] usb 1-2: nameid=90, len=35, fallback name='Extension Unit'
> [ 6241.045868] usb 1-2: [51] PU [KKKKKKKKKKKKKÃÃÃÃÃÃÃÃÃÃÃ Switch] ch = 1, val = 0/1
> [ 6241.046745] usb 1-2: desc->bUnitID=52, proto=32, nameid=82
> [ 6241.046857] usb 1-2: nameid=82, len=1, fallback name='Extension Unit'
> [ 6241.046862] usb 1-2: [52] PU [ Switch] ch = 1, val = 0/1
> 
> The device is using UAC_VERSION_2. The code in include/uapi/linux/usb/audio.h is
> a bit hard to read since uac_mixer_unit_descriptor has a variable length and the
> code is adding several offset, I'll need to add more printfs there to check if
> it is correct. I am consulting
> https://www.usb.org/sites/default/files/audio10.pdf but I am not sure if this
> covers UAC2.

UAC2 is completely different from UAC1, so you can forget that PDF.

I think I found the culprit.  The bug was that UAC2 extension unit
descriptor has a very slight difference from UAC2 processing unit
descriptor: namely, the size of bmControls field is 1 while processing
unit has 2.  And iExtension follows that, so we're reading a wrong
offset.

That's the reason a bogus nameid like 90 is read.

Could you try the patch below?


thanks,

Takashi

--- a/include/uapi/linux/usb/audio.h
+++ b/include/uapi/linux/usb/audio.h
@@ -450,6 +450,37 @@ static inline __u8 *uac_processing_unit_specific(struct uac_processing_unit_desc
 	}
 }
 
+static inline __u8 uac_extension_unit_bControlSize(struct uac_processing_unit_descriptor *desc,
+						   int protocol)
+{
+	switch (protocol) {
+	case UAC_VERSION_1:
+		return desc->baSourceID[desc->bNrInPins + 4];
+	case UAC_VERSION_2:
+		return 1; /* in UAC2, this value is constant */
+	case UAC_VERSION_3:
+		return 4; /* in UAC3, this value is constant */
+	default:
+		return 1;
+	}
+}
+
+static inline __u8 uac_extension_unit_iExtension(struct uac_processing_unit_descriptor *desc,
+						 int protocol)
+{
+	__u8 control_size = uac_extension_unit_bControlSize(desc, protocol);
+
+	switch (protocol) {
+	case UAC_VERSION_1:
+	case UAC_VERSION_2:
+	default:
+		return *(uac_processing_unit_bmControls(desc, protocol)
+			 + control_size);
+	case UAC_VERSION_3:
+		return 0; /* UAC3 does not have this field */
+	}
+}
+
 /* 4.5.2 Class-Specific AS Interface Descriptor */
 struct uac1_as_header_descriptor {
 	__u8  bLength;			/* in bytes: 7 */
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index e003b5e7b01a..ac121b10c51c 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -2318,7 +2318,7 @@ static struct procunit_info extunits[] = {
  */
 static int build_audio_procunit(struct mixer_build *state, int unitid,
 				void *raw_desc, struct procunit_info *list,
-				char *name)
+				bool extension_unit)
 {
 	struct uac_processing_unit_descriptor *desc = raw_desc;
 	int num_ins;
@@ -2335,6 +2335,8 @@ static int build_audio_procunit(struct mixer_build *state, int unitid,
 	static struct procunit_info default_info = {
 		0, NULL, default_value_info
 	};
+	const char *name = extension_unit ?
+		"Extension Unit" : "Processing Unit";
 
 	if (desc->bLength < 13) {
 		usb_audio_err(state->chip, "invalid %s descriptor (id %d)\n", name, unitid);
@@ -2448,7 +2450,10 @@ static int build_audio_procunit(struct mixer_build *state, int unitid,
 		} else if (info->name) {
 			strlcpy(kctl->id.name, info->name, sizeof(kctl->id.name));
 		} else {
-			nameid = uac_processing_unit_iProcessing(desc, state->mixer->protocol);
+			if (extension_unit)
+				nameid = uac_extension_unit_iExtension(desc, state->mixer->protocol);
+			else
+				nameid = uac_processing_unit_iProcessing(desc, state->mixer->protocol);
 			len = 0;
 			if (nameid)
 				len = snd_usb_copy_string_desc(state->chip,
@@ -2481,10 +2486,10 @@ static int parse_audio_processing_unit(struct mixer_build *state, int unitid,
 	case UAC_VERSION_2:
 	default:
 		return build_audio_procunit(state, unitid, raw_desc,
-				procunits, "Processing Unit");
+					    procunits, false);
 	case UAC_VERSION_3:
 		return build_audio_procunit(state, unitid, raw_desc,
-				uac3_procunits, "Processing Unit");
+					    uac3_procunits, false);
 	}
 }
 
@@ -2495,8 +2500,7 @@ static int parse_audio_extension_unit(struct mixer_build *state, int unitid,
 	 * Note that we parse extension units with processing unit descriptors.
 	 * That's ok as the layout is the same.
 	 */
-	return build_audio_procunit(state, unitid, raw_desc,
-				    extunits, "Extension Unit");
+	return build_audio_procunit(state, unitid, raw_desc, extunits, true);
 }
 
 /*