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

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

[Benoît Thébaudeau]

In case of a TLV dB range with all items having raw value ranges strictly within
the main raw value range reported by the driver, snd_tlv_convert_from_dB()
returned one of the main raw range boundaries, which was outside all dB range
items.

E.g.:
static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = {
	TLV_DB_RANGE_HEAD(2),
	0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1),
	48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0),
};
	SOC_SINGLE_TLV("Main Volume",
		       BD3753X_VOL_GAIN,
		       0, 255, 1, bd3753x_vol_fader_gain_att_tlv),

snd_tlv_convert_from_dB(tlv, 0, 255, 1500, &value, 1) returned 255 instead of
the expected 142.

or:
static const unsigned int max9850_tlv[] = {
	TLV_DB_RANGE_HEAD(4),
	0x18, 0x1f, TLV_DB_SCALE_ITEM(-7450, 400, 0),
	0x20, 0x33, TLV_DB_SCALE_ITEM(-4150, 200, 0),
	0x34, 0x37, TLV_DB_SCALE_ITEM(-150, 100, 0),
	0x38, 0x3f, TLV_DB_SCALE_ITEM(250, 50, 0),
};
SOC_SINGLE_TLV("Headphone Volume", MAX9850_VOLUME, 0, 0x3f, 1, max9850_tlv),

snd_tlv_convert_from_dB(tlv, 0, 0x3f, -7450, &value, 0) returned 0 instead of
the expected 0x18.

Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>

--- 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_rangemax;
+		long dbmin, dbmax, prev_submax;
 		unsigned int pos, len;
 		len = int_index(tlv[1]);
-		if (len > MAX_TLV_RANGE_SIZE)
-			return -EINVAL;
-		if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
-					 &dbmin, &dbmax))
+		if (len < 6 || len > MAX_TLV_RANGE_SIZE)
 			return -EINVAL;
-		if (db_gain <= dbmin) {
-			*value = rangemin;
-			return 0;
-		} else if (db_gain >= dbmax) {
-			*value = rangemax;
-			return 0;
-		}
 		pos = 2;
-		prev_rangemax = 0;
-		while (pos + 4 <= len) {
-			rangemin = (int)tlv[pos];
-			rangemax = (int)tlv[pos + 1];
+		prev_submax = 0;
+		do {
+			long submin, submax;
+			submin = (int)tlv[pos];
+			submax = (int)tlv[pos + 1];
+			if (rangemax < submax)
+				submax = rangemax;
 			if (!snd_tlv_get_dB_range(tlv + pos + 2,
-						  rangemin, rangemax,
+						  submin, submax,
 						  &dbmin, &dbmax) &&
 			    db_gain >= dbmin && db_gain <= dbmax)
 				return snd_tlv_convert_from_dB(tlv + pos + 2,
-							       rangemin, rangemax,
+							       submin, submax,
 							       db_gain, value, xdir);
 			else if (db_gain < dbmin) {
-				*value = xdir ? rangemin : prev_rangemax;
+				*value = xdir || pos == 2 ? submin : prev_submax;
 				return 0;
 			}
-			prev_rangemax = rangemax;
+			prev_submax = submax;
+			if (rangemax == submax)
+				break;
 			pos += int_index(tlv[pos + 3]) + 4;
-		}
-		return -EINVAL;
+		} while (pos + 4 <= len);
+		*value = prev_submax;
+		return 0;
 	}
 	case SND_CTL_TLVT_DB_SCALE: {
 		int min, step, max;
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Clemens Ladisch]

Benoît Thébaudeau wrote:
> In case of a TLV dB range with all items having raw value ranges strictly within
> the main raw value range reported by the driver, snd_tlv_convert_from_dB()
> returned one of the main raw range boundaries, which was outside all dB range
> items.

But the main raw range boundaries _are_ valid values.

I guess the problem you actually have is that convert_to_dB does not
work with these raw values?

> static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = {
> 	TLV_DB_RANGE_HEAD(2),
> 	0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1),
> 	48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0),
> };
> 	SOC_SINGLE_TLV("Main Volume",
> 		       BD3753X_VOL_GAIN,
> 		       0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
>
> snd_tlv_convert_from_dB(tlv, 0, 255, 1500, &value, 1) returned 255 instead of
> the expected 142.

But there is no guarantee that this mixer control will have only one of
those values in the TLV's range.  Furthermore, convert_to_dB will still
fail for any raw value 1..47.

This TLV is just incomplete.  Please report this bug to the driver's
author.

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?

> +			long submin, submax;
> +			submin = (int)tlv[pos];
> +			submax = (int)tlv[pos + 1];
> +			if (rangemax < submax)
> +				submax = rangemax;

Why is this check needed?  Any why not return -EINVAL?

>  			if (!snd_tlv_get_dB_range(tlv + pos + 2,
> +						  submin, submax,
>  						  &dbmin, &dbmax) &&
>  			    db_gain >= dbmin && db_gain <= dbmax)
>  				return snd_tlv_convert_from_dB(tlv + pos + 2,
> +							       submin, submax,
>  							       db_gain, value, xdir);
>  			else if (db_gain < dbmin) {
> +				*value = xdir || pos == 2 ? submin : prev_submax;
>  				return 0;
>  			}
> +			prev_submax = submax;
> +			if (rangemax == submax)
> +				break;

Why this check?

>  			pos += int_index(tlv[pos + 3]) + 4;
> +		} while (pos + 4 <= len);
> +		*value = prev_submax;
> +		return 0;
>  	}


Regards,
Clemens
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Clemens Ladisch wrote:
> Benoît Thébaudeau wrote:
> > In case of a TLV dB range with all items having raw value ranges
> > strictly within
> > the main raw value range reported by the driver,
> > snd_tlv_convert_from_dB()
> > returned one of the main raw range boundaries, which was outside
> > all dB range
> > items.
>
> But the main raw range boundaries _are_ valid values.

Not always. For instance, if a codec register has the following volume values:
0x00 to 0x20: prohibited
0x21 to 0x40: some dB values
0x41 to 0x60: prohibited
0x61 to 0xff: some dB values

