[PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart

[PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart

[Marek Behún]

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

-- 
2.31.1

[PATCH RESEND u-boot-spi 1/8] mtd: spi-nor-core: Try cleaning up in case writing BAR failed

[Marek Behún]

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

[PATCH RESEND u-boot-spi 2/8] mtd: spi-nor-core: Check return value of write_enable() in spi_nor_erase()

[Marek Behún]

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

[PATCH RESEND u-boot-spi 3/8] mtd: spi-nor-core: Don't overwrite return value if it is non-zero

[Marek Behún]

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

[PATCH RESEND u-boot-spi 4/8] mtd: spi-nor-core: Check return value of write_disable() in spi_nor_erase()

[Marek Behún]

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;
 }
-- 
2.31.1

[PATCH RESEND u-boot-spi 5/8] mtd: spi-nor-core: Don't check for zero length in spi_nor_erase()

[Marek Behún]

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

[PATCH RESEND u-boot-spi 6/8] mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase()

[Marek Behún]

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;
 }
 
-- 
2.31.1

[PATCH RESEND u-boot-spi 7/8] mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase()

[Marek Behún]

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

[PATCH RESEND u-boot-spi 8/8] mtd: mtdpart: Make mtdpart's _erase method sane

[Marek Behún]

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

Re: [PATCH RESEND u-boot-spi 1/8] mtd: spi-nor-core: Try cleaning up in case writing BAR failed

[Simon Glass]

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>

Re: [PATCH RESEND u-boot-spi 2/8] mtd: spi-nor-core: Check return value of write_enable() in spi_nor_erase()

[Simon Glass]

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>

Re: [PATCH RESEND u-boot-spi 3/8] mtd: spi-nor-core: Don't overwrite return value if it is non-zero

[Simon Glass]

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>

Re: [PATCH RESEND u-boot-spi 4/8] mtd: spi-nor-core: Check return value of write_disable() in spi_nor_erase()

[Simon Glass]

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>

Re: [PATCH RESEND u-boot-spi 5/8] mtd: spi-nor-core: Don't check for zero length in spi_nor_erase()

[Simon Glass]

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>

Re: [PATCH RESEND u-boot-spi 6/8] mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase()

[Simon Glass]

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>

Re: [PATCH RESEND u-boot-spi 7/8] mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase()

[Simon Glass]

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>

Re: [PATCH RESEND u-boot-spi 8/8] mtd: mtdpart: Make mtdpart's _erase method sane

[Simon Glass]

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:
>   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(-)Reviewed-by: Simon Glass <sjg@chromium.org>
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart

[Simon Glass]

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

Re: [PATCH RESEND u-boot-spi 1/8] mtd: spi-nor-core: Try cleaning up in case writing BAR failed

[Jagan Teki]

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>

Re: [PATCH RESEND u-boot-spi 2/8] mtd: spi-nor-core: Check return value of write_enable() in spi_nor_erase()

[Jagan Teki]

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>

Re: [PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart

[Jagan Teki]

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.

Re: [PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart

[Marek Behun]

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

Re: [PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3833 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart

[Marek Behun]

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