Jean Delvare wrote:
> Hi Roel,
>
> On Sat, 31 Jan 2009 16:04:43 +0100, Roel Kluin wrote:
>> The postfix decrement decrements timeout till -1, but the
>> warning is already triggered on 0
>>
>> Signed-off-by: Roel Kluin <roel.kluin@xxxxxxxxx>
>> ---
>> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
>> index 3e01992..0e2933f 100644
>> --- a/drivers/i2c/algos/i2c-algo-pcf.c
>> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
>> @@ -115,7 +115,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>>
>> status = get_pcf(adap, 1);
>> #ifndef STUB_I2C
>> - while (timeout-- && !(status & I2C_PCF_BB)) {
>> + while (--timeout && !(status & I2C_PCF_BB)) {
>> udelay(100); /* wait for 100 us */
>> status = get_pcf(adap, 1);
>> }
>> @@ -123,7 +123,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>> if (timeout <= 0) {
>> printk(KERN_ERR "Timeout waiting for Bus Busy\n");
>> }
>> -
>> +
>> return (timeout<=0);
>> }
>
> Never include unrelated whitespace cleanups in patches which fix bugs.
I thought it was nice in this case, since it shows where things go wrong.
Also I think it is generally considered wrong only if it affects code
that are in totally different sections - because it confuses, which it
does not if it's in the same section.
>> @@ -134,7 +134,7 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
>>
>> *status = get_pcf(adap, 1);
>> #ifndef STUB_I2C
>> - while (timeout-- && (*status & I2C_PCF_PIN)) {
>> + while (--timeout && (*status & I2C_PCF_PIN)) {
>> adap->waitforpin(adap->data);
>> *status = get_pcf(adap, 1);
>> }
>
> Not as critical as the ones in i2c-amd8111 and i2c-pxa, but still bugs,
> I agree. I am, however, not totally happy with your fix. Leaving the
> "<= 0" tests while the timeout will now stop at 0 is confusing. I think
> you should change these tests to "== 0". Other odd things in these
> functions:
I consider the "<= 0" test to be safer considering possible future
changes.
> * The timeout decrement should be _after_ the status test, otherwise
> you can exit with a timeout while the status was correct.
I agree, fixed in the patch below
> * Mixing actual error codes (-EINTR) with arbitrary negative error
> values (-1) isn't wise. We are lucky than EINTR isn't equal to 1. I
> think it would be better to return -ETIMEDOUT for timeouts rather
> than an arbitrary number.
Also fixed (for both functions).
Also I noted that in wait_for_pin() on a timeout, dependent on the
status, still a handle_lab(adap, status) and return -EINTR may occur,
which I think are wrong.
> Could you please submit a new patch fixing all the above?
>
> Thanks,
here it is:
---------------------------->8----------------8<------------------------------
* Fix that the warning was already triggered on 0, which was not yet a timeout.
* Move timeout decrement after the status test so that we don't exit with a
timeout while the status was correct.
* Replace arbitrary values with error codes: return -ETIMEDOUT; upon timeout.
* Ensure that upon a timeout we do not handle dependent on the last status.
* Local whitespace cleanups.
Signed-off-by: Roel Kluin <roel.kluin@xxxxxxxxx>
---
drivers/i2c/algos/i2c-algo-pcf.c | 41 ++++++++++++++++---------------------
1 files changed, 18 insertions(+), 23 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 3e01992..05f0f56 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -108,45 +108,40 @@ static void handle_lab(struct i2c_algo_pcf_data *adap, const int *status)
get_pcf(adap, 1)));
}
-static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
-
+static int wait_for_bb(struct i2c_algo_pcf_data *adap)
+{
+#ifndef STUB_I2C
int timeout = DEF_TIMEOUT;
- int status;
- status = get_pcf(adap, 1);
-#ifndef STUB_I2C
- while (timeout-- && !(status & I2C_PCF_BB)) {
+ while (!(get_pcf(adap, 1) & I2C_PCF_BB) && --timeout)
udelay(100); /* wait for 100 us */
- status = get_pcf(adap, 1);
- }
-#endif
+
if (timeout <= 0) {
printk(KERN_ERR "Timeout waiting for Bus Busy\n");
+ return -ETIMEDOUT;
}
-
- return (timeout<=0);
+#endif
+ return 0;
}
-static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
-
+static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
+{
+#ifndef STUB_I2C
int timeout = DEF_TIMEOUT;
- *status = get_pcf(adap, 1);
-#ifndef STUB_I2C
- while (timeout-- && (*status & I2C_PCF_PIN)) {
+ while ((*status = get_pcf(adap, 1)) & I2C_PCF_PIN && --timeout)
adap->waitforpin(adap->data);
- *status = get_pcf(adap, 1);
- }
+
+ if (timeout <= 0)
+ return -ETIMEDOUT;
+
if (*status & I2C_PCF_LAB) {
handle_lab(adap, status);
- return(-EINTR);
+ return -EINTR;
}
#endif
- if (timeout <= 0)
- return(-1);
- else
- return(0);
+ return 0;
}
/*
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
