diff mbox series

[16/31] ALSA: usb-audio: scarlett2: Add Gen 3 mixer support

Message ID fc4bb1e8cfb3019b1033afeed59badb904990115.1624294591.git.g@b4.vu (mailing list archive)
State New, archived
Headers show
Series Refactor Scarlett Gen 2 support and add Scarlett Gen 3 support | expand

Commit Message

Geoffrey D. Bennett June 21, 2021, 6:09 p.m. UTC
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(-)

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) {
diff mbox series

Patch

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
 
 struct usb_audio_term {
 	int id;
diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index 37ad77524c0b..df7492594e91 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -3060,6 +3060,10 @@  int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 	case USB_ID(0x1235, 0x8203): /* Focusrite Scarlett 6i6 2nd Gen */
 	case USB_ID(0x1235, 0x8204): /* Focusrite Scarlett 18i8 2nd Gen */
 	case USB_ID(0x1235, 0x8201): /* Focusrite Scarlett 18i20 2nd Gen */
+	case USB_ID(0x1235, 0x8212): /* Focusrite Scarlett 4i4 3rd Gen */
+	case USB_ID(0x1235, 0x8213): /* Focusrite Scarlett 8i6 3rd Gen */
+	case USB_ID(0x1235, 0x8214): /* Focusrite Scarlett 18i8 3rd Gen */
+	case USB_ID(0x1235, 0x8215): /* Focusrite Scarlett 18i20 3rd Gen */
 		err = snd_scarlett_gen2_init(mixer);
 		break;
 
diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c
index dde008ea21d7..c4bcccb5aecd 100644
--- a/sound/usb/mixer_scarlett_gen2.c
+++ b/sound/usb/mixer_scarlett_gen2.c
@@ -1,8 +1,12 @@ 
 // SPDX-License-Identifier: GPL-2.0
 /*
- *   Focusrite Scarlett 6i6/18i8/18i20 Gen 2 Driver for ALSA
+ *   Focusrite Scarlett Gen 2/3 Driver for ALSA
  *
- *   Copyright (c) 2018-2019 by Geoffrey D. Bennett <g at b4.vu>
+ *   Supported models:
+ *   - 6i6/18i8/18i20 Gen 2
+ *   - 4i4/8i6/18i8/18i20 Gen 3
+ *
+ *   Copyright (c) 2018-2021 by Geoffrey D. Bennett <g at b4.vu>
  *
  *   Based on the Scarlett (Gen 1) Driver for ALSA:
  *
@@ -19,10 +23,6 @@ 
  *   David Henningsson <david.henningsson at canonical.com>
  */
 
-/* Mixer Interface for the Focusrite Scarlett 6i6/18i8/18i20 Gen 2 audio
- * interface. Based on the Gen 1 driver and rewritten.
- */
-
 /* The protocol was reverse engineered by looking at the communication
  * between Focusrite Control 2.3.4 and the Focusrite(R) Scarlett 18i20
  * (firmware 1083) using usbmon in July-August 2018.
@@ -32,13 +32,21 @@ 
  * Scarlett 6i6 support added in June 2019 (thanks to Martin Wittmann
  * for providing usbmon output and testing).
  *
+ * Scarlett 4i4/8i6 Gen 3 support added in May 2020 (thanks to Laurent
+ * Debricon for donating a 4i4 and to Fredrik Unger for providing 8i6
+ * usbmon output and testing).
+ *
+ * Scarlett 18i8/18i20 Gen 3 support added in June 2020 (thanks to
+ * Darren Jaeckel, Alex Sedlack, and Clovis Lunel for providing usbmon
+ * output, protocol traces and testing).
+ *
  * Support for loading mixer volume and mux configuration from the
  * interface during driver initialisation added in May 2021 (thanks to
  * Vladimir Sadovnikov for figuring out how).
  *
  * This ALSA mixer gives access to:
  *  - input, output, mixer-matrix muxes
- *  - 18x10 mixer-matrix gain stages
+ *  - mixer-matrix gain stages
  *  - gain/volume/mute controls
  *  - level meters
  *  - line/inst level and pad controls
@@ -148,21 +156,21 @@  static const u16 scarlett2_mixer_values[SCARLETT2_MIXER_VALUE_COUNT] = {
 
 /* Maximum number of level and pad switches */
 #define SCARLETT2_LEVEL_SWITCH_MAX 2
-#define SCARLETT2_PAD_SWITCH_MAX 4
+#define SCARLETT2_PAD_SWITCH_MAX 8
 
 /* Maximum number of inputs to the mixer */
-#define SCARLETT2_INPUT_MIX_MAX 18
+#define SCARLETT2_INPUT_MIX_MAX 25
 
 /* Maximum number of outputs from the mixer */
-#define SCARLETT2_OUTPUT_MIX_MAX 10
+#define SCARLETT2_OUTPUT_MIX_MAX 12
 
 /* Maximum size of the data in the USB mux assignment message:
- * 18 inputs, 20 outputs, 18 matrix inputs, 8 spare
+ * 20 inputs, 20 outputs, 25 matrix inputs, 12 spare
  */
-#define SCARLETT2_MUX_MAX 64
+#define SCARLETT2_MUX_MAX 77
 
 /* Maximum number of meters (sum of output port counts) */
-#define SCARLETT2_MAX_METERS 56
+#define SCARLETT2_MAX_METERS 65
 
 /* Hardware port types:
  * - None (no input to mux)
@@ -256,7 +264,7 @@  static const struct scarlett2_port scarlett2_ports[SCARLETT2_PORT_TYPE_COUNT] =
 #define SCARLETT2_MUX_TABLES 3
 
 /* Maximum number of entries in a mux table */
-#define SCARLETT2_MAX_MUX_ENTRIES 7
+#define SCARLETT2_MAX_MUX_ENTRIES 10
 
 /* One entry within mux_assignment defines the port type and range of
  * ports to add to the set_mux message. The end of the list is marked
@@ -475,12 +483,226 @@  static const struct scarlett2_device_info s18i20_gen2_info = {
 	} },
 };
 
+static const struct scarlett2_device_info s4i4_gen3_info = {
+	.usb_id = USB_ID(0x1235, 0x8212),
+
+	.level_input_count = 2,
+	.pad_input_count = 2,
+
+	.line_out_descrs = {
+		"Monitor L",
+		"Monitor R",
+		"Headphones L",
+		"Headphones R",
+	},
+
+	.port_count = {
+		[SCARLETT2_PORT_TYPE_NONE]     = { 1, 0 },
+		[SCARLETT2_PORT_TYPE_ANALOGUE] = { 4, 4 },
+		[SCARLETT2_PORT_TYPE_MIX]      = { 6, 8 },
+		[SCARLETT2_PORT_TYPE_PCM]      = { 4, 6 },
+	},
+
+	.mux_assignment = { {
+		{ SCARLETT2_PORT_TYPE_PCM,      0,  6 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE, 0,  4 },
+		{ SCARLETT2_PORT_TYPE_MIX,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_NONE,     0, 16 },
+		{ 0,                            0,  0 },
+	}, {
+		{ SCARLETT2_PORT_TYPE_PCM,      0,  6 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE, 0,  4 },
+		{ SCARLETT2_PORT_TYPE_MIX,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_NONE,     0, 16 },
+		{ 0,                            0,  0 },
+	}, {
+		{ SCARLETT2_PORT_TYPE_PCM,      0,  6 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE, 0,  4 },
+		{ SCARLETT2_PORT_TYPE_MIX,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_NONE,     0, 16 },
+		{ 0,                            0,  0 },
+	} },
+};
+
+static const struct scarlett2_device_info s8i6_gen3_info = {
+	.usb_id = USB_ID(0x1235, 0x8213),
+
+	.level_input_count = 2,
+	.pad_input_count = 2,
+
+	.line_out_descrs = {
+		"Headphones 1 L",
+		"Headphones 1 R",
+		"Headphones 2 L",
+		"Headphones 2 R",
+	},
+
+	.port_count = {
+		[SCARLETT2_PORT_TYPE_NONE]     = { 1,  0 },
+		[SCARLETT2_PORT_TYPE_ANALOGUE] = { 6,  4 },
+		[SCARLETT2_PORT_TYPE_SPDIF]    = { 2,  2 },
+		[SCARLETT2_PORT_TYPE_MIX]      = { 8,  8 },
+		[SCARLETT2_PORT_TYPE_PCM]      = { 6, 10 },
+	},
+
+	.mux_assignment = { {
+		{ SCARLETT2_PORT_TYPE_PCM,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE, 0,  4 },
+		{ SCARLETT2_PORT_TYPE_SPDIF,    0,  2 },
+		{ SCARLETT2_PORT_TYPE_PCM,      8,  2 },
+		{ SCARLETT2_PORT_TYPE_MIX,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_NONE,     0, 18 },
+		{ 0,                            0,  0 },
+	}, {
+		{ SCARLETT2_PORT_TYPE_PCM,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE, 0,  4 },
+		{ SCARLETT2_PORT_TYPE_SPDIF,    0,  2 },
+		{ SCARLETT2_PORT_TYPE_PCM,      8,  2 },
+		{ SCARLETT2_PORT_TYPE_MIX,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_NONE,     0, 18 },
+		{ 0,                            0,  0 },
+	}, {
+		{ SCARLETT2_PORT_TYPE_PCM,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE, 0,  4 },
+		{ SCARLETT2_PORT_TYPE_SPDIF,    0,  2 },
+		{ SCARLETT2_PORT_TYPE_PCM,      8,  2 },
+		{ SCARLETT2_PORT_TYPE_MIX,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_NONE,     0, 18 },
+		{ 0,                            0,  0 },
+	} },
+};
+
+static const struct scarlett2_device_info s18i8_gen3_info = {
+	.usb_id = USB_ID(0x1235, 0x8214),
+
+	.line_out_hw_vol = 1,
+	.level_input_count = 2,
+	.pad_input_count = 2,
+
+	.line_out_descrs = {
+		"Monitor L",
+		"Monitor R",
+		"Headphones 1 L",
+		"Headphones 1 R",
+		"Headphones 2 L",
+		"Headphones 2 R",
+		"Alt Monitor L",
+		"Alt Monitor R",
+	},
+
+	.port_count = {
+		[SCARLETT2_PORT_TYPE_NONE]     = {  1,  0 },
+		[SCARLETT2_PORT_TYPE_ANALOGUE] = {  8,  8 },
+		[SCARLETT2_PORT_TYPE_SPDIF]    = {  2,  2 },
+		[SCARLETT2_PORT_TYPE_ADAT]     = {  8,  0 },
+		[SCARLETT2_PORT_TYPE_MIX]      = { 10, 20 },
+		[SCARLETT2_PORT_TYPE_PCM]      = {  8, 20 },
+	},
+
+	.mux_assignment = { {
+		{ SCARLETT2_PORT_TYPE_PCM,       0, 10 },
+		{ SCARLETT2_PORT_TYPE_PCM,      12,  8 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  0,  2 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  6,  2 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  2,  4 },
+		{ SCARLETT2_PORT_TYPE_SPDIF,     0,  2 },
+		{ SCARLETT2_PORT_TYPE_PCM,      10,  2 },
+		{ SCARLETT2_PORT_TYPE_MIX,       0, 20 },
+		{ SCARLETT2_PORT_TYPE_NONE,      0, 10 },
+		{ 0,                             0,  0 },
+	}, {
+		{ SCARLETT2_PORT_TYPE_PCM,       0, 10 },
+		{ SCARLETT2_PORT_TYPE_PCM,      12,  4 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  0,  2 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  6,  2 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  2,  4 },
+		{ SCARLETT2_PORT_TYPE_SPDIF,     0,  2 },
+		{ SCARLETT2_PORT_TYPE_PCM,      10,  2 },
+		{ SCARLETT2_PORT_TYPE_MIX,       0, 20 },
+		{ SCARLETT2_PORT_TYPE_NONE,      0, 10 },
+		{ 0,                             0,  0 },
+	}, {
+		{ SCARLETT2_PORT_TYPE_PCM,       0, 10 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  0,  2 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  6,  2 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  2,  4 },
+		{ SCARLETT2_PORT_TYPE_SPDIF,     0,  2 },
+		{ SCARLETT2_PORT_TYPE_MIX,       0, 20 },
+		{ SCARLETT2_PORT_TYPE_NONE,      0, 10 },
+		{ 0,                             0,  0 },
+	} },
+};
+
+static const struct scarlett2_device_info s18i20_gen3_info = {
+	.usb_id = USB_ID(0x1235, 0x8215),
+
+	.line_out_hw_vol = 1,
+	.level_input_count = 2,
+	.pad_input_count = 8,
+
+	.line_out_descrs = {
+		"Monitor 1 L",
+		"Monitor 1 R",
+		"Monitor 2 L",
+		"Monitor 2 R",
+		NULL,
+		NULL,
+		"Headphones 1 L",
+		"Headphones 1 R",
+		"Headphones 2 L",
+		"Headphones 2 R",
+	},
+
+	.port_count = {
+		[SCARLETT2_PORT_TYPE_NONE]     = {  1,  0 },
+		[SCARLETT2_PORT_TYPE_ANALOGUE] = {  9, 10 },
+		[SCARLETT2_PORT_TYPE_SPDIF]    = {  2,  2 },
+		[SCARLETT2_PORT_TYPE_ADAT]     = {  8,  8 },
+		[SCARLETT2_PORT_TYPE_MIX]      = { 12, 25 },
+		[SCARLETT2_PORT_TYPE_PCM]      = { 20, 20 },
+	},
+
+	.mux_assignment = { {
+		{ SCARLETT2_PORT_TYPE_PCM,       0,  8 },
+		{ SCARLETT2_PORT_TYPE_PCM,      10, 10 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  0, 10 },
+		{ SCARLETT2_PORT_TYPE_SPDIF,     0,  2 },
+		{ SCARLETT2_PORT_TYPE_ADAT,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_PCM,       8,  2 },
+		{ SCARLETT2_PORT_TYPE_MIX,       0, 25 },
+		{ SCARLETT2_PORT_TYPE_NONE,      0, 12 },
+		{ 0,                             0,  0 },
+	}, {
+		{ SCARLETT2_PORT_TYPE_PCM,       0,  8 },
+		{ SCARLETT2_PORT_TYPE_PCM,      10,  8 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  0, 10 },
+		{ SCARLETT2_PORT_TYPE_SPDIF,     0,  2 },
+		{ SCARLETT2_PORT_TYPE_ADAT,      0,  8 },
+		{ SCARLETT2_PORT_TYPE_PCM,       8,  2 },
+		{ SCARLETT2_PORT_TYPE_MIX,       0, 25 },
+		{ SCARLETT2_PORT_TYPE_NONE,      0, 10 },
+		{ 0,                             0,  0 },
+	}, {
+		{ SCARLETT2_PORT_TYPE_PCM,       0, 10 },
+		{ SCARLETT2_PORT_TYPE_ANALOGUE,  0, 10 },
+		{ SCARLETT2_PORT_TYPE_SPDIF,     0,  2 },
+		{ SCARLETT2_PORT_TYPE_NONE,      0, 24 },
+		{ 0,                             0,  0 },
+	} },
+};
+
 static const struct scarlett2_device_info *scarlett2_devices[] = {
 	/* Supported Gen 2 devices */
 	&s6i6_gen2_info,
 	&s18i8_gen2_info,
 	&s18i20_gen2_info,
 
+	/* Supported Gen 3 devices */
+	&s4i4_gen3_info,
+	&s8i6_gen3_info,
+	&s18i8_gen3_info,
+	&s18i20_gen3_info,
+
 	/* End of list */
 	NULL
 };
@@ -674,7 +896,7 @@  static int scarlett2_usb(
 	if (err != req_buf_size) {
 		usb_audio_err(
 			mixer->chip,
-			"Scarlett Gen 2 USB request result cmd %x was %d\n",
+			"Scarlett Gen 2/3 USB request result cmd %x was %d\n",
 			cmd, err);
 		err = -EINVAL;
 		goto unlock;
@@ -691,7 +913,7 @@  static int scarlett2_usb(
 	if (err != resp_buf_size) {
 		usb_audio_err(
 			mixer->chip,
-			"Scarlett Gen 2 USB response result cmd %x was %d "
+			"Scarlett Gen 2/3 USB response result cmd %x was %d "
 			"expected %d\n",
 			cmd, err, resp_buf_size);
 		err = -EINVAL;
@@ -709,7 +931,7 @@  static int scarlett2_usb(
 	    resp->pad) {
 		usb_audio_err(
 			mixer->chip,
-			"Scarlett Gen 2 USB invalid response; "
+			"Scarlett Gen 2/3 USB invalid response; "
 			   "cmd tx/rx %d/%d seq %d/%d size %d/%d "
 			   "error %d pad %d\n",
 			le32_to_cpu(req->cmd), le32_to_cpu(resp->cmd),
@@ -2485,7 +2707,7 @@  int snd_scarlett_gen2_init(struct usb_mixer_interface *mixer)
 
 	if (!(chip->setup & SCARLETT2_ENABLE)) {
 		usb_audio_info(chip,
-			"Focusrite Scarlett Gen 2 Mixer Driver disabled; "
+			"Focusrite Scarlett Gen 2/3 Mixer Driver disabled; "
 			"use options snd_usb_audio vid=0x%04x pid=0x%04x "
 			"device_setup=1 to enable and report any issues "
 			"to g@b4.vu",
@@ -2495,7 +2717,7 @@  int snd_scarlett_gen2_init(struct usb_mixer_interface *mixer)
 	}
 
 	usb_audio_info(chip,
-		"Focusrite Scarlett Gen 2 Mixer Driver enabled pid=0x%04x",
+		"Focusrite Scarlett Gen 2/3 Mixer Driver enabled pid=0x%04x",
 		USB_ID_PRODUCT(chip->usb_id));
 
 	err = snd_scarlett_gen2_controls_create(mixer);