| Message ID | s5ho9mv649x.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18-12-2017 22:48, Takashi Iwai wrote: > 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? This patch does seem to work fine for me, I can see all the mixer controls for the usb soundcard/dac. For good measure I have also tested with a couple of usb headsets and I didn't see anything amiss (they don't have any "Capture Source" controls though). Thank you for looking into this and fixing it. > 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 <tiwai@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@gmail.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@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 ? */ > + /* 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) >
2017-12-19 7:48 GMT+09:00 Takashi Iwai <tiwai@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@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@gmail.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@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 >
On Tue, 19 Dec 2017 02:04:51 +0100, Mauro Santos wrote: > > On 18-12-2017 22:48, Takashi Iwai wrote: > > 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? > > This patch does seem to work fine for me, I can see all the mixer > controls for the usb soundcard/dac. > > For good measure I have also tested with a couple of usb headsets and I > didn't see anything amiss (they don't have any "Capture Source" controls > though). > > Thank you for looking into this and fixing it. Good to hear, I queued the fix to for-linus branch. I'm going to send a pull request for rc5. thanks, Takashi
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)