[alsa-devel] Mixer regression with usb soundcard
Mauro Santos
registo.mailling at gmail.com
Mon Dec 18 20:10:44 CET 2017
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 at gmail.com>:
>> 2017-12-19 2:13 GMT+09:00 Takashi Iwai <tiwai at suse.de>:
>>> 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
More information about the Alsa-devel
mailing list