Add spi-nor SPI transfer error handling

PATCH Superseded Michal Suchanek 7 participants 20 messages Aug 14, 2015 – Jan 12, 2016

Subsystems (2)

Memory technology devices (MTD) SPI NOR

lore

Share & tools

lore

Michal Suchanek

author

2015-08-14 09:23 UTC #0 lore raw

v4 cover Add spi-nor SPI transfer error handling

Hello,

with these patches SPI transfer errors are not silently ignored but rather reported to spi-nor users.

This should prevent silently dropping data to the floor in cases when the SPI transfer fails and the failure is detected.

It has been pointed out that MTD users do not handle the case when data is read only partially so this version adds the last patch which handles this in spi-nor.

Thanks

Michal

Michal Suchanek (7):
mtd: spi-nor: change return value of read/write
mtd: m25p80: return amount of data transferred or error in read/write
mtd: fsl-quadspi: return amount of data read/written or error
mtd: spi-nor: check return value from read/write
mtd: spi-nor: stop passing around retlen
mtd: spi-nor: simplify write loop
mtd: spi-nor: add read loop

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

Signature · 1 line

2.1.4

Michal Suchanek

author

2015-08-14 09:23 UTC #1 lore raw

PATCH v4 1/7 mtd: spi-nor: change return value of read/write

Change the return value of spi-nor device read and write methods to allow returning amount of data transferred and errors as read(2)/write(2) does.

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

---

- avoid compiler warning

---
 drivers/mtd/devices/m25p80.c      | 5 +++--
 drivers/mtd/spi-nor/fsl-quadspi.c | 5 +++--
 include/linux/mtd/spi-nor.h       | 4 ++--
 3 files changed, 8 insertions(+), 6 deletions(-)

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

index d313f948b..b2fafc9 100644
@@ -75,7 +75,7 @@ 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,
+static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 			size_t *retlen, const u_char *buf)
 {
 	struct m25p *flash = nor->priv;

@@ -103,6 +103,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 	spi_sync(spi, &m);
 
 	*retlen += m.actual_length - cmd_sz;
+	return 0;
 }
 
 static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

@@ -121,7 +122,7 @@ 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,
+static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 			size_t *retlen, u_char *buf)
 {
 	struct m25p *flash = nor->priv;

drivers/mtd/spi-nor/fsl-quadspi.c +3 -2

index 52a872f..4ad2a68 100644
@@ -712,7 +712,7 @@ 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,
+static ssize_t fsl_qspi_write(struct spi_nor *nor, loff_t to,
 		size_t len, size_t *retlen, const u_char *buf)
 {
 	struct fsl_qspi *q = nor->priv;

@@ -722,9 +722,10 @@ static void fsl_qspi_write(struct spi_nor *nor, loff_t to,
 
 	/* invalid the data in the AHB buffer. */
 	fsl_qspi_invalid(q);
+	return 0;
 }
 
-static int fsl_qspi_read(struct spi_nor *nor, loff_t from,
+static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
 		size_t len, size_t *retlen, u_char *buf)
 {
 	struct fsl_qspi *q = nor->priv;

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

index e540952..7d782cb 100644
@@ -185,9 +185,9 @@ 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,
+	ssize_t (*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,
+	ssize_t (*write)(struct spi_nor *nor, loff_t to,
 			size_t len, size_t *retlen, const u_char *write_buf);
 	int (*erase)(struct spi_nor *nor, loff_t offs);

Signature · 1 line

2.1.4

Michal Suchanek

author

2015-08-14 09:23 UTC #2 lore raw

PATCH v4 2/7 mtd: m25p80: return amount of data transferred or error in read/write

In reply to Michal Suchanek

Add checking of SPI transfer errors and return them from read/write functions. Also return the amount of data transferred.

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

---
 drivers/mtd/devices/m25p80.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

drivers/mtd/devices/m25p80.c +19 -9

index b2fafc9..c93bc41 100644
@@ -83,6 +83,7 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 	struct spi_transfer t[2] = {};
 	struct spi_message m;
 	int cmd_sz = m25p_cmdsz(nor);
+	ssize_t ret;
 
 	spi_message_init(&m);

@@ -100,10 +101,15 @@ static ssize_t 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;
-	return 0;
+	ret = m.actual_length - cmd_sz;
+	if (ret < 0)
+		return -EIO;
+	*retlen += ret;
+	return ret;
 }
 
 static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

@@ -130,6 +136,7 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	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;

@@ -149,10 +156,15 @@ static ssize_t 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;
+	*retlen += ret;
+	return ret;
 }
 
 static int m25p80_erase(struct spi_nor *nor, loff_t offset)

@@ -166,9 +178,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));
 }
 
 /*

Signature · 1 line

2.1.4

Michal Suchanek

author

2015-08-14 09:23 UTC #3 lore raw

PATCH v4 3/7 mtd: fsl-quadspi: return amount of data read/written or error

In reply to Michal Suchanek

Return amount of data read/written or error as read(2)/write(2) does.

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

---
 drivers/mtd/spi-nor/fsl-quadspi.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

drivers/mtd/spi-nor/fsl-quadspi.c +11 -7

index 4ad2a68..a39f09e 100644
@@ -525,7 +525,7 @@ 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)
 {

@@ -549,8 +549,11 @@ 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) {
+		if (retlen)
+			*retlen += count;
+		return count;
+	}
 
 	return ret;
 }

@@ -704,6 +707,8 @@ 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);
+		if (ret > 0)
+			return 0;
 	} else {
 		dev_err(q->dev, "invalid cmd %d\n", opcode);
 		ret = -EINVAL;

@@ -717,12 +722,12 @@ static ssize_t fsl_qspi_write(struct spi_nor *nor, loff_t to,
 {
 	struct fsl_qspi *q = nor->priv;
 
-	fsl_qspi_nor_write(q, nor, nor->program_opcode, to,
+	ssize_t ret = fsl_qspi_nor_write(q, nor, nor->program_opcode, to,
 				(u32 *)buf, len, retlen);
 
 	/* invalid the data in the AHB buffer. */
 	fsl_qspi_invalid(q);
-	return 0;
+	return ret;
 }
 
 static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,

@@ -737,8 +742,7 @@ static ssize_t 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)

Signature · 1 line

2.1.4

Michal Suchanek

author

2015-08-14 09:23 UTC #4 lore raw

PATCH v4 4/7 mtd: spi-nor: check return value from read/write

In reply to Michal Suchanek

SPI NOR hardware drivers now return useful value from their read/write functions so check them.

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

---
 drivers/mtd/spi-nor/spi-nor.c | 50 +++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 14 deletions(-)

drivers/mtd/spi-nor/spi-nor.c +36 ~~-14~~

index d78831b..5116bbd 100644
@@ -741,7 +741,10 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	ret = nor->read(nor, from, len, retlen, buf);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
-	return ret;
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,

@@ -767,10 +770,14 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 		nor->program_opcode = SPINOR_OP_BP;
 
 		/* write one byte. */
-		nor->write(nor, to, 1, retlen, buf);
+		ret = nor->write(nor, to, 1, retlen, buf);
+		if (ret < 0)
+			goto sst_write_err;
+		WARN(ret != 1, "While writing 1 byte written %i bytes\n",
+		     (int)ret);
 		ret = spi_nor_wait_till_ready(nor);
 		if (ret)
-			goto time_out;
+			goto sst_write_err;
 	}
 	to += actual;

@@ -779,10 +786,14 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 		nor->program_opcode = SPINOR_OP_AAI_WP;
 
 		/* write two bytes. */
-		nor->write(nor, to, 2, retlen, buf + actual);
+		ret = nor->write(nor, to, 2, retlen, buf + actual);
+		if (ret < 0)
+			goto sst_write_err;
+		WARN(ret != 2, "While writing 2 bytes written %i bytes\n",
+		     (int)ret);
 		ret = spi_nor_wait_till_ready(nor);
 		if (ret)
-			goto time_out;
+			goto sst_write_err;
 		to += 2;
 		nor->sst_write_second = true;
 	}

@@ -791,21 +802,24 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 	write_disable(nor);
 	ret = spi_nor_wait_till_ready(nor);
 	if (ret)
-		goto time_out;
+		goto sst_write_err;
 
 	/* Write out trailing byte if it exists. */
 	if (actual != len) {
 		write_enable(nor);
 
 		nor->program_opcode = SPINOR_OP_BP;
-		nor->write(nor, to, 1, retlen, buf + actual);
-
+		ret = nor->write(nor, to, 1, retlen, buf + actual);
+		if (ret < 0)
+			goto sst_write_err;
+		WARN(ret != 1, "While writing 1 byte written %i bytes\n",
+		     (int)ret);
 		ret = spi_nor_wait_till_ready(nor);
 		if (ret)
-			goto time_out;
+			goto sst_write_err;
 		write_disable(nor);
 	}
-time_out:
+sst_write_err:
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
 	return ret;
 }

