Mixer regression with usb soundcard

[Jaejoong Kim <climbbb.kim@gmail.com>]

Mauro,

Could you please try debug patch(I also attach the patch file)?

And can you share your usb audio dac? Is this same device with yours?
https://hifimediy.com/index.php?route=product/product&product_id=83

  }
@@ -656,10 +656,14 @@ static int get_term_name(struct mixer_build
*state, struct usb_audio_term *iterm
  unsigned char *name, int maxlen, int term_only)
 {
  struct iterm_name_combo *names;
+ int len;

- if (iterm->name)
- return snd_usb_copy_string_desc(state, iterm->name,
+ if (iterm->name) {
+ len = snd_usb_copy_string_desc(state, iterm->name,
  name, maxlen);
+ usb_audio_err(state->chip, "[DEBUG] len:%d, iterm->name:%d\n", len,
iterm->name);
+ return len;
+ }

  /* virtual type - not a real terminal */
  if (iterm->type >> 16) {
@@ -2162,14 +2166,23 @@ static int parse_audio_selector_unit(struct
mixer_build *state, int unitid,

  nameid = uac_selector_unit_iSelector(desc);
  len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
+
+ usb_audio_err(state->chip, "[DEBUG] nameid:%d, len:%d\n", nameid, len);
+
  if (len)
  ;
- else if (nameid)
+ else if (nameid) {
  len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
  sizeof(kctl->id.name));
- else
+ usb_audio_err(state->chip, "[DEBUG] len:%d, copy_string id.name:%s\n",
+ len, (len > 0) ? kctl->id.name : " ");
+ }
+ else {
  len = get_term_name(state, &state->oterm,
      kctl->id.name, sizeof(kctl->id.name), 0);
+ usb_audio_err(state->chip, "[DEBUG] len:%d, get_term_name:%s\n",
+ len, (len > 0) ? kctl->id.name : " ");
+ }

  if (!len) {
  strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
@@ -2182,7 +2195,7 @@ static int parse_audio_selector_unit(struct
mixer_build *state, int unitid,
  append_ctl_name(kctl, " Playback Source");
  }

- usb_audio_dbg(state->chip, "[%d] SU [%s] items = %d\n",
+ usb_audio_err(state->chip, "[%d] SU [%s] items = %d\n",
      cval->head.id, kctl->id.name, desc->bNrInPins);
  return snd_usb_mixer_add_control(&cval->head, kctl);
 }


2017-12-19 2:19 GMT+09:00 Jaejoong Kim <climbbb.kim@gmail.com>:
> 2017-12-19 2:13 GMT+09:00 Takashi Iwai <tiwai@suse.de>:
>> On Mon, 18 Dec 2017 18:05:18 +0100,
>> Mauro Santos wrote:
>>>
>>> On 18-12-2017 15:45, Takashi Iwai wrote:
>>> > On Mon, 18 Dec 2017 16:30:13 +0100,
>>> > Mauro Santos wrote:
>>> >>
>>> >> On 18-12-2017 13:53, Takashi Iwai wrote:
>>> >>> On Mon, 18 Dec 2017 14:44:44 +0100,
>>> >>> Greg KH wrote:
>>> >>>>
>>> >>>> On Sun, Dec 17, 2017 at 06:56:05PM +0000, Mauro Santos wrote:
>>> >>>>> I believe this is the right place to report this problem, but if it
>>> >>>>> isn't please point me in the right direction.
>>> >>>>
>>> >>>> Adding the developer of that patch, and the sound maintainer and
>>> >>>> developers to the thread.
>>> >>>>
>>> >>>>> I have noticed that after the update from kernel 4.14.5 to 4.14.6
>>> >>>>> alsamixer does not show the usual volume controls for my usb soundcard.
>>> >>>>>
>>> >>>>> Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
>>> >>>>> check return value for usb_string() (from linux-stable) makes the
>>> >>>>> controls come back again with kernel 4.14.6.
>>> >>> (snip)
>>> >>>>
>>> >>>> This is odd, Takashi, I thought we fixed up the problem that if the
>>> >>>> string was invalid, the code would continue to go on, it's not a "real"
>>> >>>> error.  Did that not get marked for the stable trees?
>>> >>>
>>> >>> Yes, it was marked as stable, and it's odd that the revert of the
>>> >>> given commit changes the behavior in that way.
>>> >>>
>>> >>> Mauro, could you double-check whether reverting the commit really does
>>> >>> fix it as expected?  If yes, could you put some debug print at the
>>> >>> part the patch touches, and check which unit id gives len=0 from
>>> >>> snd_usb_copy_string_desc()?
>>> >>
>>> >> I'm sure reverting that patch makes things work as expected. I noticed
>>> >> the problem after an update and that is the only thing I revert on top
>>> >> of the kernel provided by the distro (Arch Linux).
>>> >
>>> > Did you revert only one patch, not both patches?
>>> > Just to be sure.
>>>
>>> I have reverted only one patch.
>>>
>>> >> Alsamixer works fine for the built in soundcard in my laptop, but the
>>> >> mixer for the usb soundcard was not working so it's specific to usb and
>>> >> only 2 patches touch the mixer.c file between 4.14.5 and 4.14.6. I've
>>> >> tried reversing both, one at a time, and only reverting this one
>>> >> restored the expected behavior.
>>> >>
>>> >> Regarding adding the debug print I'm going to need guidance. Without
>>> >> reverting anything, would adding at line 2178 of sound/usb/mixer.c the
>>> >> following be enough?
>>> >>
>>> >> printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);
>>> >>
>>> >> It would then look like this (minus the line wrapping):
>>> >> len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
>>> >> printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);
>>> >> if (len)
>>> >
>>> > Well, at that point, there should be no difference.
>>> > The difference is after that, so what I'd like to see is something
>>> > like:
>>> >
>>> > --- a/sound/usb/mixer.c
>>> > +++ b/sound/usb/mixer.c
>>> > @@ -2175,14 +2175,18 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid,
>>> >
>>> >     nameid = uac_selector_unit_iSelector(desc);
>>> >     len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
>>> > +   pr_info("XXX id=%d, check_mapped_name=%d\n", id, len);
>>> >     if (len)
>>> >             ;
>>> > -   else if (nameid)
>>> > +   else if (nameid) {
>>> >             len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
>>> >                                      sizeof(kctl->id.name));
>>> > -   else
>>> > +           pr_info("XXX id=%d, copy_string=%d\n", len);
>>> > +   } else {
>>> >             len = get_term_name(state, &state->oterm,
>>> >                                 kctl->id.name, sizeof(kctl->id.name), 0);
>>> > +           pr_info("XXX id=%d, get_term_name=%d\n", len);
>>> > +   }
>>> >
>>> >     if (!len) {
>>> >             strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
>>> >
>>> >
>>> > If you see copy_string=0, it means that your hardware reports a bogus
>>> > entry, and the driver does it correctly.  If ignoring that error
>>> > really restores the old behavior back, it essentially means that it
>>> > worked casually in the past...
>>>
>>> I have applied your patch on top of 4.14.7 without reverting anything
>>> and I was planning to reply only after I had some result but building
>>> failed (without any errors strangely).
>>>
>>> I took a second look at your patch and I have a (maybe silly/naive)
>>> question, don't the second and third pr_info calls need an extra
>>> argument? There are two %d in the string but only one variable (len).
>>
>> Yeah, sure, of course you need them :)
>> I haven't tested the patch, but just to give you an idea.
>> Sorry if you wasted your time due to that.
>
> OK. I will make a debug patch with you last debug code.
>
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -656,10 +656,14 @@ static int get_term_name(struct mixer_build
> *state, struct usb_audio_term *iterm
>                          unsigned char *name, int maxlen, int term_only)
>  {
>         struct iterm_name_combo *names;
> +       int len;
>
> -       if (iterm->name)
> -               return snd_usb_copy_string_desc(state, iterm->name,
> +       if (iterm->name) {
> +               len = snd_usb_copy_string_desc(state, iterm->name,
>                                                 name, maxlen);
> +               if (len)
> +                       return len;
> +       }

This is your potential patch not debug. To verify your potential fix,
I add more debug code in get_term_name().
I am going to bed. It's 2:46AM..

>
> I will come back soon.
>
>>
>>
>> Takashi

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 84b9f9c..6200aa2 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -595,7 +595,7 @@ int snd_usb_mixer_add_control(struct usb_mixer_elem_list *list,
 	while (snd_ctl_find_id(mixer->chip->card, &kctl->id))
 		kctl->id.index++;
 	if ((err = snd_ctl_add(mixer->chip->card, kctl)) < 0) {
-		usb_audio_dbg(mixer->chip, "cannot add control (err = %d)\n",
+		usb_audio_err(mixer->chip, "[DEBUG] cannot add control (err = %d)\n",
 			      err);
 		return err;
 	}
@@ -656,10 +656,14 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
 			 unsigned char *name, int maxlen, int term_only)
 {
 	struct iterm_name_combo *names;
+	int len;
 
-	if (iterm->name)
-		return snd_usb_copy_string_desc(state, iterm->name,
+	if (iterm->name) {
+		len = snd_usb_copy_string_desc(state, iterm->name,
 						name, maxlen);
+		usb_audio_err(state->chip, "[DEBUG] len:%d, iterm->name:%d\n", len, iterm->name);
+		return len;
+	}
 
 	/* virtual type - not a real terminal */
 	if (iterm->type >> 16) {
@@ -2162,14 +2166,23 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid,
 
 	nameid = uac_selector_unit_iSelector(desc);
 	len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
+
+	usb_audio_err(state->chip, "[DEBUG] nameid:%d, len:%d\n", nameid, len);	
+
 	if (len)
 		;
-	else if (nameid)
+	else if (nameid) {
 		len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
 					 sizeof(kctl->id.name));
-	else
+		usb_audio_err(state->chip, "[DEBUG] len:%d, copy_string id.name:%s\n",
+					len, (len > 0) ? kctl->id.name : " ");
+	}
+	else {
 		len = get_term_name(state, &state->oterm,
 				    kctl->id.name, sizeof(kctl->id.name), 0);
+		usb_audio_err(state->chip, "[DEBUG] len:%d, get_term_name:%s\n",
+					len, (len > 0) ? kctl->id.name : " ");
+	}
 
 	if (!len) {
 		strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
@@ -2182,7 +2195,7 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid,
 			append_ctl_name(kctl, " Playback Source");
 	}
 
-	usb_audio_dbg(state->chip, "[%d] SU [%s] items = %d\n",
+	usb_audio_err(state->chip, "[%d] SU [%s] items = %d\n",
 		    cval->head.id, kctl->id.name, desc->bNrInPins);
 	return snd_usb_mixer_add_control(&cval->head, kctl);
 }