* Re: Mixer regression with usb soundcard
[not found] ` <0f95a1cc-c438-ca4e-cc5f-d311e33a496e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-12-18 13:44 ` Greg KH
[not found] ` <20171218134444.GA18133-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 35+ messages in thread
From: Greg KH @ 2017-12-18 13:44 UTC (permalink / raw)
To: Mauro Santos, Jaejoong Kim, Takashi Iwai
Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-usb-u79uwXL29TY76Z2rM5mHXA
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.
>
> Amixer shows a difference between the good and bad case.
>
> Good amixer output (4.14.5):
> Simple mixer control 'PCM',0
> Capabilities: pvolume pswitch pswitch-joined
> Playback channels: Front Left - Front Right
> Limits: Playback 0 - 110
> Mono:
> Front Left: Playback 0 [0%] [-55.00dB] [on]
> Front Right: Playback 0 [0%] [-55.00dB] [on]
> Simple mixer control 'PCM Capture Source',0
> Capabilities: enum
> Items: 'Line' 'IEC958 In'
> Item0: 'Line'
> Simple mixer control 'Line',0
> Capabilities: cvolume cswitch cswitch-joined
> Capture channels: Front Left - Front Right
> Limits: Capture 0 - 104
> Front Left: Capture 20 [19%] [-30.00dB] [off]
> Front Right: Capture 20 [19%] [-30.00dB] [off]
>
> Bad amixer output (4.14.6):
> Simple mixer control 'PCM',0
> Capabilities: pvolume pswitch pswitch-joined enum
> Items: 'Line' 'IEC958 In'
> Item0: 'Line'
> Item1: 'Line'
> Simple mixer control 'Line',0
> Capabilities: cvolume cswitch cswitch-joined
> Capture channels: Front Left - Front Right
> Limits: Capture 0 - 104
> Front Left: Capture 20 [19%] [-30.00dB] [off]
> Front Right: Capture 20 [19%] [-30.00dB] [off]
>
> The output of lsusb -v for the affected sound card is attached.
>
> --
> Mauro Santos
> Bus 001 Device 006: ID 1852:7022 GYROCOM C&C Co., LTD
> Device Descriptor:
> bLength 18
> bDescriptorType 1
> bcdUSB 1.10
> bDeviceClass 0
> bDeviceSubClass 0
> bDeviceProtocol 0
> bMaxPacketSize0 8
> idVendor 0x1852 GYROCOM C&C Co., LTD
> idProduct 0x7022
> bcdDevice 0.01
> iManufacturer 1 HiFimeDIY Audio
> iProduct 2 HiFimeDIY DAC
> iSerial 0
> bNumConfigurations 1
> Configuration Descriptor:
> bLength 9
> bDescriptorType 2
> wTotalLength 428
> bNumInterfaces 4
> bConfigurationValue 1
> iConfiguration 0
> bmAttributes 0x80
> (Bus Powered)
> MaxPower 500mA
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 0
> bAlternateSetting 0
> bNumEndpoints 1
> bInterfaceClass 3 Human Interface Device
> bInterfaceSubClass 0
> bInterfaceProtocol 0
> iInterface 0
> HID Device Descriptor:
> bLength 9
> bDescriptorType 33
> bcdHID 1.00
> bCountryCode 0 Not supported
> bNumDescriptors 1
> bDescriptorType 34 Report
> wDescriptorLength 58
> Report Descriptors:
> ** UNAVAILABLE **
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x81 EP 1 IN
> bmAttributes 3
> Transfer Type Interrupt
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0012 1x 18 bytes
> bInterval 32
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 1
> bAlternateSetting 0
> bNumEndpoints 0
> bInterfaceClass 1 Audio
> bInterfaceSubClass 1 Control Device
> bInterfaceProtocol 0
> iInterface 3 SABRE 24/96 DAC_DigiT
> AudioControl Interface Descriptor:
> bLength 10
> bDescriptorType 36
> bDescriptorSubtype 1 (HEADER)
> bcdADC 1.00
> wTotalLength 92
> bInCollection 2
> baInterfaceNr( 0) 2
> baInterfaceNr( 1) 3
> AudioControl Interface Descriptor:
> bLength 12
> bDescriptorType 36
> bDescriptorSubtype 2 (INPUT_TERMINAL)
> bTerminalID 1
> wTerminalType 0x0603 Line Connector
> bAssocTerminal 0
> bNrChannels 2
> wChannelConfig 0x0003
> Left Front (L)
> Right Front (R)
> iChannelNames 0
> iTerminal 0
> AudioControl Interface Descriptor:
> bLength 12
> bDescriptorType 36
> bDescriptorSubtype 2 (INPUT_TERMINAL)
> bTerminalID 5
> wTerminalType 0x0605 SPDIF interface
> bAssocTerminal 0
> bNrChannels 2
> wChannelConfig 0x0003
> Left Front (L)
> Right Front (R)
> iChannelNames 0
> iTerminal 0
> AudioControl Interface Descriptor:
> bLength 12
> bDescriptorType 36
> bDescriptorSubtype 2 (INPUT_TERMINAL)
> bTerminalID 9
> wTerminalType 0x0101 USB Streaming
> bAssocTerminal 0
> bNrChannels 2
> wChannelConfig 0x0003
> Left Front (L)
> Right Front (R)
> iChannelNames 0
> iTerminal 0
> AudioControl Interface Descriptor:
> bLength 9
> bDescriptorType 36
> bDescriptorSubtype 3 (OUTPUT_TERMINAL)
> bTerminalID 3
> wTerminalType 0x0605 SPDIF interface
> bAssocTerminal 0
> bSourceID 16
> iTerminal 0
> AudioControl Interface Descriptor:
> bLength 9
> bDescriptorType 36
> bDescriptorSubtype 3 (OUTPUT_TERMINAL)
> bTerminalID 7
> wTerminalType 0x0101 USB Streaming
> bAssocTerminal 0
> bSourceID 11
> iTerminal 0
> AudioControl Interface Descriptor:
> bLength 10
> bDescriptorType 36
> bDescriptorSubtype 6 (FEATURE_UNIT)
> bUnitID 14
> bSourceID 1
> bControlSize 1
> bmaControls( 0) 0x01
> Mute Control
> bmaControls( 1) 0x02
> Volume Control
> bmaControls( 2) 0x02
> Volume Control
> iFeature 0
> AudioControl Interface Descriptor:
> bLength 10
> bDescriptorType 36
> bDescriptorSubtype 6 (FEATURE_UNIT)
> bUnitID 16
> bSourceID 9
> bControlSize 1
> bmaControls( 0) 0x01
> Mute Control
> bmaControls( 1) 0x02
> Volume Control
> bmaControls( 2) 0x02
> Volume Control
> iFeature 0
> AudioControl Interface Descriptor:
> bLength 8
> bDescriptorType 36
> bDescriptorSubtype 5 (SELECTOR_UNIT)
> bUnitID 11
> bNrInPins 2
> baSource( 0) 14
> baSource( 1) 5
> iSelector 0
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 2
> bAlternateSetting 0
> bNumEndpoints 0
> bInterfaceClass 1 Audio
> bInterfaceSubClass 2 Streaming
> bInterfaceProtocol 0
> iInterface 0
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 2
> bAlternateSetting 1
> bNumEndpoints 1
> bInterfaceClass 1 Audio
> bInterfaceSubClass 2 Streaming
> bInterfaceProtocol 0
> iInterface 0
> AudioStreaming Interface Descriptor:
> bLength 7
> bDescriptorType 36
> bDescriptorSubtype 1 (AS_GENERAL)
> bTerminalLink 7
> bDelay 0 frames
> wFormatTag 1 PCM
> AudioStreaming Interface Descriptor:
> bLength 26
> bDescriptorType 36
> bDescriptorSubtype 2 (FORMAT_TYPE)
> bFormatType 1 (FORMAT_TYPE_I)
> bNrChannels 2
> bSubframeSize 2
> bBitResolution 16
> bSamFreqType 6 Discrete
> tSamFreq[ 0] 8000
> tSamFreq[ 1] 16000
> tSamFreq[ 2] 32000
> tSamFreq[ 3] 44100
> tSamFreq[ 4] 48000
> tSamFreq[ 5] 96000
> Endpoint Descriptor:
> bLength 9
> bDescriptorType 5
> bEndpointAddress 0x82 EP 2 IN
> bmAttributes 9
> Transfer Type Isochronous
> Synch Type Adaptive
> Usage Type Data
> wMaxPacketSize 0x0184 1x 388 bytes
> bInterval 1
> bRefresh 0
> bSynchAddress 0
> AudioControl Endpoint Descriptor:
> bLength 7
> bDescriptorType 37
> bDescriptorSubtype 1 (EP_GENERAL)
> bmAttributes 0x01
> Sampling Frequency
> bLockDelayUnits 2 Decoded PCM samples
> wLockDelay 2 Decoded PCM samples
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 2
> bAlternateSetting 2
> bNumEndpoints 1
> bInterfaceClass 1 Audio
> bInterfaceSubClass 2 Streaming
> bInterfaceProtocol 0
> iInterface 0
> AudioStreaming Interface Descriptor:
> bLength 7
> bDescriptorType 36
> bDescriptorSubtype 1 (AS_GENERAL)
> bTerminalLink 7
> bDelay 0 frames
> wFormatTag 1 PCM
> AudioStreaming Interface Descriptor:
> bLength 26
> bDescriptorType 36
> bDescriptorSubtype 2 (FORMAT_TYPE)
> bFormatType 1 (FORMAT_TYPE_I)
> bNrChannels 2
> bSubframeSize 3
> bBitResolution 24
> bSamFreqType 6 Discrete
> tSamFreq[ 0] 8000
> tSamFreq[ 1] 16000
> tSamFreq[ 2] 32000
> tSamFreq[ 3] 44100
> tSamFreq[ 4] 48000
> tSamFreq[ 5] 96000
> Endpoint Descriptor:
> bLength 9
> bDescriptorType 5
> bEndpointAddress 0x82 EP 2 IN
> bmAttributes 9
> Transfer Type Isochronous
> Synch Type Adaptive
> Usage Type Data
> wMaxPacketSize 0x0246 1x 582 bytes
> bInterval 1
> bRefresh 0
> bSynchAddress 0
> AudioControl Endpoint Descriptor:
> bLength 7
> bDescriptorType 37
> bDescriptorSubtype 1 (EP_GENERAL)
> bmAttributes 0x01
> Sampling Frequency
> bLockDelayUnits 2 Decoded PCM samples
> wLockDelay 2 Decoded PCM samples
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 3
> bAlternateSetting 0
> bNumEndpoints 0
> bInterfaceClass 1 Audio
> bInterfaceSubClass 2 Streaming
> bInterfaceProtocol 0
> iInterface 0
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 3
> bAlternateSetting 1
> bNumEndpoints 1
> bInterfaceClass 1 Audio
> bInterfaceSubClass 2 Streaming
> bInterfaceProtocol 0
> iInterface 0
> AudioStreaming Interface Descriptor:
> bLength 7
> bDescriptorType 36
> bDescriptorSubtype 1 (AS_GENERAL)
> bTerminalLink 9
> bDelay 0 frames
> wFormatTag 1 PCM
> AudioStreaming Interface Descriptor:
> bLength 26
> bDescriptorType 36
> bDescriptorSubtype 2 (FORMAT_TYPE)
> bFormatType 1 (FORMAT_TYPE_I)
> bNrChannels 2
> bSubframeSize 2
> bBitResolution 16
> bSamFreqType 6 Discrete
> tSamFreq[ 0] 8000
> tSamFreq[ 1] 16000
> tSamFreq[ 2] 32000
> tSamFreq[ 3] 44100
> tSamFreq[ 4] 48000
> tSamFreq[ 5] 96000
> Endpoint Descriptor:
> bLength 9
> bDescriptorType 5
> bEndpointAddress 0x03 EP 3 OUT
> bmAttributes 9
> Transfer Type Isochronous
> Synch Type Adaptive
> Usage Type Data
> wMaxPacketSize 0x0184 1x 388 bytes
> bInterval 1
> bRefresh 0
> bSynchAddress 0
> AudioControl Endpoint Descriptor:
> bLength 7
> bDescriptorType 37
> bDescriptorSubtype 1 (EP_GENERAL)
> bmAttributes 0x01
> Sampling Frequency
> bLockDelayUnits 2 Decoded PCM samples
> wLockDelay 2 Decoded PCM samples
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 3
> bAlternateSetting 2
> bNumEndpoints 1
> bInterfaceClass 1 Audio
> bInterfaceSubClass 2 Streaming
> bInterfaceProtocol 0
> iInterface 0
> AudioStreaming Interface Descriptor:
> bLength 7
> bDescriptorType 36
> bDescriptorSubtype 1 (AS_GENERAL)
> bTerminalLink 9
> bDelay 0 frames
> wFormatTag 1 PCM
> AudioStreaming Interface Descriptor:
> bLength 26
> bDescriptorType 36
> bDescriptorSubtype 2 (FORMAT_TYPE)
> bFormatType 1 (FORMAT_TYPE_I)
> bNrChannels 2
> bSubframeSize 3
> bBitResolution 24
> bSamFreqType 6 Discrete
> tSamFreq[ 0] 8000
> tSamFreq[ 1] 16000
> tSamFreq[ 2] 32000
> tSamFreq[ 3] 44100
> tSamFreq[ 4] 48000
> tSamFreq[ 5] 96000
> Endpoint Descriptor:
> bLength 9
> bDescriptorType 5
> bEndpointAddress 0x03 EP 3 OUT
> bmAttributes 9
> Transfer Type Isochronous
> Synch Type Adaptive
> Usage Type Data
> wMaxPacketSize 0x0246 1x 582 bytes
> bInterval 1
> bRefresh 0
> bSynchAddress 0
> AudioControl Endpoint Descriptor:
> bLength 7
> bDescriptorType 37
> bDescriptorSubtype 1 (EP_GENERAL)
> bmAttributes 0x01
> Sampling Frequency
> bLockDelayUnits 2 Decoded PCM samples
> wLockDelay 2 Decoded PCM samples
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 3
> bAlternateSetting 3
> bNumEndpoints 1
> bInterfaceClass 1 Audio
> bInterfaceSubClass 2 Streaming
> bInterfaceProtocol 0
> iInterface 0
> AudioStreaming Interface Descriptor:
> bLength 7
> bDescriptorType 36
> bDescriptorSubtype 1 (AS_GENERAL)
> bTerminalLink 9
> bDelay 0 frames
> wFormatTag 8193 IEC1937_AC-3
> AudioStreaming Interface Descriptor:
> bLength 11
> bDescriptorType 36
> bDescriptorSubtype 2 (FORMAT_TYPE)
> bFormatType 3 (FORMAT_TYPE_III)
> bNrChannels 2
> bSubframeSize 2
> bBitResolution 16
> bSamFreqType 1 Discrete
> tSamFreq[ 0] 48000
> Endpoint Descriptor:
> bLength 9
> bDescriptorType 5
> bEndpointAddress 0x03 EP 3 OUT
> bmAttributes 9
> Transfer Type Isochronous
> Synch Type Adaptive
> Usage Type Data
> wMaxPacketSize 0x00c0 1x 192 bytes
> bInterval 1
> bRefresh 0
> bSynchAddress 0
> AudioControl Endpoint Descriptor:
> bLength 7
> bDescriptorType 37
> bDescriptorSubtype 1 (EP_GENERAL)
> bmAttributes 0x01
> Sampling Frequency
> bLockDelayUnits 2 Decoded PCM samples
> wLockDelay 2 Decoded PCM samples
> Device Status: 0x0000
> (Bus Powered)
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?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
[not found] ` <20171218134444.GA18133-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
@ 2017-12-18 13:53 ` Takashi Iwai
[not found] ` <s5hy3m0dtw0.wl-tiwai-l3A5Bk7waGM@public.gmane.org>
0 siblings, 1 reply; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 13:53 UTC (permalink / raw)
To: Greg KH
Cc: Mauro Santos, Jaejoong Kim, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-usb-u79uwXL29TY76Z2rM5mHXA
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()?
thanks,
Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
[not found] ` <s5hy3m0dtw0.wl-tiwai-l3A5Bk7waGM@public.gmane.org>
@ 2017-12-18 15:30 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 15:30 UTC (permalink / raw)
To: Takashi Iwai, Greg KH
Cc: Jaejoong Kim, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-usb-u79uwXL29TY76Z2rM5mHXA
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).
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)
--
Mauro Santos
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-18 15:45 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 15:45 UTC (permalink / raw)
To: Mauro Santos; +Cc: Greg KH, Jaejoong Kim, alsa-devel, linux-usb
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.
> 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:
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...
thanks,
Takashi
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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));
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 15:45 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 15:45 UTC (permalink / raw)
To: Mauro Santos
Cc: Greg KH, Jaejoong Kim, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-usb-u79uwXL29TY76Z2rM5mHXA
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.
> 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...
thanks,
Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
2017-12-18 15:45 ` Takashi Iwai
@ 2017-12-18 16:59 ` Jaejoong Kim
-1 siblings, 0 replies; 35+ messages in thread
From: Jaejoong Kim @ 2017-12-18 16:59 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mauro Santos, Greg KH, alsa-devel, USB list
Mauro,
thanks for the report and sorry for the make problem.
Could you try below debug patch? I add more debug code with Takashi's guideline.
return err;
}
@@ -2162,14 +2162,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 +2191,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 0:45 GMT+09:00 Takashi Iwai <tiwai@suse.de>:
> 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.
>
>> 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...
AudioControl Interface Descriptor:
bLength 8
bDescriptorType 36
bDescriptorSubtype 5 (SELECTOR_UNIT)
bUnitID 11
bNrInPins 2
baSource( 0) 14
baSource( 1) 5
iSelector 0
^^^^^^^^^
From the lsusb.txt, iSelector is '0' which means
uac_selector_unit_iSelector() return zero I think.
Thanks, jaejoong
>
>
> thanks,
>
> Takashi
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 84b9f9c..0233425 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;
}
@@ -2162,14 +2162,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 +2191,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);
}
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 16:59 ` Jaejoong Kim
0 siblings, 0 replies; 35+ messages in thread
From: Jaejoong Kim @ 2017-12-18 16:59 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mauro Santos, alsa-devel, USB list, Greg KH
[-- Attachment #1: Type: text/plain, Size: 6812 bytes --]
Mauro,
thanks for the report and sorry for the make problem.
Could you try below debug patch? I add more debug code with Takashi's guideline.
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 84b9f9c..0233425 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;
}
@@ -2162,14 +2162,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 +2191,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 0:45 GMT+09:00 Takashi Iwai <tiwai@suse.de>:
> 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.
>
>> 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...
AudioControl Interface Descriptor:
bLength 8
bDescriptorType 36
bDescriptorSubtype 5 (SELECTOR_UNIT)
bUnitID 11
bNrInPins 2
baSource( 0) 14
baSource( 1) 5
iSelector 0
^^^^^^^^^
>From the lsusb.txt, iSelector is '0' which means
uac_selector_unit_iSelector() return zero I think.
Thanks, jaejoong
>
>
> thanks,
>
> Takashi
[-- Attachment #2: usb_alsa_debug.patch --]
[-- Type: text/x-patch, Size: 1774 bytes --]
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 84b9f9c..0233425 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;
}
@@ -2162,14 +2162,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 +2191,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);
}
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-18 17:05 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 17:05 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Greg KH, Jaejoong Kim, alsa-devel, linux-usb
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).
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 17:05 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 17:05 UTC (permalink / raw)
To: Takashi Iwai
Cc: Greg KH, Jaejoong Kim, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-usb-u79uwXL29TY76Z2rM5mHXA
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).
--
Mauro Santos
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
2017-12-18 16:59 ` Jaejoong Kim
@ 2017-12-18 17:11 ` Takashi Iwai
-1 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 17:11 UTC (permalink / raw)
To: Jaejoong Kim; +Cc: Mauro Santos, Greg KH, alsa-devel, USB list
On Mon, 18 Dec 2017 17:59:38 +0100,
Jaejoong Kim wrote:
>
> AudioControl Interface Descriptor:
> bLength 8
> bDescriptorType 36
> bDescriptorSubtype 5 (SELECTOR_UNIT)
> bUnitID 11
> bNrInPins 2
> baSource( 0) 14
> baSource( 1) 5
> iSelector 0
> ^^^^^^^^^
>
> >From the lsusb.txt, iSelector is '0' which means
> uac_selector_unit_iSelector() return zero I think.
Yes, and in that case, nameid=0, thus it should go to
get_term_name(), and there must be no difference.
If any difference came from the given commit, it must be nameid!=0,
and yet snd_usb_copy_string_desc(namei) returns an error.
In anyway, another potential fix would be the patch like below.
It covers the case where an error is returned from
snb_usb_copy_string_desc() and doesn't fall back to the default name
resolution.
thanks,
Takashi
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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;
+ }
/* virtual type - not a real terminal */
if (iterm->type >> 16) {
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 17:11 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 17:11 UTC (permalink / raw)
To: Jaejoong Kim; +Cc: Mauro Santos, alsa-devel, USB list, Greg KH
On Mon, 18 Dec 2017 17:59:38 +0100,
Jaejoong Kim wrote:
>
> AudioControl Interface Descriptor:
> bLength 8
> bDescriptorType 36
> bDescriptorSubtype 5 (SELECTOR_UNIT)
> bUnitID 11
> bNrInPins 2
> baSource( 0) 14
> baSource( 1) 5
> iSelector 0
> ^^^^^^^^^
>
> >From the lsusb.txt, iSelector is '0' which means
> uac_selector_unit_iSelector() return zero I think.
Yes, and in that case, nameid=0, thus it should go to
get_term_name(), and there must be no difference.
If any difference came from the given commit, it must be nameid!=0,
and yet snd_usb_copy_string_desc(namei) returns an error.
In anyway, another potential fix would be the patch like below.
It covers the case where an error is returned from
snb_usb_copy_string_desc() and doesn't fall back to the default name
resolution.
thanks,
Takashi
--- 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;
+ }
/* virtual type - not a real terminal */
if (iterm->type >> 16) {
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
2017-12-18 17:05 ` Mauro Santos
@ 2017-12-18 17:13 ` Takashi Iwai
-1 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 17:13 UTC (permalink / raw)
To: Mauro Santos; +Cc: Greg KH, Jaejoong Kim, alsa-devel, linux-usb
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.
Takashi
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 17:13 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 17:13 UTC (permalink / raw)
To: Mauro Santos; +Cc: Greg KH, Jaejoong Kim, linux-usb, alsa-devel
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.
Takashi
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
2017-12-18 17:13 ` Takashi Iwai
@ 2017-12-18 17:19 ` Jaejoong Kim
-1 siblings, 0 replies; 35+ messages in thread
From: Jaejoong Kim @ 2017-12-18 17:19 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mauro Santos, Greg KH, alsa-devel, USB list
2017-12-19 2:13 GMT+09:00 Takashi Iwai <tiwai@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.
>
>
> Takashi
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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;
+ }
I will come back soon.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 17:19 ` Jaejoong Kim
0 siblings, 0 replies; 35+ messages in thread
From: Jaejoong Kim @ 2017-12-18 17:19 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mauro Santos, alsa-devel, USB list, Greg KH
2017-12-19 2:13 GMT+09:00 Takashi Iwai <tiwai@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;
+ }
I will come back soon.
>
>
> Takashi
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
2017-12-18 17:19 ` Jaejoong Kim
@ 2017-12-18 17:50 ` Jaejoong Kim
-1 siblings, 0 replies; 35+ messages in thread
From: Jaejoong Kim @ 2017-12-18 17:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mauro Santos, Greg KH, alsa-devel, USB list
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 <climbbb.kim@gmail.com>:
> 2017-12-19 2:13 GMT+09:00 Takashi Iwai <tiwai@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
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);
}
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 17:50 ` Jaejoong Kim
0 siblings, 0 replies; 35+ messages in thread
From: Jaejoong Kim @ 2017-12-18 17:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mauro Santos, alsa-devel, USB list, Greg KH
[-- Attachment #1: Type: text/plain, Size: 8417 bytes --]
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
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@gmail.com>:
> 2017-12-19 2:13 GMT+09:00 Takashi Iwai <tiwai@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
[-- Attachment #2: debug.patch --]
[-- Type: text/x-patch, Size: 2327 bytes --]
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);
}
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-18 18:18 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 18:18 UTC (permalink / raw)
To: Jaejoong Kim, Takashi Iwai; +Cc: Greg KH, alsa-devel, USB list
On 18-12-2017 16:59, Jaejoong Kim wrote:
> Mauro,
> thanks for the report and sorry for the make problem.
>
> Could you try below debug patch? I add more debug code with Takashi's guideline.
>
I get this when I plug the device directly to a usb3 port:
[ 63.643706] usb 1-2: new full-speed USB device number 7 using xhci_hcd
[ 63.767089] usb 1-2: device descriptor read/64, error -71
[ 64.100906] 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
[ 64.101208] 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
[ 64.208385] usb 1-2: [DEBUG] nameid:0, len:0
[ 64.208390] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[ 64.208393] usb 1-2: [11] SU [PCM] items = 2
[ 64.208881] usbcore: registered new interface driver snd-usb-audio
If I plug it to a usb2 hub where I usually have it connected the output
is almost the same:
[ 129.159898] usb 1-3.3: new full-speed USB device number 8 using xhci_hcd
[ 129.246587] usb 1-3.3: device descriptor read/64, error -32
[ 129.532552] input: HiFimeDIY Audio HiFimeDIY DAC as
/devices/pci0000:00/0000:00:14.0/usb1/1-3/1-3.3/1-3.3:1.0/0003:1852:7022.0004/input/input21
[ 129.532790] hid-generic 0003:1852:7022.0004: input,hidraw2: USB HID
v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-0000:00:14.0-3.3/input0
[ 129.560090] usb 1-3.3: [DEBUG] nameid:0, len:0
[ 129.560096] usb 1-3.3: [DEBUG] len:3, get_term_name:PCM
[ 129.560100] usb 1-3.3: [11] SU [PCM] items = 2
I'm compiling now a kernel with your other patch, I'll get back to you
with all the information I can gather on the device once I test with the
other patch.
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 84b9f9c..0233425 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;
> }
> @@ -2162,14 +2162,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 +2191,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 0:45 GMT+09:00 Takashi Iwai <tiwai@suse.de>:
>> 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.
>>
>>> 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...
>
> AudioControl Interface Descriptor:
> bLength 8
> bDescriptorType 36
> bDescriptorSubtype 5 (SELECTOR_UNIT)
> bUnitID 11
> bNrInPins 2
> baSource( 0) 14
> baSource( 1) 5
> iSelector 0
> ^^^^^^^^^
>
>>From the lsusb.txt, iSelector is '0' which means
> uac_selector_unit_iSelector() return zero I think.
>
> Thanks, jaejoong
>
>>
>>
>> thanks,
>>
>> Takashi
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 18:18 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 18:18 UTC (permalink / raw)
To: Jaejoong Kim, Takashi Iwai
Cc: Greg KH, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, USB list
On 18-12-2017 16:59, Jaejoong Kim wrote:
> Mauro,
> thanks for the report and sorry for the make problem.
>
> Could you try below debug patch? I add more debug code with Takashi's guideline.
>
I get this when I plug the device directly to a usb3 port:
[ 63.643706] usb 1-2: new full-speed USB device number 7 using xhci_hcd
[ 63.767089] usb 1-2: device descriptor read/64, error -71
[ 64.100906] 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
[ 64.101208] 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
[ 64.208385] usb 1-2: [DEBUG] nameid:0, len:0
[ 64.208390] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[ 64.208393] usb 1-2: [11] SU [PCM] items = 2
[ 64.208881] usbcore: registered new interface driver snd-usb-audio
If I plug it to a usb2 hub where I usually have it connected the output
is almost the same:
[ 129.159898] usb 1-3.3: new full-speed USB device number 8 using xhci_hcd
[ 129.246587] usb 1-3.3: device descriptor read/64, error -32
[ 129.532552] input: HiFimeDIY Audio HiFimeDIY DAC as
/devices/pci0000:00/0000:00:14.0/usb1/1-3/1-3.3/1-3.3:1.0/0003:1852:7022.0004/input/input21
[ 129.532790] hid-generic 0003:1852:7022.0004: input,hidraw2: USB HID
v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-0000:00:14.0-3.3/input0
[ 129.560090] usb 1-3.3: [DEBUG] nameid:0, len:0
[ 129.560096] usb 1-3.3: [DEBUG] len:3, get_term_name:PCM
[ 129.560100] usb 1-3.3: [11] SU [PCM] items = 2
I'm compiling now a kernel with your other patch, I'll get back to you
with all the information I can gather on the device once I test with the
other patch.
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 84b9f9c..0233425 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;
> }
> @@ -2162,14 +2162,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 +2191,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 0:45 GMT+09:00 Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>:
>> 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.
>>
>>> 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...
>
> AudioControl Interface Descriptor:
> bLength 8
> bDescriptorType 36
> bDescriptorSubtype 5 (SELECTOR_UNIT)
> bUnitID 11
> bNrInPins 2
> baSource( 0) 14
> baSource( 1) 5
> iSelector 0
> ^^^^^^^^^
>
>>From the lsusb.txt, iSelector is '0' which means
> uac_selector_unit_iSelector() return zero I think.
>
> Thanks, jaejoong
>
>>
>>
>> thanks,
>>
>> Takashi
--
Mauro Santos
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-18 19:10 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 19:10 UTC (permalink / raw)
To: Jaejoong Kim, Takashi Iwai; +Cc: Greg KH, alsa-devel, USB list
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@gmail.com>:
>> 2017-12-19 2:13 GMT+09:00 Takashi Iwai <tiwai@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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 19:10 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 19:10 UTC (permalink / raw)
To: Jaejoong Kim, Takashi Iwai
Cc: Greg KH, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, USB list
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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> 2017-12-19 2:13 GMT+09:00 Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>:
>>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-18 19:30 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 19:30 UTC (permalink / raw)
To: Mauro Santos; +Cc: Jaejoong Kim, Greg KH, alsa-devel, USB list
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?
thanks,
Takashi
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 19:30 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 19:30 UTC (permalink / raw)
To: Mauro Santos
Cc: Jaejoong Kim, Greg KH, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, USB list
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?
thanks,
Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-18 21:56 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 21:56 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jaejoong Kim, Greg KH, alsa-devel, USB list
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
diff -ur a/sound/usb/mixer.c b/sound/usb/mixer.c
--- a/sound/usb/mixer.c 2017-12-18 19:47:02.748776502 +0000
+++ b/sound/usb/mixer.c 2017-12-18 20:18:30.023770892 +0000
@@ -595,7 +595,7 @@
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, "cannot add control (err = %d)\n",
err);
return err;
}
@@ -656,10 +656,14 @@
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) {
@@ -2175,17 +2179,24 @@
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);
-
- if (!len) {
- strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
+ 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));
if (desc->bDescriptorSubtype == UAC2_CLOCK_SELECTOR)
append_ctl_name(kctl, " Clock Source");
@@ -2195,7 +2206,7 @@
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);
}
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 21:56 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 21:56 UTC (permalink / raw)
To: Takashi Iwai
Cc: Jaejoong Kim, Greg KH, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, USB list
[-- Attachment #1: Type: text/plain, Size: 2161 bytes --]
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
--
Mauro Santos
[-- Attachment #2: amixer_good.txt --]
[-- Type: text/plain, Size: 600 bytes --]
Simple mixer control 'PCM',0
Capabilities: pvolume pswitch pswitch-joined
Playback channels: Front Left - Front Right
Limits: Playback 0 - 110
Mono:
Front Left: Playback 0 [0%] [-55.00dB] [on]
Front Right: Playback 0 [0%] [-55.00dB] [on]
Simple mixer control 'PCM Capture Source',0
Capabilities: enum
Items: 'Line' 'IEC958 In'
Item0: 'Line'
Simple mixer control 'Line',0
Capabilities: cvolume cswitch cswitch-joined
Capture channels: Front Left - Front Right
Limits: Capture 0 - 104
Front Left: Capture 0 [0%] [-40.00dB] [off]
Front Right: Capture 0 [0%] [-40.00dB] [off]
[-- Attachment #3: dmesg_good.txt --]
[-- Type: text/plain, Size: 657 bytes --]
[ 63.600402] usb 1-2: new full-speed USB device number 7 using xhci_hcd
[ 63.723739] usb 1-2: device descriptor read/64, error -71
[ 64.057506] 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
[ 64.057780] 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
[ 64.154091] usb 1-2: [DEBUG] nameid:0, len:0
[ 64.154095] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[ 64.154097] usb 1-2: [11] SU [PCM Capture Source] items = 2
[ 64.154495] usbcore: registered new interface driver snd-usb-audio
[-- Attachment #4: revert_with_debug.patch --]
[-- Type: text/x-patch, Size: 2296 bytes --]
diff -ur a/sound/usb/mixer.c b/sound/usb/mixer.c
--- a/sound/usb/mixer.c 2017-12-18 19:47:02.748776502 +0000
+++ b/sound/usb/mixer.c 2017-12-18 20:18:30.023770892 +0000
@@ -595,7 +595,7 @@
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, "cannot add control (err = %d)\n",
err);
return err;
}
@@ -656,10 +656,14 @@
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) {
@@ -2175,17 +2179,24 @@
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);
-
- if (!len) {
- strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
+ 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));
if (desc->bDescriptorSubtype == UAC2_CLOCK_SELECTOR)
append_ctl_name(kctl, " Clock Source");
@@ -2195,7 +2206,7 @@
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);
}
[-- Attachment #5: amixer_bad.txt --]
[-- Type: text/plain, Size: 382 bytes --]
Simple mixer control 'PCM',0
Capabilities: pvolume pswitch pswitch-joined enum
Items: 'Line' 'IEC958 In'
Item0: 'Line'
Item1: 'Line'
Simple mixer control 'Line',0
Capabilities: cvolume cswitch cswitch-joined
Capture channels: Front Left - Front Right
Limits: Capture 0 - 104
Front Left: Capture 0 [0%] [-40.00dB] [off]
Front Right: Capture 0 [0%] [-40.00dB] [off]
[-- Attachment #6: dmesg_bad.txt --]
[-- Type: text/plain, Size: 642 bytes --]
[ 56.317038] usb 1-2: new full-speed USB device number 7 using xhci_hcd
[ 56.440424] usb 1-2: device descriptor read/64, error -71
[ 56.775327] 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
[ 56.775689] 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
[ 56.876892] usb 1-2: [DEBUG] nameid:0, len:0
[ 56.876896] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[ 56.876900] usb 1-2: [11] SU [PCM] items = 2
[ 56.877392] usbcore: registered new interface driver snd-usb-audio
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-18 22:10 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 22:10 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jaejoong Kim, Greg KH, alsa-devel, USB list
On 18-12-2017 21:56, 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.
>
Turns out there is some output, I can't echo and plug, I need to plug,
echo, replug. Dmesg outputs are attached.
> 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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 22:10 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-18 22:10 UTC (permalink / raw)
To: Takashi Iwai
Cc: Jaejoong Kim, Greg KH, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, USB list
[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]
On 18-12-2017 21:56, 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.
>
Turns out there is some output, I can't echo and plug, I need to plug,
echo, replug. Dmesg outputs are attached.
> 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
>
--
Mauro Santos
[-- Attachment #2: dmesg_replug_bad.txt --]
[-- Type: text/plain, Size: 1116 bytes --]
[ 81.753703] usb 1-2: new full-speed USB device number 9 using xhci_hcd
[ 81.877148] usb 1-2: device descriptor read/64, error -71
[ 82.210907] input: HiFimeDIY Audio HiFimeDIY DAC as /devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0005/input/input22
[ 82.211121] hid-generic 0003:1852:7022.0005: input,hidraw2: USB HID v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-0000:00:14.0-2/input0
[ 82.231042] usb 1-2: [16] FU [PCM Playback Switch] ch = 1, val = 0/1/1
[ 82.232211] usb 1-2: cannot set ctl value: req = 0x4, wValue = 0x201, wIndex = 0x1001, type = 4, data = 0x40/0x0
[ 82.232616] usb 1-2: [16] FU [PCM Playback Volume] ch = 2, val = -14080/0/128
[ 82.232626] usb 1-2: [14] FU [Line Capture Switch] ch = 1, val = 0/1/1
[ 82.233919] usb 1-2: cannot set ctl value: req = 0x4, wValue = 0x201, wIndex = 0xe01, type = 4, data = 0x40/0x0
[ 82.234301] usb 1-2: [14] FU [Line Capture Volume] ch = 2, val = -10240/3072/128
[ 82.234310] usb 1-2: [DEBUG] nameid:0, len:0
[ 82.234313] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[ 82.234315] usb 1-2: [11] SU [PCM] items = 2
[-- Attachment #3: dmesg_replug_good.txt --]
[-- Type: text/plain, Size: 1131 bytes --]
[ 59.283418] usb 1-2: new full-speed USB device number 8 using xhci_hcd
[ 59.407116] usb 1-2: device descriptor read/64, error -71
[ 59.740686] input: HiFimeDIY Audio HiFimeDIY DAC as /devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0004/input/input21
[ 59.740893] hid-generic 0003:1852:7022.0004: input,hidraw2: USB HID v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-0000:00:14.0-2/input0
[ 59.760971] usb 1-2: [16] FU [PCM Playback Switch] ch = 1, val = 0/1/1
[ 59.762056] usb 1-2: cannot set ctl value: req = 0x4, wValue = 0x201, wIndex = 0x1001, type = 4, data = 0x40/0x0
[ 59.762418] usb 1-2: [16] FU [PCM Playback Volume] ch = 2, val = -14080/0/128
[ 59.762423] usb 1-2: [14] FU [Line Capture Switch] ch = 1, val = 0/1/1
[ 59.763510] usb 1-2: cannot set ctl value: req = 0x4, wValue = 0x201, wIndex = 0xe01, type = 4, data = 0x40/0x0
[ 59.763845] usb 1-2: [14] FU [Line Capture Volume] ch = 2, val = -10240/3072/128
[ 59.763848] usb 1-2: [DEBUG] nameid:0, len:0
[ 59.763849] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[ 59.763850] usb 1-2: [11] SU [PCM Capture Source] items = 2
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-18 22:48 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 22:48 UTC (permalink / raw)
To: Mauro Santos; +Cc: Jaejoong Kim, Greg KH, alsa-devel, USB list
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.
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)
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-18 22:48 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-18 22:48 UTC (permalink / raw)
To: Mauro Santos
Cc: Jaejoong Kim, Greg KH, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, USB list
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.
Takashi
-- 8< --
From: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
---
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);
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-19 1:04 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-19 1:04 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jaejoong Kim, Greg KH, alsa-devel, USB list
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)
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-19 1:04 ` Mauro Santos
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Santos @ 2017-12-19 1:04 UTC (permalink / raw)
To: Takashi Iwai
Cc: Jaejoong Kim, Greg KH, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, USB list
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-l3A5Bk7waGM@public.gmane.org>
> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> ---
> 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)
>
--
Mauro Santos
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
2017-12-18 22:48 ` Takashi Iwai
@ 2017-12-19 2:34 ` Jaejoong Kim
-1 siblings, 0 replies; 35+ messages in thread
From: Jaejoong Kim @ 2017-12-19 2:34 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mauro Santos, Greg KH, alsa-devel, USB list
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
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-19 2:34 ` Jaejoong Kim
0 siblings, 0 replies; 35+ messages in thread
From: Jaejoong Kim @ 2017-12-19 2:34 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mauro Santos, alsa-devel, USB list, Greg KH
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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Mixer regression with usb soundcard
@ 2017-12-19 6:43 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-19 6:43 UTC (permalink / raw)
To: Mauro Santos; +Cc: Jaejoong Kim, Greg KH, alsa-devel, USB list
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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Mixer regression with usb soundcard
@ 2017-12-19 6:43 ` Takashi Iwai
0 siblings, 0 replies; 35+ messages in thread
From: Takashi Iwai @ 2017-12-19 6:43 UTC (permalink / raw)
To: Mauro Santos
Cc: Jaejoong Kim, Greg KH, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, USB list
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2017-12-19 6:43 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <0f95a1cc-c438-ca4e-cc5f-d311e33a496e@gmail.com>
[not found] ` <0f95a1cc-c438-ca4e-cc5f-d311e33a496e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-12-18 13:44 ` Mixer regression with usb soundcard Greg KH
[not found] ` <20171218134444.GA18133-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2017-12-18 13:53 ` Takashi Iwai
[not found] ` <s5hy3m0dtw0.wl-tiwai-l3A5Bk7waGM@public.gmane.org>
2017-12-18 15:30 ` Mauro Santos
2017-12-18 15:45 Takashi Iwai
2017-12-18 15:45 ` Takashi Iwai
2017-12-18 16:59 Jaejoong Kim
2017-12-18 16:59 ` Jaejoong Kim
2017-12-18 17:05 Mauro Santos
2017-12-18 17:05 ` Mauro Santos
2017-12-18 17:11 Takashi Iwai
2017-12-18 17:11 ` Takashi Iwai
2017-12-18 17:13 Takashi Iwai
2017-12-18 17:13 ` Takashi Iwai
2017-12-18 17:19 Jaejoong Kim
2017-12-18 17:19 ` Jaejoong Kim
2017-12-18 17:50 Jaejoong Kim
2017-12-18 17:50 ` Jaejoong Kim
2017-12-18 18:18 Mauro Santos
2017-12-18 18:18 ` Mauro Santos
2017-12-18 19:10 Mauro Santos
2017-12-18 19:10 ` Mauro Santos
2017-12-18 19:30 Takashi Iwai
2017-12-18 19:30 ` Takashi Iwai
2017-12-18 21:56 Mauro Santos
2017-12-18 21:56 ` Mauro Santos
2017-12-18 22:10 Mauro Santos
2017-12-18 22:10 ` Mauro Santos
2017-12-18 22:48 Takashi Iwai
2017-12-18 22:48 ` Takashi Iwai
2017-12-19 1:04 Mauro Santos
2017-12-19 1:04 ` Mauro Santos
2017-12-19 2:34 Jaejoong Kim
2017-12-19 2:34 ` Jaejoong Kim
2017-12-19 6:43 Takashi Iwai
2017-12-19 6:43 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.