Re: at24 driver - a possible problem

Re: at24 driver - a possible problem

[Wolfram Sang]

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

Hello Aleksandar,

thank you for your very detailed and very good bug-report. I need to do some
tests myself tomorrow, but I think your assumptions are correct. Possibly we
need a similar loop for the read case. I'll add the linux-i2c-list to cc for
more opinions.

Regards,

   Wolfram

On Thu, Nov 05, 2009 at 06:10:12PM +0200, Aleksandar Ivanov wrote:
> Hello Mr Brownell and Mr Sang,
> 
> I've been experimenting with the at24 driver I want to make sure that
> what I encountered is not a limitation or a bug of the driver.
> The problem that I am experiencing is that a read operation
> immediately after a write.
> 
> Here is my testing environment:
> I am using a Linux kernel 2.6.30.3 and have a board with a AT24C512 eeprom chip.
> I am using the linux system calls read and write. At the end of the
> e-mail is the source of the test app that I'm using, which does the
> following:
> 1. Opens the eeprom file for reading and writing.
> 2. Performs a 10 byte read.
> 3. Performs a 10 byte write.
> 4. Performs a 10 byte read.
> 
> The final read fails with the errno seto to ENXIO (6 /* No such device
> or address */).
> 
> I've compiled the driver with debug information so here is the log
> corresponding to the execution of the test application:
> 
> i2c-adapter i2c-0: master_xfer[0] W, addr=0x50, len=2
> i2c-adapter i2c-0: master_xfer[1] R, addr=0x50, len=10
> at24 0-0050: i2c read 10@0 --> 2
> i2c-adapter i2c-0: master_xfer[0] W, addr=0x50, len=12
> at24 0-0050: write 10@10 --> 10 (597993)
> i2c-adapter i2c-0: master_xfer[0] W, addr=0x50, len=2
> i2c-adapter i2c-0: master_xfer[1] R, addr=0x50, len=10
> at24 0-0050: i2c read 10@20 --> -6
> 
> The failure is not in 100% of the cases. What I also noted is that if
> I insert a usleep() call, or just a for loop with enough cycles, the
> final read succeeds.
> After looking carefully at your code I found that when in a write
> operation you are expecting that a previous write might still be in
> progress.
> While this is not the case in the read operation.
> Could this be the problem that I am experiencing - the final read
> fails, because the previous write operation is not finished, and the
> read does not have a timeout protection.
> 
> If you need more information, I'll be glad to help.
> Thanks in advance for your consideration!
> Aleks
> 
> 
> Here is the source of the test app:
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdio.h>
> #include <string.h>
> #include <errno.h>
> 
> #define EPPROM_FILE_NAME    "/sys/class/i2c-adapter/i2c-0/0-0050/eeprom"
> 
> int main(int argc, char *argv[])
> {
>     char            buf[10];
>     int             bytes = 0;
>     const unsigned  len = sizeof(buf);
>     int             fd = -1;
> 
>     fd = open(EPPROM_FILE_NAME, O_RDWR | O_SYNC);
>     if (-1 == fd)
>     {
>         printf("Error opening EEPROM file %s: %s.\n",
> EPPROM_FILE_NAME, strerror(errno));
>         return -1;
>     }
> 
>     bytes = read(fd, buf, len);
> 
>     if ((bytes > 0) &&
>         (len == (unsigned)bytes))
>     {
>         printf("Data read.\n");
>     }
>     else
>     {
>         printf("Failed to read from %s, bytes = %d, errno = %d, %s\n",
> EPPROM_FILE_NAME, bytes, errno, strerror(errno));
>         close(fd);
>         return -1;
>     }
> 
>     bytes = write(fd, buf, len);
> 
>     if ((bytes > 0) &&
>         (len == (unsigned)bytes))
>     {
>         printf("Data written.\n");
>     }
>     else
>     {
>         printf("Failed to write to %s, bytes = %d, %s\n",
> EPPROM_FILE_NAME, bytes, strerror(errno));
>         close(fd);
>         return -1;
>     }
> 
>     bytes = read(fd, buf, len);         // This call fails
> 
>     if ((bytes > 0) &&
>         (len == (unsigned)bytes))
>     {
>         printf("Data read.\n");
>     }
>     else
>     {
>         printf("Failed to read from %s, bytes = %d, errno = %d, %s\n",
> EPPROM_FILE_NAME, bytes, errno, strerror(errno));
>         close(fd);
>         return -1;
>     }
> 
>     close(fd);
>     return 0;
> }
> 
> 
> Here is the log from my test application:
> "Data read.
> Data written.
> Failed to read from /sys/class/i2c-adapter/i2c-0/0-0050/eeprom, bytes
> = -1, errno = 6, No such device or address"

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5064 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: at24 driver - a possible problem

