2007-05-30 07:37:47

by Bill Nottingham

[permalink] [raw]
Subject: [PATCH] alsa: fix comparison of unsigned < 0

Recent gccs emit warnings when unsigned variables are compared < 0 or >= 0.

Signed-off-by: Bill Nottingham <[email protected]>

ac97/ac97_patch.c | 3 +--
ali5451/ali5451.c | 6 ++----
ca0106/ca0106_proc.c | 4 ++--
rme9652/rme9652.c | 2 --
4 files changed, 5 insertions(+), 10 deletions(-)

diff -ru linux-2.6.21-old/sound/pci/ac97/ac97_patch.c linux-2.6.21/sound/pci/ac97/ac97_patch.c
--- linux-2.6.21-old/sound/pci/ac97/ac97_patch.c 2007-05-30 02:53:05.000000000 -0400
+++ linux-2.6.21/sound/pci/ac97/ac97_patch.c 2007-05-30 02:32:41.000000000 -0400
@@ -2086,8 +2086,7 @@
struct snd_ac97 *ac97 = snd_kcontrol_chip(kcontrol);
unsigned short val;

- if (ucontrol->value.enumerated.item[0] > 3
- || ucontrol->value.enumerated.item[0] < 0)
+ if (ucontrol->value.enumerated.item[0] > 3)
return -EINVAL;
val = ctrl2reg[ucontrol->value.enumerated.item[0]]
<< AC97_AD198X_VREF_SHIFT;
diff -ru linux-2.6.21-old/sound/pci/ali5451/ali5451.c linux-2.6.21/sound/pci/ali5451/ali5451.c
--- linux-2.6.21-old/sound/pci/ali5451/ali5451.c 2007-05-30 02:53:05.000000000 -0400
+++ linux-2.6.21/sound/pci/ali5451/ali5451.c 2007-05-30 02:33:27.000000000 -0400
@@ -2057,10 +2057,8 @@
{
if (codec->hw_initialized)
snd_ali_disable_address_interrupt(codec);
- if (codec->irq >= 0) {
- synchronize_irq(codec->irq);
- free_irq(codec->irq, codec);
- }
+ synchronize_irq(codec->irq);
+ free_irq(codec->irq, codec);
if (codec->port)
pci_release_regions(codec->pci);
pci_disable_device(codec->pci);
diff -ru linux-2.6.21-old/sound/pci/ca0106/ca0106_proc.c linux-2.6.21/sound/pci/ca0106/ca0106_proc.c
--- linux-2.6.21-old/sound/pci/ca0106/ca0106_proc.c 2007-05-30 02:53:05.000000000 -0400
+++ linux-2.6.21/sound/pci/ca0106/ca0106_proc.c 2007-05-30 02:34:19.000000000 -0400
@@ -305,7 +305,7 @@
while (!snd_info_get_line(buffer, line, sizeof(line))) {
if (sscanf(line, "%x %x", &reg, &val) != 2)
continue;
- if ((reg < 0x40) && (reg >=0) && (val <= 0xffffffff) ) {
+ if ((reg < 0x40) && (val <= 0xffffffff) ) {
spin_lock_irqsave(&emu->emu_lock, flags);
outl(val, emu->port + (reg & 0xfffffffc));
spin_unlock_irqrestore(&emu->emu_lock, flags);
@@ -406,7 +406,7 @@
while (!snd_info_get_line(buffer, line, sizeof(line))) {
if (sscanf(line, "%x %x %x", &reg, &channel_id, &val) != 3)
continue;
- if ((reg < 0x80) && (reg >=0) && (val <= 0xffffffff) && (channel_id >=0) && (channel_id <= 3) )
+ if ((reg < 0x80) && (val <= 0xffffffff) && (channel_id <= 3) )
snd_ca0106_ptr_write(emu, reg, channel_id, val);
}
}
diff -ru linux-2.6.21-old/sound/pci/rme9652/rme9652.c linux-2.6.21/sound/pci/rme9652/rme9652.c
--- linux-2.6.21-old/sound/pci/rme9652/rme9652.c 2007-05-30 02:53:05.000000000 -0400
+++ linux-2.6.21/sound/pci/rme9652/rme9652.c 2007-05-30 02:35:16.000000000 -0400
@@ -406,8 +406,6 @@
} else if (!frag)
return 0;
offset -= rme9652->max_jitter;
- if (offset < 0)
- offset += period_size * 2;
} else {
if (offset > period_size + rme9652->max_jitter) {
if (!frag)


2007-05-30 10:16:47

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH] alsa: fix comparison of unsigned < 0

Hi,

thanks for checking this.
They seem actually hitting some real bugs.


At Wed, 30 May 2007 03:35:54 -0400,
Bill Nottingham wrote:
>
> diff -ru linux-2.6.21-old/sound/pci/ac97/ac97_patch.c linux-2.6.21/sound/pci/ac97/ac97_patch.c
> --- linux-2.6.21-old/sound/pci/ac97/ac97_patch.c 2007-05-30 02:53:05.000000000 -0400
> +++ linux-2.6.21/sound/pci/ac97/ac97_patch.c 2007-05-30 02:32:41.000000000 -0400
> @@ -2086,8 +2086,7 @@
> struct snd_ac97 *ac97 = snd_kcontrol_chip(kcontrol);
> unsigned short val;
>
> - if (ucontrol->value.enumerated.item[0] > 3
> - || ucontrol->value.enumerated.item[0] < 0)
> + if (ucontrol->value.enumerated.item[0] > 3)
> return -EINVAL;
> val = ctrl2reg[ucontrol->value.enumerated.item[0]]
> << AC97_AD198X_VREF_SHIFT;

This one is fine.


> diff -ru linux-2.6.21-old/sound/pci/ali5451/ali5451.c linux-2.6.21/sound/pci/ali5451/ali5451.c
> --- linux-2.6.21-old/sound/pci/ali5451/ali5451.c 2007-05-30 02:53:05.000000000 -0400
> +++ linux-2.6.21/sound/pci/ali5451/ali5451.c 2007-05-30 02:33:27.000000000 -0400
> @@ -2057,10 +2057,8 @@
> {
> if (codec->hw_initialized)
> snd_ali_disable_address_interrupt(codec);
> - if (codec->irq >= 0) {
> - synchronize_irq(codec->irq);
> - free_irq(codec->irq, codec);
> - }
> + synchronize_irq(codec->irq);
> + free_irq(codec->irq, codec);
> if (codec->port)
> pci_release_regions(codec->pci);
> pci_disable_device(codec->pci);

Bah, the check isn't wrong but the type of codec->irq.
It should be int instead of unsigned long. I'll fix it.


> diff -ru linux-2.6.21-old/sound/pci/ca0106/ca0106_proc.c linux-2.6.21/sound/pci/ca0106/ca0106_proc.c
> --- linux-2.6.21-old/sound/pci/ca0106/ca0106_proc.c 2007-05-30 02:53:05.000000000 -0400
> +++ linux-2.6.21/sound/pci/ca0106/ca0106_proc.c 2007-05-30 02:34:19.000000000 -0400
> @@ -305,7 +305,7 @@
> while (!snd_info_get_line(buffer, line, sizeof(line))) {
> if (sscanf(line, "%x %x", &reg, &val) != 2)
> continue;
> - if ((reg < 0x40) && (reg >=0) && (val <= 0xffffffff) ) {
> + if ((reg < 0x40) && (val <= 0xffffffff) ) {

It's a 32bit unsigned int, so the check of 0xffffffff isn't needed,
too.

> spin_lock_irqsave(&emu->emu_lock, flags);
> outl(val, emu->port + (reg & 0xfffffffc));
> spin_unlock_irqrestore(&emu->emu_lock, flags);
> @@ -406,7 +406,7 @@
> while (!snd_info_get_line(buffer, line, sizeof(line))) {
> if (sscanf(line, "%x %x %x", &reg, &channel_id, &val) != 3)
> continue;
> - if ((reg < 0x80) && (reg >=0) && (val <= 0xffffffff) && (channel_id >=0) && (channel_id <= 3) )
> + if ((reg < 0x80) && (val <= 0xffffffff) && (channel_id <= 3) )
> snd_ca0106_ptr_write(emu, reg, channel_id, val);
> }
> }

Ditto.


> diff -ru linux-2.6.21-old/sound/pci/rme9652/rme9652.c linux-2.6.21/sound/pci/rme9652/rme9652.c
> --- linux-2.6.21-old/sound/pci/rme9652/rme9652.c 2007-05-30 02:53:05.000000000 -0400
> +++ linux-2.6.21/sound/pci/rme9652/rme9652.c 2007-05-30 02:35:16.000000000 -0400
> @@ -406,8 +406,6 @@
> } else if (!frag)
> return 0;
> offset -= rme9652->max_jitter;
> - if (offset < 0)
> - offset += period_size * 2;

I think this check is actually needed, but it must be cast to int.
Will fix this, too.


Takashi

2007-05-30 11:48:12

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] alsa: fix comparison of unsigned < 0

Takashi Iwai napsal(a):
> Hi,
>
> thanks for checking this.
> They seem actually hitting some real bugs.

Bug fixes are ok, but keep on your mind this (I haven't checked if this is
the case):
http://lkml.org/lkml/2006/11/28/206

> At Wed, 30 May 2007 03:35:54 -0400,
> Bill Nottingham wrote:
>> diff -ru linux-2.6.21-old/sound/pci/ac97/ac97_patch.c linux-2.6.21/sound/pci/ac97/ac97_patch.c
>> --- linux-2.6.21-old/sound/pci/ac97/ac97_patch.c 2007-05-30 02:53:05.000000000 -0400
>> +++ linux-2.6.21/sound/pci/ac97/ac97_patch.c 2007-05-30 02:32:41.000000000 -0400
>> @@ -2086,8 +2086,7 @@
>> struct snd_ac97 *ac97 = snd_kcontrol_chip(kcontrol);
>> unsigned short val;
>>
>> - if (ucontrol->value.enumerated.item[0] > 3
>> - || ucontrol->value.enumerated.item[0] < 0)
>> + if (ucontrol->value.enumerated.item[0] > 3)
>> return -EINVAL;
>> val = ctrl2reg[ucontrol->value.enumerated.item[0]]
>> << AC97_AD198X_VREF_SHIFT;
>
> This one is fine.
[...]

regards,
--
http://www.fi.muni.cz/~xslaby/ Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8 22A0 32CC 55C3 39D4 7A7E

2007-05-30 13:16:23

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH] alsa: fix comparison of unsigned < 0

At Wed, 30 May 2007 13:47:44 +0200,
Jiri Slaby wrote:
>
> Takashi Iwai napsal(a):
> > Hi,
> >
> > thanks for checking this.
> > They seem actually hitting some real bugs.
>
> Bug fixes are ok, but keep on your mind this (I haven't checked if this is
> the case):
> http://lkml.org/lkml/2006/11/28/206

Yep, I already committed real bugfixes but didn't apply the rest.


thanks,

Takashi

>
> > At Wed, 30 May 2007 03:35:54 -0400,
> > Bill Nottingham wrote:
> >> diff -ru linux-2.6.21-old/sound/pci/ac97/ac97_patch.c linux-2.6.21/sound/pci/ac97/ac97_patch.c
> >> --- linux-2.6.21-old/sound/pci/ac97/ac97_patch.c 2007-05-30 02:53:05.000000000 -0400
> >> +++ linux-2.6.21/sound/pci/ac97/ac97_patch.c 2007-05-30 02:32:41.000000000 -0400
> >> @@ -2086,8 +2086,7 @@
> >> struct snd_ac97 *ac97 = snd_kcontrol_chip(kcontrol);
> >> unsigned short val;
> >>
> >> - if (ucontrol->value.enumerated.item[0] > 3
> >> - || ucontrol->value.enumerated.item[0] < 0)
> >> + if (ucontrol->value.enumerated.item[0] > 3)
> >> return -EINVAL;
> >> val = ctrl2reg[ucontrol->value.enumerated.item[0]]
> >> << AC97_AD198X_VREF_SHIFT;
> >
> > This one is fine.
> [...]
>
> regards,
> --
> http://www.fi.muni.cz/~xslaby/ Jiri Slaby
> faculty of informatics, masaryk university, brno, cz
> e-mail: jirislaby gmail com, gpg pubkey fingerprint:
> B674 9967 0407 CE62 ACC8 22A0 32CC 55C3 39D4 7A7E
>