[alsa-devel] Mixer regression with usb soundcard

[Jaejoong Kim]

2017-12-19 7:48 GMT+09:00 Takashi Iwai <tiwai at suse.de>:
> On Mon, 18 Dec 2017 22:56:07 +0100,
> Mauro Santos wrote:
>>
>> On 18-12-2017 19:30, Takashi Iwai wrote:
>> > On Mon, 18 Dec 2017 20:10:44 +0100,
>> > Mauro Santos wrote:
>> >>
>> >> On 18-12-2017 17:50, Jaejoong Kim wrote:
>> >>> Mauro,
>> >>>
>> >>> Could you please try debug patch(I also attach the patch file)?
>> >>
>> >> With the attached patch I get the following when plugging in the usb dac
>> >> directly to a usb3 port:
>> >> [   54.391539] usb 1-2: new full-speed USB device number 7 using xhci_hcd
>> >> [   54.514996] usb 1-2: device descriptor read/64, error -71
>> >> [   54.849808] input: HiFimeDIY Audio HiFimeDIY DAC as
>> >> /devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
>> >> [   54.850168] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
>> >> v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-0000:00:14.0-2/input0
>> >> [   54.950421] usb 1-2: [DEBUG] nameid:0, len:0
>> >> [   54.950426] usb 1-2: [DEBUG] len:3, get_term_name:PCM
>> >> [   54.950429] usb 1-2: [11] SU [PCM] items = 2
>> >> [   54.950985] usbcore: registered new interface driver snd-usb-audio
>> >
>> > Hmm, the driver get the supposedly correct name string here, so I see
>> > no flaw, so far.
>> >
>> > Could you put the similar debug prints after reverting the commit and
>> > compare?  Or, at minimum, you can enable simply the kernel debug
>> > prints like below:
>> >
>> >   % echo "file sound/usb/mixer.c +p" > /sys/kernel/debug/dynamic_debug_control
>> >
>> > and re-plug the device.
>> >
>> > Also, could you attach the output of "amixer contents" on both working
>> > and non-working kernels?
>> >
>>
>> I have compiled a new kernel where I have reverted the commit and I've
>> added the debug output based on your last debug patch. I attach the
>> patch that reverts the changes and adds the debug output just in case
>> anyone wants to do a sanity check on it (don't mind the indentation I
>> think I botched that).
>>
>> With the debug patches I get no extra output when echoing to the
>> dynamic_debug/control file, I guess that's expected.
>>
>> I attach the dmesg and amixer outputs for the case without revert plus
>> debug (bad) and revert plus debug (good).
>>
>> One change does jump out:
>>
>> bad:  usb 1-2: [11] SU [PCM] items = 2
>> good: usb 1-2: [11] SU [PCM Capture Source] items = 2
>
> Thanks, that explains what went wrong.  The commit dropped the brace
> at the if-line, and it ended up with the lack of suffix unless
> get_term_name() fails.
>
> The fix patch is below.  Could you check whether it cures the issue?
>
> Also, Jaejoong, could you check whether it doesn't your device,
> either?  I believe it should keep working, but just to be sure.

I don't have an USB audio DAC with selector unit descriptor.
But your patch file looks good to me. Thanks for fix the regression.

>
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai at suse.de>
> Subject: [PATCH] ALSA: usb-audio: Fix the missing ctl name suffix at parsing
>  SU
>
> The commit 89b89d121ffc ("ALSA: usb-audio: Add check return value for
> usb_string()") added the check of the return value from
> snd_usb_copy_string_desc(), which is correct per se, but it introduced
> a regression.  In the original code, either the "Clock Source",
> "Playback Source" or "Capture Source" suffix is added after the
> terminal string, while the commit changed it to add the suffix only
> when get_term_name() is failing.  It ended up with an incorrect ctl
> name like "PCM" instead of "PCM Capture Source".
>
> Also, even the original code has a similar bug: when the ctl name is
> generated from snd_usb_copy_string_desc() for the given iSelector, it
> also doesn't put the suffix.
>
> This patch addresses these issues: the suffix is added always when no
> static mapping is found.  Also the patch tries to put more comments
> and cleans up the if/else block for better readability in order to
> avoid the same pitfall again.
>
> Fixes: 89b89d121ffc ("ALSA: usb-audio: Add check return value for usb_string()")
> Reported-by: Mauro Santos <registo.mailling at gmail.com>
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
>  sound/usb/mixer.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index afc208e1c756..60ebc99ae323 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -2173,20 +2173,25 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid,
>         kctl->private_value = (unsigned long)namelist;
>         kctl->private_free = usb_mixer_selector_elem_free;
>
> -       nameid = uac_selector_unit_iSelector(desc);
> +       /* check the static mapping table at first */
>         len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
> -       if (len)
> -               ;
> -       else if (nameid)
> -               len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
> -                                        sizeof(kctl->id.name));
> -       else
> -               len = get_term_name(state, &state->oterm,
> -                                   kctl->id.name, sizeof(kctl->id.name), 0);
> -
>         if (!len) {
> -               strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
> +               /* no mapping ? */
It does not seem necessary to have a comment.

> +               /* if iSelector is given, use it */
> +               nameid = uac_selector_unit_iSelector(desc);
> +               if (nameid)
> +                       len = snd_usb_copy_string_desc(state, nameid,
> +                                                      kctl->id.name,
> +                                                      sizeof(kctl->id.name));
> +               /* ... or pick up the terminal name at next */
> +               if (!len)
> +                       len = get_term_name(state, &state->oterm,
> +                                   kctl->id.name, sizeof(kctl->id.name), 0);
> +               /* ... or use the fixed string "USB" as the last resort */
> +               if (!len)
> +                       strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
>
> +               /* and add the proper suffix */
>                 if (desc->bDescriptorSubtype == UAC2_CLOCK_SELECTOR)
>                         append_ctl_name(kctl, " Clock Source");
>                 else if ((state->oterm.type & 0xff00) == 0x0100)
> --
> 2.15.1
>