fix error checking in spi-nor read and write functions

PATCH Superseded Michal Suchanek 2 participants 12 messages Aug 3, 2015 – Aug 5, 2015

Subsystems (2)

Memory technology devices (MTD) SPI NOR

lore

Share & tools

lore

Michal Suchanek

author

2015-08-03 18:39 UTC #0 lore raw

v2 cover fix error checking in spi-nor read and write functions

Hello,

This is update of the spi-nor patchset that implements error checking in SPI transfers.

It's been pointed out that the first patch is too big so I split it into smaller incremental patches.

With these patches writing to NOR flash connected to a SPI controller with transfer size limitation should work properly.

Also when a transfer fails it should be now reported all the way to the userspace rather than silently ignored.

Thanks

Michal

Michal Suchanek (6):
mtd: spi-nor: change return value of read/write
mtd: m25p80: Add read/write error check and return
mtd: fsl-quadspi: update read/write function prototype
mtd: spi-nor: check return value from read/write
mtd: spi-nor: stop passing around retlen
mtd: spi-nor: simplify 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, 84 insertions(+), 65 deletions(-)

Signature · 1 line

2.1.4

Michal Suchanek

author

2015-08-03 18:46 UTC #1 lore raw

PATCH v2 1/6 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]>

---
 include/linux/mtd/spi-nor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

Marek Vasut

2015-08-03 22:18 UTC #2 lore raw

Re: [PATCH v2 1/6] mtd: spi-nor: change return value of read/write

Quoted 14 lines · Michal Suchanek

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]>
---
 include/linux/mtd/spi-nor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/mtd/spi-nor.h +0 -0
index e540952..7d782cb 100644

include/linux/mtd/spi-nor.h struct spi_nor { · line 192

Quoted 10 lines

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

@@ -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);

You realize that if someone does bisect and has only this patch applied, the compiler will complain loudly about mismatching data types, right ? :)

Best regards,
Marek Vasut

Michal Suchanek

author

2015-08-04 06:43 UTC #3 lore raw

Re: [PATCH v2 1/6] mtd: spi-nor: change return value of read/write

Quoted 18 lines · Marek Vasut

Quoted 17 lines · Michal Suchanek
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]>
---
 include/linux/mtd/spi-nor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/mtd/spi-nor.h +0 -0
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);
 

Quoted 10 lines

include/linux/mtd/spi-nor.h struct spi_nor { · line 192

Quoted 7 lines

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

-     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);

You realize that if someone does bisect and has only this patch applied, the compiler will complain loudly about mismatching data types, right ? :)

Yes, the compiler prints a warning. However, only the return value which is not used changes so it should not cause any real problem. The data type in the fsl-quadspi and m25p80 drivers is matched in the following two patches.

Thanks

Michal

Marek Vasut

2015-08-04 16:42 UTC #4 lore raw

Re: [PATCH v2 1/6] mtd: spi-nor: change return value of read/write

Quoted 33 lines · Michal Suchanek

Quoted 32 lines · Marek Vasut

Quoted 31 lines · Michal Suchanek

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

---

 include/linux/mtd/spi-nor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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);

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
      int (*erase)(struct spi_nor *nor, loff_t offs);
You realize that if someone does bisect and has only this patch applied, the compiler will complain loudly about mismatching data types, right ? :)
Yes, the compiler prints a warning. However, only the return value which is not used changes so it should not cause any real problem.

Are you certain that ssize_t is equal to int , always , everywhere ? :-) Note that it is possible to do such conversion even without introducing compiler warnings along the way -- just do the data type conversion first and the other changes in subsequent patches. It will be clear that one patch does one thing then.

Quoted 7 lines

The
data type in the fsl-quadspi and m25p80 drivers is matched in the following two patches.
Thanks
Michal

Best regards,
Marek Vasut

Michal Suchanek

author

2015-08-05 08:27 UTC #5 lore raw

Re: [PATCH v2 1/6] mtd: spi-nor: change return value of read/write

Quoted 40 lines · Marek Vasut

