Alsa-Devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* focusrite scarlett 18i20 : Mixer controls with corrupted names for
@ 2019-06-22 20:55 Stefan Sauer
  2019-06-22 21:17 ` Stefan Sauer
  2019-06-25  7:54 ` Takashi Iwai
  0 siblings, 2 replies; 9+ messages in thread
From: Stefan Sauer @ 2019-06-22 20:55 UTC (permalink / raw)
  To: alsa-devel

Hi,

when I run
amixer -D hw:4 controls | sort -n -t = -k2

I get the output below

numid=1,iface=MIXER,name='KKKKKKKKKKKKKÃÃÃÃÃÃÃÃÃÃÃ Switch'
numid=2,iface=CARD,name='Internal Validity'
numid=3,iface=CARD,name='S/PDIF Validity'
numid=4,iface=CARD,name='ADAT Validity'
numid=5,iface=MIXER,name='Scarlett 18i20 USB-Sync Clock Source'
numid=6,iface=MIXER,name=' Switch'
numid=7,iface=CARD,name='Keep Interface'
numid=8,iface=MIXER,name='Master Playback Switch'
numid=9,iface=MIXER,name='Master Playback Volume'
numid=10,iface=MIXER,name='Master 1 (Monitor) Playback Switch'
numid=11,iface=MIXER,name='Master 1 (Monitor) Playback Volume'
... lots of extra lines ...

Please note the lines for numid=1 and numid=6. The first one contains some
garbage and the 2nd one look like it should start with another word.

I now added some debug printing here:
https://github.com/torvalds/linux/blob/master/sound/usb/mixer_scarlett.c#L562

and get output in dmesg:
[ 2971.642137] usb 1-2: Product: Scarlett 18i20 USB
[ 2971.642141] usb 1-2: Manufacturer: Focusrite
[ 2971.709773] new ctrl: name='Master Playback Switch', index=10, offset=1,
num=0: numid=8
[ 2971.709781] new ctrl: name='Master Playback Volume', index=10, offset=2,
num=0: numid=9
[ 2971.709788] new ctrl: name='Master 1 (Monitor) Playback Switch', index=10,
offset=1, num=1: numid=10

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

I now added more debug prints into
https://github.com/torvalds/linux/blob/master/sound/usb/mixer.c#L2431
and its the code that calls snd_usb_copy_string_desc()
[ 5750.124123] usb 1-2: nameid=90, len=35
[ 5750.124157] usb 1-2: [51] PU [KKKKKKKKKKKKKÃÃÃÃÃÃÃÃÃÃÃ Switch] ch = 1, val = 0/1
[ 5750.125241] usb 1-2: nameid=82, len=1
[ 5750.125260] usb 1-2: [52] PU [ Switch] ch = 1, val = 0/1

In both cases the returned len seems wrong and the function does not seems to
copy any useful string here. snd_usb_copy_string_desc() is just a wrapper around
usb_string().

Is my hardware returning bogus data in usb descriptors? Can this be address
through some quirks table? Any other ideas?

Thanks!
Stefan
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: focusrite scarlett 18i20 : Mixer controls with corrupted names for
  2019-06-22 20:55 focusrite scarlett 18i20 : Mixer controls with corrupted names for Stefan Sauer
