[PATCH] ALSA: usb-audio: Recurse before saving terminal properties

[PATCH] ALSA: usb-audio: Recurse before saving terminal properties

[Julian Scheel]

When the descriptor parser recurses into the clock source descriptor, the
terminals properties, like name and type, are set to the values of the clock
selector as check_input_term overwrites term->name and others in the recursive
call. As we want to only fill missing values using the descriptors higher in
the recursion tree the properties taken from the actual descriptor must be
applied after recursing.

Signed-off-by: Julian Scheel <julian@jusst.de>
---
 sound/usb/mixer.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 6b3acba..4a49944 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -715,15 +715,16 @@ static int check_input_term(struct mixer_build *state, int id,
 				term->name = d->iTerminal;
 			} else { /* UAC_VERSION_2 */
 				struct uac2_input_terminal_descriptor *d = p1;
-				term->type = le16_to_cpu(d->wTerminalType);
-				term->channels = d->bNrChannels;
-				term->chconfig = le32_to_cpu(d->bmChannelConfig);
-				term->name = d->iTerminal;
 
 				/* call recursively to get the clock selectors */
 				err = check_input_term(state, d->bCSourceID, term);
 				if (err < 0)
 					return err;
+
+				term->type = le16_to_cpu(d->wTerminalType);
+				term->channels = d->bNrChannels;
+				term->chconfig = le32_to_cpu(d->bmChannelConfig);
+				term->name = d->iTerminal;
 			}
 			return 0;
 		case UAC_FEATURE_UNIT: {
-- 
2.5.0

Re: [PATCH] ALSA: usb-audio: Recurse before saving terminal properties

[Takashi Iwai]

On Wed, 12 Aug 2015 22:14:59 +0200,
Julian Scheel wrote:
> 
> When the descriptor parser recurses into the clock source descriptor, the
> terminals properties, like name and type, are set to the values of the clock
> selector as check_input_term overwrites term->name and others in the recursive
> call. As we want to only fill missing values using the descriptors higher in
> the recursion tree the properties taken from the actual descriptor must be
> applied after recursing.
> 
> Signed-off-by: Julian Scheel <julian@jusst.de>

Thanks for the patch.  I vaguely remember of a similar patch, maybe
you who already sent.  One thing I find uncertain is whether this
breaks the current logic completely, namely:

> @@ -715,15 +715,16 @@ static int check_input_term(struct mixer_build *state, int id,
>  				term->name = d->iTerminal;
>  			} else { /* UAC_VERSION_2 */
>  				struct uac2_input_terminal_descriptor *d = p1;
> -				term->type = le16_to_cpu(d->wTerminalType);
> -				term->channels = d->bNrChannels;
> -				term->chconfig = le32_to_cpu(d->bmChannelConfig);
> -				term->name = d->iTerminal;
>  
>  				/* call recursively to get the clock selectors */
>  				err = check_input_term(state, d->bCSourceID, term);
>  				if (err < 0)
>  					return err;
> +
> +				term->type = le16_to_cpu(d->wTerminalType);
> +				term->channels = d->bNrChannels;
> +				term->chconfig = le32_to_cpu(d->bmChannelConfig);
> +				term->name = d->iTerminal;

... by this override, essentially all fields except for id are
restored.  Then what's the point to call check_input_term() for the
clock source?

Maybe Daniel can answer better how the original code is supposed to
work.  Daniel?


Takashi

Re: [PATCH] ALSA: usb-audio: Recurse before saving terminal properties

[Julian Scheel]

Am 14.08.2015 um 15:06 schrieb Takashi Iwai:
> On Wed, 12 Aug 2015 22:14:59 +0200,
> Julian Scheel wrote:
>>
>> When the descriptor parser recurses into the clock source descriptor, the
>> terminals properties, like name and type, are set to the values of the clock
>> selector as check_input_term overwrites term->name and others in the recursive
>> call. As we want to only fill missing values using the descriptors higher in
>> the recursion tree the properties taken from the actual descriptor must be
>> applied after recursing.
>>
>> Signed-off-by: Julian Scheel <julian@jusst.de>
>
> Thanks for the patch.  I vaguely remember of a similar patch, maybe
> you who already sent.

Hm, I couldn't remember to have sent a similar patch before :)

> One thing I find uncertain is whether this
> breaks the current logic completely, namely:
>
>> @@ -715,15 +715,16 @@ static int check_input_term(struct mixer_build *state, int id,
>>   				term->name = d->iTerminal;
>>   			} else { /* UAC_VERSION_2 */
>>   				struct uac2_input_terminal_descriptor *d = p1;
>> -				term->type = le16_to_cpu(d->wTerminalType);
>> -				term->channels = d->bNrChannels;
>> -				term->chconfig = le32_to_cpu(d->bmChannelConfig);
>> -				term->name = d->iTerminal;
>>
>>   				/* call recursively to get the clock selectors */
>>   				err = check_input_term(state, d->bCSourceID, term);
>>   				if (err < 0)
>>   					return err;
>> +
>> +				term->type = le16_to_cpu(d->wTerminalType);
>> +				term->channels = d->bNrChannels;
>> +				term->chconfig = le32_to_cpu(d->bmChannelConfig);
>> +				term->name = d->iTerminal;
>
> ... by this override, essentially all fields except for id are
> restored.  Then what's the point to call check_input_term() for the
> clock source?

This is a good point indeed, but due do my understanding the recursion 
is not about filling missing information, but about verifying a valid 
descriptor. So in case there are broken references in the descriptor it 
gets abandoned. I'm happy to be corrected if this understanding is wrong.

When overriding the value in the recursion as it is without this patch 
it leads to virtually any control getting the name of the Clock 
Selector... I wondered about this on several UAC2 devices (like the XMOS 
stuff), before I looked into it.

-Julian

Re: [PATCH] ALSA: usb-audio: Recurse before saving terminal properties

[Daniel Mack]

Hi,

On 08/14/2015 03:25 PM, Julian Scheel wrote:
> Am 14.08.2015 um 15:06 schrieb Takashi Iwai:

>>> @@ -715,15 +715,16 @@ static int check_input_term(struct mixer_build *state, int id,
>>>   				term->name = d->iTerminal;
>>>   			} else { /* UAC_VERSION_2 */
>>>   				struct uac2_input_terminal_descriptor *d = p1;
>>> -				term->type = le16_to_cpu(d->wTerminalType);
>>> -				term->channels = d->bNrChannels;
>>> -				term->chconfig = le32_to_cpu(d->bmChannelConfig);
>>> -				term->name = d->iTerminal;
>>>
>>>   				/* call recursively to get the clock selectors */
>>>   				err = check_input_term(state, d->bCSourceID, term);
>>>   				if (err < 0)
>>>   					return err;
>>> +
>>> +				term->type = le16_to_cpu(d->wTerminalType);
>>> +				term->channels = d->bNrChannels;
>>> +				term->chconfig = le32_to_cpu(d->bmChannelConfig);
>>> +				term->name = d->iTerminal;
>>
>> ... by this override, essentially all fields except for id are
>> restored.  Then what's the point to call check_input_term() for the
>> clock source?
> 
> This is a good point indeed, but due do my understanding the recursion 
> is not about filling missing information, but about verifying a valid 
> descriptor. So in case there are broken references in the descriptor it 
> gets abandoned. I'm happy to be corrected if this understanding is wrong.

Hmm, good point. It' been a while since I worked on this, but looking at
the code right now, you're right, the mixer unit for input terminal is
indeed just overridden by the information of the clock selector on UAC2.
That doesn't seem right.

I guess what we really want is to add a 2nd mixer control in such cases,
and that would mean we need to call into parse_audio_selector_unit()
instead.

I can cook up or test a patch next week, when I have access to test
hardware.


Thanks,
Daniel

Re: [PATCH] ALSA: usb-audio: Recurse before saving terminal properties

[Julian Scheel]

Hi Daniel,

On 16.08.15 13:45, Daniel Mack wrote:
> On 08/14/2015 03:25 PM, Julian Scheel wrote:
>> Am 14.08.2015 um 15:06 schrieb Takashi Iwai:
>
>>>> @@ -715,15 +715,16 @@ static int check_input_term(struct mixer_build *state, int id,
>>>>    				term->name = d->iTerminal;
>>>>    			} else { /* UAC_VERSION_2 */
>>>>    				struct uac2_input_terminal_descriptor *d = p1;
>>>> -				term->type = le16_to_cpu(d->wTerminalType);
>>>> -				term->channels = d->bNrChannels;
>>>> -				term->chconfig = le32_to_cpu(d->bmChannelConfig);
>>>> -				term->name = d->iTerminal;
>>>>
>>>>    				/* call recursively to get the clock selectors */
>>>>    				err = check_input_term(state, d->bCSourceID, term);
>>>>    				if (err < 0)
>>>>    					return err;
>>>> +
>>>> +				term->type = le16_to_cpu(d->wTerminalType);
>>>> +				term->channels = d->bNrChannels;
>>>> +				term->chconfig = le32_to_cpu(d->bmChannelConfig);
>>>> +				term->name = d->iTerminal;
>>>
>>> ... by this override, essentially all fields except for id are
>>> restored.  Then what's the point to call check_input_term() for the
>>> clock source?
>>
>> This is a good point indeed, but due do my understanding the recursion
>> is not about filling missing information, but about verifying a valid
>> descriptor. So in case there are broken references in the descriptor it
>> gets abandoned. I'm happy to be corrected if this understanding is wrong.
>
> Hmm, good point. It' been a while since I worked on this, but looking at
> the code right now, you're right, the mixer unit for input terminal is
> indeed just overridden by the information of the clock selector on UAC2.
> That doesn't seem right.
>
> I guess what we really want is to add a 2nd mixer control in such cases,
> and that would mean we need to call into parse_audio_selector_unit()
> instead.

Ah, you're right. The Clock Selector shall actually be exposed as a 
mixer unit. This never happened with the current code, but it just 
renamed all other selectors to the name of the clock selector if it existed.

> I can cook up or test a patch next week, when I have access to test
> hardware.

I can update my patch to reflect this as well. If you haven't done 
before I'll try to do it later today.

-Julian

Re: [PATCH] ALSA: usb-audio: Recurse before saving terminal properties

[Julian Scheel]

Hi Daniel,

On 16.08.2015 13:45, Daniel Mack wrote:
> Hi,
>
> On 08/14/2015 03:25 PM, Julian Scheel wrote:
>> Am 14.08.2015 um 15:06 schrieb Takashi Iwai:
>
>>>> @@ -715,15 +715,16 @@ static int check_input_term(struct mixer_build *state, int id,
>>>>    				term->name = d->iTerminal;
>>>>    			} else { /* UAC_VERSION_2 */
>>>>    				struct uac2_input_terminal_descriptor *d = p1;
>>>> -				term->type = le16_to_cpu(d->wTerminalType);
>>>> -				term->channels = d->bNrChannels;
>>>> -				term->chconfig = le32_to_cpu(d->bmChannelConfig);
>>>> -				term->name = d->iTerminal;
>>>>
>>>>    				/* call recursively to get the clock selectors */
>>>>    				err = check_input_term(state, d->bCSourceID, term);
>>>>    				if (err < 0)
>>>>    					return err;
>>>> +
>>>> +				term->type = le16_to_cpu(d->wTerminalType);
>>>> +				term->channels = d->bNrChannels;
>>>> +				term->chconfig = le32_to_cpu(d->bmChannelConfig);
>>>> +				term->name = d->iTerminal;
>>>
>>> ... by this override, essentially all fields except for id are
>>> restored.  Then what's the point to call check_input_term() for the
>>> clock source?
>>
>> This is a good point indeed, but due do my understanding the recursion
>> is not about filling missing information, but about verifying a valid
>> descriptor. So in case there are broken references in the descriptor it
>> gets abandoned. I'm happy to be corrected if this understanding is wrong.
>
> Hmm, good point. It' been a while since I worked on this, but looking at
> the code right now, you're right, the mixer unit for input terminal is
> indeed just overridden by the information of the clock selector on UAC2.
> That doesn't seem right.
>
> I guess what we really want is to add a 2nd mixer control in such cases,
> and that would mean we need to call into parse_audio_selector_unit()
> instead.

I just had another look at this. In fact all mixer controls are properly 
created with the current code. This happens in the loop over all 
descriptors in snd_usb_mixer_controls. parse_audio_unit is called for 
each of them. For clock selectors and selector units it calls 
parse_audio_selector_unit where the actual clock selector/selector units 
are parsed and mixer ctls are created.
So I'd renew my first comment about the recursion. It is only needed to 
verify the integrity of descriptors and hence shall not overwrite any 
values.

After patching my device to offer 2 clocks on the clock selector the 
mixer looks like this with my patch:
--
Simple mixer control 'Master',0
   Capabilities: pvolume pvolume-joined pswitch pswitch-joined
   Playback channels: Mono
   Limits: Playback 0 - 44
   Mono: Playback 26 [59%] [-36.00dB] [on]
Simple mixer control 'Clock Selector',0
   Capabilities: enum
   Items: 'Internal Clock' 'Fake Clock'
   Item0: 'Internal Clock'
Simple mixer control 'Input Selector',0
   Capabilities: enum
   Items: 'USB' 'S/PDIF'
   Item0: 'USB'
--

Running the same without my patch results in:
--
Simple mixer control 'Master',0
   Capabilities: pvolume pvolume-joined pswitch pswitch-joined
   Playback channels: Mono
   Limits: Playback 0 - 44
   Mono: Playback 26 [59%] [-36.00dB] [on]
Simple mixer control 'Clock Selector',0
   Capabilities: enum
   Items: 'Internal Clock' 'Fake Clock'
   Item0: 'Internal Clock'
Simple mixer control 'Input Selector',0
   Capabilities: enum
   Items: 'Clock Selector' 'Clock Selector'
   Item0: 'Clock Selector'
--

So you can see, in both cases the mixer controls are generated as it 
should be. But the original code applies the wrong labels to all the 
Input Terminals associated to Input Selector. I think keeping the 
recursion, but not using any of the term values from the recursion is 
the correct approach as it ensures there are no dead ends in the 
descriptor hierarchy (i.e. referencing non-existant clock entities), but 
keeps the actual terminal properties correct.
What shall be changed in my original patch is that the term->id shall 
also be reset after recursing into check_input_term, as otherwise the 
input terminals are stored with the clock selectors id. Would you agree 
on that?

Thanks,
Julian

Re: [PATCH] ALSA: usb-audio: Recurse before saving terminal properties

[Daniel Mack]

On 08/17/2015 10:35 AM, Julian Scheel wrote:
> On 16.08.2015 13:45, Daniel Mack wrote:

>> Hmm, good point. It' been a while since I worked on this, but looking at
>> the code right now, you're right, the mixer unit for input terminal is
>> indeed just overridden by the information of the clock selector on UAC2.
>> That doesn't seem right.
>>
>> I guess what we really want is to add a 2nd mixer control in such cases,
>> and that would mean we need to call into parse_audio_selector_unit()
>> instead.
> 
> I just had another look at this. In fact all mixer controls are properly 
> created with the current code. This happens in the loop over all 
> descriptors in snd_usb_mixer_controls. parse_audio_unit is called for 
> each of them. For clock selectors and selector units it calls 
> parse_audio_selector_unit where the actual clock selector/selector units 
> are parsed and mixer ctls are created.
> So I'd renew my first comment about the recursion. It is only needed to 
> verify the integrity of descriptors and hence shall not overwrite any 
> values.

Yes, you're right. So your first patch on this is actually correct, but
given it caused confusion, I guess it deserves a comment in both the
code and the commit log :)

Care to resend with those comments added?



Thanks,
Daniel

Re: [PATCH] ALSA: usb-audio: Recurse before saving terminal properties

[Johan Aires Rastén]

On 14 August 2015 at 15:06, Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 12 Aug 2015 22:14:59 +0200,
> Julian Scheel wrote:
>>
>> When the descriptor parser recurses into the clock source descriptor, the
>> terminals properties, like name and type, are set to the values of the clock
>> selector as check_input_term overwrites term->name and others in the recursive
>> call. As we want to only fill missing values using the descriptors higher in
>> the recursion tree the properties taken from the actual descriptor must be
>> applied after recursing.
>>
>> Signed-off-by: Julian Scheel <julian@jusst.de>
>
> Thanks for the patch.  I vaguely remember of a similar patch, maybe
> you who already sent.  One thing I find uncertain is whether this
> breaks the current logic completely, namely:
>

Maybe that vague memory relates to me asking about this code a while
ago. I never sent any patch though because I couldn't figure out what
the correct behaviour should be. Hats off to Julian for fixing it,
this is a step on the way to get the Asus U7 fully working.

>> @@ -715,15 +715,16 @@ static int check_input_term(struct mixer_build *state, int id,
>>                               term->name = d->iTerminal;
>>                       } else { /* UAC_VERSION_2 */
>>                               struct uac2_input_terminal_descriptor *d = p1;
>> -                             term->type = le16_to_cpu(d->wTerminalType);
>> -                             term->channels = d->bNrChannels;
>> -                             term->chconfig = le32_to_cpu(d->bmChannelConfig);
>> -                             term->name = d->iTerminal;
>>
>>                               /* call recursively to get the clock selectors */
>>                               err = check_input_term(state, d->bCSourceID, term);
>>                               if (err < 0)
>>                                       return err;
>> +
>> +                             term->type = le16_to_cpu(d->wTerminalType);
>> +                             term->channels = d->bNrChannels;
>> +                             term->chconfig = le32_to_cpu(d->bmChannelConfig);
>> +                             term->name = d->iTerminal;
>
> ... by this override, essentially all fields except for id are
> restored.  Then what's the point to call check_input_term() for the
> clock source?
>
> Maybe Daniel can answer better how the original code is supposed to
> work.  Daniel?
>
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel