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
From: Cédric Cano <[email protected]>
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
---
drivers/mtd/devices/m25p80.c 2012-09-03 17:35:16.159741656 +0200 renamed from drivers/mtd/devices/m25p80.c 2012-08-26 04:32:13.000000000 +0200 +26 -13
@@ -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);
---
Quoted 2 lines · Cédric Cano
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.
Quoted 2 lines
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.
Review
Quoted 5 lines
---
linux-3.5.3/drivers/mtd/devices/m25p80.c2012-08-26
04:32:13.000000000 +0200
+++linux-3.5.3/drivers/mtd/devices/m25p80.c2012-09-03
17:35:16.159741656 +0200(quoted diff) +0
-0@@ -199,7 +199,7 @@
Could you please use "git format-patch" for your submissions, and base them on the current head of tree?
Thanks.
Quoted 19 lines · Kevin Cernekee
Quoted 2 lines · Cédric Cano
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.
Quoted 2 lines
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.
Quoted 4 lines
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).
Quoted 14 lines
Side note - it would be helpful to add kerneldoc markup to
include/linux/mtd/mtd.hfor structmtd_infoand the new mtd_* wrapper functions, so we have a clear definition of what the parameters and return values should look like.Review
Quoted 5 lines
---
linux-3.5.3/drivers/mtd/devices/m25p80.c2012-08-26
04:32:13.000000000 +0200
+++linux-3.5.3/drivers/mtd/devices/m25p80.c2012-09-03
17:35:16.159741656 +0200(quoted diff) +0
-0@@ -199,7 +199,7 @@Could you please use "git format-patch" for your submissions, and base them on the current head of tree?
Quoted 2 lines
Thanks.
OK, I will repost my second patch with this method.
Thanks.