@ 2019-06-22 21:17 ` Stefan Sauer
  2019-06-23 11:28   ` Stefan Sauer
  2019-06-25  7:54 ` Takashi Iwai
  1 sibling, 1 reply; 9+ messages in thread
From: Stefan Sauer @ 2019-06-22 21:17 UTC (permalink / raw)
  To: alsa-devel

Quick followup. Here is the lsusb -v output for the device:
https://gist.github.com/ensonic/2d286a85bbf4aca25e6f36b3569849c3

I noticed two warnings:
Warning: Junk at end of descriptor (17 bytes):
        ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 00
Could those be the cause? I don't now how to map the 'nameid' index to anything
in the lsub output though.

Am 22.06.19 um 22:55 schrieb Stefan Sauer:
> Hi,
> 
> when I run
> amixer -D hw:4 controls | sort -n -t = -k2
> 
> I get the output below
> 
> numid=1,iface=MIXER,name='KKKKKKKKKKKKKÃÃÃÃÃÃÃÃÃÃÃ Switch'
> numid=2,iface=CARD,name='Internal Validity'
> numid=3,iface=CARD,name='S/PDIF Validity'
> numid=4,iface=CARD,name='ADAT Validity'
> numid=5,iface=MIXER,name='Scarlett 18i20 USB-Sync Clock Source'
> numid=6,iface=MIXER,name=' Switch'
> numid=7,iface=CARD,name='Keep Interface'
> numid=8,iface=MIXER,name='Master Playback Switch'
> numid=9,iface=MIXER,name='Master Playback Volume'
> numid=10,iface=MIXER,name='Master 1 (Monitor) Playback Switch'
> numid=11,iface=MIXER,name='Master 1 (Monitor) Playback Volume'
> ... lots of extra lines ...
> 
> Please note the lines for numid=1 and numid=6. The first one contains some
> garbage and the 2nd one look like it should start with another word.
> 
> I now added some debug printing here:
> https://github.com/torvalds/linux/blob/master/sound/usb/mixer_scarlett.c#L562
> 
> and get output in dmesg:
> [ 2971.642137] usb 1-2: Product: Scarlett 18i20 USB
> [ 2971.642141] usb 1-2: Manufacturer: Focusrite
> [ 2971.709773] new ctrl: name='Master Playback Switch', index=10, offset=1,
> num=0: numid=8
> [ 2971.709781] new ctrl: name='Master Playback Volume', index=10, offset=2,
> num=0: numid=9
> [ 2971.709788] new ctrl: name='Master 1 (Monitor) Playback Switch', index=10,
> offset=1, num=1: numid=10
> 
> 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
> 
> I now added more debug prints into
> https://github.com/torvalds/linux/blob/master/sound/usb/mixer.c#L2431
> and its the code that calls snd_usb_copy_string_desc()
> [ 5750.124123] usb 1-2: nameid=90, len=35
> [ 5750.124157] usb 1-2: [51] PU [KKKKKKKKKKKKKÃÃÃÃÃÃÃÃÃÃÃ Switch] ch = 1, val = 0/1
> [ 5750.125241] usb 1-2: nameid=82, len=1
> [ 5750.125260] usb 1-2: [52] PU [ Switch] ch = 1, val = 0/1
> 
> In both cases the returned len seems wrong and the function does not seems to
> copy any useful string here. snd_usb_copy_string_desc() is just a wrapper around
> usb_string().
> 
> Is my hardware returning bogus data in usb descriptors? Can this be address
> through some quirks table? Any other ideas?
> 
> Thanks!
> Stefan
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: focusrite scarlett 18i20 : Mixer controls with corrupted names for
  2019-06-22 21:17 ` Stefan Sauer
