[01/17] ALSA: usb-audio: scarlett2: Fix wrong resume call

Commit Message

Geoffrey D. Bennett June 22, 2021, 5 p.m. UTC

From: Takashi Iwai <[e-mail]>

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.

Tested-by: Geoffrey D. Bennett <[e-mail]>
Cc: <[e-mail]>
Signed-off-by: Takashi Iwai <[e-mail]>
---
 sound/usb/mixer.c               | 3 +++
 sound/usb/mixer.h               | 1 +
 sound/usb/mixer_scarlett_gen2.c | 7 ++++++-
 3 files changed, 10 insertions(+), 1 deletion(-)

Comments

Takashi Iwai June 22, 2021, 7:42 p.m. UTC | #1

On Tue, 22 Jun 2021 19:00:49 +0200,
Geoffrey D. Bennett wrote:
> 
> From: Takashi Iwai <[e-mail]>
> 
> 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.
> 
> Tested-by: Geoffrey D. Bennett <[e-mail]>
> Cc: <[e-mail]>
> Signed-off-by: Takashi Iwai <[e-mail]>

When submitting a patch, you have to your Signed-off-by line even if
you are no author.  Could you give it?  Just reply with a proper SOB,
then I'll fix manually.


thanks,

Takashi

Geoffrey D. Bennett June 23, 2021, 1:03 a.m. UTC | #2

On Tue, Jun 22, 2021 at 09:42:10PM +0200, Takashi Iwai wrote:
> On Tue, 22 Jun 2021 19:00:49 +0200,
> Geoffrey D. Bennett wrote:
> > 
> > From: Takashi Iwai <[e-mail]>
> > 
> > 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.
> > 
> > Tested-by: Geoffrey D. Bennett <[e-mail]>
> > Cc: <[e-mail]>
> > Signed-off-by: Takashi Iwai <[e-mail]>
> 
> When submitting a patch, you have to your Signed-off-by line even if
> you are no author.  Could you give it?  Just reply with a proper SOB,
> then I'll fix manually.

Of course. You know how many times I've read
Documentation/process/submitting-patches.rst ? Not enough for it to
sink in, apparently :). Please append my SOB:

Signed-off-by: Geoffrey D. Bennett <[e-mail]>

Thanks,
Geoffrey.

Takashi Iwai June 23, 2021, 6:39 a.m. UTC | #3

On Wed, 23 Jun 2021 03:03:27 +0200,
Geoffrey D. Bennett wrote:
> 
> On Tue, Jun 22, 2021 at 09:42:10PM +0200, Takashi Iwai wrote:
> > On Tue, 22 Jun 2021 19:00:49 +0200,
> > Geoffrey D. Bennett wrote:
> > > 
> > > From: Takashi Iwai <[e-mail]>
> > > 
> > > 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.
> > > 
> > > Tested-by: Geoffrey D. Bennett <[e-mail]>
> > > Cc: <[e-mail]>
> > > Signed-off-by: Takashi Iwai <[e-mail]>
> > 
> > When submitting a patch, you have to your Signed-off-by line even if
> > you are no author.  Could you give it?  Just reply with a proper SOB,
> > then I'll fix manually.
> 
> Of course. You know how many times I've read
> Documentation/process/submitting-patches.rst ? Not enough for it to
> sink in, apparently :). Please append my SOB:
> 
> Signed-off-by: Geoffrey D. Bennett <[e-mail]>

OK, now all patches have been merged.


Thanks!

Takashi

Hin-Tak Leung June 25, 2021, 2:09 a.m. UTC | #4

On Wednesday, 23 June 2021, 07:39:48 BST, Takashi Iwai <[e-mail]> wrote:


> > Of course. You know how many times I've read
> > Documentation/process/submitting-patches.rst ? Not enough for it to
> > sink in, apparently :). Please append my SOB:
> > 
> > Signed-off-by: Geoffrey D. Bennett <[e-mail]>

> OK, now all patches have been merged.

To avoid this kind of problems, many years ago, I have had (see 'man git-config' for details):

 git config format.signoff true

set in my local kernel dev repo clone. This way "git format-patch ..." would automatically add my sign-off for anything I "git format-patch" of . It is easier to remove the line if not needed, or just ignore the line if irrelevant, than to add the line every time. I did that to a few repos which explicitly requires signoff's. (wine is another one).



Back to this series of patches, I think it is appropriate to add
 Acked-by: Hin-Tak <[e-mail]>

or 'Cc: ' as I was involved in packaging and adapting the patch series as for dkms / out-of-tree build for testing, so that I get e-mails, etc if it gets moved about / back-ported, though I was not involved in actually testing /using the patch series.

Regards,
Hin-Tak

Patch

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 dde008ea21d7..c4689c401d6e 100644
--- a/sound/usb/mixer_scarlett_gen2.c
+++ b/sound/usb/mixer_scarlett_gen2.c
@@ -1136,10 +1136,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) {