mbox series

[00/31] Refactor Scarlett Gen 2 support and add Scarlett Gen 3 support

Message ID cover.1624294591.git.g@b4.vu
Headers show
Series Refactor Scarlett Gen 2 support and add Scarlett Gen 3 support | expand

Message

Geoffrey D. Bennett June 21, 2021, 6:09 p.m. UTC
This patch set broadly:
- Refactors the Scarlett Gen 2 support to make the Gen 3 mixer support
  trivial to add
- Fixes a couple of minor issues with the Gen 2 support (low priority
  for stable; the issues were not reported by any user)
- Adds support for Gen 3 devices with and without a mixer
- Adds support for the major features new with the Gen 3 devices

Geoffrey D. Bennett (31):
  ALSA: usb-audio: scarlett2: Add usb_tx/rx functions
  ALSA: usb-audio: scarlett2: Update initialisation sequence
  ALSA: usb-audio: scarlett2: Fix 6i6 Gen 2 line out descriptions
  ALSA: usb-audio: scarlett2: Always enable interrupt polling
  ALSA: usb-audio: scarlett2: Add "Sync Status" control
  ALSA: usb-audio: scarlett2: Merge common line in capture strings
  ALSA: usb-audio: scarlett2: Reformat scarlett2_config_items[]
  ALSA: usb-audio: scarlett2: Improve device info lookup
  ALSA: usb-audio: scarlett2: Move info lookup out of init function
  ALSA: usb-audio: scarlett2: Remove repeated device info comments
  ALSA: usb-audio: scarlett2: Add scarlett2_vol_ctl_write() helper
  ALSA: usb-audio: scarlett2: Add mute support
  ALSA: usb-audio: scarlett2: Allow arbitrary ordering of mux entries
  ALSA: usb-audio: scarlett2: Split struct scarlett2_ports
  ALSA: usb-audio: scarlett2: Fix Level Meter control
  ALSA: usb-audio: scarlett2: Add Gen 3 mixer support
  ALSA: usb-audio: scarlett2: Add support for "input-other" notify
  ALSA: usb-audio: scarlett2: Add Gen 3 MSD mode switch
  ALSA: usb-audio: scarlett2: Move get config above set config
  ALSA: usb-audio: scarlett2: Allow bit-level access to config
  ALSA: usb-audio: scarlett2: Add support for Solo and 2i2 Gen 3
  ALSA: usb-audio: scarlett2: Add "air" switch support
  ALSA: usb-audio: scarlett2: Add phantom power switch support
  ALSA: usb-audio: scarlett2: Add direct monitor support
  ALSA: usb-audio: scarlett2: Label 18i8 Gen 3 line outputs correctly
  ALSA: usb-audio: scarlett2: Split up sw_hw_enum_ctl_put()
  ALSA: usb-audio: scarlett2: Add sw_hw_ctls and mux_ctls
  ALSA: usb-audio: scarlett2: Update mux controls to allow updates
  ALSA: usb-audio: scarlett2: Add speaker switching support
  ALSA: usb-audio: scarlett2: Update get_config to do endian conversion
  ALSA: usb-audio: scarlett2: Add support for the talkback feature

 sound/usb/mixer.c               |    2 +-
 sound/usb/mixer_quirks.c        |    6 +
 sound/usb/mixer_scarlett_gen2.c | 2725 +++++++++++++++++++++++++------
 3 files changed, 2239 insertions(+), 494 deletions(-)


base-commit: 6c0a2078134aba6a77291554035304df9e16b85c

Comments

Takashi Iwai June 22, 2021, 7 a.m. UTC | #1
On Mon, 21 Jun 2021 20:09:48 +0200,
Geoffrey D. Bennett wrote:
> 
> Add mixer support for the Focusrite Scarlett 4i4, 8i6, 18i8, and 18i20
> Gen 3 devices.
> 
> Signed-off-by: Geoffrey D. Bennett <g@b4.vu>
> ---
>  sound/usb/mixer.c               |   2 +-
>  sound/usb/mixer_quirks.c        |   4 +
>  sound/usb/mixer_scarlett_gen2.c | 260 +++++++++++++++++++++++++++++---
>  3 files changed, 246 insertions(+), 20 deletions(-)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 428d581f988f..ba4aa1eacb04 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -50,7 +50,7 @@
>  #include "mixer_quirks.h"
>  #include "power.h"
>  
> -#define MAX_ID_ELEMS	256
> +#define MAX_ID_ELEMS	512

