Re: [PATCH] i2c-pxa.c: timeouts off by 1

[Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>]

On Thu, 23 Apr 2009 17:02:18 +0200
Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> On Thu, 23 Apr 2009 16:27:39 +0200, Roel Kluin wrote:
> > Ok, here's for drivers/i2c/busses/i2c-pxa.c. Note that I found another,
> > the last hunk.
> > --------------------------->8-------------8<------------------------------
> > With `while (timeout--)' timeout reaches -1 after the loop, so the tests
> > below are off by one.
> > 
> > Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> 
> Ben, Wolfram, I'll let you handle this one as it's an arm driver.
> 

The patch looks OK, but the original code is weird.

: static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c)
: {
: 	int timeout = DEF_TIMEOUT;
: 
: 	while (timeout-- && readl(_ISR(i2c)) & (ISR_IBB | ISR_UB)) {
: 		if ((readl(_ISR(i2c)) & ISR_SAD) != 0)
: 			timeout += 4;
: 
: 		msleep(2);
: 		show_state(i2c);
: 	}
: 
: 	if (timeout < 0)
: 		show_state(i2c);
: 
: 	return timeout < 0 ? I2C_RETRY : 0;
: }

The timeout+=4 inside the loop makes my brain hurt.  It makes the loop
potentially almost-infinite.  By effectively doing timeout+=3 each time
we'll break out of the loop after we've wrapped through 0x100000000
three times.  Or something.  Help!



Also, i2c_pxa_pio_set_master() does

	long timeout = 2 * DEF_TIMEOUT;

whereas i2c_pxa_wait_bus_not_bus() does

	int timeout = DEF_TIMEOUT;


`int' seems an appropriate choice.