From patchwork Mon Dec 18 22:48:58 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 10122037 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 9C5F760327 for ; Mon, 18 Dec 2017 22:49:07 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8C46528991 for ; Mon, 18 Dec 2017 22:49:07 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7FC0928B49; Mon, 18 Dec 2017 22:49:07 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9B64728991 for ; Mon, 18 Dec 2017 22:49:06 +0000 (UTC) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id 4A721266F1D; Mon, 18 Dec 2017 23:49:04 +0100 (CET) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id E2C162671EE; Mon, 18 Dec 2017 23:49:02 +0100 (CET) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 6F7F8266E90 for ; Mon, 18 Dec 2017 23:49:00 +0100 (CET) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id ED9FAAEA4; Mon, 18 Dec 2017 22:48:58 +0000 (UTC) Date: Mon, 18 Dec 2017 23:48:58 +0100 Message-ID: From: Takashi Iwai To: Mauro Santos In-Reply-To: References: <0f95a1cc-c438-ca4e-cc5f-d311e33a496e@gmail.com> <20171218134444.GA18133@kroah.com> <9dff0299-5621-2670-50b6-ee1851bb40f4@gmail.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Cc: Greg KH , Jaejoong Kim , USB list , alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Mixer regression with usb soundcard X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP 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. Takashi -- 8< -- From: Takashi Iwai 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 Cc: Signed-off-by: Takashi Iwai --- 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 ? */ + /* 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)