then the kernel driver will use the existing SOC_SINGLE_TLV macro, which, like
most TLV macros, automatically sets min to 0. The driver then relies on the
associated TLV range description to filter out invalid raw values.

This is not a driver/kernel issue. The TLV already filters out invalid raw
values between range items, like 0x41 to 0x60 in the example above, so there is
no reason why it would not also be the case for raw values before the 1st dB
range and after the last one.

If you wanted to make the global dB range match the global raw range in the
driver, almost all kernel TLV macros would have to have another version taking a
min, which would not really be useful as explained above.

> I guess the problem you actually have is that convert_to_dB does not
> work with these raw values?

No. These raw values are not supposed to be used. They are invalid for the
hardware. The issue is that convert_from_dB may return raw values outside of all
valid dB ranges.

> > static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = {
> > 	TLV_DB_RANGE_HEAD(2),
> > 	0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1),
> > 	48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0),
> > };
> > 	SOC_SINGLE_TLV("Main Volume",
> > 		       BD3753X_VOL_GAIN,
> > 		       0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
> >
> > snd_tlv_convert_from_dB(tlv, 0, 255, 1500, &value, 1) returned 255
> > instead of
> > the expected 142.
>
> But there is no guarantee that this mixer control will have only one
> of
> those values in the TLV's range.

What do you mean? convert_from_dB already relied on the fact that TLV ranges are
sorted in ascending dB order.

> Furthermore, convert_to_dB will
> still
> fail for any raw value 1..47.
>
> This TLV is just incomplete.  Please report this bug to the driver's
> author.

It is complete. See my explanations with the other example given above. There is
no reason for invalid raw values to be able to exist between the dB ranges, and
not before the 1st dB range or after the last one.

> 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.

> > +			long submin, submax;
> > +			submin = (int)tlv[pos];
> > +			submax = (int)tlv[pos + 1];
> > +			if (rangemax < submax)
> > +				submax = rangemax;
>
> Why is this check needed?  Any why not return -EINVAL?

Same as for snd_tlv_get_dB_range: to handle the case where the driver narrows
down for a given control the range of values reported by the TLV.

The ranges accepted must be the intersection of the TLV ranges and of the global
range of raw values for each control. All this issue can be summed up this way.

> >  			if (!snd_tlv_get_dB_range(tlv + pos + 2,
> > +						  submin, submax,
> >  						  &dbmin, &dbmax) &&
> >  			    db_gain >= dbmin && db_gain <= dbmax)
> >  				return snd_tlv_convert_from_dB(tlv + pos + 2,
> > +							       submin, submax,
> >  							       db_gain, value, xdir);
> >  			else if (db_gain < dbmin) {
> > +				*value = xdir || pos == 2 ? submin : prev_submax;
> >  				return 0;
> >  			}
> > +			prev_submax = submax;
> > +			if (rangemax == submax)
> > +				break;
>
> Why this check?

Same as for snd_tlv_get_dB_range: because for the case "if (rangemax < submax)"
above, you have reached the final range item you may be interested in, so it's
pointless to handle the following ones.

> >  			pos += int_index(tlv[pos + 3]) + 4;
> > +		} while (pos + 4 <= len);
> > +		*value = prev_submax;
> > +		return 0;
> >  	}
>
>
> Regards,
> Clemens

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

At Fri, 30 Mar 2012 12:41:44 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> Clemens Ladisch wrote:
> > Benoît Thébaudeau wrote:
> > > In case of a TLV dB range with all items having raw value ranges
> > > strictly within
> > > the main raw value range reported by the driver,
> > > snd_tlv_convert_from_dB()
> > > returned one of the main raw range boundaries, which was outside
> > > all dB range
> > > items.
> >
> > But the main raw range boundaries _are_ valid values.
> 
> Not always. For instance, if a codec register has the following volume values:
> 0x00 to 0x20: prohibited
> 0x21 to 0x40: some dB values
> 0x41 to 0x60: prohibited
> 0x61 to 0xff: some dB values

You can't prohibit the values that are exposed as accessible.
The driver may return an error -EINVAL or such, but it doesn't mean
that it's allowed to give a hole in the TLV dB definition.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

At Fri, 30 Mar 2012 13:44:43 +0200,
Takashi Iwai wrote:
> 
> At Fri, 30 Mar 2012 12:41:44 +0200 (CEST),
> Benoît Thébaudeau wrote:
> > 
> > Clemens Ladisch wrote:
> > > Benoît Thébaudeau wrote:
> > > > In case of a TLV dB range with all items having raw value ranges
> > > > strictly within
> > > > the main raw value range reported by the driver,
> > > > snd_tlv_convert_from_dB()
> > > > returned one of the main raw range boundaries, which was outside
> > > > all dB range
> > > > items.
> > >
> > > But the main raw range boundaries _are_ valid values.
> > 
> > Not always. For instance, if a codec register has the following volume values:
> > 0x00 to 0x20: prohibited
> > 0x21 to 0x40: some dB values
> > 0x41 to 0x60: prohibited
> > 0x61 to 0xff: some dB values
> 
> You can't prohibit the values that are exposed as accessible.
> The driver may return an error -EINVAL or such, but it doesn't mean
> that it's allowed to give a hole in the TLV dB definition.

But, don't get me wrong with the statement above.  Your fix can be an
improvement indeed.  The fact that the function relies on
range_min/max blindly isn't described anywhere, and it'd be better to
parse the range properly, of course, especially it's an exported
function, not only used as an internal function.

There may be minor coding issues as Clemens suggested, but they can be
easily fixed.