This change requires the explanation.
Usually the unit id is a byte per definition, so it can't be over
256.


thanks,

Takashi
Vladimir Sadovnikov June 22, 2021, 7:07 a.m. UTC | #2
Hello Takashi!

Since Focusrite devices are too advanced in settings, the overall amount of 256 
controls is not enough for these devices (like 18i20).
I would like also to extend this constant up to 1024 or even more since adding 
support of software configuration of the device also
can exceed the amount of 512 control elements.

Let's assume we have a mute switch for each mixer gain setting. For the 18i20 
device this will give:
12 inputs * 25 outputs = 300 mute switches.

So I think this constant should be increased rapidly up to 1024 or even to 2048.

Best,
Vladimir

22.06.2021 10:00, Takashi Iwai пишет:
> On Mon, 21 Jun 2021 20:09:48 +0200,
> Geoffrey D. Bennett wrote:
>> Add mixer support for the Focusrite Scarlett 4i4, 8i6, 18i8, and 18i20
>> Gen 3 devices.
>>
>> Signed-off-by: Geoffrey D. Bennett <g@b4.vu>
>> ---
>>   sound/usb/mixer.c               |   2 +-
>>   sound/usb/mixer_quirks.c        |   4 +
>>   sound/usb/mixer_scarlett_gen2.c | 260 +++++++++++++++++++++++++++++---
>>   3 files changed, 246 insertions(+), 20 deletions(-)
>>
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 428d581f988f..ba4aa1eacb04 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -50,7 +50,7 @@
>>   #include "mixer_quirks.h"
>>   #include "power.h"
>>   
>> -#define MAX_ID_ELEMS	256
>> +#define MAX_ID_ELEMS	512
> This change requires the explanation.
> Usually the unit id is a byte per definition, so it can't be over
> 256.
>
>
> thanks,
>
> Takashi
Geoffrey D. Bennett June 22, 2021, 7:24 a.m. UTC | #3
On Tue, Jun 22, 2021 at 09:00:19AM +0200, Takashi Iwai wrote:
> On Mon, 21 Jun 2021 20:09:48 +0200,
> Geoffrey D. Bennett wrote:
> > 
> > Add mixer support for the Focusrite Scarlett 4i4, 8i6, 18i8, and 18i20
> > Gen 3 devices.
> > 
> > Signed-off-by: Geoffrey D. Bennett <g@b4.vu>
> > ---
> >  sound/usb/mixer.c               |   2 +-
> >  sound/usb/mixer_quirks.c        |   4 +
> >  sound/usb/mixer_scarlett_gen2.c | 260 +++++++++++++++++++++++++++++---
> >  3 files changed, 246 insertions(+), 20 deletions(-)
> > 
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index 428d581f988f..ba4aa1eacb04 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -50,7 +50,7 @@
> >  #include "mixer_quirks.h"
> >  #include "power.h"
> >  
> > -#define MAX_ID_ELEMS	256
> > +#define MAX_ID_ELEMS	512
> 
> This change requires the explanation.
> Usually the unit id is a byte per definition, so it can't be over
> 256.

Before making this change we were getting a buffer overflow in
mixer->id_elems[] (snd_usb_mixer_add_list()) because more than 256
controls were being added for the 18i20 Gen 3 device. I will send a
replacement patch with an updated comment.
Takashi Iwai June 22, 2021, 7:34 a.m. UTC | #4
On Tue, 22 Jun 2021 09:07:20 +0200,
Vladimir Sadovnikov wrote:
> 
> Hello Takashi!
> 
> Since Focusrite devices are too advanced in settings, the overall
> amount of 256 controls is not enough for these devices (like 18i20).
> I would like also to extend this constant up to 1024 or even more
> since adding support of software configuration of the device also
> can exceed the amount of 512 control elements.

This define isn't for the total number of mixer elements.  Instead,
it's just a size of the bitmap table that contains the head of the
linked list for each unit id (in the sense of USB mixer spec).
So the number of mixer elements is unlimited.


Takashi

> 
> Let's assume we have a mute switch for each mixer gain setting. For
> the 18i20 device this will give:
> 12 inputs * 25 outputs = 300 mute switches.
> 
> So I think this constant should be increased rapidly up to 1024 or even to 2048.
> 
> Best,
> Vladimir
> 
> 22.06.2021 10:00, Takashi Iwai пишет:
> > On Mon, 21 Jun 2021 20:09:48 +0200,
> > Geoffrey D. Bennett wrote:
> >> Add mixer support for the Focusrite Scarlett 4i4, 8i6, 18i8, and 18i20
> >> Gen 3 devices.
> >>
> >> Signed-off-by: Geoffrey D. Bennett <g@b4.vu>
> >> ---
> >>   sound/usb/mixer.c               |   2 +-
> >>   sound/usb/mixer_quirks.c        |   4 +
> >>   sound/usb/mixer_scarlett_gen2.c | 260 +++++++++++++++++++++++++++++---
> >>   3 files changed, 246 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> >> index 428d581f988f..ba4aa1eacb04 100644
> >> --- a/sound/usb/mixer.c
> >> +++ b/sound/usb/mixer.c
> >> @@ -50,7 +50,7 @@
> >>   #include "mixer_quirks.h"
> >>   #include "power.h"
> >>   -#define MAX_ID_ELEMS	256
> >> +#define MAX_ID_ELEMS	512
> > This change requires the explanation.
> > Usually the unit id is a byte per definition, so it can't be over
> > 256.
> >
> >
> > thanks,
> >
> > Takashi
> 
>
Geoffrey D. Bennett June 22, 2021, 7:44 a.m. UTC | #5
On Tue, Jun 22, 2021 at 09:34:25AM +0200, Takashi Iwai wrote:
> On Tue, 22 Jun 2021 09:07:20 +0200,
> Vladimir Sadovnikov wrote:
> > 
> > Hello Takashi!
> > 
> > Since Focusrite devices are too advanced in settings, the overall
> > amount of 256 controls is not enough for these devices (like 18i20).
> > I would like also to extend this constant up to 1024 or even more
> > since adding support of software configuration of the device also
> > can exceed the amount of 512 control elements.
> 
> This define isn't for the total number of mixer elements.  Instead,
> it's just a size of the bitmap table that contains the head of the
> linked list for each unit id (in the sense of USB mixer spec).
> So the number of mixer elements is unlimited.

Sorry I don't understand what's going on then. Am I calling
snd_usb_mixer_add_control() wrong? Because when I called it more than
MAX_ID_ELEMS times I got a buffer overflow in mixer->id_elems[] (from
memory, I can confirm tonight).

snd_usb_create_mixer() has:

        mixer->id_elems = kcalloc(MAX_ID_ELEMS, sizeof(*mixer->id_elems),
                                  GFP_KERNEL);

snd_usb_mixer_add_control() called from mixer_scarlett_gen2.c ends up
at snd_usb_mixer_add_list() which does:

        list->next_id_elem = mixer->id_elems[list->id];
        mixer->id_elems[list->id] = list;

And list->id was going over MAX_ID_ELEMS.
Takashi Iwai June 22, 2021, 7:58 a.m. UTC | #6
On Tue, 22 Jun 2021 09:44:54 +0200,
Geoffrey D. Bennett wrote:
> 
> On Tue, Jun 22, 2021 at 09:34:25AM +0200, Takashi Iwai wrote:
> > On Tue, 22 Jun 2021 09:07:20 +0200,
> > Vladimir Sadovnikov wrote:
> > > 
> > > Hello Takashi!
> > > 
> > > Since Focusrite devices are too advanced in settings, the overall
> > > amount of 256 controls is not enough for these devices (like 18i20).
> > > I would like also to extend this constant up to 1024 or even more
> > > since adding support of software configuration of the device also
> > > can exceed the amount of 512 control elements.
> > 
> > This define isn't for the total number of mixer elements.  Instead,
> > it's just a size of the bitmap table that contains the head of the
> > linked list for each unit id (in the sense of USB mixer spec).
> > So the number of mixer elements is unlimited.
> 
> Sorry I don't understand what's going on then. Am I calling
> snd_usb_mixer_add_control() wrong? Because when I called it more than
> MAX_ID_ELEMS times I got a buffer overflow in mixer->id_elems[] (from
> memory, I can confirm tonight).
> 
> snd_usb_create_mixer() has:
> 
>         mixer->id_elems = kcalloc(MAX_ID_ELEMS, sizeof(*mixer->id_elems),
>                                   GFP_KERNEL);
> 
> snd_usb_mixer_add_control() called from mixer_scarlett_gen2.c ends up
> at snd_usb_mixer_add_list() which does:
> 
>         list->next_id_elem = mixer->id_elems[list->id];
>         mixer->id_elems[list->id] = list;
> 
> And list->id was going over MAX_ID_ELEMS.