[Jean Delvare]

On Thu, 5 Nov 2009 18:25:37 +0100, Wolfram Sang wrote:
> Hello Aleksandar,
> 
> thank you for your very detailed and very good bug-report. I need to do some
> tests myself tomorrow, but I think your assumptions are correct. Possibly we
> need a similar loop for the read case. I'll add the linux-i2c-list to cc for
> more opinions.

This makes a lot of sense. When a write operation is in progress, the
EEPROM is busy and may not ack its address. Whether the request is
another write or a read probably doesn't make a difference, a nack is a
nack. So the same waiting loop that we have for writes, is certainly
needed for reads as well. Shouldn't be too difficult. Any volunteer? I
can review the patch when it's done.

-- 
Jean Delvare

Re: at24 driver - a possible problem

[Wolfram Sang]

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


> This makes a lot of sense. When a write operation is in progress, the
> EEPROM is busy and may not ack its address. Whether the request is
> another write or a read probably doesn't make a difference, a nack is a
> nack. So the same waiting loop that we have for writes, is certainly
> needed for reads as well. Shouldn't be too difficult. Any volunteer? I
> can review the patch when it's done.

If nobody is faster ;), I will do it this weekend.

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

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

Re: at24 driver - a possible problem

[Aleksandar Ivanov]

Here is what system-call behaviour I was expecting from the position
of a user space developer.
You can then say whether it is impossible, incorrect or inefficient.

When I found that the read() was failing I at once suspected that a
write() was not finished at the time of the call.
So I added the O_SYNC flag to the open() call. (The man says: O_SYNC
- The file is opened for synchronous I/O. Any write()s on the
resulting file descriptor will block the calling process until the
data has been physically written to the underlying hardware.)
But obviously this didn't fix the issue.

Correct me if I'm wrong but I would expect from a synchronous write()
call to NOT finish before the data has been "phisically written".
And in this case of course the read operation wouldn't need a waiting loop.
What do you think?

Regards,
Aleks

2009/11/6 Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
>
>> This makes a lot of sense. When a write operation is in progress, the
>> EEPROM is busy and may not ack its address. Whether the request is
>> another write or a read probably doesn't make a difference, a nack is a
>> nack. So the same waiting loop that we have for writes, is certainly
>> needed for reads as well. Shouldn't be too difficult. Any volunteer? I
>> can review the patch when it's done.
>
> If nobody is faster ;), I will do it this weekend.
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAkr0G0EACgkQD27XaX1/VRsWPgCgr43owCqfBvtuQHtCz5bY9nZg
> rT0An26MIi9s6NobqIAeKgDKWvky9/pp
> =0Jrc
> -----END PGP SIGNATURE-----
>
>

Re: at24 driver - a possible problem

[David Brownell]

On Friday 06 November 2009, Aleksandar Ivanov wrote:
> So I added the O_SYNC flag to the open() call. (The man says: O_SYNC
> - The file is opened for synchronous I/O. Any write()s on the
> resulting file descriptor will block the calling process until the
> data has been physically written to the underlying hardware.)
> But obviously this didn't fix the issue.
> 
> Correct me if I'm wrong but I would expect from a synchronous write()
> call to NOT finish before the data has been "phisically written".
> And in this case of course the read operation wouldn't need a waiting loop.
> What do you think?

Makes sense to me.  Another bug to fix.  :)

Re: at24 driver - a possible problem

[Wolfram Sang]

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

> > Correct me if I'm wrong but I would expect from a synchronous write()
> > call to NOT finish before the data has been "phisically written".
> > And in this case of course the read operation wouldn't need a waiting loop.
> > What do you think?
> 
> Makes sense to me.  Another bug to fix.  :)

I think the waiting loop in the read-path won't hurt and will be convenient for
a number of users. I also think it would be nice if O_SYNC would be supported,
so the write will only return after the EEPROM finished the write cycle.

Still, the latter one beats me right now. The sysfs-bin-write gets a kobject
and the O_SYNC is placed in the flags of a filp during open. Is there a some
connection between those? I'm not even sure if O_SYNC should be handled at the
sysfs-layer instead of inside the driver?

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

Re: at24 driver - a possible problem

[David Brownell]

On Sunday 08 November 2009, Wolfram Sang wrote:
> Still, the latter one beats me right now. The sysfs-bin-write gets a kobject
> and the O_SYNC is placed in the flags of a filp during open. Is there a some
> connection between those?

I'd have to look carefully, but ISTR there isn't.


> I'm not even sure if O_SYNC should be handled at the 
> sysfs-layer instead of inside the driver?
> 

Re: at24 driver - a possible problem

[Jean Delvare]

On Sun, 8 Nov 2009 21:23:31 +0100, Wolfram Sang wrote:
> > > Correct me if I'm wrong but I would expect from a synchronous write()
> > > call to NOT finish before the data has been "phisically written".
> > > And in this case of course the read operation wouldn't need a waiting loop.
> > > What do you think?
> > 
> > Makes sense to me.  Another bug to fix.  :)
> 
> I think the waiting loop in the read-path won't hurt and will be convenient for
> a number of users. I also think it would be nice if O_SYNC would be supported,
> so the write will only return after the EEPROM finished the write cycle.
> 
> Still, the latter one beats me right now. The sysfs-bin-write gets a kobject
> and the O_SYNC is placed in the flags of a filp during open. Is there a some
> connection between those? I'm not even sure if O_SYNC should be handled at the
> sysfs-layer instead of inside the driver?

I'm not sure how sysfs could handle it. What backs up the sysfs file is
driver-specific, so only the driver knows how to ensure that the data
has been written. In the at24 driver case, the only way AFAICS is to
try to read one byte back from the EEPROM and only return when the read
is successful.

But looking at the sysfs API, I can't see a way to retrieve O_SYNC from
the driver, nor room for a callback that would let sysfs handle it. It
is entirely possible that sysfs simply has no support for O flags. This
would have to be discussed at a higher level.

-- 
Jean Delvare

Re: at24 driver - a possible problem

[Wolfram Sang]

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


> > Still, the latter one beats me right now. The sysfs-bin-write gets a kobject
> > and the O_SYNC is placed in the flags of a filp during open. Is there a some
> > connection between those? I'm not even sure if O_SYNC should be handled at the
> > sysfs-layer instead of inside the driver?
> 
> I'm not sure how sysfs could handle it. What backs up the sysfs file is
> driver-specific, so only the driver knows how to ensure that the data
> has been written. In the at24 driver case, the only way AFAICS is to
> try to read one byte back from the EEPROM and only return when the read
> is successful.

ACK. Sorry, I was not specific here. I also imagined that a solution must look
like:

	if (flag_however_I_get_it == O_SYNC)
		at24_eeprom_read(...)

most probably in at24_write().

What I meant with "handled at sysfs-layer" was that it felt wrong to look for
some complicated way from the kobject to the O_SYNC flag. It appeared to me
that it might be more suitable to let the sysfs-layer recognize the flag during
open and then, maybe, set another flag in a struct which is easier to reach for
a bin-file. Then again, I wondered how many sysfs-bin files would really need
that and that the "eeprom"-bin file might be a gray area (because of really
acessing a media).

> is entirely possible that sysfs simply has no support for O flags. This
> would have to be discussed at a higher level.

Most probably.

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

Re: at24 driver - a possible problem

[Aleksandar Ivanov]

I also tested the patch and it looks like it fixes the problem.
Thank you for the efforts:)

2009/11/9 Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
>
>> > Still, the latter one beats me right now. The sysfs-bin-write gets a kobject
>> > and the O_SYNC is placed in the flags of a filp during open. Is there a some
>> > connection between those? I'm not even sure if O_SYNC should be handled at the
>> > sysfs-layer instead of inside the driver?
>>
>> I'm not sure how sysfs could handle it. What backs up the sysfs file is
>> driver-specific, so only the driver knows how to ensure that the data
>> has been written. In the at24 driver case, the only way AFAICS is to
>> try to read one byte back from the EEPROM and only return when the read
>> is successful.
>
> ACK. Sorry, I was not specific here. I also imagined that a solution must look
> like:
>
>        if (flag_however_I_get_it == O_SYNC)
>                at24_eeprom_read(...)
>
> most probably in at24_write().
>
> What I meant with "handled at sysfs-layer" was that it felt wrong to look for
> some complicated way from the kobject to the O_SYNC flag. It appeared to me
> that it might be more suitable to let the sysfs-layer recognize the flag during
> open and then, maybe, set another flag in a struct which is easier to reach for
> a bin-file. Then again, I wondered how many sysfs-bin files would really need
> that and that the "eeprom"-bin file might be a gray area (because of really
> acessing a media).
>
>> is entirely possible that sysfs simply has no support for O flags. This
>> would have to be discussed at a higher level.
>
> Most probably.
>
> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAkr33JUACgkQD27XaX1/VRuNZACgtsi5ORcR4PrmxNWNXs8IU4z5
> 6f8AnA6i5IhWuwg5OaX/RafoTfm3xi+/
> =4vXM
> -----END PGP SIGNATURE-----
>
>

[PATCH] at24: use timeout also for read

[Wolfram Sang]

Writes may take some time on EEPROMs, so for consecutive writes, we already
have a loop waiting for the EEPROM to become ready. Use such a loop for reads,
too, in case somebody wants to immediately read after a write. Detailed bug
report and test case can be found here:

http://article.gmane.org/gmane.linux.drivers.i2c/4660

Reported-by: Aleksandar Ivanov <ivanov.aleks-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---

I could reproduce the errornous behaviour and this patch fixes it for me.

 drivers/misc/eeprom/at24.c |   78 ++++++++++++++++++++++++++-----------------
 1 files changed, 47 insertions(+), 31 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index db39f4a..78aa46c 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -158,6 +158,7 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
 	struct i2c_msg msg[2];
 	u8 msgbuf[2];
 	struct i2c_client *client;
+	unsigned long timeout, read_time;
 	int status, i;
 
 	memset(msg, 0, sizeof(msg));
@@ -183,47 +184,62 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
 	if (count > io_limit)
 		count = io_limit;
 
-	/* Smaller eeproms can work given some SMBus extension calls */
 	if (at24->use_smbus) {
+		/* Smaller eeproms can work given some SMBus extension calls */
 		if (count > I2C_SMBUS_BLOCK_MAX)
 			count = I2C_SMBUS_BLOCK_MAX;
-		status = i2c_smbus_read_i2c_block_data(client, offset,
-				count, buf);
-		dev_dbg(&client->dev, "smbus read %zu@%d --> %d\n",
-				count, offset, status);
-		return (status < 0) ? -EIO : status;
+	} else {
+		/*
+		 * When we have a better choice than SMBus calls, use a
+		 * combined I2C message. Write address; then read up to
+		 * io_limit data bytes. Note that read page rollover helps us
+		 * here (unlike writes). msgbuf is u8 and will cast to our
+		 * needs.
+		 */
+		i = 0;
+		if (at24->chip.flags & AT24_FLAG_ADDR16)
+			msgbuf[i++] = offset >> 8;
+		msgbuf[i++] = offset;
+
+		msg[0].addr = client->addr;
+		msg[0].buf = msgbuf;
+		msg[0].len = i;
+
+		msg[1].addr = client->addr;
+		msg[1].flags = I2C_M_RD;
+		msg[1].buf = buf;
+		msg[1].len = count;
 	}
 
 	/*
-	 * When we have a better choice than SMBus calls, use a combined
-	 * I2C message. Write address; then read up to io_limit data bytes.
-	 * Note that read page rollover helps us here (unlike writes).
-	 * msgbuf is u8 and will cast to our needs.
+	 * Reads fail if the previous write didn't complete yet. We may
+	 * loop a few times until this one succeeds, waiting at least
+	 * long enough for one entire page write to work.
 	 */
