fix incorrect test in trident_ac97_set(); sound/oss/trident.c

PATCH In review Roel Kluin 3 participants 5 messages Nov 7, 2007 – Nov 8, 2007 Sound

Share & tools

Roel Kluin

author

2007-11-07 18:34 UTC #0 lore raw

PATCH 1/1 fix incorrect test in trident_ac97_set(); sound/oss/trident.c

If count reaches zero, the loop ends, but the postfix decrement subtracts it. so, testing for 'count == 0' will not work.

Signed-off-by Roel Kluin <[email protected]>

---

sound/oss/trident.c +2 -2

index 96adc47..94b5fb4 100644
@@ -2939,7 +2939,7 @@ trident_ac97_set(struct ac97_codec *codec, u8 reg, u16 val)
 
 	data |= (mask | (reg & AC97_REG_ADDR));
 
-	if (count == 0) {
+	if (count == -1) {
 		printk(KERN_ERR "trident: AC97 CODEC write timed out.\n");
 		spin_unlock_irqrestore(&card->lock, flags);
 		return;

@@ -2999,7 +2999,7 @@ trident_ac97_get(struct ac97_codec *codec, u8 reg)
 	} while (count--);
 	spin_unlock_irqrestore(&card->lock, flags);
 
-	if (count == 0) {
+	if (count == -1) {
 		printk(KERN_ERR "trident: AC97 CODEC read timed out.\n");
 		data = 0;
 	}

Ray Lee

2007-11-07 18:44 UTC #1 lore raw

Quoted 19 lines · Roel Kluin

If count reaches zero, the loop ends, but the postfix decrement subtracts it. so, testing for 'count == 0' will not work.

Signed-off-by Roel Kluin <[email protected]>

---

sound/oss/trident.c +1 -1

index 96adc47..94b5fb4 100644
@@ -2939,7 +2939,7 @@ trident_ac97_set(struct ac97_codec *codec, u8 reg, u16 val)
 
        data |= (mask | (reg & AC97_REG_ADDR));
 
-       if (count == 0) {
+       if (count == -1) {
                printk(KERN_ERR "trident: AC97 CODEC write timed out.\n");
                spin_unlock_irqrestore(&card->lock, flags);
                return;

sound/oss/trident.c trident_ac97_get(struct ac97_codec *codec, u8 reg) · line 3005

Quoted 8 lines

sound/oss/trident.c +1 -1

@@ -2999,7 +2999,7 @@ trident_ac97_get(struct ac97_codec *codec, u8 reg)
        } while (count--);
        spin_unlock_irqrestore(&card->lock, flags);
 
-       if (count == 0) {
+       if (count == -1) {
                printk(KERN_ERR "trident: AC97 CODEC read timed out.\n");
                data = 0;
        }

You didn't test this: count is unsigned. Change the loop condition to be --count instead.

Ray

Roel Kluin

author

2007-11-07 18:50 UTC #2 lore raw

Quoted 32 lines · Ray Lee

Quoted 19 lines · Roel Kluin

If count reaches zero, the loop ends, but the postfix decrement subtracts it. so, testing for 'count == 0' will not work.

Signed-off-by Roel Kluin <[email protected]>

---

sound/oss/trident.c +1 -1

index 96adc47..94b5fb4 100644
@@ -2939,7 +2939,7 @@ trident_ac97_set(struct ac97_codec *codec, u8 reg, u16 val)
 
        data |= (mask | (reg & AC97_REG_ADDR));
 
-       if (count == 0) {
+       if (count == -1) {
                printk(KERN_ERR "trident: AC97 CODEC write timed out.\n");
                spin_unlock_irqrestore(&card->lock, flags);
                return;

sound/oss/trident.c trident_ac97_get(struct ac97_codec *codec, u8 reg) · line 3005

Quoted 8 lines

sound/oss/trident.c +1 -1

@@ -2999,7 +2999,7 @@ trident_ac97_get(struct ac97_codec *codec, u8 reg)
        } while (count--);
        spin_unlock_irqrestore(&card->lock, flags);
 
-       if (count == 0) {
+       if (count == -1) {
                printk(KERN_ERR "trident: AC97 CODEC read timed out.\n");
                data = 0;
        }

You didn't test this: count is unsigned. Change the loop condition to be --count instead.

Quoted 1 line

Ray

Yep, you're right, here:
--
If count reaches zero, the loop ends, but the postfix decrement still subtracts:
testing for 'count == 0' will not work.

Signed-off-by Roel Kluin <[email protected]>

---

sound/oss/trident.c +2 -2

index 96adc47..6959ee1 100644
@@ -2935,7 +2935,7 @@ trident_ac97_set(struct ac97_codec *codec, u8 reg, u16 val)
 	do {
 		if ((inw(TRID_REG(card, address)) & busy) == 0)
 			break;
-	} while (count--);
+	} while (--count);
 
 	data |= (mask | (reg & AC97_REG_ADDR));

@@ -2996,7 +2996,7 @@ trident_ac97_get(struct ac97_codec *codec, u8 reg)
 		data = inl(TRID_REG(card, address));
 		if ((data & busy) == 0)
 			break;
-	} while (count--);
+	} while (--count);
 	spin_unlock_irqrestore(&card->lock, flags);
 
 	if (count == 0) {

Ray Lee

Reviewed-by

2007-11-07 19:04 UTC #3 lore raw

Quoted 27 lines · Roel Kluin

If count reaches zero, the loop ends, but the postfix decrement still subtracts:
testing for 'count == 0' will not work.

Signed-off-by Roel Kluin <[email protected]>

---

sound/oss/trident.c +2 -2

index 96adc47..6959ee1 100644
@@ -2935,7 +2935,7 @@ trident_ac97_set(struct ac97_codec *codec, u8 reg, u16 val)
        do {
                if ((inw(TRID_REG(card, address)) & busy) == 0)
                        break;
-       } while (count--);
+       } while (--count);
 
        data |= (mask | (reg & AC97_REG_ADDR));

@@ -2996,7 +2996,7 @@ trident_ac97_get(struct ac97_codec *codec, u8 reg)
                data = inl(TRID_REG(card, address));
                if ((data & busy) == 0)
                        break;
-       } while (count--);
+       } while (--count);
 
        spin_unlock_irqrestore(&card->lock, flags);

Quoted 2 lines

        if (count == 0) {

Thanks, much better. In the future, please also CC: the appropriate maintainers, or Andrew Morton if you're at a loss...

Reviewed-by Ray Lee <[email protected]>

Muli Ben-Yehuda

Acked-by

2007-11-08 06:06 UTC #4 lore raw

Quoted 27 lines · Ray Lee

Quoted 25 lines · Roel Kluin

If count reaches zero, the loop ends, but the postfix decrement still subtracts: testing for 'count == 0' will not work.

Signed-off-by Roel Kluin <[email protected]>

---

sound/oss/trident.c +2 -2

index 96adc47..6959ee1 100644
@@ -2935,7 +2935,7 @@ trident_ac97_set(struct ac97_codec *codec, u8 reg, u16 val)
        do {
                if ((inw(TRID_REG(card, address)) & busy) == 0)
                        break;
-       } while (count--);
+       } while (--count);
 
        data |= (mask | (reg & AC97_REG_ADDR));

@@ -2996,7 +2996,7 @@ trident_ac97_get(struct ac97_codec *codec, u8 reg)
                data = inl(TRID_REG(card, address));
                if ((data & busy) == 0)
                        break;
-       } while (count--);
+       } while (--count);

Quoted 7 lines

Quoted 4 lines
sound/oss/trident.c +0 -0
        spin_unlock_irqrestore(&card->lock, flags);
 
        if (count == 0) {
Thanks, much better. In the future, please also CC: the appropriate maintainers, or Andrew Morton if you're at a loss...

Indeed.

Quoted 1 line

Reviewed-by Ray Lee <[email protected]>

Acked-by Muli Ben-Yehuda <[email protected]>

Andrew, can you please push to Linus?

Thanks,
Muli