[i2c] [PATCH] Add a new-style driver for most I2C EEPROMs

Trent Piepho xyzzy at speakeasy.org
Fri Apr 18 10:07:51 CEST 2008


On Thu, 17 Apr 2008, David Brownell wrote:
> > are left-overs from earlier revisions and can be thrown out?
>
> Both of those tests should be moved into the for() test,
> so the tail end of the loop always does an msleep.  It'd
> be clearer as:
>
> 	timeout = ... ;
> 	retries = 0;
> 	while (retries++ < 3 || time_before(jiffies, timeout)) {
> 		... try write, quit loop on success ...
> 		msleep(1);
> 	}
>
> Yes, that loop has lots of leftovers.  :(

Certainly did.

Note that your loop has a flaw that can cause it to timeout incorrectly.

After the msleep's delay is used up, the process will enter the run
queue.  But there is no guarantee that the scheduler will select it to
run vs the other processes in the run queue!  It's entirely possible that
msleep(1) won't return for 1000s of ms.  On a 100 HZ system, 30 ms or
more would be quite common.  You get 20 just from the msleep, and 10 more
every time another processes runs.

So the loop tries the write for the first time, fails (which is perfectly
acceptable), then sleeps for more than timeout jiffies.  The timeout
expires and the write is never tried again.

Generally, if you want to wait at least a certain amount time for a
condition to become true, you must be sure to check the condition at
least once AFTER the time limit.  I.e, if you want to give the eeprom at
least 25 ms to respond, you need to try to write after at least 25 ms.
If 25 ms have passed, but the last time you tried to write was before 25
ms, then you didn't really give it 25 ms to respond.

Though it's less likely, on a preemptable kernel, you could be preempted
between setting timeout and the loop check, in which case the write might
never even be attempted at all!

Here's two examples of a good way to do this, depending on how you want
the control flow to go.

	unsigned long timeout = ... ;
	for (;;) {
		unsigned long wtime = jiffies;
		if (write() == success)
			break;
		if (time_after_eq(wtime, timeout)) {
			... failure ...
			return -ESOMETHING;
		}
		msleep(1);
	}
	... success ...


	unsigned long timeout = ... ;
	for (wtime = timeout; time_after(wtime, timeout);) {
		wtime = jiffies;
		if (write() == success) {
			... success ...
			return 0;
		}
		msleep(1);
	}
	... failure ...



More information about the i2c mailing list