So, the motivation here is to make the code more robust and
consistent although the examples you raised seem like rather buggy
driver implementations.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [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:
>>> In case of a TLV dB range with all items having raw value ranges strictly within
>>> the main raw value range reported by the driver, snd_tlv_convert_from_dB()
>>> returned one of the main raw range boundaries, which was outside
>>> all dB range items.
>>
>> But the main raw range boundaries _are_ valid values.
>
> Not always. For instance, if a codec register has the following volume values:
> 0x00 to 0x20: prohibited
> 0x21 to 0x40: some dB values
> 0x41 to 0x60: prohibited
> 0x61 to 0xff: some dB values
>
> then the kernel driver will use the existing SOC_SINGLE_TLV macro, which, like
> most TLV macros, automatically sets min to 0. The driver then relies on the
> associated TLV range description to filter out invalid raw values.

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.

>>> static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = {
>>> 	TLV_DB_RANGE_HEAD(2),
>>> 	0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1),
>>> 	48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0),
>>> };
>>> 	SOC_SINGLE_TLV(... 0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
>>>
>>> snd_tlv_convert_from_dB(tlv, 0, 255, 1500, &value, 1) returned 255
>>> instead of the expected 142.
>>
>> But there is no guarantee that this mixer control will have only one of
>> those values in the TLV's range.
>
> What do you mean?

It _is_ possible to have raw values 1..47 and 143..255 (and
convert_to_dB will fail for those).

>> 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.


Regards,
Clemens
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Takashi Iwai wrote:
> At Fri, 30 Mar 2012 13:44:43 +0200,
> Takashi Iwai wrote:
> >
> > At Fri, 30 Mar 2012 12:41:44 +0200 (CEST),
> > Benoît Thébaudeau wrote:
> > >
> > > Clemens Ladisch wrote:
> > > > Benoît Thébaudeau wrote:
> > > > > In case of a TLV dB range with all items having raw value
> > > > > ranges
> > > > > strictly within
> > > > > the main raw value range reported by the driver,
> > > > > snd_tlv_convert_from_dB()
> > > > > returned one of the main raw range boundaries, which was
> > > > > outside
> > > > > all dB range
> > > > > items.
> > > >
> > > > But the main raw range boundaries _are_ valid values.
> > >
> > > Not always. For instance, if a codec register has the following
> > > volume values:
> > > 0x00 to 0x20: prohibited
> > > 0x21 to 0x40: some dB values
> > > 0x41 to 0x60: prohibited
> > > 0x61 to 0xff: some dB values
> >
> > You can't prohibit the values that are exposed as accessible.
> > The driver may return an error -EINVAL or such, but it doesn't mean
> > that it's allowed to give a hole in the TLV dB definition.
>
> But, don't get me wrong with the statement above.  Your fix can be an
> improvement indeed.  The fact that the function relies on
> range_min/max blindly isn't described anywhere, and it'd be better to
> parse the range properly, of course, especially it's an exported
> function, not only used as an internal function.
>
> There may be minor coding issues as Clemens suggested, but they can
> be
> easily fixed.
>
> So, the motivation here is to make the code more robust and
> consistent

Yes, that's what this patch aims at.

> although the examples you raised seem like rather buggy
> driver implementations.

Why? And you say that there shouldn't be holes in TLV ranges. I don't see why
either. I have not read anywhere that the ALSA APIs forbid this, and by the way
it works. Moreover, the example above with some prohibited ranges would be a
hardware example, not a driver example. It should be possible to handle all
hardware cases.

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Clemens Ladisch wrote:
> Benoît Thébaudeau wrote:
> > Clemens Ladisch wrote:
> >> Benoît Thébaudeau wrote:
> >>> In case of a TLV dB range with all items having raw value ranges
> >>> strictly within
> >>> the main raw value range reported by the driver,
> >>> snd_tlv_convert_from_dB()
> >>> returned one of the main raw range boundaries, which was outside
> >>> all dB range items.
> >>
> >> But the main raw range boundaries _are_ valid values.
> >
> > Not always. For instance, if a codec register has the following
> > volume values:
> > 0x00 to 0x20: prohibited
> > 0x21 to 0x40: some dB values
> > 0x41 to 0x60: prohibited
> > 0x61 to 0xff: some dB values
> >
> > then the kernel driver will use the existing SOC_SINGLE_TLV macro,
> > which, like
> > most TLV macros, automatically sets min to 0. The driver then
> > relies on the
> > associated TLV range description to filter out invalid raw values.
>
> 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, known by everybody, that may cause issues if not applied?

Here is my point of view. Let's take a real example, which corresponds to the
volume table of the ROHM BD37534 on page 26:
http://www.rohm.com/products/databook/audio/pdf/bd37531fv-e.pdf

This is what gave the 1st example of my patch comment:
static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = {
	TLV_DB_RANGE_HEAD(2),
	0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1),
	48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0),
};
	SOC_SINGLE_TLV("Main Volume",
		       BD3753X_VOL_GAIN,
		       0, 255, 1, bd3753x_vol_fader_gain_att_tlv),

This solution is the only possible one using existing ALSA SOC TLV macros. This
indeed breaks the rule that you gave above, but is it really an issue? See
below.

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?

Do you think that it is not a so common case, and hence that it does not deserve
to be handled by the general macros?

It is also the case for the TLV320AIC3104, page 0/register 19/field D6-D3 on
page 51:
http://www.ti.com/lit/ds/symlink/tlv320aic3104.pdf

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?


> >> 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, 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.

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Benoît Thébaudeau wrote:
> 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

It would rather be:
 SW 0x00: HW 0xff: mute
 SW 0x01 to 0x5f: HW 0xcf to 0x71: -79 dB to +15 dB by +1 dB steps

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

At Fri, 30 Mar 2012 15:07:27 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> Takashi Iwai wrote:
> > At Fri, 30 Mar 2012 13:44:43 +0200,
> > Takashi Iwai wrote:
> > >
> > > At Fri, 30 Mar 2012 12:41:44 +0200 (CEST),
> > > Benoît Thébaudeau wrote:
> > > >
> > > > Clemens Ladisch wrote:
> > > > > Benoît Thébaudeau wrote:
> > > > > > In case of a TLV dB range with all items having raw value
> > > > > > ranges
> > > > > > strictly within
> > > > > > the main raw value range reported by the driver,
> > > > > > snd_tlv_convert_from_dB()
> > > > > > returned one of the main raw range boundaries, which was
> > > > > > outside
> > > > > > all dB range
> > > > > > items.
> > > > >
> > > > > But the main raw range boundaries _are_ valid values.
> > > >
> > > > Not always. For instance, if a codec register has the following
> > > > volume values:
> > > > 0x00 to 0x20: prohibited
> > > > 0x21 to 0x40: some dB values
> > > > 0x41 to 0x60: prohibited
> > > > 0x61 to 0xff: some dB values
> > >
> > > You can't prohibit the values that are exposed as accessible.
> > > The driver may return an error -EINVAL or such, but it doesn't mean
> > > that it's allowed to give a hole in the TLV dB definition.
> >
> > But, don't get me wrong with the statement above.  Your fix can be an
> > improvement indeed.  The fact that the function relies on
> > range_min/max blindly isn't described anywhere, and it'd be better to
> > parse the range properly, of course, especially it's an exported
> > function, not only used as an internal function.
> >
> > There may be minor coding issues as Clemens suggested, but they can
> > be
> > easily fixed.
> >
> > So, the motivation here is to make the code more robust and
> > consistent
> 
> Yes, that's what this patch aims at.
> 
> > although the examples you raised seem like rather buggy
> > driver implementations.
> 
> Why? And you say that there shouldn't be holes in TLV ranges. I don't see why
> either.

Because holes have been never defined to be allowed.  Rather, in the
API level, all values are supposed to be accessible.

It's possible that a driver temporarily returns an error when
accessing some value.  But, it's supposed to be an temporay error.
Some drivers may abuse this to implement a hole.

> I have not read anywhere that the ALSA APIs forbid this,

"Not forbidden" doesn't mean it's always allowed and recommended :)

> and by the way
> it works. Moreover, the example above with some prohibited ranges would be a
> hardware example, not a driver example. It should be possible to handle all
> hardware cases.

The ALSA control values don't have to be exactly as same as the
hardware raw values at all.  Rather they should be converted to be
human-friendly form.  If there is a hole, it should be omitted and
converted in the driver side to be continous.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Clemens Ladisch wrote:
> I'll see if I can cobble together some TLV checking code ...

Great.

> > Because it's useless to wait until the end of the loop to return
> > -EINVAL,
> 
> It is not necessary to optimize for error cases.

It's not an optimization, but a simplification. Otherwise, pos would have to be
tested again after the loop to differentiate the loop-skipped case from the
at-least-one-loop-iteration cases to return either -EINVAL or the prev submax.

> > 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?

It doesn't. It's just that it's useless to keep a while here as the 1st
iteration will necessarily occur thanks to the previous if.

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Benoît Thébaudeau wrote:
> Clemens Ladisch wrote:
> > I'll see if I can cobble together some TLV checking code ...
> 
> Great.

In all the hardware examples that I've found, the worst case for holes is
always, like for the BD37534:
mute
hole
valid dB range
hole

So it's perhaps not worth developing something very complicated in the kernel
to handle all possible cases of TLV range holes. I think that most real-life
cases will be handled if we consider a min-max raw HW range combined with a mute
value automatically set at the extreme value of the bit-field, with an optional
inversion. Something like:

#define SOC_SINGLE_MUTABLE_TLV(xname, reg, shift, min, max, invert, tlv_array)

In the BD driver example that I gave:
static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = {
	TLV_DB_RANGE_HEAD(2),
	0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1),
	48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0),
};
	SOC_SINGLE_TLV("Main Volume",
		       BD3753X_VOL_GAIN,
		       0, 255, 1, bd3753x_vol_fader_gain_att_tlv),

would become:
static const DECLARE_TLV_DB_SCALE(bd3753x_vol_fader_gain_att_tlv, -8000, 100, 1);
	SOC_SINGLE_MUTABLE_TLV("Main Volume",
			       BD3753X_VOL_GAIN,
			       0, 0x71, 0xcf, 1, bd3753x_vol_fader_gain_att_tlv),

The range exposed by .info would be 0 to 0x5f, 0 being the mute HW value 0xff,
and 0x01 to 0x5f matching the HW 0xcf to 0x71.

The SOC_*_MUTABLE_TLV set of macros would use dedicated .info/.get/.put
callbacks. They could theoretically be merged into the snd_soc_*_volsw callbacks
if a new field were added to struct soc_mixer_control to indicate this use case.
Or the invert field could be replaced with a set of option flags.

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

At Fri, 30 Mar 2012 15:58:51 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> Clemens Ladisch wrote:
> > I'll see if I can cobble together some TLV checking code ...
> 
> Great.
> 
> > > Because it's useless to wait until the end of the loop to return
> > > -EINVAL,
> > 
> > It is not necessary to optimize for error cases.
> 
> It's not an optimization, but a simplification. Otherwise, pos would have to be
> tested again after the loop to differentiate the loop-skipped case from the
> at-least-one-loop-iteration cases to return either -EINVAL or the prev submax.

Unless it's really a hot path, such a micro-optimization doesn't give
much value.

Rather better to keep the standard style that makes people easier to
read the code.  A do-while loop with a large block is usually less
readable than a normal while loop.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

At Fri, 30 Mar 2012 17:20:21 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> Benoît Thébaudeau wrote:
> > Clemens Ladisch wrote:
> > > I'll see if I can cobble together some TLV checking code ...
> > 
> > Great.
> 
> In all the hardware examples that I've found, the worst case for holes is
> always, like for the BD37534:
> mute
> hole
> valid dB range
> hole
> 
> So it's perhaps not worth developing something very complicated in the kernel
> to handle all possible cases of TLV range holes.

Well, it's not only about the TLV dB information.  The user-space has
no idea where holes exist, and we have no way to expose this
information, so far.  That is, you can access to some random raw
values and now you'll get always errors.  This is obviously a bug of
the driver.

So, please don't take it as an example to _fix_ the alsa-lib code.
Making it working for such a buggy driver isn't called as a fix.
It's called as a workaround.  And, a workaround is usually ugly.

However, beyond that, there is a good advantage in your change; it
can make the code more robust.  We should concentrate on that point.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Takashi Iwai wrote:
> Benoît Thébaudeau wrote:
> > 
> > Benoît Thébaudeau wrote:
> > > Clemens Ladisch wrote:
> > > > I'll see if I can cobble together some TLV checking code ...
> > > 
> > > Great.
> > 
> > In all the hardware examples that I've found, the worst case for
> > holes is
> > always, like for the BD37534:
> > mute
> > hole
> > valid dB range
> > hole
> > 
> > So it's perhaps not worth developing something very complicated in
> > the kernel
> > to handle all possible cases of TLV range holes.
> 
> Well, it's not only about the TLV dB information.  The user-space has
> no idea where holes exist, and we have no way to expose this
> information, so far.  That is, you can access to some random raw
> values and now you'll get always errors.  This is obviously a bug of
> the driver.
> 
> So, please don't take it as an example to _fix_ the alsa-lib code.
> Making it working for such a buggy driver isn't called as a fix.
> It's called as a workaround.  And, a workaround is usually ugly.
> 
> However, beyond that, there is a good advantage in your change; it
> can make the code more robust.  We should concentrate on that point.

Yes. This is not what I meant here. Clemens seemed to be about to propose
a generic solution in the kernel to handle hardware volume range holes so as to
remove them when exposed to the user space. What I meant in my previous message
was that his solution would probably be sufficient if it only handled one hole
between the mute value and the other values, and another hole after the valid
values. That would ease the development of drivers for these cases, and thus
avoid buggy drivers in the first place.

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Takashi Iwai wrote:
> Benoît Thébaudeau wrote:
> >
> > Clemens Ladisch wrote:
> > > I'll see if I can cobble together some TLV checking code ...
> >
> > Great.
> >
> > > > Because it's useless to wait until the end of the loop to
> > > > return
> > > > -EINVAL,
> > >
> > > It is not necessary to optimize for error cases.
> >
> > It's not an optimization, but a simplification. Otherwise, pos
> > would have to be
> > tested again after the loop to differentiate the loop-skipped case
> > from the
> > at-least-one-loop-iteration cases to return either -EINVAL or the
> > prev submax.
>
> Unless it's really a hot path, such a micro-optimization doesn't give
> much value.
>
> Rather better to keep the standard style that makes people easier to
> read the code.  A do-while loop with a large block is usually less
> readable than a normal while loop.

Then, the patch becomes:

--- 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_rangemax;
+		long dbmin, dbmax, prev_submax;
 		unsigned int pos, len;
 		len = int_index(tlv[1]);
-		if (len > MAX_TLV_RANGE_SIZE)
-			return -EINVAL;
-		if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
-					 &dbmin, &dbmax))
+		if (len < 6 || len > MAX_TLV_RANGE_SIZE)
 			return -EINVAL;
-		if (db_gain <= dbmin) {
-			*value = rangemin;
-			return 0;
-		} else if (db_gain >= dbmax) {
-			*value = rangemax;
-			return 0;
-		}
 		pos = 2;
-		prev_rangemax = 0;
+		prev_submax = 0;
 		while (pos + 4 <= len) {
-			rangemin = (int)tlv[pos];
-			rangemax = (int)tlv[pos + 1];
+			long submin, submax;
+			submin = (int)tlv[pos];
+			submax = (int)tlv[pos + 1];
+			if (rangemax < submax)
+				submax = rangemax;
 			if (!snd_tlv_get_dB_range(tlv + pos + 2,
-						  rangemin, rangemax,
+						  submin, submax,
 						  &dbmin, &dbmax) &&
 			    db_gain >= dbmin && db_gain <= dbmax)
 				return snd_tlv_convert_from_dB(tlv + pos + 2,
-							       rangemin, rangemax,
+							       submin, submax,
 							       db_gain, value, xdir);
 			else if (db_gain < dbmin) {
-				*value = xdir ? rangemin : prev_rangemax;
+				*value = xdir || pos == 2 ? submin : prev_submax;
 				return 0;
 			}
-			prev_rangemax = rangemax;
+			prev_submax = submax;
+			if (rangemax == submax)
+				break;
 			pos += int_index(tlv[pos + 3]) + 4;
 		}
-		return -EINVAL;
+		*value = prev_submax;
+		return 0;
 	}
 	case SND_CTL_TLVT_DB_SCALE: {
 		int min, step, max;
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Hi Takashi, Clemens, all,

Takashi Iwai wrote:
> Benoît Thébaudeau wrote:
> > 
> > Clemens Ladisch wrote:
> > > I'll see if I can cobble together some TLV checking code ...
> > 
> > Great.
> > 
> > > > Because it's useless to wait until the end of the loop to
> > > > return
> > > > -EINVAL,
> > > 
> > > It is not necessary to optimize for error cases.
> > 
> > It's not an optimization, but a simplification. Otherwise, pos
> > would have to be
> > tested again after the loop to differentiate the loop-skipped case
> > from the
> > at-least-one-loop-iteration cases to return either -EINVAL or the
> > prev submax.
> 
> Unless it's really a hot path, such a micro-optimization doesn't give
> much value.
> 
> Rather better to keep the standard style that makes people easier to
> read the code.  A do-while loop with a large block is usually less
> readable than a normal while loop.

Would you agree with the following revision?

[PATCH] alsa-lib/tlv: improve robustness of raw value ranges

snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is
exported, it is better for robustness and consistency to parse the range
properly, which this patch does.

Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>

--- 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_rangemax;
+                long dbmin, dbmax, prev_submax;
                 unsigned int pos, len;
                 len = int_index(tlv[1]);
-                if (len > MAX_TLV_RANGE_SIZE)
-                        return -EINVAL;
-                if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
-                                         &dbmin, &dbmax))
+                if (len < 6 || len > MAX_TLV_RANGE_SIZE)
                         return -EINVAL;
-                if (db_gain <= dbmin) {
-                        *value = rangemin;
-                        return 0;
-                } else if (db_gain >= dbmax) {
-                        *value = rangemax;
-                        return 0;
-                }
                 pos = 2;
