[PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart
Hello,
I accidentally forgot to send this series to U-Boot's mailing list last time, meaning it did not end up in patchwork, so now I am resending it. Sorry for this mess.
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().
Moreover I would also like to start a discussion regarding the MTD subsystem: - U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ macros - this was done to make it easier to port Linux's patches to U-Boot - the problem is that it seems nobody did port Linux's MTD patches to U-Boot for a long time, the code is many times different from Linux', and it would be very hard to align it - therefore I propose to get rid of all the #ifdefs, remove the Linux specific code, and continue developing the code independently from Linux. This would make it impossible to apply Linux patches in some kind of automatic way, but this is currently already impossible anyway What do you guys think?
Marek
Marek Behún (8): 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-nor-core: Don't check for zero length in 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
drivers/mtd/mtdpart.c | 26 +++++++++++++------- drivers/mtd/spi/spi-nor-core.c | 44 ++++++++++++++++++++++++++-------- 2 files changed, 52 insertions(+), 18 deletions(-)
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 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 99e2f16349..7ce8dc5502 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -927,7 +927,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);
On Wed, 14 Jul 2021 at 17:51, Marek Behún marek.behun@nic.cz wrote:
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 Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org
drivers/mtd/spi/spi-nor-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Thu, Jul 15, 2021 at 5:21 AM Marek Behún marek.behun@nic.cz wrote:
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 Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Reviewed-by: Jagan Teki jagan@amarulasolutions.com
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 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 7ce8dc5502..8fd1d684f2 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -929,7 +929,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)
On Wed, 14 Jul 2021 at 17:51, Marek Behún marek.behun@nic.cz wrote:
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 Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org
drivers/mtd/spi/spi-nor-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Thu, Jul 15, 2021 at 5:21 AM Marek Behún marek.behun@nic.cz wrote:
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 Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org
Reviewed-by: Jagan Teki jagan@amarulasolutions.com
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 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 8fd1d684f2..48c82b94aa 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -907,7 +907,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); @@ -947,7 +947,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);
On Wed, 14 Jul 2021 at 17:51, Marek Behún marek.behun@nic.cz wrote:
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 Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org
drivers/mtd/spi/spi-nor-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
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 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 48c82b94aa..d8cc1389e3 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -951,7 +951,9 @@ erase_err: if (!ret) ret = err; #endif - write_disable(nor); + err = write_disable(nor); + if (!ret) + ret = err;
return ret; }
On Wed, 14 Jul 2021 at 17:51, Marek Behún marek.behun@nic.cz wrote:
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 Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org
drivers/mtd/spi/spi-nor-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
This check is already done in mtdcore's mtd_erase(), no reason to do this here as well.
Signed-off-by: Marek Behún marek.behun@nic.cz Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org --- drivers/mtd/spi/spi-nor-core.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index d8cc1389e3..8ae033c576 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -912,9 +912,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;
On Wed, 14 Jul 2021 at 17:51, Marek Behún marek.behun@nic.cz wrote:
This check is already done in mtdcore's mtd_erase(), no reason to do this here as well.
Signed-off-by: Marek Behún marek.behun@nic.cz Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org
drivers/mtd/spi/spi-nor-core.c | 3 --- 1 file changed, 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
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 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 8ae033c576..927823930c 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -906,6 +906,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;
@@ -913,12 +914,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 @@ -942,6 +948,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); @@ -952,6 +959,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; }
On Wed, 14 Jul 2021 at 17:51, Marek Behún marek.behun@nic.cz wrote:
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 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(-)
Reviewed-by: Simon Glass sjg@chromium.org
May it possible to interrupt the spi_nor_erase() function.
Signed-off-by: Marek Behún marek.behun@nic.cz 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 927823930c..81ecd1fe5a 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -927,6 +927,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)
On Wed, 14 Jul 2021 at 17:51, Marek Behún marek.behun@nic.cz wrote:
May it possible to interrupt the spi_nor_erase() function.
Signed-off-by: Marek Behún marek.behun@nic.cz Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org
drivers/mtd/spi/spi-nor-core.c | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
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 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);
On Wed, 14 Jul 2021 at 17:51, Marek Behún marek.behun@nic.cz wrote:
The _erase() method of the mtdpart driver, part_erase(), currently implements offset shifting (for given mtdpart partition) in a weird way:
- part_erase() adds partition offset to block address
- parent driver's _erase() method is called
- parent driver's _erase() method calls mtd_erase_callback()
- 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 Tested-by: Masami Hiramatsu masami.hiramatsu@linaro.org
drivers/mtd/mtdpart.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)Reviewed-by: Simon Glass sjg@chromium.org
Reviewed-by: Simon Glass sjg@chromium.org
Hi Marek,
On Wed, 14 Jul 2021 at 17:51, Marek Behún marek.behun@nic.cz wrote:
Hello,
I accidentally forgot to send this series to U-Boot's mailing list last time, meaning it did not end up in patchwork, so now I am resending it. Sorry for this mess.
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().
Moreover I would also like to start a discussion regarding the MTD subsystem:
- U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ macros
- this was done to make it easier to port Linux's patches to U-Boot
- the problem is that it seems nobody did port Linux's MTD patches to U-Boot for a long time, the code is many times different from Linux', and it would be very hard to align it
- therefore I propose to get rid of all the #ifdefs, remove the Linux specific code, and continue developing the code independently from Linux. This would make it impossible to apply Linux patches in some kind of automatic way, but this is currently already impossible anyway
What do you guys think?
That sounds good to me, but I wonder what is the gap today?
Are you and/or Jagen going to take on this effort?
Regards, Simon
Hi Marek,
On Thu, Jul 15, 2021 at 5:21 AM Marek Behún marek.behun@nic.cz wrote:
Hello,
I accidentally forgot to send this series to U-Boot's mailing list last time, meaning it did not end up in patchwork, so now I am resending it. Sorry for this mess.
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().
Moreover I would also like to start a discussion regarding the MTD subsystem:
- U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ macros
- this was done to make it easier to port Linux's patches to U-Boot
- the problem is that it seems nobody did port Linux's MTD patches to U-Boot for a long time, the code is many times different from Linux', and it would be very hard to align it
- therefore I propose to get rid of all the #ifdefs, remove the Linux specific code, and continue developing the code independently from Linux. This would make it impossible to apply Linux patches in some kind of automatic way, but this is currently already impossible anyway
What do you guys think?
Marek
Marek Behún (8): 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-nor-core: Don't check for zero length in 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
Found the build error with CI [1], would you please check?
[1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345
Jagan.
On Wed, 21 Jul 2021 21:46:56 +0530 Jagan Teki jagan@amarulasolutions.com wrote:
Hi Marek,
On Thu, Jul 15, 2021 at 5:21 AM Marek Behún marek.behun@nic.cz wrote:
Hello,
I accidentally forgot to send this series to U-Boot's mailing list last time, meaning it did not end up in patchwork, so now I am resending it. Sorry for this mess.
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().
Moreover I would also like to start a discussion regarding the MTD subsystem:
- U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ macros
- this was done to make it easier to port Linux's patches to U-Boot
- the problem is that it seems nobody did port Linux's MTD patches to U-Boot for a long time, the code is many times different from Linux', and it would be very hard to align it
- therefore I propose to get rid of all the #ifdefs, remove the Linux specific code, and continue developing the code independently from Linux. This would make it impossible to apply Linux patches in some kind of automatic way, but this is currently already impossible anyway
What do you guys think?
Marek
Marek Behún (8): 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-nor-core: Don't check for zero length in 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
Found the build error with CI [1], would you please check?
[1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345
Jagan.
Jagan, I am unable to get the output of the failed CI tests. Probably because I do not have an account at source.denx.de.
I tried to run "sandbox with clang" CI scenario on my local machine and it is successful.
Can you send me outputs of the failed scenarios?
Marek
On Thu, Jul 22, 2021 at 10:44:59PM +0200, Marek Behun wrote:
On Wed, 21 Jul 2021 21:46:56 +0530 Jagan Teki jagan@amarulasolutions.com wrote:
Hi Marek,
On Thu, Jul 15, 2021 at 5:21 AM Marek Behún marek.behun@nic.cz wrote:
Hello,
I accidentally forgot to send this series to U-Boot's mailing list last time, meaning it did not end up in patchwork, so now I am resending it. Sorry for this mess.
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().
Moreover I would also like to start a discussion regarding the MTD subsystem:
- U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ macros
- this was done to make it easier to port Linux's patches to U-Boot
- the problem is that it seems nobody did port Linux's MTD patches to U-Boot for a long time, the code is many times different from Linux', and it would be very hard to align it
- therefore I propose to get rid of all the #ifdefs, remove the Linux specific code, and continue developing the code independently from Linux. This would make it impossible to apply Linux patches in some kind of automatic way, but this is currently already impossible anyway
What do you guys think?
Marek
Marek Behún (8): 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-nor-core: Don't check for zero length in 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
Found the build error with CI [1], would you please check?
[1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345
Jagan.
Jagan, I am unable to get the output of the failed CI tests. Probably because I do not have an account at source.denx.de.
Jagan, please check the permissions on your tree, the results should be set to public.
Hi Jagan,
On Wed, 21 Jul 2021 21:46:56 +0530 Jagan Teki jagan@amarulasolutions.com wrote:
Found the build error with CI [1], would you please check?
[1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345
Jagan.
OK I think I've found out what is the problem. I've pushed new version into github CI to check if it builds correctly.
The problem seems to be that after this series the function spi_nor_erase() calls mtd_erase_callback(), which is declared in the header file include/linux/mtd/mtd.h, if CONFIG_MTD_PARTITIONS is enabled, and defined as a static inline function otherwise.
The problem is that for some boards we have CONFIG_MTD_PARTITIONS together with CONFIG_SPL_SPI_FLASH_SUPPORT. But in SPL, mtdpart.c (where mtd_erase_callback() is defined) is not compiled at all.
Thus this leads to undefined reference to mtd_erase_callback().
This is another proof that the whole mtd subsystem has become a gross spaghetti code where hacks upon hacks were introduced by different people to solve different purposes, and the result makes me angry. :-D
We really need to rewrite this.
Anyway, for now I will just send v2 of this series with another patch fixing this issue, once CI ends smoothly.
Marek
participants (5)
-
Jagan Teki -
Marek Behun -
Marek Behún
-
Simon Glass -
Tom Rini