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 > > 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 > It is similar to that but I bought mine in August of 2013. The description of my device when I bought it was: HifiMeDiy Sabre USB DAC. Digital to Audio Converter 96khz/24bit (incl USB to optical converter feature). I have opened the box and on the PCB mine says UAE23 v1.3A. It uses a tenor te7022l usb receiver plus a sabre es9032p dac if it matters. > 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); > } > > > 2017-12-19 2:19 GMT+09:00 Jaejoong Kim <climbbb.kim@xxxxxxxxx>: >> 2017-12-19 2:13 GMT+09:00 Takashi Iwai <tiwai@xxxxxxx>: >>> 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 -- Mauro Santos -- 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
