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

[PATCH 1/2] 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.

Also add some logging when bad blocks are encountered.

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 | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 7f25144098b..1d1845bce44 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();
 
@@ -569,9 +571,32 @@ 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)) {
+			printf("Bad block: failed to read at offset 0x%llx, skipping.\n", off);
 			off += mtd->erasesize;
 			continue;
 		}
-- 
2.43.0

[PATCH 2/2] 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>
[add {} for better readability]
Signed-off-by: Radek Dostál <radek.dostal@streamunlimited.com>
---
 cmd/mtd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 1d1845bce44..9f6e3226c15 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -519,7 +519,15 @@ 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)
+			default_len = mtd->size - start_off;
+		else
+			default_len = 0;
+	}
+
 	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 1/2] cmd: mtd: fix read failure on default full partition read with bad blocks

[Michael Nazzareno Trimarchi]

HI Peter


On Tue, Dec 16, 2025 at 2:41 PM Peter Suti
<peter.suti@streamunlimited.com> wrote:
>
> 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.
>
> Also add some logging when bad blocks are encountered.
>
> 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 | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/mtd.c b/cmd/mtd.c
> index 7f25144098b..1d1845bce44 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;
> +       }

Would like to avoid adding more debug stuff or printing here.

>
>         led_activity_blink();
>
> @@ -569,9 +571,32 @@ 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);

This is ok as print, to inform about the boundary check

> +                       remaining = 0;
> +                       ret = CMD_RET_SUCCESS;

What is the drawback to failing here, I mean you can still verify the
integrity. GIve a script example
where this is needed in the commit message

> +                       break;
> +               }
> +
>                 /* 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;

This is an extra print for now, you can propose it on a separated patch

Michael

>                         continue;
>                 }
> --
> 2.43.0
>


--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH 2/2] cmd: mtd: fix default length calculation when offset is specified

[Michael Nazzareno Trimarchi]

HI

On Tue, Dec 16, 2025 at 2:41 PM Peter Suti
<peter.suti@streamunlimited.com> wrote:
>
> 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>
> [add {} for better readability]
> Signed-off-by: Radek Dostál <radek.dostal@streamunlimited.com>
> ---
>  cmd/mtd.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/mtd.c b/cmd/mtd.c
> index 1d1845bce44..9f6e3226c15 100644
> --- a/cmd/mtd.c
> +++ b/cmd/mtd.c
> @@ -519,7 +519,15 @@ 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)
> +                       default_len = mtd->size - start_off;
> +               else
> +                       default_len = 0;

Should it fail if the requested offset is bigger than mtd->size

Michael

> +       }
> +
>         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
>


-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

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

[Miquel Raynal]

Hi,

On 16/12/2025 at 14:40:41 +01, Peter Suti <peter.suti@streamunlimited.com> wrote:

> 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.
>
> Also add some logging when bad blocks are encountered.
>
> 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).

Thanks for the improvement!

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH 2/2] cmd: mtd: fix default length calculation when offset is specified

[Miquel Raynal]

On 16/12/2025 at 14:40:42 +01, Peter Suti <peter.suti@streamunlimited.com> wrote:

> 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>
> [add {} for better readability]
> Signed-off-by: Radek Dostál <radek.dostal@streamunlimited.com>

Makes sense.

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

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

[Peter Suti]

Hi Michael,

Thanks for the review!

On 12/20/25 09:39, Michael Nazzareno Trimarchi wrote:
>> diff --git a/cmd/mtd.c b/cmd/mtd.c
>> index 7f25144098b..1d1845bce44 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;
>> +       }
> Would like to avoid adding more debug stuff or printing here.
Sure, I'll send it as a separate patch in V2 with the other debug 
commit, then upstream can decide if it's worth taking or not.
>> +                       remaining = 0;
>> +                       ret = CMD_RET_SUCCESS;
> What is the drawback to failing here, I mean you can still verify the
> integrity. GIve a script example
> where this is needed in the commit message

This behavior is similar to the raw `nand` command which adjusts the 
size of the read with adjust_size_for_badblocks().
It is explained in the cover letter.

>
>> +                       break;
>> +               }
>> +
>>                  /* 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;
> This is an extra print for now, you can propose it on a separated patch
>
I'll send it as a separate patch with the other debug commit.

Best regards,
Peter

Re: [PATCH 2/2] cmd: mtd: fix default length calculation when offset is specified

[Peter Suti]

Hi Michael,

On 12/20/25 09:42, Michael Nazzareno Trimarchi wrote:
> On Tue, Dec 16, 2025 at 2:41 PM Peter Suti
> <peter.suti@streamunlimited.com> wrote:
>> ---
>>   cmd/mtd.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmd/mtd.c b/cmd/mtd.c
>> index 1d1845bce44..9f6e3226c15 100644
>> --- a/cmd/mtd.c
>> +++ b/cmd/mtd.c
>> @@ -519,7 +519,15 @@ 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)
>> +                       default_len = mtd->size - start_off;
>> +               else
>> +                       default_len = 0;
> Should it fail if the requested offset is bigger than mtd->size
>
> Michael

I think that would make more sense, yes. I'll send V2 with that fix.

Best Regards, Peter