@ 2019-06-23 11:28   ` Stefan Sauer
  0 siblings, 0 replies; 9+ messages in thread
From: Stefan Sauer @ 2019-06-23 11:28 UTC (permalink / raw)
  To: alsa-devel

Another followup. I made a little python tool to dump all string descriptors via
pyusb - here is the code and the output for the device. No idea why lsusb hides
string descriptors.
https://gist.github.com/ensonic/00fa344a05c87093058a98a62893e3b4

I just dumped the first 100, since I haven't figured how to check for the number
of strings provided by the device. In any case there aren't any useful strings
beyond index=62, but the mixer.c code is accessing 82, 90. Will now figure how
it gets that number.

If there are ready made useful tools that I should use instead of writing them
myself in python, please let me know. Thanks

Stefan

Am 22.06.19 um 23:17 schrieb Stefan Sauer:
> Quick followup. Here is the lsusb -v output for the device:
> https://gist.github.com/ensonic/2d286a85bbf4aca25e6f36b3569849c3
> 
> I noticed two warnings:
> Warning: Junk at end of descriptor (17 bytes):
>         ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 00
> Could those be the cause? I don't now how to map the 'nameid' index to anything
> in the lsub output though.
> 
> Am 22.06.19 um 22:55 schrieb Stefan Sauer:
>> Hi,
>>
>> when I run
>> amixer -D hw:4 controls | sort -n -t = -k2
>>
>> I get the output below
>>
>> numid=1,iface=MIXER,name='KKKKKKKKKKKKKÃÃÃÃÃÃÃÃÃÃÃ Switch'
>> numid=2,iface=CARD,name='Internal Validity'
>> numid=3,iface=CARD,name='S/PDIF Validity'
>> numid=4,iface=CARD,name='ADAT Validity'
>> numid=5,iface=MIXER,name='Scarlett 18i20 USB-Sync Clock Source'
>> numid=6,iface=MIXER,name=' Switch'
>> numid=7,iface=CARD,name='Keep Interface'
>> numid=8,iface=MIXER,name='Master Playback Switch'
>> numid=9,iface=MIXER,name='Master Playback Volume'
>> numid=10,iface=MIXER,name='Master 1 (Monitor) Playback Switch'
>> numid=11,iface=MIXER,name='Master 1 (Monitor) Playback Volume'
>> ... lots of extra lines ...
>>
>> Please note the lines for numid=1 and numid=6. The first one contains some
>> garbage and the 2nd one look like it should start with another word.
>>
>> I now added some debug printing here:
>> https://github.com/torvalds/linux/blob/master/sound/usb/mixer_scarlett.c#L562
>>
>> and get output in dmesg:
>> [ 2971.642137] usb 1-2: Product: Scarlett 18i20 USB
>> [ 2971.642141] usb 1-2: Manufacturer: Focusrite
>> [ 2971.709773] new ctrl: name='Master Playback Switch', index=10, offset=1,
>> num=0: numid=8
>> [ 2971.709781] new ctrl: name='Master Playback Volume', index=10, offset=2,
>> num=0: numid=9
>> [ 2971.709788] new ctrl: name='Master 1 (Monitor) Playback Switch', index=10,
>> offset=1, num=1: numid=10
>>
>> 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
>>
>> I now added more debug prints into
>> https://github.com/torvalds/linux/blob/master/sound/usb/mixer.c#L2431
>> and its the code that calls snd_usb_copy_string_desc()
>> [ 5750.124123] usb 1-2: nameid=90, len=35
>> [ 5750.124157] usb 1-2: [51] PU [KKKKKKKKKKKKKÃÃÃÃÃÃÃÃÃÃÃ Switch] ch = 1, val = 0/1
>> [ 5750.125241] usb 1-2: nameid=82, len=1
>> [ 5750.125260] usb 1-2: [52] PU [ Switch] ch = 1, val = 0/1
>>
>> In both cases the returned len seems wrong and the function does not seems to
>> copy any useful string here. snd_usb_copy_string_desc() is just a wrapper around
>> usb_string().
>>
>> Is my hardware returning bogus data in usb descriptors? Can this be address
>> through some quirks table? Any other ideas?
>>
>> Thanks!
>> Stefan
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: focusrite scarlett 18i20 : Mixer controls with corrupted names for
  2019-06-22 20:55 focusrite scarlett 18i20 : Mixer controls with corrupted names for Stefan Sauer
  2019-06-22 21:17 ` Stefan Sauer
@ 2019-06-25  7:54 ` Takashi Iwai
  2019-06-26 20:06   ` Stefan Sauer
  1 sibling, 1 reply; 9+ messages in thread