@@ -834,14 +848,18 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 	/* do all the bytes fit onto one page? */
 	if (page_offset + len <= nor->page_size) {
-		nor->write(nor, to, len, retlen, buf);
+		ret = nor->write(nor, to, len, retlen, buf);
+		if (ret < 0)
+			goto write_err;
 	} else {
 		/* the size of data remaining on the first page */
 		page_size = nor->page_size - page_offset;
-		nor->write(nor, to, page_size, retlen, buf);
+		ret = nor->write(nor, to, page_size, retlen, buf);
+		if (ret < 0)
+			goto write_err;
 
 		/* write everything in nor->page_size chunks */
-		for (i = page_size; i < len; i += page_size) {
+		for (i = ret; i < len; ) {
 			page_size = len - i;
 			if (page_size > nor->page_size)
 				page_size = nor->page_size;

@@ -852,7 +870,11 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 			write_enable(nor);
 
-			nor->write(nor, to + i, page_size, retlen, buf + i);
+			ret = nor->write(nor, to + i, page_size, retlen,
+					 buf + i);
+			if (ret < 0)
+				goto write_err;
+			i += ret;
 		}
 	}

Signature · 1 line

2.1.4

Michal Suchanek

author

2015-08-14 09:23 UTC #5 lore raw

PATCH v4 5/7 mtd: spi-nor: stop passing around retlen

In reply to Michal Suchanek

Do not pass retlen to hardware driver read/write functions. Update it in spi-nor generic driver instead.

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

---
 drivers/mtd/devices/m25p80.c      |  6 ++----
 drivers/mtd/spi-nor/fsl-quadspi.c | 16 ++++++----------
 drivers/mtd/spi-nor/spi-nor.c     | 21 +++++++++++++--------
 include/linux/mtd/spi-nor.h       |  4 ++--
 4 files changed, 23 insertions(+), 24 deletions(-)

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

index c93bc41..d8f064b 100644
@@ -76,7 +76,7 @@ static int m25p80_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
 }
 
 static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
-			size_t *retlen, const u_char *buf)
+			    const u_char *buf)
 {
 	struct m25p *flash = nor->priv;
 	struct spi_device *spi = flash->spi;

@@ -108,7 +108,6 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 	ret = m.actual_length - cmd_sz;
 	if (ret < 0)
 		return -EIO;
-	*retlen += ret;
 	return ret;
 }

@@ -129,7 +128,7 @@ static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
  * may be any size provided it is within the physical boundaries.
  */
 static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
-			size_t *retlen, u_char *buf)
+			   u_char *buf)
 {
 	struct m25p *flash = nor->priv;
 	struct spi_device *spi = flash->spi;

@@ -163,7 +162,6 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	ret = m.actual_length - m25p_cmdsz(nor) - dummy;
 	if (ret < 0)
 		return -EIO;
-	*retlen += ret;
 	return ret;
 }

drivers/mtd/spi-nor/fsl-quadspi.c +6 ~~-10~~

index a39f09e..cef3d85 100644
@@ -527,7 +527,7 @@ static inline void fsl_qspi_invalid(struct fsl_qspi *q)
 
 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,11 +549,8 @@ static ssize_t 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) {
-		if (retlen)
-			*retlen += count;
+	if (ret == 0)
 		return count;
-	}
 
 	return ret;
 }

@@ -706,7 +703,7 @@ 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 {

@@ -718,12 +715,11 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
 }
 
 static ssize_t fsl_qspi_write(struct spi_nor *nor, loff_t to,
-		size_t len, size_t *retlen, const u_char *buf)
+			      size_t len, const u_char *buf)
 {
 	struct fsl_qspi *q = nor->priv;
-
 	ssize_t ret = fsl_qspi_nor_write(q, nor, nor->program_opcode, to,
-				(u32 *)buf, len, retlen);
+					 (u32 *)buf, len);
 
 	/* invalid the data in the AHB buffer. */
 	fsl_qspi_invalid(q);

@@ -731,7 +727,7 @@ static ssize_t fsl_qspi_write(struct spi_nor *nor, loff_t to,
 }
 
 static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
-		size_t len, size_t *retlen, u_char *buf)
+			     size_t len, u_char *buf)
 {
 	struct fsl_qspi *q = nor->priv;
 	u8 cmd = nor->read_opcode;

drivers/mtd/spi-nor/spi-nor.c +13 -8

index 5116bbd..837e3df 100644
@@ -738,12 +738,13 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (ret)
 		return ret;
 
-	ret = nor->read(nor, from, len, retlen, buf);
+	ret = nor->read(nor, from, len, buf);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
 	if (ret < 0)
 		return ret;
 
+	*retlen += ret;
 	return 0;
 }

@@ -770,7 +771,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 		nor->program_opcode = SPINOR_OP_BP;
 
 		/* write one byte. */
-		ret = nor->write(nor, to, 1, retlen, buf);
+		ret = nor->write(nor, to, 1, buf);
 		if (ret < 0)
 			goto sst_write_err;
 		WARN(ret != 1, "While writing 1 byte written %i bytes\n",

@@ -786,7 +787,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 		nor->program_opcode = SPINOR_OP_AAI_WP;
 
 		/* write two bytes. */
-		ret = nor->write(nor, to, 2, retlen, buf + actual);
+		ret = nor->write(nor, to, 2, buf + actual);
 		if (ret < 0)
 			goto sst_write_err;
 		WARN(ret != 2, "While writing 2 bytes written %i bytes\n",

@@ -809,7 +810,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 		write_enable(nor);
 
 		nor->program_opcode = SPINOR_OP_BP;
-		ret = nor->write(nor, to, 1, retlen, buf + actual);
+		ret = nor->write(nor, to, 1, buf + actual);
 		if (ret < 0)
 			goto sst_write_err;
 		WARN(ret != 1, "While writing 1 byte written %i bytes\n",

@@ -818,8 +819,10 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 		if (ret)
 			goto sst_write_err;
 		write_disable(nor);
+		actual += 1;
 	}
 sst_write_err:
+	*retlen += actual;
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
 	return ret;
 }

@@ -848,15 +851,17 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 	/* do all the bytes fit onto one page? */
 	if (page_offset + len <= nor->page_size) {
-		ret = nor->write(nor, to, len, retlen, buf);
+		ret = nor->write(nor, to, len, buf);
 		if (ret < 0)
 			goto write_err;
+		*retlen += ret;
 	} else {
 		/* the size of data remaining on the first page */
 		page_size = nor->page_size - page_offset;
-		ret = nor->write(nor, to, page_size, retlen, buf);
+		ret = nor->write(nor, to, page_size, buf);
 		if (ret < 0)
 			goto write_err;
+		*retlen += ret;
 
 		/* write everything in nor->page_size chunks */
 		for (i = ret; i < len; ) {

@@ -870,10 +875,10 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 			write_enable(nor);
 
-			ret = nor->write(nor, to + i, page_size, retlen,
-					 buf + i);
+			ret = nor->write(nor, to + i, page_size, buf + i);
 			if (ret < 0)
 				goto write_err;
+			*retlen += ret;
 			i += ret;
 		}
 	}

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

index 7d782cb..45972ee 100644
@@ -186,9 +186,9 @@ struct spi_nor {
 			int write_enable);
 
 	ssize_t (*read)(struct spi_nor *nor, loff_t from,
-			size_t len, size_t *retlen, u_char *read_buf);
+			size_t len, u_char *read_buf);
 	ssize_t (*write)(struct spi_nor *nor, loff_t to,
-			size_t len, size_t *retlen, const u_char *write_buf);
+			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

Michal Suchanek

author

2015-08-14 09:23 UTC #6 lore raw

PATCH v4 6/7 mtd: spi-nor: simplify write loop

In reply to Michal Suchanek

The spi-nor write loop assumes that what is passed to the hardware driver 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.

This also means that write can start mid-page any time so there is no special case for first page.

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 837e3df..e0ae9cf 100644
@@ -836,7 +836,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);

@@ -845,45 +845,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;
-		ret = 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

Brian Norris

2015-11-19 23:19 UTC #7 lore raw

Re: [PATCH v4 6/7] mtd: spi-nor: simplify write loop

Hi Michal,

Sorry this has sat so long...

Quoted 5 lines · Michal Suchanek

The spi-nor write loop assumes that what is passed to the hardware driver 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.

Have you seen write() return less than the page size? I know you've struggled with a SPI driver that can't do "very" (for some definition of very) long transfers, due to unknown bugs, but I thought that "very" was much larger than the page size.

Quoted 2 lines

This also means that write can start mid-page any time so there is no special case for first page.

I think we'd have some problems if we start seeing hardware that can't write ~256 bytes at a time. If nothing else, this can be a problem because some SPI NOR flash are known to have inferior reliability if you regularly write in small chunks. I believe this is because they actually use some kind of internal ECC.

So, if you're just guarding against a theoretical behavior, perhaps it's best if this is done with some kind of assertion, as I'd rather not encourage non-aligned writes if possible. I notice you used BUG_ON() in another patch, but I'd suggest maybe something less harsh, like WARN() or WARN_ONCE().

Brian

Quoted 82 lines

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 837e3df..e0ae9cf 100644
@@ -836,7 +836,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);

@@ -845,45 +845,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;
-		ret = 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

Michal Suchanek

author

2015-11-20 19:00 UTC #8 lore raw

Re: [PATCH v4 6/7] mtd: spi-nor: simplify write loop

Quoted 8 lines · Brian Norris

Hi Michal,
Sorry this has sat so long...
Quoted 3 lines · Michal Suchanek
The spi-nor write loop assumes that what is passed to the hardware driver write() is what gets written.

Quoted 7 lines

Quoted 2 lines
When write() writes less than page size at once data is dropped on the floor. Check the amount of data writen.
Have you seen write() return less than the page size? I know you've struggled with a SPI driver that can't do "very" (for some definition of very) long transfers, due to unknown bugs, but I thought that "very" was much larger than the page size.

Not in the case of a driver that can transfer 63 bytes at a time. It because the DMA engine needed for normal operation was not merged yet, though.

Quoted 10 lines

Quoted 3 lines
This also means that write can start mid-page any time so there is no special case for first page.
I think we'd have some problems if we start seeing hardware that can't write ~256 bytes at a time. If nothing else, this can be a problem because some SPI NOR flash are known to have inferior reliability if you regularly write in small chunks. I believe this is because they actually use some kind of internal ECC.

Quoted 5 lines

So, if you're just guarding against a theoretical behavior, perhaps it's best if this is done with some kind of assertion, as I'd rather not encourage non-aligned writes if possible. I notice you used BUG_ON() in another patch, but I'd suggest maybe something less harsh, like WARN() or WARN_ONCE().

I agree that a warning is in order in the case when whole page cannot be written in one go.

Thanks

Michal

Michal Suchanek

author

2015-08-14 09:31 UTC #9 lore raw

PATCH v4 7/7 mtd: spi-nor: add read loop

In reply to Michal Suchanek

mtdblock and ubi do not handle the situation when read returns less data than requested. Loop in spi-nor until buffer is filled or an error is returned.

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

---
 drivers/mtd/spi-nor/spi-nor.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

drivers/mtd/spi-nor/spi-nor.c +14 -6

index e0ae9cf..246fac7 100644
@@ -738,14 +738,22 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (ret)
 		return ret;
 
-	ret = nor->read(nor, from, len, buf);
+	while (len) {
+		ret = nor->read(nor, from, len, buf);
+		if (ret <= 0)
+			goto read_err;
+
+		BUG_ON(ret > len);
+		*retlen += ret;
+		buf += ret;
+		from += ret;
+		len -= ret;
+	}
+	ret = 0;
 
+read_err:
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
-	if (ret < 0)
-		return ret;
-
-	*retlen += ret;
-	return 0;
+	return ret;
 }
 
 static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,

Signature · 1 line

2.1.4

Andrew Murray

2015-08-14 10:02 UTC #10 lore raw

Re: [PATCH v4 7/7] mtd: spi-nor: add read loop

Quoted 15 lines · Michal Suchanek

mtdblock and ubi do not handle the situation when read returns less data than requested. Loop in spi-nor until buffer is filled or an error is returned.
Signed-off-by Michal Suchanek <[email protected]>
---
 drivers/mtd/spi-nor/spi-nor.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
drivers/mtd/spi-nor/spi-nor.c +0 -0
index e0ae9cf..246fac7 100644
@@ -738,14 +738,22 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
        if (ret)

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, · line 747

Quoted 10 lines

drivers/mtd/spi-nor/spi-nor.c +7 -1

                return ret;
 
-       ret = nor->read(nor, from, len, buf);
+       while (len) {
+               ret = nor->read(nor, from, len, buf);
+               if (ret <= 0)
+                       goto read_err;
+
+               BUG_ON(ret > len);
+               *retlen += ret;

Is *retlen initialized to 0 anywhere?

Andrew Murray

Quoted 24 lines

drivers/mtd/spi-nor/spi-nor.c +7 -6

+               buf += ret;
+               from += ret;
+               len -= ret;
+       }
+       ret = 0;
 
+read_err:
        spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
-       if (ret < 0)
-               return ret;
-
-       *retlen += ret;
-       return 0;
+       return ret;
 }
 
 static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
--

2.1.4

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Michal Suchanek

author

2015-08-14 10:08 UTC #11 lore raw

Re: [PATCH v4 7/7] mtd: spi-nor: add read loop

Quoted 18 lines · Andrew Murray

Quoted 17 lines · Michal Suchanek
mtdblock and ubi do not handle the situation when read returns less data than requested. Loop in spi-nor until buffer is filled or an error is returned.
Signed-off-by Michal Suchanek <[email protected]>
---
 drivers/mtd/spi-nor/spi-nor.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
drivers/mtd/spi-nor/spi-nor.c +0 -0
index e0ae9cf..246fac7 100644
@@ -738,14 +738,22 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
        if (ret)
                return ret;
 

Quoted 10 lines

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, · line 747
Quoted 8 lines
drivers/mtd/spi-nor/spi-nor.c +7 -1
-       ret = nor->read(nor, from, len, buf);
+       while (len) {
+               ret = nor->read(nor, from, len, buf);
+               if (ret <= 0)
+                       goto read_err;
+
+               BUG_ON(ret > len);
+               *retlen += ret;
Is *retlen initialized to 0 anywhere?

It's initialized in mtdcore and passed into mtd->_read.

Yes, the interface is really awkward.

int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
u_char *buf)

{
        int ret_code;
        *retlen = 0;

Thanks

Michal

Hou Zhiqiang

2015-11-05 03:39 UTC #12 lore raw

RE: [PATCH v4 7/7] mtd: spi-nor: add read loop

Hi Michal,

Does this have any updates?

Thanks,
Zhiqiang

Quoted 56 lines

-----Original Message-----
From: Michal Suchanek [mailto:[email protected]]
Sent: 2015年8月14日 18:08
To: Andrew Murray
Cc: Hou Zhiqiang-B48286; Huang Shijie; David Woodhouse; Brian Norris; Xu Han-B45815; Rafał Miłecki; Huang Shijie; Ben Hutchings; Marek Vasut; Gabor Juhos; Bean Huo 霍斌斌,; MTD Maling List; LKML
Subject: Re: [PATCH v4 7/7] mtd: spi-nor: add read loop
Quoted 10 lines · Andrew Murray
Quoted 9 lines · Michal Suchanek
mtdblock and ubi do not handle the situation when read returns less data than requested. Loop in spi-nor until buffer is filled or an error is returned.
Signed-off-by Michal Suchanek <[email protected]>
---
 drivers/mtd/spi-nor/spi-nor.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
Quoted 5 lines
drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd,
Quoted 5 lines
(quoted diff) +0 -0
b/drivers/mtd/spi-nor/spi-nor.c index e0ae9cf..246fac7 100644
drivers/mtd/spi-nor/spi-nor.c +0 -0
@@ -738,14 +738,22 @@ static int spi_nor_read(struct mtd_info *mtd,
loff_t from, size_t len,
Quoted 12 lines
drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, · line 747
Quoted 11 lines
drivers/mtd/spi-nor/spi-nor.c +7 -1
        if (ret)
                return ret;
 
-       ret = nor->read(nor, from, len, buf);
+       while (len) {
+               ret = nor->read(nor, from, len, buf);
+               if (ret <= 0)
+                       goto read_err;
+
+               BUG_ON(ret > len);
+               *retlen += ret;
Quoted 1 line
Is *retlen initialized to 0 anywhere?
It's initialized in mtdcore and passed into mtd->_read.
Yes, the interface is really awkward.
int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
u_char *buf)
{
        int ret_code;
        *retlen = 0;
Thanks
Michal

Michal Suchanek

author

2015-11-20 19:19 UTC #13 lore raw

Re: [PATCH v4 7/7] mtd: spi-nor: add read loop

Hello,

Quoted 3 lines · Hou Zhiqiang

Hi Michal,
Does this have any updates?

I will try to update this patchset when it's agreed how the limit on transfer size is handled. Writing less data than was requested is acceptable for spi-nor but might disrupt other drivers so perhaps some preemptive mechanism with the SPI master saying how much data it can transfer and the drivers looking at that value would be preferred.

Thanks

Michal

Zhiqiang Hou

2016-01-12 06:35 UTC #14 lore raw

RE: [PATCH v4 7/7] mtd: spi-nor: add read loop

Hi Michal,

Thanks for your info, and I see you have updated the patchset on Dec.02. Is it the conclusion, right?

Quoted 14 lines

-----Original Message-----
From: Michal Suchanek [mailto:[email protected]]
Sent: 2015年11月21日 3:19
To: Hou Zhiqiang-B48286
Cc: Andrew Murray; Huang Shijie; David Woodhouse; Brian Norris; Xu Han- B45815; Rafa? Mi?ecki; Huang Shijie; Ben Hutchings; Marek Vasut; Gabor Juhos; Bean Huo 霍斌斌,; MTD Maling List; LKML
Subject: Re: [PATCH v4 7/7] mtd: spi-nor: add read loop
Hello,
Quoted 2 lines · Hou Zhiqiang
Hi Michal,

Quoted 8 lines

Quoted 1 line
Does this have any updates?
I will try to update this patchset when it's agreed how the limit on transfer size is handled. Writing less data than was requested is acceptable for spi-nor but might disrupt other drivers so perhaps some preemptive mechanism with the SPI master saying how much data it can transfer and the drivers looking at that value would be preferred.

Thanks,
Zhiqiang

Brian Norris

2015-11-19 23:39 UTC #15 lore raw

Re: [PATCH v4 7/7] mtd: spi-nor: add read loop

In reply to Michal Suchanek

+ Heiner

Quoted 3 lines · Michal Suchanek

mtdblock and ubi do not handle the situation when read returns less data than requested. Loop in spi-nor until buffer is filled or an error is returned.

I'm slightly torn on this patch. I believe this is inspired by issues in the SPI layer. But I believe there is some agreement from the SPI layer that protocol drivers (such as m25p80.c/spi-nor.c) should not have to worry about spi_messages getting truncated [1]. Heiner is looking at that.

But on the other hand, it's possible that some non-SPI driver would have similar limitations, and so spi-nor.c should be handling the issue.

Quoted 10 lines

Signed-off-by Michal Suchanek <[email protected]>
---
 drivers/mtd/spi-nor/spi-nor.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
drivers/mtd/spi-nor/spi-nor.c +0 -0
index e0ae9cf..246fac7 100644

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, · line 744

Quoted 8 lines

drivers/mtd/spi-nor/spi-nor.c +4 -1

@@ -738,14 +738,22 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (ret)
 		return ret;
 
-	ret = nor->read(nor, from, len, buf);
+	while (len) {
+		ret = nor->read(nor, from, len, buf);
+		if (ret <= 0)
+			goto read_err;

Do we really want to exit silently when ret==0, but len!=0?

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, · line 746

Quoted 2 lines

drivers/mtd/spi-nor/spi-nor.c +2 -0
+
+		BUG_ON(ret > len);

Maybe the condition here should be 'ret > len || len == 0', since this shouldn't happen.

Also, please don't use BUG_ON(). Even though this is really unexpected, we don't need to crash the system. Perhaps:

		if (WARN_ON(ret > len || ret == 0)) {
			ret = -EIO;
			goto read_err;
		}

Quoted 8 lines

drivers/mtd/spi-nor/spi-nor.c +7 -0

+		*retlen += ret;
+		buf += ret;
+		from += ret;
+		len -= ret;
+	}
+	ret = 0;
 
+read_err:

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, · line 759

Quoted 10 lines

drivers/mtd/spi-nor/spi-nor.c +1 -5

 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
-	if (ret < 0)
-		return ret;
-
-	*retlen += ret;
-	return 0;
+	return ret;
 }
 
 static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,

Brian

[1] http://www.spinics.net/lists/linux-spi/msg05877.html
"RfC: Handle SPI controller limitations like maximum message length"

I'm honestly not really sure what the conclusion from that thread is, so far. It seems like the SPI master "should" be exposing its max length to the SPI core, but it's unclear whether that would get propagated as a short write/read (i.e., shorter-than-expected spi_message::actual_length), or whether the SPI core should be somehow splitting up the messages into manageable chunks for us. I'm not even sure if the latter is legal for things like read-from-flash; might this cause the chip select to get toggled, potentially disrupting the operation?

Anyway, in the former case, we need something like your patch. But in the latter case, we actually don't need anything, other than maybe an assertion that ret == len.

Heiner Kallweit

2015-11-20 06:27 UTC #16 lore raw

Re: [PATCH v4 7/7] mtd: spi-nor: add read loop

Am 20.11.2015 um 00:39 schrieb Brian Norris:

Quoted 75 lines

+ Heiner
Quoted 3 lines · Michal Suchanek
mtdblock and ubi do not handle the situation when read returns less data than requested. Loop in spi-nor until buffer is filled or an error is returned.
I'm slightly torn on this patch. I believe this is inspired by issues in the SPI layer. But I believe there is some agreement from the SPI layer that protocol drivers (such as m25p80.c/spi-nor.c) should not have to worry about spi_messages getting truncated [1]. Heiner is looking at that.
But on the other hand, it's possible that some non-SPI driver would have similar limitations, and so spi-nor.c should be handling the issue.
Quoted 10 lines
Signed-off-by Michal Suchanek <[email protected]>
---
 drivers/mtd/spi-nor/spi-nor.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
drivers/mtd/spi-nor/spi-nor.c +0 -0
index e0ae9cf..246fac7 100644
drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, · line 744
Quoted 8 lines
drivers/mtd/spi-nor/spi-nor.c +4 -1
@@ -738,14 +738,22 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (ret)
 		return ret;
 
-	ret = nor->read(nor, from, len, buf);
+	while (len) {
+		ret = nor->read(nor, from, len, buf);
+		if (ret <= 0)
+			goto read_err;
Do we really want to exit silently when ret==0, but len!=0?
drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, · line 746
Quoted 2 lines
drivers/mtd/spi-nor/spi-nor.c +2 -0
+
+		BUG_ON(ret > len);
Maybe the condition here should be 'ret > len || len == 0', since this shouldn't happen.
Also, please don't use BUG_ON(). Even though this is really unexpected, we don't need to crash the system. Perhaps:
		if (WARN_ON(ret > len || ret == 0)) {
			ret = -EIO;
			goto read_err;
		}
Quoted 8 lines
drivers/mtd/spi-nor/spi-nor.c +7 -0
+		*retlen += ret;
+		buf += ret;
+		from += ret;
+		len -= ret;
+	}
+	ret = 0;
 
+read_err:
drivers/mtd/spi-nor/spi-nor.c static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, · line 759
Quoted 10 lines
drivers/mtd/spi-nor/spi-nor.c +1 -5
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
-	if (ret < 0)
-		return ret;
-
-	*retlen += ret;
-	return 0;
+	return ret;
 }
 
 static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
Brian
[1] http://www.spinics.net/lists/linux-spi/msg05877.html
"RfC: Handle SPI controller limitations like maximum message length"

Quoted 9 lines

I'm honestly not really sure what the conclusion from that thread is, so far. It seems like the SPI master "should" be exposing its max length to the SPI core, but it's unclear whether that would get propagated as a short write/read (i.e., shorter-than-expected spi_message::actual_length), or whether the SPI core should be somehow splitting up the messages into manageable chunks for us. I'm not even sure if the latter is legal for things like read-from-flash; might this cause the chip select to get toggled, potentially disrupting the operation?

That's exactly my issue with Freescale ESPI.
You provide a message length (up to 64K) to the chip and it sets / resets CS internally. Resuming a short read requires to set the (SPI NOR) start address for the next read properly.
And this can be done by the protocol driver only.
Currently the fsl-espi controller driver includes some ugly (protocol driver) logic to deal with this.
One consequence is that this controller driver can be used with SPI NOR devices only.

At a first glance I see two options:

1. The controller driver returns an error like EMSGSIZE and the number of read bytes. Then the protocol driver can loop and assemble the chunks.

2. The SPI master exposes the SPI message length constraint and the protocol driver can proactively deal with this limitation.

I have a little headache with option 1 because I'm not sure that we can in general rely on the number of read bytes if an error is returned. Some controller driver might return whatever value assuming that the caller ignores it anyway if an error is returned.

Options 2 I like better as it doesn't require the controller driver to handle an error situation like a short read in a specific way.

And it seems like Mark could be fine with an additional member of spi_master like size_t max_msg_size.
The idea of a struct encapsulating all constraints he didn't like too much.

Quoted 5 lines

Anyway, in the former case, we need something like your patch. But in the latter case, we actually don't need anything, other than maybe an assertion that ret == len.

Bean Huo 霍斌斌 (beanhuo)

2015-08-15 01:51 UTC #17 lore raw

In reply to Michal Suchanek

Quoted 11 lines

Hello,
with these patches SPI transfer errors are not silently ignored but rather reported to spi-nor users.
This should prevent silently dropping data to the floor in cases when the SPI transfer fails and the failure is detected.
It has been pointed out that MTD users do not handle the case when data is read only partially so this version adds the last patch which handles this in spi-nor.
Thanks
Michal

Seems parallel nand read/write also has the same condition.

Michal Suchanek

author

2015-08-16 10:20 UTC #18 lore raw

Hello,

Quoted 4 lines · Bean Huo 霍斌斌 (beanhuo)

Quoted 3 lines
Hello,
with these patches SPI transfer errors are not silently ignored but rather reported to spi-nor users.

Quoted 8 lines

Quoted 7 lines
This should prevent silently dropping data to the floor in cases when the SPI transfer fails and the failure is detected.
It has been pointed out that MTD users do not handle the case when data is read only partially so this version adds the last patch which handles this in spi-nor.
Thanks
Michal
Seems parallel nand read/write also has the same condition.

I am not familiar with parallel NAND drivers so I have no idea if parallel nand can fail in similar way.

As I understand it the parallel nand controller is dedicated piece of hardware just for accessing the nand so there may not be any problems similar to what the generic SPI bus has.

Thanks

Michal

Brian Norris

2015-11-19 23:43 UTC #19 lore raw

In reply to Michal Suchanek

Hi Michal,

Quoted 14 lines · Michal Suchanek

Hello,
with these patches SPI transfer errors are not silently ignored but rather reported to spi-nor users.
This should prevent silently dropping data to the floor in cases when the SPI transfer fails and the failure is detected.
It has been pointed out that MTD users do not handle the case when data is read only partially so this version adds the last patch which handles this in spi-nor.
Thanks

Quoted 8 lines

Michal
Michal Suchanek (7):
mtd: spi-nor: change return value of read/write
mtd: m25p80: return amount of data transferred or error in read/write
mtd: fsl-quadspi: return amount of data read/written or error
mtd: spi-nor: check return value from read/write
mtd: spi-nor: stop passing around retlen

Patches 1 to 5 look good to me, though there is a new spi-nor driver since you sent this. Can you rebase/fixup?

Quoted 2 lines

mtd: spi-nor: simplify write loop
mtd: spi-nor: add read loop

I posted some comments for these. I get the feeling that patch 6 is over-complicated and should be just a simple assertion. And I'm still not 100% sure on the approach for patch 7, since it's not clear whether the responsibility lies in the SPI layer or in MTD.

Brian

Quoted 9 lines

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

Signature · 1 line

2.1.4