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.

Stefan

thanks,

Takashi

--- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -2358,8 +2358,10 @@ static int build_audio_procunit(struct mixer_build *state, int unitid, for (info = list; info && info->type; info++) if (info->type == type) break;

• if (!info || !info->type)

• if (!info || !info->type) {

• pr_info("XXX unit %d to default_info\n", unitid);
  

  info = &default_info;

• }

  for (valinfo = info->values; valinfo->control; valinfo++) { __u8 *controls = uac_processing_unit_bmControls(desc, state->mixer->protocol);

@@ -2375,8 +2377,11 @@ static int build_audio_procunit(struct mixer_build *state, int unitid, }

map = find_map(state->map, unitid, valinfo->control);

• if (map)
  

• 	pr_info("XXX map found for unit %d\n", unitid);
  

  if (check_ignored_ctl(map)) continue;
• cval = kzalloc(sizeof(*cval), GFP_KERNEL); if (!cval) return -ENOMEM;

@@ -2444,19 +2449,26 @@ static int build_audio_procunit(struct mixer_build *state, int unitid, kctl->private_free = snd_usb_mixer_elem_free;

if (check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name))) {

• 	pr_info("XXX name copied from map: '%s' for unit %d\n", kctl->id.name, unitid);
  /* nothing */ ;
  

  } else if (info->name) { strlcpy(kctl->id.name, info->name, sizeof(kctl->id.name));

• 	pr_info("XXX name copied from info: '%s' for unit %d\n", kctl->id.name, unitid);
  

  } else { nameid = uac_processing_unit_iProcessing(desc, state->mixer->protocol);

• 	pr_info("XXX nameid=%d, unit %d\n", nameid, unitid);
  len = 0;
  

• 	if (nameid)
  

• 	if (nameid) {
  	len = snd_usb_copy_string_desc(state->chip,
  				       nameid,
  				       kctl->id.name,
  				       sizeof(kctl->id.name));
  

• 	if (!len)
  

• 		pr_info("XXX copy string desc '%s'\n", kctl->id.name);
  

• 	}
  

• 	if (!len) {
  	strlcpy(kctl->id.name, name, sizeof(kctl->id.name));
  

• 		pr_info("XXX copy default name '%s'\n", kctl->id.name);
  

• 	}
  

  } append_ctl_name(kctl, " "); append_ctl_name(kctl, valinfo->suffix);

---

Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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); }

/*

On Sat, 29 Jun 2019 14:32:21 +0200, Stefan Sauer wrote:

Am 26.06.19 um 23:42 schrieb 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?

Thanks for the patch! With that I get nameid=0 for both PUs:

[ 976.445182] usb 1-2: desc->bUnitID=51, proto=32, nameid=0 [ 976.445186] usb 1-2: fallback name='Extension Unit' [ 976.445189] usb 1-2: [51] PU [Extension Unit Switch] ch = 1, val = 0/1 [ 976.446052] usb 1-2: [40] SU [Scarlett 18i20 USB-Sync Clock Source] items = 3 [ 976.446059] usb 1-2: desc->bUnitID=52, proto=32, nameid=0 [ 976.446061] usb 1-2: fallback name='Extension Unit' [ 976.446064] usb 1-2: [52] PU [Extension Unit Switch] ch = 1, val = 0/1

on eocmment for the patch below.

Good, that's the expected behavior. The string index 0 indicates no specific string, so it falls back.

...

...

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);
  

if this is for 'extension_units' can we still use the offset helper for 'processing_units'?

Yes, until this point, both extension units and processing units are identical. The difference comes after that.

Is the UAC2 spec available freely, if so with a link, I can probably figure this out.

You should be able to find on the net, I suppose.

In anyway I'm going to prepare the proper patch for merging to upstream.

thanks,

Takashi