* [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart
@ 2021-10-05 13:55 Marek Behún
2021-10-05 13:55 ` [PATCH u-boot-spi v3 01/10] mtd: spi-nor-core: Try cleaning up in case writing BAR failed Marek Behún
` (9 more replies)
0 siblings, 10 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:55 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
The original cover letter said:
this patch series fixes the `mtd erase` command when used with mtdpart
with a partition of non-zero offset.
Currently when the `mtd erase` command is used for such a partition,
it does not erase all blocks. Instead after a block is erased, the next
block address not current block address + block size, but current block
address + block size + partition offset, due to spi_nor_erase() not
calling mtd_erase_callback():
=> mtd erase "Rescue system"..
Erasing 0x00000000 ... 0x006fffff (1792 eraseblock(s))
jedec_spi_nor spi-nor@0: at 0x100000, len 4096
jedec_spi_nor spi-nor@0: at 0x201000, len 4096
jedec_spi_nor spi-nor@0: at 0x302000, len 4096
jedec_spi_nor spi-nor@0: at 0x403000, len 4096
jedec_spi_nor spi-nor@0: at 0x504000, len 4096
jedec_spi_nor spi-nor@0: at 0x605000, len 4096
jedec_spi_nor spi-nor@0: at 0x706000, len 4096
This series adds some fixes to spi_nor_erase() function, then adds
calling of mtd_erase_callback() to fix this bug.
The series also contains an improvement - adding the posibility to
interrupt spi_nor_erase() with Ctrl+C; and another one - making mtdpart's
_erase() method more sane so that the above mentioned bug will not occur
even if underlying driver does not call mtd_erase_callback().
Finally the last patch removes mtd_erase_callback() entirely, since:
- all provided callbacks across U-Boot are no-ops
- mtd_erase_callback() is abused for completely different purpose
than the original one (as explained in last commit message)
Marek
Changes since v2:
- added patch (5) adding check for zero length into legacy spi_flash_*()
functions
- changes patch (6, previously 5) to remove check for zero length also
from spi_nor_write()
Changes since v1:
- fixed CI bugs (by removing mtd_erase_callback() entirely)
Marek Behún (10):
mtd: spi-nor-core: Try cleaning up in case writing BAR failed
mtd: spi-nor-core: Check return value of write_enable() in
spi_nor_erase()
mtd: spi-nor-core: Don't overwrite return value if it is non-zero
mtd: spi-nor-core: Check return value of write_disable() in
spi_nor_erase()
mtd: spi-flash: Check for zero length in legacy spi_flash_*()
mtd: spi-nor-core: Don't check for zero length in spi_nor_write() /
spi_nor_erase()
mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase()
mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase()
mtd: mtdpart: Make mtdpart's _erase method sane
mtd: Remove mtd_erase_callback() entirely
cmd/onenand.c | 9 ++----
drivers/mtd/altera_qspi.c | 3 --
drivers/mtd/cfi_mtd.c | 1 -
drivers/mtd/mtdconcat.c | 11 -------
drivers/mtd/mtdcore.c | 8 ------
drivers/mtd/mtdpart.c | 23 ++++-----------
drivers/mtd/nand/raw/nand_base.c | 4 ---
drivers/mtd/onenand/onenand_base.c | 3 --
drivers/mtd/spi/sf_mtd.c | 1 -
drivers/mtd/spi/spi-nor-core.c | 46 +++++++++++++++++++++---------
drivers/mtd/ubi/io.c | 13 ---------
env/onenand.c | 4 +--
fs/yaffs2/yaffs_mtdif.c | 1 -
include/linux/mtd/mtd.h | 11 -------
include/nand.h | 1 -
include/spi_flash.h | 9 ++++++
16 files changed, 51 insertions(+), 97 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 01/10] mtd: spi-nor-core: Try cleaning up in case writing BAR failed
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
@ 2021-10-05 13:55 ` Marek Behún
2021-10-05 13:55 ` [PATCH u-boot-spi v3 02/10] mtd: spi-nor-core: Check return value of write_enable() in spi_nor_erase() Marek Behún
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:55 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún, Simon Glass,
Masami Hiramatsu
From: Marek Behún <marek.behun@nic.cz>
Use the cleanup codepath of spi_nor_erase() also in the event of failure
of writing the BAR register.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
drivers/mtd/spi/spi-nor-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index f1b4e5ea8e..d1e2bb47f7 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -929,7 +929,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
#ifdef CONFIG_SPI_FLASH_BAR
ret = write_bar(nor, addr);
if (ret < 0)
- return ret;
+ goto erase_err;
#endif
write_enable(nor);
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 02/10] mtd: spi-nor-core: Check return value of write_enable() in spi_nor_erase()
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
2021-10-05 13:55 ` [PATCH u-boot-spi v3 01/10] mtd: spi-nor-core: Try cleaning up in case writing BAR failed Marek Behún
@ 2021-10-05 13:55 ` Marek Behún
2021-10-05 13:55 ` [PATCH u-boot-spi v3 03/10] mtd: spi-nor-core: Don't overwrite return value if it is non-zero Marek Behún
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:55 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún, Simon Glass,
Masami Hiramatsu
From: Marek Behún <marek.behun@nic.cz>
The spi_nor_erase() function does not check return value of the
write_enable() call. Fix this.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
drivers/mtd/spi/spi-nor-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index d1e2bb47f7..11255aaab0 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -931,7 +931,9 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
if (ret < 0)
goto erase_err;
#endif
- write_enable(nor);
+ ret = write_enable(nor);
+ if (ret < 0)
+ goto erase_err;
ret = spi_nor_erase_sector(nor, addr);
if (ret < 0)
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 03/10] mtd: spi-nor-core: Don't overwrite return value if it is non-zero
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
2021-10-05 13:55 ` [PATCH u-boot-spi v3 01/10] mtd: spi-nor-core: Try cleaning up in case writing BAR failed Marek Behún
2021-10-05 13:55 ` [PATCH u-boot-spi v3 02/10] mtd: spi-nor-core: Check return value of write_enable() in spi_nor_erase() Marek Behún
@ 2021-10-05 13:55 ` Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 04/10] mtd: spi-nor-core: Check return value of write_disable() in spi_nor_erase() Marek Behún
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:55 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún, Simon Glass,
Masami Hiramatsu
From: Marek Behún <marek.behun@nic.cz>
The cleanup code of the spi_nor_erase() function overwrites the ret
variable with return value of clean_bar(), even if the ret variable is
already set. Fix this.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
drivers/mtd/spi/spi-nor-core.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 11255aaab0..a6007acd1a 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -909,7 +909,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
{
struct spi_nor *nor = mtd_to_spi_nor(mtd);
u32 addr, len, rem;
- int ret;
+ int ret, err;
dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
(long long)instr->len);
@@ -949,7 +949,9 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
erase_err:
#ifdef CONFIG_SPI_FLASH_BAR
- ret = clean_bar(nor);
+ err = clean_bar(nor);
+ if (!ret)
+ ret = err;
#endif
write_disable(nor);
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 04/10] mtd: spi-nor-core: Check return value of write_disable() in spi_nor_erase()
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
` (2 preceding siblings ...)
2021-10-05 13:55 ` [PATCH u-boot-spi v3 03/10] mtd: spi-nor-core: Don't overwrite return value if it is non-zero Marek Behún
@ 2021-10-05 13:56 ` Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 05/10] mtd: spi-flash: Check for zero length in legacy spi_flash_*() Marek Behún
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:56 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún, Simon Glass,
Masami Hiramatsu
From: Marek Behún <marek.behun@nic.cz>
The cleanup code of spi_nor_erase() function calls write_disable(), but
does not return it's return value even in case of failure. Fix this.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
drivers/mtd/spi/spi-nor-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index a6007acd1a..d49f7faf99 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -953,7 +953,9 @@ erase_err:
if (!ret)
ret = err;
#endif
- write_disable(nor);
+ err = write_disable(nor);
+ if (!ret)
+ ret = err;
return ret;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 05/10] mtd: spi-flash: Check for zero length in legacy spi_flash_*()
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
` (3 preceding siblings ...)
2021-10-05 13:56 ` [PATCH u-boot-spi v3 04/10] mtd: spi-nor-core: Check return value of write_disable() in spi_nor_erase() Marek Behún
@ 2021-10-05 13:56 ` Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 06/10] mtd: spi-nor-core: Don't check for zero length in spi_nor_write() / spi_nor_erase() Marek Behún
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:56 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Check for zero length in the legacy spi_flash_read() /
spi_flash_write() / spi_flash_erase() functions.
On zero length, return 0 immediately, don't call the underlying method.
Rationale:
- these legacy functions call the _read(), _write() and _erase() methods
of struct mtd
- the DM callers of these methods already check for zero length
- making all callers of these methods check for zero length makes it
possible to remove the check from implementations of these _read(),
_write() and _erase() methods
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
include/spi_flash.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/spi_flash.h b/include/spi_flash.h
index 3d747c925b..4d4ae89c19 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -165,6 +165,9 @@ static inline int spi_flash_read(struct spi_flash *flash, u32 offset,
struct mtd_info *mtd = &flash->mtd;
size_t retlen;
+ if (!len)
+ return 0;
+
return mtd->_read(mtd, offset, len, &retlen, buf);
}
@@ -174,6 +177,9 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset,
struct mtd_info *mtd = &flash->mtd;
size_t retlen;
+ if (!len)
+ return 0;
+
return mtd->_write(mtd, offset, len, &retlen, buf);
}
@@ -188,6 +194,9 @@ static inline int spi_flash_erase(struct spi_flash *flash, u32 offset,
return -EINVAL;
}
+ if (!len)
+ return 0;
+
memset(&instr, 0, sizeof(instr));
instr.addr = offset;
instr.len = len;
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 06/10] mtd: spi-nor-core: Don't check for zero length in spi_nor_write() / spi_nor_erase()
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
` (4 preceding siblings ...)
2021-10-05 13:56 ` [PATCH u-boot-spi v3 05/10] mtd: spi-flash: Check for zero length in legacy spi_flash_*() Marek Behún
@ 2021-10-05 13:56 ` Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 07/10] mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase() Marek Behún
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:56 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
This check is already done in all callers: mtdcore's mtd_write() /
mtd_erase(), legacy spi_nor_write() / spi_flash_erase(). No reason to do
this here as well.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
Changes since v2:
- remove check for zero length also from spi_nor_write()
---
drivers/mtd/spi/spi-nor-core.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index d49f7faf99..4c2590d0c5 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -914,9 +914,6 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
(long long)instr->len);
- if (!instr->len)
- return 0;
-
div_u64_rem(instr->len, mtd->erasesize, &rem);
if (rem)
return -EINVAL;
@@ -1671,9 +1668,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);
- if (!len)
- return 0;
-
for (i = 0; i < len; ) {
ssize_t written;
loff_t addr = to + i;
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 07/10] mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase()
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
` (5 preceding siblings ...)
2021-10-05 13:56 ` [PATCH u-boot-spi v3 06/10] mtd: spi-nor-core: Don't check for zero length in spi_nor_write() / spi_nor_erase() Marek Behún
@ 2021-10-05 13:56 ` Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 08/10] mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase() Marek Behún
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:56 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún, Simon Glass,
Masami Hiramatsu
From: Marek Behún <marek.behun@nic.cz>
The spi_nor_erase() function does not call mtd_erase_callback() as it
should.
The mtdpart code currently implements the subtraction of partition
offset in mtd_erase_callback().
This results in partition offset being added prior calling
spi_nor_erase(), but not subtracted back on return. The result is that
the `mtd erase` command does not erase the whole partition, only some of
it's blocks:
=> mtd erase "Rescue system"
Erasing 0x00000000 ... 0x006fffff (1792 eraseblock(s))
jedec_spi_nor spi-nor@0: at 0x100000, len 4096
jedec_spi_nor spi-nor@0: at 0x201000, len 4096
jedec_spi_nor spi-nor@0: at 0x302000, len 4096
jedec_spi_nor spi-nor@0: at 0x403000, len 4096
jedec_spi_nor spi-nor@0: at 0x504000, len 4096
jedec_spi_nor spi-nor@0: at 0x605000, len 4096
jedec_spi_nor spi-nor@0: at 0x706000, len 4096
This is obviously wrong.
Add proper calling of mtd_erase_callback() into the spi_nor_erase()
function.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reported-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
drivers/mtd/spi/spi-nor-core.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 4c2590d0c5..451f289339 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -908,6 +908,7 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
{
struct spi_nor *nor = mtd_to_spi_nor(mtd);
+ bool addr_known = false;
u32 addr, len, rem;
int ret, err;
@@ -915,12 +916,17 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
(long long)instr->len);
div_u64_rem(instr->len, mtd->erasesize, &rem);
- if (rem)
- return -EINVAL;
+ if (rem) {
+ ret = -EINVAL;
+ goto erase_err_callback;
+ }
addr = instr->addr;
len = instr->len;
+ instr->state = MTD_ERASING;
+ addr_known = true;
+
while (len) {
WATCHDOG_RESET();
#ifdef CONFIG_SPI_FLASH_BAR
@@ -944,6 +950,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
goto erase_err;
}
+ addr_known = false;
erase_err:
#ifdef CONFIG_SPI_FLASH_BAR
err = clean_bar(nor);
@@ -954,6 +961,15 @@ erase_err:
if (!ret)
ret = err;
+erase_err_callback:
+ if (ret) {
+ instr->fail_addr = addr_known ? addr : MTD_FAIL_ADDR_UNKNOWN;
+ instr->state = MTD_ERASE_FAILED;
+ } else {
+ instr->state = MTD_ERASE_DONE;
+ }
+ mtd_erase_callback(instr);
+
return ret;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 08/10] mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase()
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
` (6 preceding siblings ...)
2021-10-05 13:56 ` [PATCH u-boot-spi v3 07/10] mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase() Marek Behún
@ 2021-10-05 13:56 ` Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 09/10] mtd: mtdpart: Make mtdpart's _erase method sane Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 10/10] mtd: Remove mtd_erase_callback() entirely Marek Behún
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:56 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún, Simon Glass,
Masami Hiramatsu
From: Marek Behún <marek.behun@nic.cz>
May it possible to interrupt the spi_nor_erase() function.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
drivers/mtd/spi/spi-nor-core.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 451f289339..1e30b6cb27 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -929,6 +929,11 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
while (len) {
WATCHDOG_RESET();
+ if (ctrlc()) {
+ addr_known = false;
+ ret = -EINTR;
+ goto erase_err;
+ }
#ifdef CONFIG_SPI_FLASH_BAR
ret = write_bar(nor, addr);
if (ret < 0)
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 09/10] mtd: mtdpart: Make mtdpart's _erase method sane
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
` (7 preceding siblings ...)
2021-10-05 13:56 ` [PATCH u-boot-spi v3 08/10] mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase() Marek Behún
@ 2021-10-05 13:56 ` Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 10/10] mtd: Remove mtd_erase_callback() entirely Marek Behún
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:56 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún, Simon Glass,
Masami Hiramatsu
From: Marek Behún <marek.behun@nic.cz>
The _erase() method of the mtdpart driver, part_erase(), currently
implements offset shifting (for given mtdpart partition) in a weird way:
1. part_erase() adds partition offset to block address
2. parent driver's _erase() method is called
3. parent driver's _erase() method calls mtd_erase_callback()
4. mtd_erase_callback() subtracts partition offset from block address
so that the callback function is given correct address
The problem here is that if the parent's driver does not call
mtd_erase_callback() in some scenario (this was recently a case for
spi_nor_erase(), which did not call mtd_erase_callback() at all), the
offset is not shifted back.
Moreover the code would be more readable if part_erase() not only added
partition offset before calling parent's _erase(), but also subtracted
it back afterwards. Currently the mtd_erase_callback() is expected to do
this subtracting since it does have to do it anyway.
Add the more steps to this procedure:
5. mtd_erase_callback() adds partition offset to block address so that
it returns the the erase_info structure members as it received them
6. part_erase() subtracts partition offset from block address
This makes the code more logical and also prevents errors in case
parent's driver does not call mtd_erase_callback() for some reason.
(BTW, the purpose of mtd_erase_callback() in Linux is to inform the
caller that it is done, since in Linux erasing is done asynchronously.
We are abusing the purpose of mtd_erase_callback() in U-Boot for
completely different purpose. The callback function itself has empty
implementation in all cases in U-Boot.)
Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
drivers/mtd/mtdpart.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index aa58f722da..6ab481a7b1 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -446,24 +446,34 @@ static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
int ret;
instr->addr += mtd->offset;
+
ret = mtd->parent->_erase(mtd->parent, instr);
- if (ret) {
- if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
- instr->fail_addr -= mtd->offset;
- instr->addr -= mtd->offset;
- }
+ if (ret && instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
+ instr->fail_addr -= mtd->offset;
+
+ instr->addr -= mtd->offset;
+
return ret;
}
void mtd_erase_callback(struct erase_info *instr)
{
- if (instr->mtd->_erase == part_erase) {
+ if (!instr->callback)
+ return;
+
+ if (instr->mtd->_erase == part_erase && instr->len) {
if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
instr->fail_addr -= instr->mtd->offset;
instr->addr -= instr->mtd->offset;
}
- if (instr->callback)
- instr->callback(instr);
+
+ instr->callback(instr);
+
+ if (instr->mtd->_erase == part_erase && instr->len) {
+ if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
+ instr->fail_addr += instr->mtd->offset;
+ instr->addr += instr->mtd->offset;
+ }
}
EXPORT_SYMBOL_GPL(mtd_erase_callback);
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH u-boot-spi v3 10/10] mtd: Remove mtd_erase_callback() entirely
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
` (8 preceding siblings ...)
2021-10-05 13:56 ` [PATCH u-boot-spi v3 09/10] mtd: mtdpart: Make mtdpart's _erase method sane Marek Behún
@ 2021-10-05 13:56 ` Marek Behún
9 siblings, 0 replies; 11+ messages in thread
From: Marek Behún @ 2021-10-05 13:56 UTC (permalink / raw)
To: Jagan Teki, Tom Rini
Cc: u-boot, Patrick Delaunay, Pali Rohár, Patrice Chotard,
Marek Vasut, Pratyush Yadav, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
The original purpose of mtd_erase_callback() in Linux at the time it was
imported to U-Boot, was to inform the caller that erasing is done (since
it was an asynchronous operation).
All supplied callback methods in U-Boot do nothing, but the
mtd_erase_callback() function was (until previous patch) grossly abused
in U-Boot's mtdpart implementation for completely different purpose.
Since we got rid of the abusement, remove the mtd_erase_callback()
function and the .callback member from struct erase_info entirely, in
order to avoid such problems in the future.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/onenand.c | 9 ++-------
drivers/mtd/altera_qspi.c | 3 ---
drivers/mtd/cfi_mtd.c | 1 -
drivers/mtd/mtdconcat.c | 11 -----------
drivers/mtd/mtdcore.c | 8 --------
drivers/mtd/mtdpart.c | 21 ---------------------
drivers/mtd/nand/raw/nand_base.c | 4 ----
drivers/mtd/onenand/onenand_base.c | 3 ---
drivers/mtd/spi/sf_mtd.c | 1 -
drivers/mtd/spi/spi-nor-core.c | 5 ++---
drivers/mtd/ubi/io.c | 13 -------------
env/onenand.c | 4 +---
fs/yaffs2/yaffs_mtdif.c | 1 -
include/linux/mtd/mtd.h | 11 -----------
include/nand.h | 1 -
15 files changed, 5 insertions(+), 91 deletions(-)
diff --git a/cmd/onenand.c b/cmd/onenand.c
index 852ed5c7b2..592985a7ee 100644
--- a/cmd/onenand.c
+++ b/cmd/onenand.c
@@ -186,9 +186,7 @@ next:
static int onenand_block_erase(u32 start, u32 size, int force)
{
struct onenand_chip *this = mtd->priv;
- struct erase_info instr = {
- .callback = NULL,
- };
+ struct erase_info instr = {};
loff_t ofs;
int ret;
int blocksize = 1 << this->erase_shift;
@@ -219,10 +217,7 @@ static int onenand_block_erase(u32 start, u32 size, int force)
static int onenand_block_test(u32 start, u32 size)
{
struct onenand_chip *this = mtd->priv;
- struct erase_info instr = {
- .callback = NULL,
- .priv = 0,
- };
+ struct erase_info instr = {};
int blocks;
loff_t ofs;
diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
index 7bac599a54..d31391f36a 100644
--- a/drivers/mtd/altera_qspi.c
+++ b/drivers/mtd/altera_qspi.c
@@ -153,7 +153,6 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
putc('\n');
instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
instr->state = MTD_ERASE_FAILED;
- mtd_erase_callback(instr);
return -EIO;
}
flash = pdata->base + addr;
@@ -177,7 +176,6 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
writel(stat, ®s->isr); /* clear isr */
instr->fail_addr = addr;
instr->state = MTD_ERASE_FAILED;
- mtd_erase_callback(instr);
return -EIO;
}
if (flash_verbose)
@@ -189,7 +187,6 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
addr += mtd->erasesize;
}
instr->state = MTD_ERASE_DONE;
- mtd_erase_callback(instr);
return 0;
}
diff --git a/drivers/mtd/cfi_mtd.c b/drivers/mtd/cfi_mtd.c
index 78293caa2f..2295bb7220 100644
--- a/drivers/mtd/cfi_mtd.c
+++ b/drivers/mtd/cfi_mtd.c
@@ -58,7 +58,6 @@ static int cfi_mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
}
instr->state = MTD_ERASE_DONE;
- mtd_erase_callback(instr);
return 0;
}
diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index 684bc94998..af3c4765c4 100644
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -338,14 +338,6 @@ concat_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops)
return -EINVAL;
}
-static void concat_erase_callback(struct erase_info *instr)
-{
- /* Nothing to do here in U-Boot */
-#ifndef __UBOOT__
- wake_up((wait_queue_head_t *) instr->priv);
-#endif
-}
-
static int concat_dev_erase(struct mtd_info *mtd, struct erase_info *erase)
{
int err;
@@ -358,7 +350,6 @@ static int concat_dev_erase(struct mtd_info *mtd, struct erase_info *erase)
init_waitqueue_head(&waitq);
erase->mtd = mtd;
- erase->callback = concat_erase_callback;
erase->priv = (unsigned long) &waitq;
/*
@@ -498,8 +489,6 @@ static int concat_erase(struct mtd_info *mtd, struct erase_info *instr)
if (err)
return err;
- if (instr->callback)
- instr->callback(instr);
return 0;
}
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 9496903e86..1d45fb55c7 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -906,13 +906,6 @@ void __put_mtd_device(struct mtd_info *mtd)
}
EXPORT_SYMBOL_GPL(__put_mtd_device);
-/*
- * Erase is an asynchronous operation. Device drivers are supposed
- * to call instr->callback() whenever the operation completes, even
- * if it completes with a failure.
- * Callers are supposed to pass a callback function and wait for it
- * to be called before writing to the block.
- */
int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
{
if (instr->addr > mtd->size || instr->len > mtd->size - instr->addr)
@@ -922,7 +915,6 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
if (!instr->len) {
instr->state = MTD_ERASE_DONE;
- mtd_erase_callback(instr);
return 0;
}
return mtd->_erase(mtd, instr);
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 6ab481a7b1..a435ce6d07 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -456,27 +456,6 @@ static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
return ret;
}
-void mtd_erase_callback(struct erase_info *instr)
-{
- if (!instr->callback)
- return;
-
- if (instr->mtd->_erase == part_erase && instr->len) {
- if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
- instr->fail_addr -= instr->mtd->offset;
- instr->addr -= instr->mtd->offset;
- }
-
- instr->callback(instr);
-
- if (instr->mtd->_erase == part_erase && instr->len) {
- if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
- instr->fail_addr += instr->mtd->offset;
- instr->addr += instr->mtd->offset;
- }
-}
-EXPORT_SYMBOL_GPL(mtd_erase_callback);
-
static int part_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
{
return mtd->parent->_lock(mtd->parent, ofs + mtd->offset, len);
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index b533683dfe..f7616985d9 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3602,10 +3602,6 @@ erase_exit:
chip->select_chip(mtd, -1);
nand_release_device(mtd);
- /* Do call back function */
- if (!ret)
- mtd_erase_callback(instr);
-
/* Return more or less happy */
return ret;
}
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index 46aeef258d..56e1858de4 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1836,9 +1836,6 @@ int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
erase_exit:
ret = instr->state == MTD_ERASE_DONE ? 0 : -EIO;
- /* Do call back function */
- if (!ret)
- mtd_erase_callback(instr);
/* Deselect and wake up anyone waiting on the device */
onenand_release_device(mtd);
diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c
index 04de868080..0aed28a52b 100644
--- a/drivers/mtd/spi/sf_mtd.c
+++ b/drivers/mtd/spi/sf_mtd.c
@@ -46,7 +46,6 @@ static int spi_flash_mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
}
instr->state = MTD_ERASE_DONE;
- mtd_erase_callback(instr);
return 0;
}
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 1e30b6cb27..ef846a8ced 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -918,7 +918,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
div_u64_rem(instr->len, mtd->erasesize, &rem);
if (rem) {
ret = -EINVAL;
- goto erase_err_callback;
+ goto err;
}
addr = instr->addr;
@@ -966,14 +966,13 @@ erase_err:
if (!ret)
ret = err;
-erase_err_callback:
+err:
if (ret) {
instr->fail_addr = addr_known ? addr : MTD_FAIL_ADDR_UNKNOWN;
instr->state = MTD_ERASE_FAILED;
} else {
instr->state = MTD_ERASE_DONE;
}
- mtd_erase_callback(instr);
return ret;
}
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index b8b878b918..14be95b74b 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -304,18 +304,6 @@ int ubi_io_write(struct ubi_device *ubi, const void *buf, int pnum, int offset,
return err;
}
-/**
- * erase_callback - MTD erasure call-back.
- * @ei: MTD erase information object.
- *
- * Note, even though MTD erase interface is asynchronous, all the current
- * implementations are synchronous anyway.
- */
-static void erase_callback(struct erase_info *ei)
-{
- wake_up_interruptible((wait_queue_head_t *)ei->priv);
-}
-
/**
* do_sync_erase - synchronously erase a physical eraseblock.
* @ubi: UBI device description object
@@ -346,7 +334,6 @@ retry:
ei.mtd = ubi->mtd;
ei.addr = (loff_t)pnum * ubi->peb_size;
ei.len = ubi->peb_size;
- ei.callback = erase_callback;
ei.priv = (unsigned long)&wq;
err = mtd_erase(ubi->mtd, &ei);
diff --git a/env/onenand.c b/env/onenand.c
index c8da3ff811..1faa2cb62a 100644
--- a/env/onenand.c
+++ b/env/onenand.c
@@ -73,9 +73,7 @@ static int env_onenand_save(void)
#endif
loff_t env_addr = CONFIG_ENV_ADDR;
size_t retlen;
- struct erase_info instr = {
- .callback = NULL,
- };
+ struct erase_info instr = {};
ret = env_export(&env_new);
if (ret)
diff --git a/fs/yaffs2/yaffs_mtdif.c b/fs/yaffs2/yaffs_mtdif.c
index d338f9aa91..50fed2d4b1 100644
--- a/fs/yaffs2/yaffs_mtdif.c
+++ b/fs/yaffs2/yaffs_mtdif.c
@@ -145,7 +145,6 @@ int nandmtd_EraseBlockInNAND(struct yaffs_dev *dev, int blockNumber)
ei.len = dev->data_bytes_per_chunk * dev->param.chunks_per_block;
ei.time = 1000;
ei.retries = 2;
- ei.callback = NULL;
ei.priv = (u_long) dev;
/* Todo finish off the ei if required */
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 3b302fb8c3..7455400981 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -51,7 +51,6 @@ struct erase_info {
u_long retries;
unsigned dev;
unsigned cell;
- void (*callback) (struct erase_info *self);
u_long priv;
u_char state;
struct erase_info *next;
@@ -535,16 +534,6 @@ extern int unregister_mtd_user (struct mtd_notifier *old);
#endif
void *mtd_kmalloc_up_to(const struct mtd_info *mtd, size_t *size);
-#ifdef CONFIG_MTD_PARTITIONS
-void mtd_erase_callback(struct erase_info *instr);
-#else
-static inline void mtd_erase_callback(struct erase_info *instr)
-{
- if (instr->callback)
- instr->callback(instr);
-}
-#endif
-
static inline int mtd_is_bitflip(int err) {
return err == -EUCLEAN;
}
diff --git a/include/nand.h b/include/nand.h
index 80dd6469bc..efe193078b 100644
--- a/include/nand.h
+++ b/include/nand.h
@@ -68,7 +68,6 @@ static inline int nand_erase(struct mtd_info *info, loff_t off, size_t size)
instr.mtd = info;
instr.addr = off;
instr.len = size;
- instr.callback = 0;
return mtd_erase(info, &instr);
}
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-10-05 13:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-05 13:55 [PATCH u-boot-spi v3 00/10] Fix `mtd erase` when used with mtdpart Marek Behún
2021-10-05 13:55 ` [PATCH u-boot-spi v3 01/10] mtd: spi-nor-core: Try cleaning up in case writing BAR failed Marek Behún
2021-10-05 13:55 ` [PATCH u-boot-spi v3 02/10] mtd: spi-nor-core: Check return value of write_enable() in spi_nor_erase() Marek Behún
2021-10-05 13:55 ` [PATCH u-boot-spi v3 03/10] mtd: spi-nor-core: Don't overwrite return value if it is non-zero Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 04/10] mtd: spi-nor-core: Check return value of write_disable() in spi_nor_erase() Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 05/10] mtd: spi-flash: Check for zero length in legacy spi_flash_*() Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 06/10] mtd: spi-nor-core: Don't check for zero length in spi_nor_write() / spi_nor_erase() Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 07/10] mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase() Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 08/10] mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase() Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 09/10] mtd: mtdpart: Make mtdpart's _erase method sane Marek Behún
2021-10-05 13:56 ` [PATCH u-boot-spi v3 10/10] mtd: Remove mtd_erase_callback() entirely Marek Behún
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.