[alsa-devel] [PATCH] alsa-lib/tlv: fix handling of raw value ranges

[Clemens Ladisch]

Benoît Thébaudeau wrote:
> Clemens Ladisch wrote:
>> Benoît Thébaudeau wrote:
>>> Clemens Ladisch wrote:
>> All raw values described by the .info callback, returned by the .get
>> callback, and especially accepted by the .put callback, are valid.
>>
>> The TLV information just _describes_ values, it cannot _restrict_
>> them.
>>
>> If a register has invalid values, the driver must somehow map between
>> raw control values and valid register values.
>
> Now I understand how you see things. But is it an official and absolute ALSA
> rule,

Yes.

> known by everybody,

No.

> that may cause issues if not applied?

Yes.

> The hardware values are:
>  0x00 to 0x70: prohibited
>  0x71 to 0xcf: +15 dB to -79 dB by -1 dB steps
>  0xd0 to 0xfe: prohibited
>  0xff: mute
>
> To respect your rule, the driver would have to implement its own .info/.get/.put
> callbacks, giving the following mapping:
>   SW 0x00: HW 0xff: mute
>   SW 0x01 to 0x5f: HW 0x71 to 0xcf: +15 dB to -79 dB by -1 dB steps
>
> But why compel the driver to complicate things this way if it can work with
> standard macros and holes in the TLV ranges?

Because it _cannot_ work with standard macros and holes.

> It is also probably the case for many other existing hardware.
>
> Do you think that the solution would be rather to add common kernel ALSA stuff
> to handle such mappings?

Yes.


I'll see if I can cobble together some TLV checking code ...


>>>> As for the patch, while I'm not sure whether it is needed at all,
>>>> I also
>>>> don't quite understand the reason for all the changes.
>>>>
>>>>> --- alsa-lib/src/control/tlv.c
>>>>> +++ alsa-lib/src/control/tlv.c
>>>>> @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int
>>>>>  {
>>>>>  	switch (tlv[0]) {
>>>>>  	case SND_CTL_TLVT_DB_RANGE: {
>>>>> +		long dbmin, dbmax, prev_submax;
>>>>>  		unsigned int pos, len;
>>>>>  		len = int_index(tlv[1]);
>>>>> +		if (len < 6 || len > MAX_TLV_RANGE_SIZE)
>>>>>  			return -EINVAL;
>>>>>  		pos = 2;
>>>>> +		prev_submax = 0;
>>>>> +		do {
>>>>
>>>> Why is this not a while loop?
>>>
>>> Because the added "if (len < 6" makes it pointless.
>>
>> Then why did you add the len<6 check?  This is, in effect, just the same
>> as the old while condition.
>
> Because it's useless to wait until the end of the loop to return -EINVAL,

It is not necessary to optimize for error cases.

> and with my patch, the after-loop code becomes the fallback case for beyond-
> final-range values, so it avoids adding a weird extra check there.

But why does this force you to use do instead of while?


Regards,
Clemens