-	i = 0;
-	if (at24->chip.flags & AT24_FLAG_ADDR16)
-		msgbuf[i++] = offset >> 8;
-	msgbuf[i++] = offset;
-
-	msg[0].addr = client->addr;
-	msg[0].buf = msgbuf;
-	msg[0].len = i;
+	timeout = jiffies + msecs_to_jiffies(write_timeout);
+	do {
+		read_time = jiffies;
+		if (at24->use_smbus) {
+			status = i2c_smbus_read_i2c_block_data(client, offset,
+					count, buf);
+			if (status == 0)
+				status = count;
+		} else {
+			status = i2c_transfer(client->adapter, msg, 2);
+			if (status == 2)
+				status = count;
+		}
+		dev_dbg(&client->dev, "read %zu@%d --> %zd (%ld)\n",
+				count, offset, status, jiffies);
 
-	msg[1].addr = client->addr;
-	msg[1].flags = I2C_M_RD;
-	msg[1].buf = buf;
-	msg[1].len = count;
+		if (status == count)
+			return count;
 
-	status = i2c_transfer(client->adapter, msg, 2);
-	dev_dbg(&client->dev, "i2c read %zu@%d --> %d\n",
-			count, offset, status);
+		/* REVISIT: at HZ=100, this is sloooow */
+		msleep(1);
+	} while (time_before(read_time, timeout));
 
