From patchwork Mon Dec 18 17:50:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jaejoong Kim X-Patchwork-Id: 10121275 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 8942760327 for ; Mon, 18 Dec 2017 17:51:01 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 795A628AFA for ; Mon, 18 Dec 2017 17:51:01 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6DDD228BE0; Mon, 18 Dec 2017 17:51:01 +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.8 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, T_DKIM_INVALID autolearn=no 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 E72F028CE1 for ; Mon, 18 Dec 2017 17:50:59 +0000 (UTC) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id 7B589267561; Mon, 18 Dec 2017 18:50:57 +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 C5ACD267563; Mon, 18 Dec 2017 18:50:55 +0100 (CET) Received: from mail-io0-f196.google.com (mail-io0-f196.google.com [209.85.223.196]) by alsa0.perex.cz (Postfix) with ESMTP id 1F990267544 for ; Mon, 18 Dec 2017 18:50:52 +0100 (CET) Received: by mail-io0-f196.google.com with SMTP id o2so10677630ioe.8 for ; Mon, 18 Dec 2017 09:50:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=bVOKW9hD4iCuF1VUjIiefC8CmJBkLVdfqiPOFgePJtA=; b=Pj4xkt6aY5AN7lc47a0Hp+2vZeKKAIZFf6EewgikWlZiUncxtmmnHSgaiJXAWfH97x RRxt8RHnRn7tjOre9+mVChUPSehq22O3R+V4YHntgEVattsoSdLlTyGZXM0OaMNuNLls wRq/nuqELLMQdPm1E1ATCav/irKrm0jiET3cVyaB+Gn6Jzp6b9c1Ol14z9GKT/n5IhJy gYloQZ7FMaA+bzZbfaUmeRPAbEAfzfKGPf6j+TSWLMMFT5O5MyVoIAyvpz6Q2k5ag9IO P9hK9+980SQ/y24vSLgfjW2SRRAlDR00Ok27uBerrQTJrbev9PY/u+lIey8KFgmZwtGn y6xw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=bVOKW9hD4iCuF1VUjIiefC8CmJBkLVdfqiPOFgePJtA=; b=EbLOXVQqEoj4Ax7yce4lXw3x9VHMP54qUlrXWCEIKBMTh7tOMtgli2gXNgqzZzk8JK BlI7ESuUYbhgi1PqL7IJ/jFYbgjqtTaCc8WufPQvuMagS3gvafphOMafb3HtWzdVIaGf BZqJjGVx2h3DCGprzHsp9ju9LWn1AkZ8yyBHrlzLQONAxrM40wGdtwBlLZO7nLkwA1eA karERZEQHbe2XKKiMtDRgpIteybdeUT6/jJCHHiGPuu8L41cqBtC8HqOHue9BZxzCCjs R/k1tWm7RAzz+Oz1xUoif3+NjG8UabsMN4pUiOSaasWRJFTbrwLgZkagBMA8XgL2IPMo jWow== X-Gm-Message-State: AKGB3mIXrKFSB4GhmZfrkdThdzX85tf52MkqNLPSsjrzqDLScSqw4vSF roa5rNsTWYe04Oz/elofs6V+EbXjLQIR5J5nP/c= X-Google-Smtp-Source: ACJfBos5yikiIsGU22DmeN5qxkIcvNbAIcv9mvrWcXlKGQw7XYix4YAqO0/MsqhEfluPPRAN8C/vWYacv+T6WW00pcw= X-Received: by 10.107.183.195 with SMTP id h186mr701290iof.159.1513619451058; Mon, 18 Dec 2017 09:50:51 -0800 (PST) MIME-Version: 1.0 Received: by 10.79.164.2 with HTTP; Mon, 18 Dec 2017 09:50:50 -0800 (PST) In-Reply-To: References: <0f95a1cc-c438-ca4e-cc5f-d311e33a496e@gmail.com> <20171218134444.GA18133@kroah.com> <9dff0299-5621-2670-50b6-ee1851bb40f4@gmail.com> From: Jaejoong Kim Date: Tue, 19 Dec 2017 02:50:50 +0900 Message-ID: To: Takashi Iwai Cc: Mauro Santos , alsa-devel@alsa-project.org, USB list , Greg KH 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 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 : > 2017-12-19 2:13 GMT+09:00 Takashi Iwai : >> 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); }