-                prev_rangemax = 0;
+                prev_submax = 0;
                 while (pos + 4 <= len) {
-                        rangemin = (int)tlv[pos];
-                        rangemax = (int)tlv[pos + 1];
+                        long submin, submax;
+                        submin = (int)tlv[pos];
+                        submax = (int)tlv[pos + 1];
+                        if (rangemax < submax)
+                                submax = rangemax;
                         if (!snd_tlv_get_dB_range(tlv + pos + 2,
-                                                  rangemin, rangemax,
+                                                  submin, submax,
                                                   &dbmin, &dbmax) &&
                             db_gain >= dbmin && db_gain <= dbmax)
                                 return snd_tlv_convert_from_dB(tlv + pos + 2,
-                                                               rangemin, rangemax,
+                                                               submin, submax,
                                                                db_gain, value, xdir);
                         else if (db_gain < dbmin) {
-                                *value = xdir ? rangemin : prev_rangemax;
+                                *value = xdir || pos == 2 ? submin : prev_submax;
                                 return 0;
                         }
-                        prev_rangemax = rangemax;
+                        prev_submax = submax;
+                        if (rangemax == submax)
+                                break;
                         pos += int_index(tlv[pos + 3]) + 4;
                 }
-                return -EINVAL;
+                *value = prev_submax;
+                return 0;
         }
         case SND_CTL_TLVT_DB_SCALE: {
                 int min, step, max;
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

At Mon, 21 May 2012 21:38:22 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> Hi Takashi, Clemens, all,
> 
> Takashi Iwai wrote:
> > Benoît Thébaudeau wrote:
> > > 
> > > Clemens Ladisch wrote:
> > > > I'll see if I can cobble together some TLV checking code ...
> > > 
> > > Great.
> > > 
> > > > > Because it's useless to wait until the end of the loop to
> > > > > return
> > > > > -EINVAL,
> > > > 
> > > > It is not necessary to optimize for error cases.
> > > 
> > > It's not an optimization, but a simplification. Otherwise, pos
> > > would have to be
> > > tested again after the loop to differentiate the loop-skipped case
> > > from the
> > > at-least-one-loop-iteration cases to return either -EINVAL or the
> > > prev submax.
> > 
> > Unless it's really a hot path, such a micro-optimization doesn't give
> > much value.
> > 
> > Rather better to keep the standard style that makes people easier to
> > read the code.  A do-while loop with a large block is usually less
> > readable than a normal while loop.
> 
> Would you agree with the following revision?

Yes, the patch looks good to me.
But I couldn't apply it because the mail is encoded in
quoted-printable.  Could you fix it, or if it's difficult, give an
attachment?


thanks,

Takashi

> [PATCH] alsa-lib/tlv: improve robustness of raw value ranges
> 
> snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is
> exported, it is better for robustness and consistency to parse the range
> properly, which this patch does.
> 
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> 
> --- 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_rangemax;
> +                long dbmin, dbmax, prev_submax;
>                  unsigned int pos, len;
>                  len = int_index(tlv[1]);
> -                if (len > MAX_TLV_RANGE_SIZE)
> -                        return -EINVAL;
> -                if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
> -                                         &dbmin, &dbmax))
> +                if (len < 6 || len > MAX_TLV_RANGE_SIZE)
>                          return -EINVAL;
> -                if (db_gain <= dbmin) {
> -                        *value = rangemin;
> -                        return 0;
> -                } else if (db_gain >= dbmax) {
> -                        *value = rangemax;
> -                        return 0;
> -                }
>                  pos = 2;
> -                prev_rangemax = 0;
> +                prev_submax = 0;
>                  while (pos + 4 <= len) {
> -                        rangemin = (int)tlv[pos];
> -                        rangemax = (int)tlv[pos + 1];
> +                        long submin, submax;
> +                        submin = (int)tlv[pos];
> +                        submax = (int)tlv[pos + 1];
> +                        if (rangemax < submax)
> +                                submax = rangemax;
>                          if (!snd_tlv_get_dB_range(tlv + pos + 2,
> -                                                  rangemin, rangemax,
> +                                                  submin, submax,
>                                                    &dbmin, &dbmax) &&
>                              db_gain >= dbmin && db_gain <= dbmax)
>                                  return snd_tlv_convert_from_dB(tlv + pos + 2,
> -                                                               rangemin, rangemax,
> +                                                               submin, submax,
>                                                                 db_gain, value, xdir);
>                          else if (db_gain < dbmin) {
> -                                *value = xdir ? rangemin : prev_rangemax;
> +                                *value = xdir || pos == 2 ? submin : prev_submax;
>                                  return 0;
>                          }
> -                        prev_rangemax = rangemax;
> +                        prev_submax = submax;
> +                        if (rangemax == submax)
> +                                break;
>                          pos += int_index(tlv[pos + 3]) + 4;
>                  }
> -                return -EINVAL;
> +                *value = prev_submax;
> +                return 0;
>          }
>          case SND_CTL_TLVT_DB_SCALE: {
>                  int min, step, max;
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

[-- Attachment #1: Type: text/plain, Size: 345 bytes --]

Takashi Iwai wrote:
> Yes, the patch looks good to me.
> But I couldn't apply it because the mail is encoded in
> quoted-printable.  Could you fix it, or if it's difficult, give an
> attachment?

Sorry about that. I can't avoid that with my current mailer, so I attach the
patch. Tell me if it's OK for you like this.

Regards,
Benoît

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: improve-tlv-raw-val-robustness.patch --]
[-- Type: text/x-patch; name=improve-tlv-raw-val-robustness.patch, Size: 3160 bytes --]

alsa-lib/tlv: improve robustness of raw value ranges

snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is
exported, it is better for robustness and consistency to parse the range
properly, which this patch does.

Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>

--- 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_rangemax;
+                long dbmin, dbmax, prev_submax;
                 unsigned int pos, len;
                 len = int_index(tlv[1]);
-                if (len > MAX_TLV_RANGE_SIZE)
-                        return -EINVAL;
-                if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
-                                         &dbmin, &dbmax))
+                if (len < 6 || len > MAX_TLV_RANGE_SIZE)
                         return -EINVAL;
-                if (db_gain <= dbmin) {
-                        *value = rangemin;
-                        return 0;
-                } else if (db_gain >= dbmax) {
-                        *value = rangemax;
-                        return 0;
-                }
                 pos = 2;
