eeprom/at25: bugfix "not ready" timeout after write

From: Sebastian Heutling <heutling-U9zb0o/[email protected]>

Under certain circumstances msleep(1) within the loop, which waits for the EEPROM to be finished, might take longer than the timeout. On the next loop the status register might now return to be ready and therefore the loop finishes. The following check now tests if a timeout occurred and if so returns an error although the device reported it was ready.

This fix replaces testing the occurrence of the timeout by testing the "not ready" bit in the status register.

Signed-off-by Sebastian Heutling <heutling-U9zb0o/[email protected]>

Signed-off-by David Brownell <[email protected]>

---
 drivers/misc/eeprom/at25.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/misc/eeprom/at25.c +1 -1

@@ -173,6 +173,7 @@ at25_ee_write(struct at25_data *at25, co
 		unsigned	segment;
 		unsigned	offset = (unsigned) off;
 		u8		*cp = bounce + 1;
+		int		sr;
 
 		*cp = AT25_WREN;
 		status = spi_write(at25->spi, cp, 1);

@@ -214,7 +215,6 @@ at25_ee_write(struct at25_data *at25, co
 		timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT);
 		retries = 0;
 		do {
-			int	sr;
 
 			sr = spi_w8r8(at25->spi, AT25_RDSR);
 			if (sr < 0 || (sr & AT25_SR_nRDY)) {

Quoted 37 lines · On Fri, 3 Jul 2009 18:36:27 -0700 David Brownell…

From: Sebastian Heutling <heutling-U9zb0o/[email protected]>

Under certain circumstances msleep(1) within the loop, which waits for the EEPROM to be finished, might take longer than the timeout. On the next loop the status register might now return to be ready and therefore the loop finishes. The following check now tests if a timeout occurred and if so returns an error although the device reported it was ready.

This fix replaces testing the occurrence of the timeout by testing the "not ready" bit in the status register.

Signed-off-by Sebastian Heutling <heutling-U9zb0o/[email protected]>

Signed-off-by David Brownell <[email protected]>

---
 drivers/misc/eeprom/at25.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/misc/eeprom/at25.c +1 -1

@@ -173,6 +173,7 @@ at25_ee_write(struct at25_data *at25, co
 		unsigned	segment;
 		unsigned	offset = (unsigned) off;
 		u8		*cp = bounce + 1;
+		int		sr;
 
 		*cp = AT25_WREN;
 		status = spi_write(at25->spi, cp, 1);

@@ -214,7 +215,6 @@ at25_ee_write(struct at25_data *at25, co
 		timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT);
 		retries = 0;
 		do {
-			int	sr;
 
 			sr = spi_w8r8(at25->spi, AT25_RDSR);
 			if (sr < 0 || (sr & AT25_SR_nRDY)) {

drivers/misc/eeprom/at25.c at25_ee_write(struct at25_data *at25, co · line 234

Quoted 8 lines

drivers/misc/eeprom/at25.c +1 -1

@@ -228,7 +228,7 @@ at25_ee_write(struct at25_data *at25, co
 				break;
 		} while (retries++ < 3 || time_before_eq(jiffies, timeout));
 
-		if (time_after(jiffies, timeout)) {
+		if ((sr < 0) || (sr & AT25_SR_nRDY)) {
 			dev_err(&at25->spi->dev,
 				"write %d bytes offset %d, "
 				"timeout after %u msecs\n",

Seems OK.

The code's a bit nonsensical though:

		/* Wait for non-busy status */
		timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT);
		retries = 0;
		do {

			sr = spi_w8r8(at25->spi, AT25_RDSR);
			if (sr < 0 || (sr & AT25_SR_nRDY)) {
				dev_dbg(&at25->spi->dev,
					"rdsr --> %d (%02x)\n", sr, sr);
				/* at HZ=100, this is sloooow */
				msleep(1);
				continue;
			}
			if (!(sr & AT25_SR_nRDY))
				break;
		} while (retries++ < 3 || time_before_eq(jiffies, timeout));

EE_TIMEOUT is 25 msecs and `retries' is 3 msecs. So the time_before_eq() will never trigger unless there's a tremendous amount of CPU load or something. It seems a bit ripe for some rationalisation.

Seems OK.

The code's a bit nonsensical though:

                /* Wait for non-busy status */
                timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT); retries = 0;
                do {

                        sr = spi_w8r8(at25->spi, AT25_RDSR);
                        if (sr < 0 || (sr & AT25_SR_nRDY)) { dev_dbg(&at25->spi->dev,
                                        "rdsr --> %d (%02x)\n", sr, sr); /* at HZ=100, this is sloooow */ msleep(1);
                                continue;
                        }
                        if (!(sr & AT25_SR_nRDY))
                                break;
                } while (retries++ < 3 || time_before_eq(jiffies, timeout));

EE_TIMEOUT is 25 msecs and `retries' is 3 msecs.  So the time_before_eq() will never trigger unless there's a tremendous amount of CPU load or something.  It seems a bit ripe for some rationalisation.