[PATCH] pcm: softvol: fix signedness of tlv data type

[PATCH] pcm: softvol: fix signedness of tlv data type

[Jörg Krause]

For the softvol plugin the TLV data are written wrong, e.g. for the default values
of min_dB = -51 dB and max_dB = 0 dB, alsactl generates the following state:

	control.1 {
                iface MIXER
                name Master
                value.0 255
                value.1 255
                comment {
                        access 'read write user'
                        type INTEGER
                        count 2
                        range '0 - 255'
                        tlv '00000001000000080000000000000014'
                        dbmin 0
                        dbmax 5100
                        dbvalue.0 5100
                        dbvalue.1 5100
                }
        }

As both min_dB and max_dB can be negative numbers, the tlv type must not be
unsigned. This fixes the TLV and the correct state is generated:

	control.1 {
                iface MIXER
                name Master
                value.0 255
                value.1 255
                comment {
                        access 'read write user'
                        type INTEGER
                        count 2
                        range '0 - 255'
                        tlv '0000000100000008ffffec1400000014'
                        dbmin -5100
                        dbmax 0
                        dbvalue.0 0
                        dbvalue.1 0
                }
        }

Also tested for different combinations of min_dB and max_dB other than the
default values.

Signed-off-by: Jörg Krause <joerg.krause@embedded.rocks>
---
 src/pcm/pcm_softvol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pcm/pcm_softvol.c b/src/pcm/pcm_softvol.c
index 802aa4b..1748667 100644
--- a/src/pcm/pcm_softvol.c
+++ b/src/pcm/pcm_softvol.c
@@ -655,7 +655,7 @@ static void snd_pcm_softvol_dump(snd_pcm_t *pcm, snd_output_t *out)
 
 static int add_tlv_info(snd_pcm_softvol_t *svol, snd_ctl_elem_info_t *cinfo)
 {
-	unsigned int tlv[4];
+	int tlv[4];
 	tlv[0] = SND_CTL_TLVT_DB_SCALE;
 	tlv[1] = 2 * sizeof(int);
 	tlv[2] = svol->min_dB * 100;
@@ -768,7 +768,7 @@ static int softvol_load_control(snd_pcm_t *pcm, snd_pcm_softvol_t *svol,
 			}
 		} else if (svol->max_val > 1) {
 			/* check TLV availability */
-			unsigned int tlv[4];
+			int tlv[4];
 			err = snd_ctl_elem_tlv_read(svol->ctl, &cinfo->id, tlv, sizeof(tlv));
 			if (err < 0)
 				add_tlv_info(svol, cinfo);
-- 
2.8.2

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

Re: [PATCH] pcm: softvol: fix signedness of tlv data type

[Clemens Ladisch]

Jörg Krause wrote:
> For the softvol plugin the TLV data are written wrong, e.g. for the default values
> of min_dB = -51 dB and max_dB = 0 dB, alsactl generates the following state:
>
>                         tlv '00000001000000080000000000000014'
>
> As both min_dB and max_dB can be negative numbers, the tlv type must not be
> unsigned.

TLVs always are unsigned.  But they can contain bit patterns that are to
be interpreted as signed.

> -	unsigned int tlv[4];
> +	int tlv[4];
>  	tlv[0] = SND_CTL_TLVT_DB_SCALE;
>  	tlv[1] = 2 * sizeof(int);
>  	tlv[2] = svol->min_dB * 100;

The problem is that conversion of a negative floating-point number into
an unsigned integer results in undefined behaviour, and on your
architecture (whatever it is), this results in zero.

The correct way to handle this would be something like this:

  tlv[i] = (int)(...);


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

Re: [PATCH] pcm: softvol: fix signedness of tlv data type

[Jörg Krause]

Hello,

On Sa, 2016-05-07 at 09:06 +0200, Clemens Ladisch wrote:
> Jörg Krause wrote:
> > For the softvol plugin the TLV data are written wrong, e.g. for the
> > default values
> > of min_dB = -51 dB and max_dB = 0 dB, alsactl generates the
> > following state:
> > 
> >                         tlv '00000001000000080000000000000014'
> > 
> > As both min_dB and max_dB can be negative numbers, the tlv type
> > must not be
> > unsigned.
> 
> TLVs always are unsigned.  But they can contain bit patterns that are
> to
> be interpreted as signed.

I see!

> > -	unsigned int tlv[4];
> > +	int tlv[4];
> >  	tlv[0] = SND_CTL_TLVT_DB_SCALE;
> >  	tlv[1] = 2 * sizeof(int);
> >  	tlv[2] = svol->min_dB * 100;
> 
> The problem is that conversion of a negative floating-point number
> into
> an unsigned integer results in undefined behaviour, and on your
> architecture (whatever it is), this results in zero.
> 
> The correct way to handle this would be something like this:
> 
>   tlv[i] = (int)(...);

This fixes the issue for my ARM target, many thanks! I'll send an
updated version.

Many thanks for the review!

Best regards,
Jörg Krause
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel