Add error checking to spi-nor read and write

PATCH In review Michal Suchanek 2 participants 6 messages Jul 28, 2015 – Jul 29, 2015

Subsystems (2)

Memory technology devices (MTD) SPI NOR

lore

Share & tools

lore

Michal Suchanek

author

2015-07-28 09:23 UTC #0 lore raw

cover Add error checking to spi-nor read and write

Hello,

I am working with half-broken SPI drivers which sometimes fail and spi-nor completely ignores that leading to silent data corruption.

With these patches I get actual I/O errors when something fails.

Testing on different hardware would be appreciated, especially fsl-qspi.

I don't recall any comment on these patches so I am resending them separately.

Thanks

Michal

Michal Suchanek (2):
mtd: spi-nor: rework spi nor read and write.
mtd: spi-nor: rework write loop

 drivers/mtd/devices/m25p80.c      | 33 ++++++++++------
 drivers/mtd/spi-nor/fsl-quadspi.c | 29 +++++++-------
 drivers/mtd/spi-nor/spi-nor.c     | 79 +++++++++++++++++++++------------------
 include/linux/mtd/spi-nor.h       |  8 ++--
 4 files changed, 83 insertions(+), 66 deletions(-)

Signature · 1 line

2.1.4

Michal Suchanek

author

2015-07-28 09:23 UTC #1 lore raw

PATCH 1/2 mtd: spi-nor: rework spi nor read and write.

The spi_nor read and write functions pass thru the mtd retlen to the chip-specific read and write function. This makes it difficult to check for errors in read and write functions and these errors are not checked. This leads to silent data corruption.

This patch styles the chip-specific read and write function as unix read(2) and write(2) and updates the retlen in the spi-nor generic driver.

This also makes it possible to check for write errors. When pl330 fails to transfer the flash data over SPI I get I/O error instead of 4M of zeroes.

I do not have sst and fsl hardware to test with.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mtd/devices/m25p80.c      | 33 ++++++++++++++---------
 drivers/mtd/spi-nor/fsl-quadspi.c | 29 ++++++++++----------
 drivers/mtd/spi-nor/spi-nor.c     | 57 ++++++++++++++++++++++++++++-----------
 include/linux/mtd/spi-nor.h       |  8 +++---
 4 files changed, 81 insertions(+), 46 deletions(-)

drivers/mtd/devices/m25p80.c +21 ~~-12~~

index d313f948b..d8f064b 100644
@@ -75,14 +75,15 @@ static int m25p80_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
 	return spi_write(spi, flash->command, len + 1);
 }
 
-static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
-			size_t *retlen, const u_char *buf)
+static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
+			    const u_char *buf)
 {
 	struct m25p *flash = nor->priv;
 	struct spi_device *spi = flash->spi;
 	struct spi_transfer t[2] = {};
 	struct spi_message m;
 	int cmd_sz = m25p_cmdsz(nor);
+	ssize_t ret;
 
 	spi_message_init(&m);

@@ -100,9 +101,14 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 	t[1].len = len;
 	spi_message_add_tail(&t[1], &m);
 
-	spi_sync(spi, &m);
+	ret = spi_sync(spi, &m);
+	if (ret)
+		return ret;
 
-	*retlen += m.actual_length - cmd_sz;
+	ret = m.actual_length - cmd_sz;
+	if (ret < 0)
+		return -EIO;
+	return ret;
 }
 
 static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

@@ -121,14 +127,15 @@ static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
  * Read an address range from the nor chip.  The address range
  * may be any size provided it is within the physical boundaries.
  */
-static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
-			size_t *retlen, u_char *buf)
+static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
+			   u_char *buf)
 {
 	struct m25p *flash = nor->priv;
 	struct spi_device *spi = flash->spi;
 	struct spi_transfer t[2];
 	struct spi_message m;
 	unsigned int dummy = nor->read_dummy;
+	ssize_t ret;
 
 	/* convert the dummy cycles to the number of bytes */
 	dummy /= 8;

@@ -148,10 +155,14 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	t[1].len = len;
 	spi_message_add_tail(&t[1], &m);
 
-	spi_sync(spi, &m);
+	ret = spi_sync(spi, &m);
+	if (ret)
+		return ret;
 
-	*retlen = m.actual_length - m25p_cmdsz(nor) - dummy;
-	return 0;
+	ret = m.actual_length - m25p_cmdsz(nor) - dummy;
+	if (ret < 0)
+		return -EIO;
+	return ret;
 }
 
 static int m25p80_erase(struct spi_nor *nor, loff_t offset)

@@ -165,9 +176,7 @@ static int m25p80_erase(struct spi_nor *nor, loff_t offset)
 	flash->command[0] = nor->erase_opcode;
 	m25p_addr2cmd(nor, offset, flash->command);
 
-	spi_write(flash->spi, flash->command, m25p_cmdsz(nor));
-
-	return 0;
+	return spi_write(flash->spi, flash->command, m25p_cmdsz(nor));
 }
 
 /*

drivers/mtd/spi-nor/fsl-quadspi.c +15 ~~-14~~

index 52a872f..cef3d85 100644
@@ -525,9 +525,9 @@ static inline void fsl_qspi_invalid(struct fsl_qspi *q)
 	writel(reg, q->iobase + QUADSPI_MCR);
 }
 
-static int fsl_qspi_nor_write(struct fsl_qspi *q, struct spi_nor *nor,
+static ssize_t fsl_qspi_nor_write(struct fsl_qspi *q, struct spi_nor *nor,
 				u8 opcode, unsigned int to, u32 *txbuf,
-				unsigned count, size_t *retlen)
+				unsigned count)
 {
 	int ret, i, j;
 	u32 tmp;

@@ -549,8 +549,8 @@ static int fsl_qspi_nor_write(struct fsl_qspi *q, struct spi_nor *nor,
 	/* Trigger it */
 	ret = fsl_qspi_runcmd(q, opcode, to, count);
 
-	if (ret == 0 && retlen)
-		*retlen += count;
+	if (ret == 0)
+		return count;
 
 	return ret;
 }

@@ -703,7 +703,9 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
 
 	} else if (len > 0) {
 		ret = fsl_qspi_nor_write(q, nor, opcode, 0,
-					(u32 *)buf, len, NULL);
+					(u32 *)buf, len);
+		if (ret > 0)
+			return 0;
 	} else {
 		dev_err(q->dev, "invalid cmd %d\n", opcode);
 		ret = -EINVAL;

@@ -712,20 +714,20 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
 	return ret;
 }
 
-static void fsl_qspi_write(struct spi_nor *nor, loff_t to,
-		size_t len, size_t *retlen, const u_char *buf)
+static ssize_t fsl_qspi_write(struct spi_nor *nor, loff_t to,
+			      size_t len, const u_char *buf)
 {
 	struct fsl_qspi *q = nor->priv;
-
-	fsl_qspi_nor_write(q, nor, nor->program_opcode, to,
-				(u32 *)buf, len, retlen);
+	ssize_t ret = fsl_qspi_nor_write(q, nor, nor->program_opcode, to,
+					 (u32 *)buf, len);
 
 	/* invalid the data in the AHB buffer. */
 	fsl_qspi_invalid(q);
+	return ret;
 }
 
-static int fsl_qspi_read(struct spi_nor *nor, loff_t from,
-		size_t len, size_t *retlen, u_char *buf)
+static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
+			     size_t len, u_char *buf)
 {
 	struct fsl_qspi *q = nor->priv;
 	u8 cmd = nor->read_opcode;

@@ -736,8 +738,7 @@ static int fsl_qspi_read(struct spi_nor *nor, loff_t from,
 	/* Read out the data directly from the AHB buffer.*/
 	memcpy(buf, q->ahb_base + q->chip_base_addr + from, len);
 
-	*retlen += len;
-	return 0;
+	return len;
 }
 
 static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)

drivers/mtd/spi-nor/spi-nor.c +41 ~~-16~~

115 lines not on this page · raw message

include/linux/mtd/spi-nor.h +4 -4

index e540952..45972ee 100644
@@ -185,10 +185,10 @@ struct spi_nor {
 	int (*write_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
 			int write_enable);
 
-	int (*read)(struct spi_nor *nor, loff_t from,
-			size_t len, size_t *retlen, u_char *read_buf);
-	void (*write)(struct spi_nor *nor, loff_t to,
-			size_t len, size_t *retlen, const u_char *write_buf);
+	ssize_t (*read)(struct spi_nor *nor, loff_t from,
+			size_t len, u_char *read_buf);
+	ssize_t (*write)(struct spi_nor *nor, loff_t to,
+			size_t len, const u_char *write_buf);
 	int (*erase)(struct spi_nor *nor, loff_t offs);
 
 	int (*flash_lock)(struct spi_nor *nor, loff_t ofs, uint64_t len);

Signature · 1 line

2.1.4

Marek Vasut

2015-07-28 18:15 UTC #2 lore raw

Re: [PATCH 1/2] mtd: spi-nor: rework spi nor read and write.

Quoted 49 lines · Michal Suchanek

The spi_nor read and write functions pass thru the mtd retlen to the chip-specific read and write function. This makes it difficult to check for errors in read and write functions and these errors are not checked. This leads to silent data corruption.
This patch styles the chip-specific read and write function as unix read(2) and write(2) and updates the retlen in the spi-nor generic driver.
This also makes it possible to check for write errors. When pl330 fails to transfer the flash data over SPI I get I/O error instead of 4M of zeroes.
I do not have sst and fsl hardware to test with.
Signed-off-by Michal Suchanek <[email protected]>
---
 drivers/mtd/devices/m25p80.c      | 33 ++++++++++++++---------
 drivers/mtd/spi-nor/fsl-quadspi.c | 29 ++++++++++----------
 drivers/mtd/spi-nor/spi-nor.c     | 57
++++++++++++++++++++++++++++----------- include/linux/mtd/spi-nor.h      
|  8 +++---
 4 files changed, 81 insertions(+), 46 deletions(-)
drivers/mtd/devices/m25p80.c +3 -2
index d313f948b..d8f064b 100644
@@ -75,14 +75,15 @@ static int m25p80_write_reg(struct spi_nor *nor, u8
 opcode, u8 *buf, int len, return spi_write(spi, flash->command, len + 1);
 }
 
-static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
-			size_t *retlen, const u_char *buf)
+static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
+			    const u_char *buf)
 {
 	struct m25p *flash = nor->priv;
 	struct spi_device *spi = flash->spi;
 	struct spi_transfer t[2] = {};
 	struct spi_message m;
 	int cmd_sz = m25p_cmdsz(nor);
+	ssize_t ret;
 
 	spi_message_init(&m);
 
@@ -100,9 +101,14 @@ static void m25p80_write(struct spi_nor *nor, loff_t
 to, size_t len, t[1].len = len;
 	spi_message_add_tail(&t[1], &m);
 

drivers/mtd/devices/m25p80.c static void m25p80_write(struct spi_nor *nor, loff_t · line 111

Quoted 10 lines

drivers/mtd/devices/m25p80.c +7 -2

-	spi_sync(spi, &m);
+	ret = spi_sync(spi, &m);
+	if (ret)
+		return ret;
 
-	*retlen += m.actual_length - cmd_sz;
+	ret = m.actual_length - cmd_sz;
+	if (ret < 0)
+		return -EIO;
+	return ret;

I'd prefer to just add the return value and keep the retlen to keep error codes and transfer length separate.

btw. you change the transfer length from unsigned to signed type -- long transfer might get interpreted as an error.

[...]

Michal Suchanek

author

2015-07-29 04:17 UTC #3 lore raw

Re: [PATCH 1/2] mtd: spi-nor: rework spi nor read and write.

Quoted 55 lines · Marek Vasut

Quoted 54 lines · Michal Suchanek

This patch styles the chip-specific read and write function as unix read(2) and write(2) and updates the retlen in the spi-nor generic driver.

This also makes it possible to check for write errors. When pl330 fails to transfer the flash data over SPI I get I/O error instead of 4M of zeroes.

I do not have sst and fsl hardware to test with.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mtd/devices/m25p80.c      | 33 ++++++++++++++---------
 drivers/mtd/spi-nor/fsl-quadspi.c | 29 ++++++++++----------
 drivers/mtd/spi-nor/spi-nor.c     | 57
++++++++++++++++++++++++++++----------- include/linux/mtd/spi-nor.h
|  8 +++---
 4 files changed, 81 insertions(+), 46 deletions(-)

drivers/mtd/devices/m25p80.c +6 -3

index d313f948b..d8f064b 100644
@@ -75,14 +75,15 @@ static int m25p80_write_reg(struct spi_nor *nor, u8
 opcode, u8 *buf, int len, return spi_write(spi, flash->command, len + 1);
 }
 
-static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
-                     size_t *retlen, const u_char *buf)
+static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
+                         const u_char *buf)
 {
      struct m25p *flash = nor->priv;
      struct spi_device *spi = flash->spi;
      struct spi_transfer t[2] = {};
      struct spi_message m;
      int cmd_sz = m25p_cmdsz(nor);
+     ssize_t ret;
 
      spi_message_init(&m);

@@ -100,9 +101,14 @@ static void m25p80_write(struct spi_nor *nor, loff_t
 to, size_t len, t[1].len = len;
      spi_message_add_tail(&t[1], &m);
 
-     spi_sync(spi, &m);
+     ret = spi_sync(spi, &m);
+     if (ret)
+             return ret;

Quoted 8 lines

drivers/mtd/devices/m25p80.c static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len, · line 111
Quoted 5 lines
drivers/mtd/devices/m25p80.c +4 -1
-     *retlen += m.actual_length - cmd_sz;
+     ret = m.actual_length - cmd_sz;
+     if (ret < 0)
+             return -EIO;
+     return ret;
I'd prefer to just add the return value and keep the retlen to keep error codes and transfer length separate.

I prefer to not pass around retlen because passing it around

- causes code duplication
- makes the code harder to understand

Quoted 3 lines

btw. you change the transfer length from unsigned to signed type -- long transfer might get interpreted as an error.

Note that ssize_t is supposed to be enough for write(2) so when it does not suffice you have a real system-wide problem.

That aside NOR flash sizes tend to be in the order of megabytes so this concern is very theoretical.

Thanks

Michal

Marek Vasut

2015-07-29 17:03 UTC #4 lore raw

Re: [PATCH 1/2] mtd: spi-nor: rework spi nor read and write.

Quoted 76 lines · Michal Suchanek

Quoted 66 lines · Marek Vasut
Quoted 65 lines · Michal Suchanek
The spi_nor read and write functions pass thru the mtd retlen to the chip-specific read and write function. This makes it difficult to check for errors in read and write functions and these errors are not checked. This leads to silent data corruption.
This patch styles the chip-specific read and write function as unix read(2) and write(2) and updates the retlen in the spi-nor generic driver.
This also makes it possible to check for write errors. When pl330 fails to transfer the flash data over SPI I get I/O error instead of 4M of zeroes.
I do not have sst and fsl hardware to test with.
Signed-off-by Michal Suchanek <[email protected]>
---
 drivers/mtd/devices/m25p80.c      | 33 ++++++++++++++---------
 drivers/mtd/spi-nor/fsl-quadspi.c | 29 ++++++++++----------
 drivers/mtd/spi-nor/spi-nor.c     | 57
++++++++++++++++++++++++++++----------- include/linux/mtd/spi-nor.h
| 8 +++---
 4 files changed, 81 insertions(+), 46 deletions(-)
drivers/mtd/devices/m25p80.c +0 -0
index d313f948b..d8f064b 100644
@@ -75,14 +75,15 @@ static int m25p80_write_reg(struct spi_nor *nor, u8
opcode, u8 *buf, int len, return spi_write(spi, flash->command, len + 1);
}
drivers/mtd/devices/m25p80.c +6 -3
-static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
-                     size_t *retlen, const u_char *buf)
+static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
+                         const u_char *buf)
 
 {
 
      struct m25p *flash = nor->priv;
      struct spi_device *spi = flash->spi;
      struct spi_transfer t[2] = {};
      struct spi_message m;
      int cmd_sz = m25p_cmdsz(nor);
 
+     ssize_t ret;
 
      spi_message_init(&m);
@@ -100,9 +101,14 @@ static void m25p80_write(struct spi_nor *nor,
 loff_t to, size_t len, t[1].len = len;
 
      spi_message_add_tail(&t[1], &m);
 
-     spi_sync(spi, &m);
+     ret = spi_sync(spi, &m);
+     if (ret)
+             return ret;
 
Quoted 8 lines
drivers/mtd/devices/m25p80.c static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len, · line 111
Quoted 5 lines
drivers/mtd/devices/m25p80.c +4 -1
-     *retlen += m.actual_length - cmd_sz;
+     ret = m.actual_length - cmd_sz;
+     if (ret < 0)
+             return -EIO;
+     return ret;
I'd prefer to just add the return value and keep the retlen to keep error codes and transfer length separate.

Quoted 4 lines

I prefer to not pass around retlen because passing it around
- causes code duplication
- makes the code harder to understand

Not sure I want to argue on this one, since it looks like a matter of taste. Adding the error return first and removing the retlen second might make the patch more readable though.

Quoted 8 lines

Quoted 2 lines
btw. you change the transfer length from unsigned to signed type -- long transfer might get interpreted as an error.
Note that ssize_t is supposed to be enough for write(2) so when it does not suffice you have a real system-wide problem.
That aside NOR flash sizes tend to be in the order of megabytes so this concern is very theoretical.

I do hope so :)

Michal Suchanek

author

2015-07-28 09:23 UTC #5 lore raw

PATCH 2/2 mtd: spi-nor: rework write loop

In reply to Michal Suchanek

The write loop assumes that what is passed to write() is what gets written. When write() writes less than page size at once data is dropped on the floor. Check the amount of data writen.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mtd/spi-nor/spi-nor.c | 48 ++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 33 deletions(-)

drivers/mtd/spi-nor/spi-nor.c +15 ~~-33~~

index c19fa7d..1cbe0be 100644
@@ -837,7 +837,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	size_t *retlen, const u_char *buf)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
-	u32 page_offset, page_size, i;
+	size_t page_offset, page_remain, i;
 	int ret;
 
 	dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);

@@ -846,45 +846,27 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (ret)
 		return ret;
 
-	write_enable(nor);
-
-	page_offset = to & (nor->page_size - 1);
+	for (i = 0; i < len; ) {
+		int written;
 
-	/* do all the bytes fit onto one page? */
-	if (page_offset + len <= nor->page_size) {
-		ret = nor->write(nor, to, len, buf);
-		if (ret < 0)
-			goto write_err;
-		*retlen += ret;
-	} else {
+		page_offset = to & (nor->page_size - 1);
 		/* the size of data remaining on the first page */
-		page_size = nor->page_size - page_offset;
-		nor->write(nor, to, page_size, buf);
+		page_remain = min_t(size_t,
+				    nor->page_size - page_offset, len - i);
+
+		write_enable(nor);
+		ret = nor->write(nor, to + i, page_remain, buf + i);
 		if (ret < 0)
 			goto write_err;
-		*retlen += ret;
-
-		/* write everything in nor->page_size chunks */
-		for (i = ret; i < len; ) {
-			page_size = len - i;
-			if (page_size > nor->page_size)
-				page_size = nor->page_size;
-
-			ret = spi_nor_wait_till_ready(nor);
-			if (ret)
-				goto write_err;
+		written = ret;
 
-			write_enable(nor);
-
-			ret = nor->write(nor, to + i, page_size, buf + i);
-			if (ret < 0)
-				goto write_err;
-			*retlen += ret;
-			i += ret;
-		}
+		ret = spi_nor_wait_till_ready(nor);
+		if (ret)
+			goto write_err;
+		*retlen += written;
+		i += written;
 	}
 
-	ret = spi_nor_wait_till_ready(nor);
 write_err:
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
 	return ret;

Signature · 1 line

2.1.4