Quoted 39 lines · Michal Suchanek
Quoted 37 lines · Marek Vasut
Quoted 22 lines · Michal Suchanek
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]>
---
 include/linux/mtd/spi-nor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/mtd/spi-nor.h +1 -1
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,
include/linux/mtd/spi-nor.h struct spi_nor {
Quoted 10 lines
include/linux/mtd/spi-nor.h +1 -1
 
                      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);
You realize that if someone does bisect and has only this patch applied, the compiler will complain loudly about mismatching data types, right ? :)

Quoted 4 lines

Quoted 2 lines
Yes, the compiler prints a warning. However, only the return value which is not used changes so it should not cause any real problem.
Are you certain that ssize_t is equal to int , always , everywhere ? :-)

No, it's larger or equal.

Quoted 4 lines

Note that it is possible to do such conversion even without introducing compiler warnings along the way -- just do the data type conversion first and the other changes in subsequent patches. It will be clear that one patch does one thing then.

If this is the only concern with these patches it is easily amended ;-)

Thanks

Michal

Marek Vasut

2015-08-05 09:11 UTC #6 lore raw

Re: [PATCH v2 1/6] mtd: spi-nor: change return value of read/write

Quoted 42 lines · Michal Suchanek

Quoted 41 lines · Marek Vasut
Quoted 40 lines · Michal Suchanek
Quoted 39 lines · Marek Vasut
Quoted 23 lines · Michal Suchanek
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]>
---
 include/linux/mtd/spi-nor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/include/linux/mtd/spi-nor.h index e540952..7d782cb 100644
include/linux/mtd/spi-nor.h +1 -1
@@ -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,
include/linux/mtd/spi-nor.h struct spi_nor {
Quoted 10 lines
include/linux/mtd/spi-nor.h +1 -1
 
                      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);
You realize that if someone does bisect and has only this patch applied, the compiler will complain loudly about mismatching data types, right ?

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
:)
Yes, the compiler prints a warning. However, only the return value which is not used changes so it should not cause any real problem.
Are you certain that ssize_t is equal to int , always , everywhere ? :-)
No, it's larger or equal.

Not per any standard to my knowledge, but yeah. I don't think it's a good idea to do this change alone.

Quoted 6 lines

Quoted 4 lines
Note that it is possible to do such conversion even without introducing compiler warnings along the way -- just do the data type conversion first and the other changes in subsequent patches. It will be clear that one patch does one thing then.
If this is the only concern with these patches it is easily amended ;-)

It'd be nice if the others reviewed it as well, additional feedback would be nice.

Best regards,
Marek Vasut

Michal Suchanek

author

2015-08-03 18:39 UTC #7 lore raw

PATCH v2 2/6 mtd: m25p80: Add read/write error check and return

In reply to Michal Suchanek

Add checking of SPI transfer errors and return them from read/write functions.

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

---
 drivers/mtd/devices/m25p80.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

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

index d313f948b..c93bc41 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;

@@ -83,6 +83,7 @@ static void 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,9 +101,15 @@ 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;
+	*retlen += ret;
+	return ret;
 }
 
 static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

@@ -121,7 +128,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;

@@ -129,6 +136,7 @@ static int 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;

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

@@ -165,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-03 18:39 UTC #8 lore raw

PATCH v2 3/6 mtd: fsl-quadspi: update read/write function prototype

In reply to Michal Suchanek

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

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

---
 drivers/mtd/spi-nor/fsl-quadspi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

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

index 52a872f..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;

@@ -712,19 +717,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,
+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;
 
-	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 ret;
 }
 
-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;

@@ -736,8 +742,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)

Signature · 1 line

2.1.4

Michal Suchanek

author

2015-08-03 18:47 UTC #9 lore raw

PATCH v2 4/6 mtd: spi-nor: check return value from read/write

In reply to Michal Suchanek

SPI NOR hardware drivers now return useful values 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-03 18:47 UTC #10 lore raw

PATCH v2 5/6 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-03 18:47 UTC #11 lore raw

PATCH v2 6/6 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