Here, list->id is the *USB* mixer unit id, not the ALSA control id or
whatever the internal index.  The former should be a byte, hence it
can't be over 256.

That said, the scarlett2 code calls the function in a wrong way.  It
has worked casually, so far, just because the core code doesn't use
the unit id number for significant roles.

So, as a quick workaround, simply pass 0 or any fixed number under 256
to list->id (i.e. elem->head.id in scarlett2_add_new_ctl()).  That's
all, and the elements are chained in the linked list.


Takashi
Takashi Iwai June 22, 2021, 8:12 a.m. UTC | #7
On Tue, 22 Jun 2021 09:58:27 +0200,
Takashi Iwai wrote:
> 
> On Tue, 22 Jun 2021 09:44:54 +0200,
> Geoffrey D. Bennett wrote:
> > 
> > On Tue, Jun 22, 2021 at 09:34:25AM +0200, Takashi Iwai wrote:
> > > On Tue, 22 Jun 2021 09:07:20 +0200,
> > > Vladimir Sadovnikov wrote:
> > > > 
> > > > Hello Takashi!
> > > > 
> > > > Since Focusrite devices are too advanced in settings, the overall
> > > > amount of 256 controls is not enough for these devices (like 18i20).
> > > > I would like also to extend this constant up to 1024 or even more
> > > > since adding support of software configuration of the device also
> > > > can exceed the amount of 512 control elements.
> > > 
> > > This define isn't for the total number of mixer elements.  Instead,
> > > it's just a size of the bitmap table that contains the head of the
> > > linked list for each unit id (in the sense of USB mixer spec).
> > > So the number of mixer elements is unlimited.
> > 
> > Sorry I don't understand what's going on then. Am I calling
> > snd_usb_mixer_add_control() wrong? Because when I called it more than
> > MAX_ID_ELEMS times I got a buffer overflow in mixer->id_elems[] (from
> > memory, I can confirm tonight).
> > 
> > snd_usb_create_mixer() has:
> > 
> >         mixer->id_elems = kcalloc(MAX_ID_ELEMS, sizeof(*mixer->id_elems),
> >                                   GFP_KERNEL);
> > 
> > snd_usb_mixer_add_control() called from mixer_scarlett_gen2.c ends up
> > at snd_usb_mixer_add_list() which does:
> > 
> >         list->next_id_elem = mixer->id_elems[list->id];
> >         mixer->id_elems[list->id] = list;
> > 
> > And list->id was going over MAX_ID_ELEMS.
> 
> Here, list->id is the *USB* mixer unit id, not the ALSA control id or
> whatever the internal index.  The former should be a byte, hence it
> can't be over 256.
> 
> That said, the scarlett2 code calls the function in a wrong way.  It
> has worked casually, so far, just because the core code doesn't use
> the unit id number for significant roles.

... and looking again at the code, actually it does matter.
The USB audio mixer code calls the resume for each mixer element, and
the default resume code (default_mixer_resume()) is applied for a
control with usb_mixer_elem_info.val_type == USB_MIXER_BOOLEAN and
channels == 1.  It seems that scarlett2 has some of them, and the
resume procedure is applied wrongly with an invalid unit id.

We need to define a special mixer value type (e.g. USB_MIXER_VENDOR),
and use this for the all scarlett2 mixer elements, in addition to the
below:

> So, as a quick workaround, simply pass 0 or any fixed number under 256
> to list->id (i.e. elem->head.id in scarlett2_add_new_ctl()).  That's
> all, and the elements are chained in the linked list.