-	if (status == 2)
-		return count;
-	else if (status >= 0)
-		return -EIO;
-	else
-		return status;
+	return -ETIMEDOUT;
 }
 
 static ssize_t at24_read(struct at24_data *at24,
-- 
1.6.3.3

Re: [PATCH] at24: use timeout also for read

[Wolfram Sang]

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

On Sun, Nov 08, 2009 at 09:14:57PM +0100, Wolfram Sang wrote:
> Writes may take some time on EEPROMs, so for consecutive writes, we already
> have a loop waiting for the EEPROM to become ready. Use such a loop for reads,
> too, in case somebody wants to immediately read after a write. Detailed bug
> report and test case can be found here:
> 
> http://article.gmane.org/gmane.linux.drivers.i2c/4660
> 
> Reported-by: Aleksandar Ivanov <ivanov.aleks-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

Jean, you are probably busy, just a short note will do: Is this scheduled for
2.6.33?

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

Re: [PATCH] at24: use timeout also for read

[Jean Delvare]

On Mon, 16 Nov 2009 19:50:30 +0100, Wolfram Sang wrote:
> On Sun, Nov 08, 2009 at 09:14:57PM +0100, Wolfram Sang wrote:
> > Writes may take some time on EEPROMs, so for consecutive writes, we already
> > have a loop waiting for the EEPROM to become ready. Use such a loop for reads,
> > too, in case somebody wants to immediately read after a write. Detailed bug
> > report and test case can be found here:
> > 
> > http://article.gmane.org/gmane.linux.drivers.i2c/4660
> > 
> > Reported-by: Aleksandar Ivanov <ivanov.aleks-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> > Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> 
> Jean, you are probably busy, just a short note will do: Is this scheduled for
> 2.6.33?

Yes it is, but I must find some time to review and test your change
first. Busy...

-- 
Jean Delvare

Re: [PATCH] at24: use timeout also for read

[Jean Delvare]

Hi Wolfram,

Sorry for the late review.

On Sun,  8 Nov 2009 21:14:57 +0100, Wolfram Sang wrote:
> Writes may take some time on EEPROMs, so for consecutive writes, we already
> have a loop waiting for the EEPROM to become ready. Use such a loop for reads,
> too, in case somebody wants to immediately read after a write. Detailed bug
> report and test case can be found here:
> 
> http://article.gmane.org/gmane.linux.drivers.i2c/4660
> 
> Reported-by: Aleksandar Ivanov <ivanov.aleks-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
> 
> I could reproduce the errornous behaviour and this patch fixes it for me.

Looks overall good, with just one comment:

> 
>  drivers/misc/eeprom/at24.c |   78 ++++++++++++++++++++++++++-----------------
>  1 files changed, 47 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index db39f4a..78aa46c 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -158,6 +158,7 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
>  	struct i2c_msg msg[2];
>  	u8 msgbuf[2];
>  	struct i2c_client *client;
> +	unsigned long timeout, read_time;
>  	int status, i;
>  
>  	memset(msg, 0, sizeof(msg));
> @@ -183,47 +184,62 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
>  	if (count > io_limit)
>  		count = io_limit;
>  
> -	/* Smaller eeproms can work given some SMBus extension calls */
>  	if (at24->use_smbus) {
> +		/* Smaller eeproms can work given some SMBus extension calls */
>  		if (count > I2C_SMBUS_BLOCK_MAX)
>  			count = I2C_SMBUS_BLOCK_MAX;
> -		status = i2c_smbus_read_i2c_block_data(client, offset,
> -				count, buf);
> -		dev_dbg(&client->dev, "smbus read %zu@%d --> %d\n",
> -				count, offset, status);
> -		return (status < 0) ? -EIO : status;
> +	} else {
> +		/*
> +		 * When we have a better choice than SMBus calls, use a
> +		 * combined I2C message. Write address; then read up to
> +		 * io_limit data bytes. Note that read page rollover helps us
> +		 * here (unlike writes). msgbuf is u8 and will cast to our
> +		 * needs.
> +		 */
> +		i = 0;
> +		if (at24->chip.flags & AT24_FLAG_ADDR16)
> +			msgbuf[i++] = offset >> 8;
> +		msgbuf[i++] = offset;
> +
> +		msg[0].addr = client->addr;
> +		msg[0].buf = msgbuf;
> +		msg[0].len = i;
> +
> +		msg[1].addr = client->addr;
> +		msg[1].flags = I2C_M_RD;
> +		msg[1].buf = buf;
> +		msg[1].len = count;
>  	}
>  
>  	/*
> -	 * When we have a better choice than SMBus calls, use a combined
> -	 * I2C message. Write address; then read up to io_limit data bytes.
> -	 * Note that read page rollover helps us here (unlike writes).
> -	 * msgbuf is u8 and will cast to our needs.
> +	 * Reads fail if the previous write didn't complete yet. We may
> +	 * loop a few times until this one succeeds, waiting at least
> +	 * long enough for one entire page write to work.
>  	 */
> -	i = 0;
> -	if (at24->chip.flags & AT24_FLAG_ADDR16)
> -		msgbuf[i++] = offset >> 8;
> -	msgbuf[i++] = offset;
> -
> -	msg[0].addr = client->addr;
> -	msg[0].buf = msgbuf;
> -	msg[0].len = i;
> +	timeout = jiffies + msecs_to_jiffies(write_timeout);
> +	do {
> +		read_time = jiffies;
> +		if (at24->use_smbus) {
> +			status = i2c_smbus_read_i2c_block_data(client, offset,
> +					count, buf);
> +			if (status == 0)
> +				status = count;

I don't think this is needed. i2c_smbus_read_i2c_block_data() returns
the number of bytes read, or a negative error code. I don't think it
can ever return 0 (and if it did, it would not mean success.) Thus
returning status without additional processing should be fine.

> +		} else {
> +			status = i2c_transfer(client->adapter, msg, 2);
> +			if (status == 2)
> +				status = count;
> +		}
> +		dev_dbg(&client->dev, "read %zu@%d --> %zd (%ld)\n",
> +				count, offset, status, jiffies);
>  
> -	msg[1].addr = client->addr;
> -	msg[1].flags = I2C_M_RD;
> -	msg[1].buf = buf;
> -	msg[1].len = count;
> +		if (status == count)
> +			return count;
>  
> -	status = i2c_transfer(client->adapter, msg, 2);
> -	dev_dbg(&client->dev, "i2c read %zu@%d --> %d\n",
> -			count, offset, status);
> +		/* REVISIT: at HZ=100, this is sloooow */
> +		msleep(1);
> +	} while (time_before(read_time, timeout));
>  
> -	if (status == 2)
> -		return count;
> -	else if (status >= 0)
> -		return -EIO;
> -	else
> -		return status;
> +	return -ETIMEDOUT;
>  }
>  
>  static ssize_t at24_read(struct at24_data *at24,

Other than that this looks good. If the above change is OK with you
then I can push this fix to Linus quickly.

-- 
Jean Delvare

Re: [PATCH] at24: use timeout also for read

[Jean Delvare]

On Sun, 22 Nov 2009 21:08:46 +0100, Jean Delvare wrote:
> Other than that this looks good. If the above change is OK with you
> then I can push this fix to Linus quickly.

Ping Wolfram. This becomes urgent...

-- 
Jean Delvare

Re: [PATCH] at24: use timeout also for read

[Wolfram Sang]

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

On Wed, Nov 25, 2009 at 09:09:07AM +0100, Jean Delvare wrote:
> On Sun, 22 Nov 2009 21:08:46 +0100, Jean Delvare wrote:
> > Other than that this looks good. If the above change is OK with you
> > then I can push this fix to Linus quickly.
> 
> Ping Wolfram. This becomes urgent...

Argh, sorry, this message slipped through. Will work on it right now!

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

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

[PATCH V2] at24: use timeout also for read

[Wolfram Sang]

Writes may take some time on EEPROMs, so for consecutive writes, we already
have a loop waiting for the EEPROM to become ready. Use such a loop for reads,
too, in case somebody wants to immediately read after a write. Detailed bug
report and test case can be found here:

http://article.gmane.org/gmane.linux.drivers.i2c/4660

Reported-by: Aleksandar Ivanov <ivanov.aleks-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Tested-by: Aleksandar Ivanov <ivanov.aleks-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---

I could reproduce the errornous behaviour and this patch fixes it for me.

Jean, you can use this version or modify V1 yourself. Whatever is faster/easier
for you.

Changes since V1:

- Use return value from i2c_smbus_read_i2c_block_data() directly as it returns the
  number of bytes transferred.

 drivers/misc/eeprom/at24.c |   76 ++++++++++++++++++++++++++------------------
 1 files changed, 45 insertions(+), 31 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index db39f4a..cb0ff68 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -158,6 +158,7 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
 	struct i2c_msg msg[2];
 	u8 msgbuf[2];
 	struct i2c_client *client;
+	unsigned long timeout, read_time;
 	int status, i;
 
 	memset(msg, 0, sizeof(msg));
@@ -183,47 +184,60 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
 	if (count > io_limit)
 		count = io_limit;
 
-	/* Smaller eeproms can work given some SMBus extension calls */
 	if (at24->use_smbus) {
+		/* Smaller eeproms can work given some SMBus extension calls */
 		if (count > I2C_SMBUS_BLOCK_MAX)
 			count = I2C_SMBUS_BLOCK_MAX;
-		status = i2c_smbus_read_i2c_block_data(client, offset,
-				count, buf);
-		dev_dbg(&client->dev, "smbus read %zu@%d --> %d\n",
-				count, offset, status);
-		return (status < 0) ? -EIO : status;
+	} else {
+		/*
+		 * When we have a better choice than SMBus calls, use a
+		 * combined I2C message. Write address; then read up to
+		 * io_limit data bytes. Note that read page rollover helps us
+		 * here (unlike writes). msgbuf is u8 and will cast to our
+		 * needs.
+		 */
+		i = 0;
+		if (at24->chip.flags & AT24_FLAG_ADDR16)
+			msgbuf[i++] = offset >> 8;
+		msgbuf[i++] = offset;
+
+		msg[0].addr = client->addr;
+		msg[0].buf = msgbuf;
+		msg[0].len = i;
+
+		msg[1].addr = client->addr;
+		msg[1].flags = I2C_M_RD;
+		msg[1].buf = buf;
+		msg[1].len = count;
 	}
 
 	/*
-	 * When we have a better choice than SMBus calls, use a combined
-	 * I2C message. Write address; then read up to io_limit data bytes.
-	 * Note that read page rollover helps us here (unlike writes).
-	 * msgbuf is u8 and will cast to our needs.
+	 * Reads fail if the previous write didn't complete yet. We may
+	 * loop a few times until this one succeeds, waiting at least
+	 * long enough for one entire page write to work.
 	 */
-	i = 0;
-	if (at24->chip.flags & AT24_FLAG_ADDR16)
-		msgbuf[i++] = offset >> 8;
-	msgbuf[i++] = offset;
-
-	msg[0].addr = client->addr;
-	msg[0].buf = msgbuf;
-	msg[0].len = i;
+	timeout = jiffies + msecs_to_jiffies(write_timeout);
+	do {
+		read_time = jiffies;
+		if (at24->use_smbus) {
+			status = i2c_smbus_read_i2c_block_data(client, offset,
+					count, buf);
+		} else {
+			status = i2c_transfer(client->adapter, msg, 2);
+			if (status == 2)
+				status = count;
+		}
+		dev_dbg(&client->dev, "read %zu@%d --> %zd (%ld)\n",
+				count, offset, status, jiffies);
 
-	msg[1].addr = client->addr;
-	msg[1].flags = I2C_M_RD;
-	msg[1].buf = buf;
-	msg[1].len = count;
+		if (status == count)
+			return count;
 
-	status = i2c_transfer(client->adapter, msg, 2);
-	dev_dbg(&client->dev, "i2c read %zu@%d --> %d\n",
-			count, offset, status);
+		/* REVISIT: at HZ=100, this is sloooow */
+		msleep(1);
+	} while (time_before(read_time, timeout));
 
-	if (status == 2)
-		return count;
-	else if (status >= 0)
-		return -EIO;
-	else
-		return status;
+	return -ETIMEDOUT;
 }
 
 static ssize_t at24_read(struct at24_data *at24,
-- 
1.6.3.3

Re: [PATCH V2] at24: use timeout also for read

[Jean Delvare]

On Wed, 25 Nov 2009 10:37:56 +0100, Wolfram Sang wrote:
> Writes may take some time on EEPROMs, so for consecutive writes, we already
> have a loop waiting for the EEPROM to become ready. Use such a loop for reads,
> too, in case somebody wants to immediately read after a write. Detailed bug
> report and test case can be found here:
> 
> http://article.gmane.org/gmane.linux.drivers.i2c/4660
> 
> Reported-by: Aleksandar Ivanov <ivanov.aleks-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Tested-by: Aleksandar Ivanov <ivanov.aleks-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
> 
> I could reproduce the errornous behaviour and this patch fixes it for me.
> 
> Jean, you can use this version or modify V1 yourself. Whatever is faster/easier
> for you.
> 
> Changes since V1:
> 
> - Use return value from i2c_smbus_read_i2c_block_data() directly as it returns the
>   number of bytes transferred.
> 
>  drivers/misc/eeprom/at24.c |   76 ++++++++++++++++++++++++++------------------
>  1 files changed, 45 insertions(+), 31 deletions(-)

Thanks. Patch applied, I'll prepare a pull request for Linus later
today.

-- 
Jean Delvare