-                prev_rangemax = 0;
+                prev_submax = 0;
                 while (pos + 4 <= len) {
-                        rangemin = (int)tlv[pos];
-                        rangemax = (int)tlv[pos + 1];
+                        long submin, submax;
+                        submin = (int)tlv[pos];
+                        submax = (int)tlv[pos + 1];
+                        if (rangemax < submax)
+                                submax = rangemax;
                         if (!snd_tlv_get_dB_range(tlv + pos + 2,
-                                                  rangemin, rangemax,
+                                                  submin, submax,
                                                   &dbmin, &dbmax) &&
                             db_gain >= dbmin && db_gain <= dbmax)
                                 return snd_tlv_convert_from_dB(tlv + pos + 2,
-                                                               rangemin, rangemax,
+                                                               submin, submax,
                                                                db_gain, value, xdir);
                         else if (db_gain < dbmin) {
-                                *value = xdir ? rangemin : prev_rangemax;
+                                *value = xdir || pos == 2 ? submin : prev_submax;
                                 return 0;
                         }
-                        prev_rangemax = rangemax;
+                        prev_submax = submax;
+                        if (rangemax == submax)
+                                break;
                         pos += int_index(tlv[pos + 3]) + 4;
                 }
-                return -EINVAL;
+                *value = prev_submax;
+                return 0;
         }
         case SND_CTL_TLVT_DB_SCALE: {
                 int min, step, max;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

At Tue, 22 May 2012 04:19:12 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> Takashi Iwai wrote:
> > Yes, the patch looks good to me.
> > But I couldn't apply it because the mail is encoded in
> > quoted-printable.  Could you fix it, or if it's difficult, give an
> > attachment?
> 
> Sorry about that. I can't avoid that with my current mailer, so I attach the
> patch. Tell me if it's OK for you like this.

It's OK to put an attachment, but I failed to apply it.
Please check the patch you sent can be really applied cleanly to
alsa-lib git tree.


thanks,

Takashi

> 
> Regards,
> Benoît
> [2 improve-tlv-raw-val-robustness.patch <text/x-patch (base64)>]
> alsa-lib/tlv: improve robustness of raw value ranges
> 
> snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is
> exported, it is better for robustness and consistency to parse the range
> properly, which this patch does.
> 
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> 
> --- 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_rangemax;
> +                long dbmin, dbmax, prev_submax;
>                  unsigned int pos, len;
>                  len = int_index(tlv[1]);
> -                if (len > MAX_TLV_RANGE_SIZE)
> -                        return -EINVAL;
> -                if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
> -                                         &dbmin, &dbmax))
> +                if (len < 6 || len > MAX_TLV_RANGE_SIZE)
>                          return -EINVAL;
> -                if (db_gain <= dbmin) {
> -                        *value = rangemin;
> -                        return 0;
> -                } else if (db_gain >= dbmax) {
> -                        *value = rangemax;
> -                        return 0;
> -                }
>                  pos = 2;
> -                prev_rangemax = 0;
> +                prev_submax = 0;
>                  while (pos + 4 <= len) {
> -                        rangemin = (int)tlv[pos];
> -                        rangemax = (int)tlv[pos + 1];
> +                        long submin, submax;
> +                        submin = (int)tlv[pos];
> +                        submax = (int)tlv[pos + 1];
> +                        if (rangemax < submax)
> +                                submax = rangemax;
>                          if (!snd_tlv_get_dB_range(tlv + pos + 2,
> -                                                  rangemin, rangemax,
> +                                                  submin, submax,
>                                                    &dbmin, &dbmax) &&
>                              db_gain >= dbmin && db_gain <= dbmax)
>                                  return snd_tlv_convert_from_dB(tlv + pos + 2,
> -                                                               rangemin, rangemax,
> +                                                               submin, submax,
>                                                                 db_gain, value, xdir);
>                          else if (db_gain < dbmin) {
> -                                *value = xdir ? rangemin : prev_rangemax;
> +                                *value = xdir || pos == 2 ? submin : prev_submax;
>                                  return 0;
>                          }
> -                        prev_rangemax = rangemax;
> +                        prev_submax = submax;
> +                        if (rangemax == submax)
> +                                break;
>                          pos += int_index(tlv[pos + 3]) + 4;
>                  }
> -                return -EINVAL;
> +                *value = prev_submax;
> +                return 0;
>          }
>          case SND_CTL_TLVT_DB_SCALE: {
>                  int min, step, max;
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]

Hi Takashi,

Takashi Iwai wrote:
> It's OK to put an attachment, but I failed to apply it.
> Please check the patch you sent can be really applied cleanly to
> alsa-lib git tree.

My copy/paste damaged tabs. Hopefully, this should now work.

Regards,
Benoît

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: improve-tlv-raw-val-robustness.patch --]
[-- Type: text/x-patch; name=improve-tlv-raw-val-robustness.patch, Size: 1971 bytes --]

alsa-lib/tlv: improve robustness of raw value ranges

snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is
exported, it is better for robustness and consistency to parse the range
properly, which this patch does.

Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>

--- 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_rangemax;
+		long dbmin, dbmax, prev_submax;
 		unsigned int pos, len;
 		len = int_index(tlv[1]);
-		if (len > MAX_TLV_RANGE_SIZE)
+		if (len < 6 || len > MAX_TLV_RANGE_SIZE)
 			return -EINVAL;
-		if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
-					 &dbmin, &dbmax))
-			return -EINVAL;
-		if (db_gain <= dbmin) {
-			*value = rangemin;
-			return 0;
-		} else if (db_gain >= dbmax) {
-			*value = rangemax;
-			return 0;
-		}
 		pos = 2;
-		prev_rangemax = 0;
+		prev_submax = 0;
 		while (pos + 4 <= len) {
-			rangemin = (int)tlv[pos];
-			rangemax = (int)tlv[pos + 1];
+			long submin, submax;
+			submin = (int)tlv[pos];
+			submax = (int)tlv[pos + 1];
+			if (rangemax < submax)
+				submax = rangemax;
 			if (!snd_tlv_get_dB_range(tlv + pos + 2,
-						  rangemin, rangemax,
+						  submin, submax,
 						  &dbmin, &dbmax) &&
 			    db_gain >= dbmin && db_gain <= dbmax)
 				return snd_tlv_convert_from_dB(tlv + pos + 2,
-							       rangemin, rangemax,
+							       submin, submax,
 							       db_gain, value, xdir);
 			else if (db_gain < dbmin) {
-				*value = xdir ? rangemin : prev_rangemax;
+				*value = xdir || pos == 2 ? submin : prev_submax;
 				return 0;
 			}
-			prev_rangemax = rangemax;
+			prev_submax = submax;
+			if (rangemax == submax)
+				break;
 			pos += int_index(tlv[pos + 3]) + 4;
 		}