Takashi
Geoffrey D. Bennett June 22, 2021, 8:18 a.m. UTC | #8
On Tue, Jun 22, 2021 at 09:58:27AM +0200, Takashi Iwai wrote:
> On Tue, 22 Jun 2021 09:44:54 +0200,
> Geoffrey D. Bennett wrote:
> > 
> > On Tue, Jun 22, 2021 at 09:34:25AM +0200, Takashi Iwai wrote:
> > > On Tue, 22 Jun 2021 09:07:20 +0200,
> > > Vladimir Sadovnikov wrote:
> > > > 
> > > > Hello Takashi!
> > > > 
> > > > Since Focusrite devices are too advanced in settings, the overall
> > > > amount of 256 controls is not enough for these devices (like 18i20).
> > > > I would like also to extend this constant up to 1024 or even more
> > > > since adding support of software configuration of the device also
> > > > can exceed the amount of 512 control elements.
> > > 
> > > This define isn't for the total number of mixer elements.  Instead,
> > > it's just a size of the bitmap table that contains the head of the
> > > linked list for each unit id (in the sense of USB mixer spec).
> > > So the number of mixer elements is unlimited.
> > 
> > Sorry I don't understand what's going on then. Am I calling
> > snd_usb_mixer_add_control() wrong? Because when I called it more than
> > MAX_ID_ELEMS times I got a buffer overflow in mixer->id_elems[] (from
> > memory, I can confirm tonight).
> > 
> > snd_usb_create_mixer() has:
> > 
> >         mixer->id_elems = kcalloc(MAX_ID_ELEMS, sizeof(*mixer->id_elems),
> >                                   GFP_KERNEL);
> > 
> > snd_usb_mixer_add_control() called from mixer_scarlett_gen2.c ends up
> > at snd_usb_mixer_add_list() which does:
> > 
> >         list->next_id_elem = mixer->id_elems[list->id];
> >         mixer->id_elems[list->id] = list;
> > 
> > And list->id was going over MAX_ID_ELEMS.
> 
> Here, list->id is the *USB* mixer unit id, not the ALSA control id or
> whatever the internal index.  The former should be a byte, hence it
> can't be over 256.
> 
> That said, the scarlett2 code calls the function in a wrong way.  It
> has worked casually, so far, just because the core code doesn't use
> the unit id number for significant roles.
> 
> So, as a quick workaround, simply pass 0 or any fixed number under 256
> to list->id (i.e. elem->head.id in scarlett2_add_new_ctl()).  That's
> all, and the elements are chained in the linked list.

Okay, I will fix that tonight.

Were patches 1-15 of this set of 31 acceptable? If so, I will send a
new set with this fix and the remainder of the patches.
Takashi Iwai June 22, 2021, 8:34 a.m. UTC | #9
On Tue, 22 Jun 2021 10:18:39 +0200,
Geoffrey D. Bennett wrote:
> 
> On Tue, Jun 22, 2021 at 09:58:27AM +0200, Takashi Iwai wrote:
> > On Tue, 22 Jun 2021 09:44:54 +0200,
> > Geoffrey D. Bennett wrote:
> > > 
> > > On Tue, Jun 22, 2021 at 09:34:25AM +0200, Takashi Iwai wrote:
> > > > On Tue, 22 Jun 2021 09:07:20 +0200,
> > > > Vladimir Sadovnikov wrote:
> > > > > 
> > > > > Hello Takashi!
> > > > > 
> > > > > Since Focusrite devices are too advanced in settings, the overall
> > > > > amount of 256 controls is not enough for these devices (like 18i20).
> > > > > I would like also to extend this constant up to 1024 or even more
> > > > > since adding support of software configuration of the device also
> > > > > can exceed the amount of 512 control elements.
> > > > 
> > > > This define isn't for the total number of mixer elements.  Instead,
> > > > it's just a size of the bitmap table that contains the head of the
> > > > linked list for each unit id (in the sense of USB mixer spec).
> > > > So the number of mixer elements is unlimited.
> > > 
> > > Sorry I don't understand what's going on then. Am I calling
> > > snd_usb_mixer_add_control() wrong? Because when I called it more than
> > > MAX_ID_ELEMS times I got a buffer overflow in mixer->id_elems[] (from
> > > memory, I can confirm tonight).
> > > 
> > > snd_usb_create_mixer() has:
> > > 
> > >         mixer->id_elems = kcalloc(MAX_ID_ELEMS, sizeof(*mixer->id_elems),
> > >                                   GFP_KERNEL);
> > > 
> > > snd_usb_mixer_add_control() called from mixer_scarlett_gen2.c ends up
> > > at snd_usb_mixer_add_list() which does:
> > > 
> > >         list->next_id_elem = mixer->id_elems[list->id];
> > >         mixer->id_elems[list->id] = list;
> > > 
> > > And list->id was going over MAX_ID_ELEMS.
> > 
> > Here, list->id is the *USB* mixer unit id, not the ALSA control id or
> > whatever the internal index.  The former should be a byte, hence it
> > can't be over 256.
> > 
> > That said, the scarlett2 code calls the function in a wrong way.  It
> > has worked casually, so far, just because the core code doesn't use
> > the unit id number for significant roles.
> > 
> > So, as a quick workaround, simply pass 0 or any fixed number under 256
> > to list->id (i.e. elem->head.id in scarlett2_add_new_ctl()).  That's
> > all, and the elements are chained in the linked list.
> 
> Okay, I will fix that tonight.
> 
> Were patches 1-15 of this set of 31 acceptable? If so, I will send a
> new set with this fix and the remainder of the patches.

I don't see other issues through a quick glance.

And, the additional fix is below.
If this works, please include this at first.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: usb-audio: scarlett2: Fix wrong resume call

The current way of the scarlett2 mixer code managing the
usb_mixer_elem_info object is wrong in two ways: it passes its
internal index to the head.id field, and the val_type field is
uninitialized.  This ended up with the wrong execution at the resume
because a bogus unit id is passed wrongly.  Also, in the later code
extensions, we'll have more mixer elements, and passing the index will
overflow the unit id size (of 256).

This patch corrects those issues.  It introduces a new value type,
USB_MIXER_BESPOKEN, which indicates a non-standard mixer element, and
use this type for all scarlett2 mixer elements, as well as
initializing the fixed unit id 0 for avoiding the overflow.

Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/mixer.h               | 1 +
 sound/usb/mixer_scarlett_gen2.c | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
index e5a01f17bf3c..ea41e7a1f7bf 100644
--- a/sound/usb/mixer.h
+++ b/sound/usb/mixer.h
@@ -55,6 +55,7 @@ enum {
 	USB_MIXER_U16,
 	USB_MIXER_S32,
 	USB_MIXER_U32,
+	USB_MIXER_BESPOKEN,	/* non-standard type */
 };
 
 typedef void (*usb_mixer_elem_dump_func_t)(struct snd_info_buffer *buffer,
diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c
index 2e1937b072ee..ed2f16a5fc87 100644
--- a/sound/usb/mixer_scarlett_gen2.c
+++ b/sound/usb/mixer_scarlett_gen2.c
@@ -1070,10 +1070,15 @@ static int scarlett2_add_new_ctl(struct usb_mixer_interface *mixer,
 	if (!elem)
 		return -ENOMEM;
 
+	/* We set USB_MIXER_BESPOKEN type, so that the core USB mixer code
+	 * ignores them for resume and other operations.
+	 * Also, the head.id field is set to 0, as we don't use this field.
+	 */
 	elem->head.mixer = mixer;
 	elem->control = index;
-	elem->head.id = index;
+	elem->head.id = 0;
 	elem->channels = channels;
+	elem->val_type = USB_MIXER_BESPOKEN;
 
 	kctl = snd_ctl_new1(ncontrol, elem);
 	if (!kctl) {
Takashi Iwai June 22, 2021, 9:01 a.m. UTC | #10
On Tue, 22 Jun 2021 10:34:54 +0200,
Takashi Iwai wrote:
> 
> On Tue, 22 Jun 2021 10:18:39 +0200,
> Geoffrey D. Bennett wrote:
> > 
> > On Tue, Jun 22, 2021 at 09:58:27AM +0200, Takashi Iwai wrote:
> > > On Tue, 22 Jun 2021 09:44:54 +0200,
> > > Geoffrey D. Bennett wrote:
> > > > 
> > > > On Tue, Jun 22, 2021 at 09:34:25AM +0200, Takashi Iwai wrote:
> > > > > On Tue, 22 Jun 2021 09:07:20 +0200,
> > > > > Vladimir Sadovnikov wrote:
> > > > > > 
> > > > > > Hello Takashi!
> > > > > > 
> > > > > > Since Focusrite devices are too advanced in settings, the overall
> > > > > > amount of 256 controls is not enough for these devices (like 18i20).
> > > > > > I would like also to extend this constant up to 1024 or even more
> > > > > > since adding support of software configuration of the device also
> > > > > > can exceed the amount of 512 control elements.
> > > > > 
> > > > > This define isn't for the total number of mixer elements.  Instead,
> > > > > it's just a size of the bitmap table that contains the head of the
> > > > > linked list for each unit id (in the sense of USB mixer spec).
> > > > > So the number of mixer elements is unlimited.
> > > > 
> > > > Sorry I don't understand what's going on then. Am I calling
> > > > snd_usb_mixer_add_control() wrong? Because when I called it more than
> > > > MAX_ID_ELEMS times I got a buffer overflow in mixer->id_elems[] (from
> > > > memory, I can confirm tonight).
> > > > 
> > > > snd_usb_create_mixer() has:
> > > > 
> > > >         mixer->id_elems = kcalloc(MAX_ID_ELEMS, sizeof(*mixer->id_elems),
> > > >                                   GFP_KERNEL);
> > > > 
> > > > snd_usb_mixer_add_control() called from mixer_scarlett_gen2.c ends up
> > > > at snd_usb_mixer_add_list() which does:
> > > > 
> > > >         list->next_id_elem = mixer->id_elems[list->id];
> > > >         mixer->id_elems[list->id] = list;
> > > > 
> > > > And list->id was going over MAX_ID_ELEMS.
> > > 
> > > Here, list->id is the *USB* mixer unit id, not the ALSA control id or
> > > whatever the internal index.  The former should be a byte, hence it
> > > can't be over 256.
> > > 
> > > That said, the scarlett2 code calls the function in a wrong way.  It
> > > has worked casually, so far, just because the core code doesn't use
> > > the unit id number for significant roles.
> > > 
> > > So, as a quick workaround, simply pass 0 or any fixed number under 256
> > > to list->id (i.e. elem->head.id in scarlett2_add_new_ctl()).  That's
> > > all, and the elements are chained in the linked list.
> > 
> > Okay, I will fix that tonight.
> > 
> > Were patches 1-15 of this set of 31 acceptable? If so, I will send a
> > new set with this fix and the remainder of the patches.
> 
> I don't see other issues through a quick glance.
> 
> And, the additional fix is below.
> If this works, please include this at first.

It was missing one piece.  The revised patch is below.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: usb-audio: scarlett2: Fix wrong resume call

The current way of the scarlett2 mixer code managing the
usb_mixer_elem_info object is wrong in two ways: it passes its
internal index to the head.id field, and the val_type field is
uninitialized.  This ended up with the wrong execution at the resume
because a bogus unit id is passed wrongly.  Also, in the later code
extensions, we'll have more mixer elements, and passing the index will
overflow the unit id size (of 256).

This patch corrects those issues.  It introduces a new value type,
USB_MIXER_BESPOKEN, which indicates a non-standard mixer element, and
use this type for all scarlett2 mixer elements, as well as
initializing the fixed unit id 0 for avoiding the overflow.

Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/mixer.c               | 3 +++
 sound/usb/mixer.h               | 1 +
 sound/usb/mixer_scarlett_gen2.c | 7 ++++++-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 428d581f988f..0f578dabd094 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -3605,6 +3605,9 @@ static int restore_mixer_value(struct usb_mixer_elem_list *list)
 	struct usb_mixer_elem_info *cval = mixer_elem_list_to_info(list);
 	int c, err, idx;
 
+	if (cval->val_type == USB_MIXER_BESPOKEN)
+		return 0;
+
 	if (cval->cmask) {
 		idx = 0;
 		for (c = 0; c < MAX_CHANNELS; c++) {
diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
index e5a01f17bf3c..ea41e7a1f7bf 100644
--- a/sound/usb/mixer.h
+++ b/sound/usb/mixer.h
@@ -55,6 +55,7 @@ enum {
 	USB_MIXER_U16,
 	USB_MIXER_S32,
 	USB_MIXER_U32,
+	USB_MIXER_BESPOKEN,	/* non-standard type */
 };
 
 typedef void (*usb_mixer_elem_dump_func_t)(struct snd_info_buffer *buffer,
diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c
index 2e1937b072ee..ed2f16a5fc87 100644
--- a/sound/usb/mixer_scarlett_gen2.c
+++ b/sound/usb/mixer_scarlett_gen2.c
@@ -1070,10 +1070,15 @@ static int scarlett2_add_new_ctl(struct usb_mixer_interface *mixer,
 	if (!elem)
 		return -ENOMEM;
 
+	/* We set USB_MIXER_BESPOKEN type, so that the core USB mixer code
+	 * ignores them for resume and other operations.
+	 * Also, the head.id field is set to 0, as we don't use this field.
+	 */
 	elem->head.mixer = mixer;
 	elem->control = index;
-	elem->head.id = index;
+	elem->head.id = 0;
 	elem->channels = channels;
+	elem->val_type = USB_MIXER_BESPOKEN;
 
 	kctl = snd_ctl_new1(ncontrol, elem);
 	if (!kctl) {