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

Stefan Sauer ensonic at hora-obscura.de
Sat Jun 29 14:32:21 CEST 2019


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



More information about the Alsa-devel mailing list