From: Takashi Iwai @ 2019-06-25  7:54 UTC (permalink / raw)
  To: Stefan Sauer; +Cc: alsa-devel

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,

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: focusrite scarlett 18i20 : Mixer controls with corrupted names for
  2019-06-25  7:54 ` Takashi Iwai
@ 2019-06-26 20:06   ` Stefan Sauer
  2019-06-26 20:13     ` Stefan Sauer
  2019-06-26 21:42     ` Takashi Iwai
  0 siblings, 2 replies; 9+ messages in thread
From: Stefan Sauer @ 2019-06-26 20:06 UTC (permalink / raw)
  To: alsa-devel

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
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: focusrite scarlett 18i20 : Mixer controls with corrupted names for
  2019-06-26 20:06   ` Stefan Sauer
@ 2019-06-26 20:13     ` Stefan Sauer
  2019-06-26 21:42     ` Takashi Iwai
  1 sibling, 0 replies; 9+ messages in thread
From: Stefan Sauer @ 2019-06-26 20:13 UTC (permalink / raw)
  To: alsa-devel

Am 26.06.19 um 22:06 schrieb Stefan Sauer:
> 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

and with the bUnitID I get to these descriptors from lsusb:

AudioControl Interface Descriptor:
        bLength                16
        bDescriptorType        36
        bDescriptorSubtype      9 (EXTENSION_UNIT)
        bUnitID                51
        wExtensionCode     0x0000
        bNrInPins               1
        baSourceID(0)           2
        bNrChannels            20
        bmChannelConfig    0x00000000
        iChannelNames           0
        bmControls           0x03
          Enable Control (read/write)
        iExtension              0
AudioControl Interface Descriptor:
        bLength                16
        bDescriptorType        36
        bDescriptorSubtype      9 (EXTENSION_UNIT)
        bUnitID                52
        wExtensionCode     0x0000
        bNrInPins               1
        baSourceID(0)           1
        bNrChannels            20
        bmChannelConfig    0x00000000
        iChannelNames           0
        bmControls           0x03
          Enable Control (read/write)
        iExtension              0

> 
> 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
>>
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: focusrite scarlett 18i20 : Mixer controls with corrupted names for
  2019-06-26 20:06   ` Stefan Sauer
  2019-06-26 20:13     ` Stefan Sauer
@ 2019-06-26 21:42     ` Takashi Iwai
  2019-06-29 12:32       ` Stefan Sauer
  1 sibling, 1 reply; 9+ messages in thread
From: Takashi Iwai @ 2019-06-26 21:42 UTC (permalink / raw)
  To: Stefan Sauer; +Cc: 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);
 }
 
 /*
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: focusrite scarlett 18i20 : Mixer controls with corrupted names for
  2019-06-26 21:42     ` Takashi Iwai
@ 2019-06-29 12:32       ` Stefan Sauer
  2019-06-29 15:31         ` Takashi Iwai
  0 siblings, 1 reply; 9+ messages in thread
From: Stefan Sauer @ 2019-06-29 12:32 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

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.

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

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

> +	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);
>  }
>  
>  /*
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: focusrite scarlett 18i20 : Mixer controls with corrupted names for
  2019-06-29 12:32       ` Stefan Sauer
@ 2019-06-29 15:31         ` Takashi Iwai
  0 siblings, 0 replies; 9+ messages in thread
From: Takashi Iwai @ 2019-06-29 15:31 UTC (permalink / raw)
  To: Stefan Sauer; +Cc: alsa-devel

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-06-29 15:31 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-22 20:55 focusrite scarlett 18i20 : Mixer controls with corrupted names for Stefan Sauer
2019-06-22 21:17 ` Stefan Sauer
2019-06-23 11:28   ` Stefan Sauer
2019-06-25  7:54 ` Takashi Iwai
2019-06-26 20:06   ` Stefan Sauer
2019-06-26 20:13     ` Stefan Sauer
2019-06-26 21:42     ` Takashi Iwai
2019-06-29 12:32       ` Stefan Sauer
2019-06-29 15:31         ` Takashi Iwai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox