2017-12-19 7:48 GMT+09:00 Takashi Iwai <tiwai@xxxxxxx>:
> 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@xxxxxxx>
> 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@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
