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

[Takashi Iwai]

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