-		return -EINVAL;
+		*value = prev_submax;
+		return 0;
 	}
 	case SND_CTL_TLVT_DB_SCALE: {
 		int min, step, max;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

At Tue, 22 May 2012 18:06:34 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> Hi Takashi,
> 
> Takashi Iwai wrote:
> > It's OK to put an attachment, but I failed to apply it.
> > Please check the patch you sent can be really applied cleanly to
> > alsa-lib git tree.
> 
> My copy/paste damaged tabs. Hopefully, this should now work.

Thanks, applied now.


Takashi

> 
> Regards,
> Benoît
> [2 improve-tlv-raw-val-robustness.patch <text/x-patch (base64)>]
> alsa-lib/tlv: improve robustness of raw value ranges
> 
> snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is
> exported, it is better for robustness and consistency to parse the range
> properly, which this patch does.
> 
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> 
> --- 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_rangemax;
> +		long dbmin, dbmax, prev_submax;
>  		unsigned int pos, len;
>  		len = int_index(tlv[1]);
> -		if (len > MAX_TLV_RANGE_SIZE)
> +		if (len < 6 || len > MAX_TLV_RANGE_SIZE)
>  			return -EINVAL;
> -		if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
> -					 &dbmin, &dbmax))
> -			return -EINVAL;
> -		if (db_gain <= dbmin) {
> -			*value = rangemin;
> -			return 0;
> -		} else if (db_gain >= dbmax) {
> -			*value = rangemax;
> -			return 0;
> -		}
>  		pos = 2;
> -		prev_rangemax = 0;
> +		prev_submax = 0;
>  		while (pos + 4 <= len) {
> -			rangemin = (int)tlv[pos];
> -			rangemax = (int)tlv[pos + 1];
> +			long submin, submax;
> +			submin = (int)tlv[pos];
> +			submax = (int)tlv[pos + 1];
> +			if (rangemax < submax)
> +				submax = rangemax;
>  			if (!snd_tlv_get_dB_range(tlv + pos + 2,
> -						  rangemin, rangemax,
> +						  submin, submax,
>  						  &dbmin, &dbmax) &&
>  			    db_gain >= dbmin && db_gain <= dbmax)
>  				return snd_tlv_convert_from_dB(tlv + pos + 2,
> -							       rangemin, rangemax,
> +							       submin, submax,
>  							       db_gain, value, xdir);
>  			else if (db_gain < dbmin) {
> -				*value = xdir ? rangemin : prev_rangemax;
> +				*value = xdir || pos == 2 ? submin : prev_submax;
>  				return 0;
>  			}
> -			prev_rangemax = rangemax;
> +			prev_submax = submax;
> +			if (rangemax == submax)
> +				break;
>  			pos += int_index(tlv[pos + 3]) + 4;
>  		}
> -		return -EINVAL;
> +		*value = prev_submax;
> +		return 0;
>  	}
>  	case SND_CTL_TLVT_DB_SCALE: {
>  		int min, step, max;
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[James Courtier-Dutton]

On 30 March 2012 11:41, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:
> Clemens Ladisch wrote:
>> Benoît Thébaudeau wrote:
>> > In case of a TLV dB range with all items having raw value ranges
>> > strictly within
>> > the main raw value range reported by the driver,
>> > snd_tlv_convert_from_dB()
>> > returned one of the main raw range boundaries, which was outside
>> > all dB range
>> > items.
>>
>> But the main raw range boundaries _are_ valid values.
>
> Not always. For instance, if a codec register has the following volume values:
> 0x00 to 0x20: prohibited
> 0x21 to 0x40: some dB values
> 0x41 to 0x60: prohibited
> 0x61 to 0xff: some dB values
>

In the userspace to kernel interface. A mixer control must have min/max values.
When writing the value, all values within the range of min to max must
be valid values.
There are just too many user land tools that rely on this fact.
Think of the use case or "Next step louder please". If the currently
written value is 0x40, the current user land mixer apps would try to
write 0x41, without even bothering to look at the dB tables.
If you instead change the driver to map:
0x21 to 0x40 -> 0x00 to 0x1f
0x61 to 0xff -> 0x20 to 0xBE
and set min to 0, and max to 0xbe
The user space tools will then work fine.

The dB conversion tables are just a friendly way of displaying a
slightly more useful figure to the user than a percentage.
If you have hardware with gaps like the above, the kernel driver will
have to be fixed to remove the prohibited ranges in the kernel to
userspace API.
This sort of conversion is done in many other alsa kernel drivers.

Kind Regards

James
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Benoît Thébaudeau]

Hi James,

On 2 June 2012 10:09, James Courtier-Dutton <james.dutton@gmail.com> wrote:
> In the userspace to kernel interface. A mixer control must have
> min/max values.
> When writing the value, all values within the range of min to max
> must
> be valid values.
> There are just too many user land tools that rely on this fact.
> Think of the use case or "Next step louder please". If the currently
> written value is 0x40, the current user land mixer apps would try to
> write 0x41, without even bothering to look at the dB tables.
> If you instead change the driver to map:
> 0x21 to 0x40 -> 0x00 to 0x1f
> 0x61 to 0xff -> 0x20 to 0xBE
> and set min to 0, and max to 0xbe
> The user space tools will then work fine.
> 
> The dB conversion tables are just a friendly way of displaying a
> slightly more useful figure to the user than a percentage.
> If you have hardware with gaps like the above, the kernel driver will
> have to be fixed to remove the prohibited ranges in the kernel to
> userspace API.
> This sort of conversion is done in many other alsa kernel drivers.

Thanks for the details. I'll do that.

Regards,
Benoît
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel