[PATCH 0/3] cmd: mtd: fix default read behavior and boundary checks

[PATCH 0/3] cmd: mtd: fix default read behavior and boundary checks

[Peter Suti]

This series addresses two issues in the 'mtd read' command regarding how
default read lengths are calculated and handled when no size argument is
provided.

The first patch fixes a failure when reading a full NAND partition that
contains bad blocks. Currently, skipping bad blocks increments the physical
offset without adjusting the remaining byte count, causing the read to
attempt accessing past the end of the partition. The fix ensures that
default full-partition reads are truncated gracefully at the physical
boundary rather than returning an error.
This behaviour is similar to the raw `nand` command which adjusts the size
of the read with adjust_size_for_badblocks().

The second patch fixes the default length calculation when a starting
offset is specified. Previously, the command defaulted to reading the
entire partition size ('mtd->size') starting from the offset, which
inevitably exceeds the partition bounds. The fix adjusts the default length
to 'mtd->size - offset'.

Changes in v2:
 - Separate commit for additional debug logs for bad blocks.
 - Return error when start_off >= mtd->size.
 - Resent to fix threading issues from v1.

Link to V1: https://lore.kernel.org/u-boot/cover.1765891388.git.peter.suti@streamunlimited.com/T/#u

Peter Suti (3):
  cmd: mtd: add logs when bad blocks are encountered
  cmd: mtd: fix read failure on default full partition read with bad
    blocks
  cmd: mtd: fix default length calculation when offset is specified

 cmd/mtd.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v2 0/3] cmd: mtd: fix default read behavior and boundary checks

[Peter Suti]

This series addresses two issues in the 'mtd read' command regarding how
default read lengths are calculated and handled when no size argument is
provided.

The first patch fixes a failure when reading a full NAND partition that
contains bad blocks. Currently, skipping bad blocks increments the physical
offset without adjusting the remaining byte count, causing the read to
attempt accessing past the end of the partition. The fix ensures that
default full-partition reads are truncated gracefully at the physical
boundary rather than returning an error.
This behaviour is similar to the raw `nand` command which adjusts the size
of the read with adjust_size_for_badblocks().

The second patch fixes the default length calculation when a starting
offset is specified. Previously, the command defaulted to reading the
entire partition size ('mtd->size') starting from the offset, which
inevitably exceeds the partition bounds. The fix adjusts the default length
to 'mtd->size - offset'.

Changes in v2:
 - Separate commit for additional debug logs for bad blocks.
 - Return error when start_off >= mtd->size.
 - Resent to fix threading issues from v1.

Link to v1: https://lore.kernel.org/u-boot/cover.1765891388.git.peter.suti@streamunlimited.com/T/#u

Peter Suti (3):
  cmd: mtd: add logs when bad blocks are encountered
  cmd: mtd: fix read failure on default full partition read with bad
    blocks
  cmd: mtd: fix default length calculation when offset is specified

 cmd/mtd.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] cmd: mtd: add logs when bad blocks are encountered

[Peter Suti]

Signed-off-by: Peter Suti <peter.suti@streamunlimited.com>
---
 cmd/mtd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 7f25144098b..32733e8f11f 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -559,8 +559,10 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	/* Search for the first good block after the given offset */
 	off = start_off;
-	while (mtd_block_isbad(mtd, off))
+	while (mtd_block_isbad(mtd, off)) {
+		printf("Bad block: failed to read at offset 0x%llx, skipping.\n", off);
 		off += mtd->erasesize;
+	}
 
 	led_activity_blink();
 
@@ -572,6 +574,7 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
 		/* Skip the block if it is bad */
 		if (mtd_is_aligned_with_block_size(mtd, off) &&
 		    mtd_block_isbad(mtd, off)) {
+			printf("Bad block: failed to read at offset 0x%llx, skipping.\n", off);
 			off += mtd->erasesize;
 			continue;
 		}
-- 
2.43.0

[PATCH v2 2/3] cmd: mtd: fix read failure on default full partition read with bad blocks

[Peter Suti]

When performing an 'mtd read' on a NAND partition, the presence of bad
blocks can cause the command to fail with error -22 (EINVAL) if the
requested size is equal to (or close to) the partition size.
When size is not provided, the whole partition is read by default.

This issue occurs because the bad block skipping logic increments the
physical read offset ('off') without decrementing the 'remaining' byte
count. If enough bad blocks are skipped, the 'off' pointer eventually
slides past the end of the partition boundary ('mtd->size') while
'remaining' is still non-zero. The subsequent call to 'mtd_read_oob'
then fails its internal boundary checks.

The Scenario:
  Partition Size: 0x1200000
  Bad Blocks:     1
  Command:        mtd read swufit ${loadaddr}
                  (Defaults to reading full 0x1200000 bytes)

  1. U-Boot attempts to read 0x1200000 valid bytes.
  2. It encounters the bad block and skips it (off += mtd->erasesize).
  3. To satisfy the full request, it must read
     (0x1200000 + mtd->erasesize) physical bytes.
  4. The read pointer 'off' exceeds 'mtd->size'.
  5. Command aborts with: "Read on swufit failed with error -22".

Add a boundary check at the start of the read loop. If the read pointer
'off' reaches the end of the physical device, check if the user explicitly
requested a size (argc >= 2).

* If no size was specified (default read): Stop the read gracefully,
  print a notice, and return success. We assume the user wants "all
  available data" up to the physical limit.
* If a size was explicitly specified: Continue standard execution. This
  will result in an error (-22) as expected, since the specific requirement
  cannot be met.

It is physically impossible to read the full logical size of a partition
if it contains bad blocks (as the bad blocks consume physical space
required for the data). In this case, truncating the read is reasonable.
For common use cases like loading FIT images (which often
contain padding at the end), missing the trailing 0xFF data is acceptable,
whereas failing the entire load operation is not.

NOTE: This changes the behavior of 'mtd read' when no size is provided.
      Previously, it failed if the full read was impossible. Now, it
      succeeds with a truncated buffer. The caller is responsible for
      verifying data integrity (e.g., via fitImage hash verification).

Signed-off-by: Peter Suti <peter.suti@streamunlimited.com>
[polished the comment and commit message]
Signed-off-by: Radek Dostál <radek.dostal@streamunlimited.com>
---
 cmd/mtd.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 32733e8f11f..1d1845bce44 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -571,6 +571,28 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	/* Loop over the pages to do the actual read/write */
 	while (remaining) {
+		/*
+		 * Boundary Check: Bad block skipping may push the physical read offset
+		 * past the partition end (mtd->size).
+		 * If the read pointer 'off' has reached the end of the physical flash,
+		 * we physically cannot read more.
+		 *
+		 * Check 'argc' to see if the user explicitly requested a specific size.
+		 * * If argc < 2, the user did NOT specify a size (defaulted to partition size).
+		 *   In this case, we treat "end of flash" as a natural stopping point
+		 *   and truncate the read gracefully.
+		 * * If argc >= 2, the user explicitly asked for N bytes.
+		 *   In this case, we do NOT truncate. We let the loop continue, which
+		 *   will trigger the standard error (-22) because the request cannot be met.
+		 */
+		if (argc < 2 && off >= mtd->size) {
+			printf("Notice: Reached end of partition with %lld bytes remaining. "
+			       "Truncating read.\n", remaining);
+			remaining = 0;
+			ret = CMD_RET_SUCCESS;
+			break;
+		}
+
 		/* Skip the block if it is bad */
 		if (mtd_is_aligned_with_block_size(mtd, off) &&
 		    mtd_block_isbad(mtd, off)) {
-- 
2.43.0

[PATCH v2 3/3] cmd: mtd: fix default length calculation when offset is specified

[Peter Suti]

When the 'mtd read' command is issued with an offset but no size (argc=1),
the length defaults to the full partition size ('mtd->size').

This calculation is incorrect because reading 'mtd->size' bytes starting
from a non-zero offset results in a read request that extends past the
end of the partition. This causes the MTD layer to return error -22
(EINVAL).

Fix the default length calculation to be 'mtd->size - offset' so that
reads starting from an offset effectively read "to the end of the
partition" rather than attempting to read out of bounds.

Signed-off-by: Peter Suti <peter.suti@streamunlimited.com>
---
 cmd/mtd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 1d1845bce44..29189925b24 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -519,7 +519,17 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
 		goto out_put_mtd;
 	}
 
-	default_len = dump ? mtd->writesize : mtd->size;
+	if (dump) {
+		default_len = mtd->writesize;
+	} else {
+		if (start_off >= mtd->size) {
+			printf("Start offset 0x%llx is greater or equal to mtd size 0x%llx\n", start_off, mtd->size);
+			ret = CMD_RET_FAILURE;
+			goto out_put_mtd;
+		}
+		default_len = mtd->size - start_off;
+	}
+
 	len = argc > 1 ? hextoul(argv[1], NULL) : default_len;
 	if (!mtd_is_aligned_with_min_io_size(mtd, len)) {
 		len = round_up(len, mtd->writesize);
-- 
2.43.0

Re: [PATCH v2 1/3] cmd: mtd: add logs when bad blocks are encountered

[Quentin Schulz]

Hi Peter,

On 1/12/26 10:09 AM, Peter Suti wrote:
> [You don't often get email from peter.suti@streamunlimited.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 

We generally do not like having commits with empty commit logs. What 
made you want to write this patch? Why should we take this in?

> Signed-off-by: Peter Suti <peter.suti@streamunlimited.com>
> ---
>   cmd/mtd.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/mtd.c b/cmd/mtd.c
> index 7f25144098b..32733e8f11f 100644
> --- a/cmd/mtd.c
> +++ b/cmd/mtd.c
> @@ -559,8 +559,10 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
> 
>          /* Search for the first good block after the given offset */
>          off = start_off;
> -       while (mtd_block_isbad(mtd, off))
> +       while (mtd_block_isbad(mtd, off)) {
> +               printf("Bad block: failed to read at offset 0x%llx, skipping.\n", off);

I'm not verse in flashes... do we know if a block is a bad block by 
reading it?

Because the code you're looking at can be reached by mtd read, mtd write 
or mtd dump, so maybe we shouldn't say "read" in the error message?

Finally, is this something we should be printing in the command? What 
about the other users (env/mtd.c, dfu, the onenand command). Should we 
rather print (using the logger even maybe?) in the MTD core? Also, 
should we print even if this is not critical? I assume skipping bad 
blocks isn't always an issue?

Cheers,
Quentin