[PATCH 1/2] drivers/mtd/devices/m25p80.c: Fix return code of read/write mtd callbacks

[PATCH 1/2] drivers/mtd/devices/m25p80.c: Fix return code of read/write mtd callbacks

[Cédric Cano]

From: Cédric Cano <ccano@interfaceconcept.com>

For SPI Flash devices, MTD read and write functions returns "1" if Flash 
is busy (this status is returned by "wait_till_ready" fnuction). This 
return code is not an error code: if device is busy, MTD read or write 
are successful with 1 byte length.

This patch fixes code returned by "wait_till_ready", "m25p80_erase", 
"m25p80_read", "m25p80_write" and "sst_write" functions: the error code 
is -EBUSY now.

C. Cano

Signed-off-by: Cédric Cano <ccano@interfaceconcept.com>

---
--- linux-3.5.3/drivers/mtd/devices/m25p80.c    2012-08-26 
04:32:13.000000000 +0200
+++ linux-3.5.3/drivers/mtd/devices/m25p80.c    2012-09-03 
17:35:16.159741656 +0200
@@ -199,7 +199,7 @@

      } while (!time_after_eq(jiffies, deadline));

-    return 1;
+    return -EBUSY;
  }

  /*
@@ -209,12 +209,15 @@
   */
  static int erase_chip(struct m25p *flash)
  {
+    int    ret;
+
      pr_debug("%s: %s %lldKiB\n", dev_name(&flash->spi->dev), __func__,
              (long long)(flash->mtd.size >> 10));

      /* Wait until finished previous write command. */
-    if (wait_till_ready(flash))
-        return 1;
+    ret = wait_till_ready(flash);
+    if (ret)
+        return ret;

      /* Send write enable, then erase commands. */
      write_enable(flash);
@@ -249,12 +252,15 @@
   */
  static int erase_sector(struct m25p *flash, u32 offset)
  {
+    int    ret;
+
      pr_debug("%s: %s %dKiB at 0x%08x\n", dev_name(&flash->spi->dev),
              __func__, flash->mtd.erasesize / 1024, offset);

      /* Wait until finished previous write command. */
-    if (wait_till_ready(flash))
-        return 1;
+    ret = wait_till_ready(flash);
+    if (ret)
+        return ret;

      /* Send write enable, then erase commands. */
      write_enable(flash);
@@ -283,6 +289,7 @@
      struct m25p *flash = mtd_to_m25p(mtd);
      u32 addr,len;
      uint32_t rem;
+    int ret;

      pr_debug("%s: %s at 0x%llx, len %lld\n", dev_name(&flash->spi->dev),
              __func__, (long long)instr->addr,
@@ -299,10 +306,11 @@

      /* whole-chip erase? */
      if (len == flash->mtd.size) {
-        if (erase_chip(flash)) {
+        ret = erase_chip(flash);
+        if (ret) {
              instr->state = MTD_ERASE_FAILED;
              mutex_unlock(&flash->lock);
-            return -EIO;
+            return ret;
          }

      /* REVISIT in some cases we could speed up erasing large regions
@@ -313,10 +321,11 @@
      /* "sector"-at-a-time erase */
      } else {
          while (len) {
-            if (erase_sector(flash, addr)) {
+            ret = erase_sector(flash, addr);
+            if (ret) {
                  instr->state = MTD_ERASE_FAILED;
                  mutex_unlock(&flash->lock);
-                return -EIO;
+                return ret;
              }

              addr += mtd->erasesize;
@@ -339,6 +348,7 @@
  static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
      size_t *retlen, u_char *buf)
  {
+    int ret;
      struct m25p *flash = mtd_to_m25p(mtd);
      struct spi_transfer t[2];
      struct spi_message m;
@@ -364,10 +374,11 @@
      mutex_lock(&flash->lock);

      /* Wait till previous write/erase is done. */
-    if (wait_till_ready(flash)) {
+    ret = wait_till_ready(flash);
+    if (ret) {
          /* REVISIT status return?? */
          mutex_unlock(&flash->lock);
-        return 1;
+        return ret;
      }

      /* FIXME switch to OPCODE_FAST_READ.  It's required for higher
@@ -396,6 +407,7 @@
  static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len,
      size_t *retlen, const u_char *buf)
  {
+    int ret;
      struct m25p *flash = mtd_to_m25p(mtd);
      u32 page_offset, page_size;
      struct spi_transfer t[2];
@@ -417,9 +429,10 @@
      mutex_lock(&flash->lock);

      /* Wait until finished previous write command. */
-    if (wait_till_ready(flash)) {
+    ret = wait_till_ready(flash);
+    if (ret) {
          mutex_unlock(&flash->lock);
-        return 1;
+        return ret;
      }

      write_enable(flash);
---

Re: [PATCH 1/2] drivers/mtd/devices/m25p80.c: Fix return code of read/write mtd callbacks

[Kevin Cernekee]

On Mon, Sep 3, 2012 at 8:58 AM, Cédric Cano <ccano@interfaceconcept.com> wrote:
> For SPI Flash devices, MTD read and write functions returns "1" if Flash is
> busy (this status is returned by "wait_till_ready" fnuction). This return

"function"

It is usually a good idea to wrap the commit text around 72-74
characters so that it is readable in "git log" on an 80-column
terminal.

> code is not an error code: if device is busy, MTD read or write are
> successful with 1 byte length.

Looking at mtd_read() - typically the transfer length is passed back
through the "size_t *retlen" pointer, not the function's return value.

In Linux 3.5, if m25p80_read() returns 1, mtd_read() will interpret it
as a bitflip count not a byte count.  That isn't the correct behavior
for a NOR device, but it probably wouldn't cause the symptom you are
reporting.

In Linux 3.4 and older, some functions like mtdchar_read() could
interpret the '1' as an error code and pass it back as-is to the
caller (who is expecting either a negative error code or a positive
length).  Is this the case you are running into?

Side note - it would be helpful to add kerneldoc markup to
include/linux/mtd/mtd.h for struct mtd_info and the new mtd_* wrapper
functions, so we have a clear definition of what the parameters and
return values should look like.

> --- linux-3.5.3/drivers/mtd/devices/m25p80.c    2012-08-26
> 04:32:13.000000000 +0200
> +++ linux-3.5.3/drivers/mtd/devices/m25p80.c    2012-09-03
> 17:35:16.159741656 +0200
> @@ -199,7 +199,7 @@

Could you please use "git format-patch" for your submissions, and base
them on the current head of tree?

Thanks.

Re: [PATCH 1/2] drivers/mtd/devices/m25p80.c: Fix return code of read/write mtd callbacks

[Cédric Cano]

Le 03/09/2012 20:26, Kevin Cernekee a écrit :
> On Mon, Sep 3, 2012 at 8:58 AM, Cédric Cano <ccano@interfaceconcept.com> wrote:
>> For SPI Flash devices, MTD read and write functions returns "1" if Flash is
>> busy (this status is returned by "wait_till_ready" fnuction). This return
> "function"
>
> It is usually a good idea to wrap the commit text around 72-74
> characters so that it is readable in "git log" on an 80-column
> terminal.
>
>> code is not an error code: if device is busy, MTD read or write are
>> successful with 1 byte length.
> Looking at mtd_read() - typically the transfer length is passed back
> through the "size_t *retlen" pointer, not the function's return value.
>
> In Linux 3.5, if m25p80_read() returns 1, mtd_read() will interpret it
> as a bitflip count not a byte count.  That isn't the correct behavior
> for a NOR device, but it probably wouldn't cause the symptom you are
> reporting.
>
> In Linux 3.4 and older, some functions like mtdchar_read() could
> interpret the '1' as an error code and pass it back as-is to the
> caller (who is expecting either a negative error code or a positive
> length).  Is this the case you are running into?

You're alright: I'm doing my test on kernel 3.4. In this case, '1' is
the return code of mtd_read() that is considered as a length, not an
error.
In kernel 3.5, it should work without this patch (I can't test it).

>
> Side note - it would be helpful to add kerneldoc markup to
> include/linux/mtd/mtd.h for struct mtd_info and the new mtd_* wrapper
> functions, so we have a clear definition of what the parameters and
> return values should look like.
>
>> --- linux-3.5.3/drivers/mtd/devices/m25p80.c    2012-08-26
>> 04:32:13.000000000 +0200
>> +++ linux-3.5.3/drivers/mtd/devices/m25p80.c    2012-09-03
>> 17:35:16.159741656 +0200
>> @@ -199,7 +199,7 @@
> Could you please use "git format-patch" for your submissions, and base
> them on the current head of tree?
>
> Thanks.
>
OK, I will repost my second patch with this method.

Thanks.