Re: [PATCH] i2c: timeouts reach -1

[Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Jean Delvare wrote:
> On Sat, 31 Jan 2009 22:10:43 +0100, Jean Delvare wrote:
>> Hi Roel,
>>
>> On Sat, 31 Jan 2009 11:22:58 +0100, Roel Kluin wrote:
>>> With a postfix decrement these timeouts reach -1 rather than 0,
>>> but after the loop it is tested whether they have become 0.
>>>
>>> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> (...)
>>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>>> index 6af6814..6379ec1 100644
>>> --- a/drivers/i2c/busses/i2c-pxa.c
>>> +++ b/drivers/i2c/busses/i2c-pxa.c
>>> @@ -644,7 +644,7 @@ static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c,
>>>  
>>>  	i2c_pxa_start_message(i2c);
>>>  
>>> -	while (timeout-- && i2c->msg_num > 0) {
>>> +	while (--timeout && i2c->msg_num > 0) {
>>>  		i2c_pxa_handler(0, i2c);
>>>  		udelay(10);
>>>  	}
>> Good catch. Applied, thanks for reporting.
> 
> On second thought, shouldn't the msg_num test be done first and the
> timeout test second? With the current order, you could exit with a
> timeout error while all the messages were successfully transferred.

Thanks again for the review, How about:

----------------------------->8-----------------8<------------------------------
With a postfix decrement these timeouts reach -1 rather than 0, but after the
loop it is tested whether they have become 0. also test before the decrement

As pointed out by Jean Delvare, the msg_num should be tested before the timeout.
With the current order, you could exit with a timeout error while all the
messages were successfully transferred.

Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
diff --git a/drivers/i2c/busses/i2c-amd8111.c b/drivers/i2c/busses/i2c-amd8111.c
index edab519..a73346b 100644
--- a/drivers/i2c/busses/i2c-amd8111.c
+++ b/drivers/i2c/busses/i2c-amd8111.c
@@ -72,10 +72,10 @@ static unsigned int amd_ec_wait_write(struct amd_smbus *smbus)
 {
 	int timeout = 500;
 
-	while (timeout-- && (inb(smbus->base + AMD_EC_SC) & AMD_EC_SC_IBF))
+	while ((inb(smbus->base + AMD_EC_SC) & AMD_EC_SC_IBF) && --timeout)
 		udelay(1);
 
-	if (!timeout) {
+	if (timeout <= 0) {
 		dev_warn(&smbus->dev->dev,
 			 "Timeout while waiting for IBF to clear\n");
 		return -ETIMEDOUT;
@@ -88,10 +88,10 @@ static unsigned int amd_ec_wait_read(struct amd_smbus *smbus)
 {
 	int timeout = 500;
 
-	while (timeout-- && (~inb(smbus->base + AMD_EC_SC) & AMD_EC_SC_OBF))
+	while ((~inb(smbus->base + AMD_EC_SC) & AMD_EC_SC_OBF) && --timeout)
 		udelay(1);
 
-	if (!timeout) {
+	if (timeout <= 0) {
 		dev_warn(&smbus->dev->dev,
 			 "Timeout while waiting for OBF to set\n");
 		return -ETIMEDOUT;
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 6af6814..04751fa 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -644,7 +644,7 @@ static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c,
 
 	i2c_pxa_start_message(i2c);
 
-	while (timeout-- && i2c->msg_num > 0) {
+	while (i2c->msg_num > 0 && --timeout) {
 		i2c_pxa_handler(0, i2c);
 		udelay(10);
 	}
@@ -657,7 +657,7 @@ static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c,
 	ret = i2c->msg_idx;
 
 out:
-	if (timeout == 0)
+	if (timeout <= 0)
 		i2c_pxa_scream_blue_murder(i2c, "timeout");
 
 	return ret;