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

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]

On Thu, Apr 23, 2009 at 04:35:07PM -0700, Andrew Morton wrote:
> 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, 

Yup.

Acked-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

> 
> : 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!

Well, it only adds 4 if there was its own I2C-slave address detected. Without
knowing the details, I assume there is a reason; I wouldn't dare changing it.

> 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.

This driver seems to have quite some more potential for cleaning up :)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]