Re: Mixer regression with usb soundcard

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux