Skip to contentSkip to conversation

i2c: test off by one in {piix4,vt596}_transaction()

PATCH Applied Roel Kluin 2 participants 4 messages Dec 27, 2009 – Jan 6, 2010

Subsystems (2)
I2C subsystem host drivers I2C/smbus controller drivers for pc
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 i2c: test off by one in {piix4,vt596}_transaction()

With `while (timeout++ < MAX_TIMEOUT)' timeout reaches MAX_TIMEOUT + 1 after the loop This is probably unlikely to produce a problem.

Signed-off-by Roel Kluin <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c  |    2 +-
 drivers/i2c/busses/i2c-viapro.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-piix4.c +1 -1
index 1e245e9..d8e0df0 100644
@@ -329,7 +329,7 @@ static int piix4_transaction(void)
 		msleep(1);
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout >= MAX_TIMEOUT) {
+	if (timeout > MAX_TIMEOUT) {
 		dev_err(&piix4_adapter.dev, "SMBus Timeout!\n");
 		result = -ETIMEDOUT;
 	}
drivers/i2c/busses/i2c-viapro.c +1 -1
index e4b1543..8a2e0d5 100644
@@ -168,7 +168,7 @@ static int vt596_transaction(u8 size)
 	} while ((temp & 0x01) && (timeout++ < MAX_TIMEOUT));
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout >= MAX_TIMEOUT) {
+	if (timeout > MAX_TIMEOUT) {
 		result = -ETIMEDOUT;
 		dev_err(&vt596_adapter.dev, "SMBus timeout!\n");
 	}
#1 lore raw
Quoted 27 lines · Roel Kluin

With `while (timeout++ < MAX_TIMEOUT)' timeout reaches MAX_TIMEOUT + 1 after the loop This is probably unlikely to produce a problem.

Signed-off-by Roel Kluin <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c  |    2 +-
 drivers/i2c/busses/i2c-viapro.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-piix4.c +1 -1
index 1e245e9..d8e0df0 100644
@@ -329,7 +329,7 @@ static int piix4_transaction(void)
 		msleep(1);
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout >= MAX_TIMEOUT) {
+	if (timeout > MAX_TIMEOUT) {
 		dev_err(&piix4_adapter.dev, "SMBus Timeout!\n");
 		result = -ETIMEDOUT;
 	}
drivers/i2c/busses/i2c-viapro.c +0 -0
index e4b1543..8a2e0d5 100644

drivers/i2c/busses/i2c-viapro.c static int vt596_transaction(u8 size) · line 174

Quoted 8 lines
drivers/i2c/busses/i2c-viapro.c +1 -1
@@ -168,7 +168,7 @@ static int vt596_transaction(u8 size)
 	} while ((temp & 0x01) && (timeout++ < MAX_TIMEOUT));
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout >= MAX_TIMEOUT) {
+	if (timeout > MAX_TIMEOUT) {
 		result = -ETIMEDOUT;
 		dev_err(&vt596_adapter.dev, "SMBus timeout!\n");
 	}

That's right... but I'd rather change the loops to use "++timeout" and leave the conditions as is (or maybe change it to "=="). I think it's easier to read that way. Would that be OK with you?

Signature · 1 line
Jean Delvare
author
#2 lore raw

With `while (timeout++ < MAX_TIMEOUT)' timeout reaches MAX_TIMEOUT + 1 after the loop This is probably unlikely to produce a problem.

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

---

Quoted 3 lines

That's right... but I'd rather change the loops to use "++timeout" and leave the conditions as is (or maybe change it to "=="). I think it's easier to read that way. Would that be OK with you?

Ok,

 drivers/i2c/busses/i2c-piix4.c  |    4 ++--
 drivers/i2c/busses/i2c-viapro.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-piix4.c +2 -2
index 1e245e9..e56e4b6 100644
@@ -324,12 +324,12 @@ static int piix4_transaction(void)
 	else
 		msleep(1);
 
-	while ((timeout++ < MAX_TIMEOUT) &&
+	while ((++timeout < MAX_TIMEOUT) &&
 	       ((temp = inb_p(SMBHSTSTS)) & 0x01))
 		msleep(1);
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout >= MAX_TIMEOUT) {
+	if (timeout == MAX_TIMEOUT) {
 		dev_err(&piix4_adapter.dev, "SMBus Timeout!\n");
 		result = -ETIMEDOUT;
 	}
drivers/i2c/busses/i2c-viapro.c +2 -2
index e4b1543..a84a909 100644
@@ -165,10 +165,10 @@ static int vt596_transaction(u8 size)
 	do {
 		msleep(1);
 		temp = inb_p(SMBHSTSTS);
-	} while ((temp & 0x01) && (timeout++ < MAX_TIMEOUT));
+	} while ((temp & 0x01) && (++timeout < MAX_TIMEOUT));
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout >= MAX_TIMEOUT) {
+	if (timeout == MAX_TIMEOUT) {
 		result = -ETIMEDOUT;
 		dev_err(&vt596_adapter.dev, "SMBus timeout!\n");
 	}
Applied
#3 lore raw
Quoted 41 lines · Roel Kluin

With `while (timeout++ < MAX_TIMEOUT)' timeout reaches MAX_TIMEOUT + 1 after the loop This is probably unlikely to produce a problem.

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

---

Quoted 3 lines

That's right... but I'd rather change the loops to use "++timeout" and leave the conditions as is (or maybe change it to "=="). I think it's easier to read that way. Would that be OK with you?

Ok,

 drivers/i2c/busses/i2c-piix4.c  |    4 ++--
 drivers/i2c/busses/i2c-viapro.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-piix4.c +2 -2
index 1e245e9..e56e4b6 100644
@@ -324,12 +324,12 @@ static int piix4_transaction(void)
 	else
 		msleep(1);
 
-	while ((timeout++ < MAX_TIMEOUT) &&
+	while ((++timeout < MAX_TIMEOUT) &&
 	       ((temp = inb_p(SMBHSTSTS)) & 0x01))
 		msleep(1);
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout >= MAX_TIMEOUT) {
+	if (timeout == MAX_TIMEOUT) {
 		dev_err(&piix4_adapter.dev, "SMBus Timeout!\n");
 		result = -ETIMEDOUT;
 	}
drivers/i2c/busses/i2c-viapro.c +0 -0
index e4b1543..a84a909 100644
@@ -165,10 +165,10 @@ static int vt596_transaction(u8 size)
 	do {
 		msleep(1);

drivers/i2c/busses/i2c-viapro.c static int vt596_transaction(u8 size) · line 174

Quoted 10 lines
drivers/i2c/busses/i2c-viapro.c +2 -2
 		temp = inb_p(SMBHSTSTS);
-	} while ((temp & 0x01) && (timeout++ < MAX_TIMEOUT));
+	} while ((temp & 0x01) && (++timeout < MAX_TIMEOUT));
 
 	/* If the SMBus is still busy, we give up */
-	if (timeout >= MAX_TIMEOUT) {
+	if (timeout == MAX_TIMEOUT) {
 		result = -ETIMEDOUT;
 		dev_err(&vt596_adapter.dev, "SMBus timeout!\n");
 	}

Applied, thanks.

Signature · 1 line
Jean Delvare