Re: [PATCH v2] cmd: mtd: fix speed measurement in the speed benchmark

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello,

On 26/08/2025 at 22:21:31 +03, Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu> wrote:

> The shown speed inversely proportional to the data size.

                 ^ is

> See the output:
>
>   spi-nand: spi_nand nand@0: Micron SPI NAND was found.
>   spi-nand: spi_nand nand@0: 256 MiB, block size: 128 KiB, page size: 2048, OOB size: 128
>   ...
>   => mtd read.benchmark spi-nand0 $loadaddr 0 0x40000
>   Reading 262144 byte(s) (128 page(s)) at offset 0x00000000
>   Read speed: 63kiB/s
>   => mtd read.benchmark spi-nand0 $loadaddr 0 0x20000
>   Reading 131072 byte(s) (64 page(s)) at offset 0x00000000
>   Read speed: 127kiB/s
>   => mtd read.benchmark spi-nand0 $loadaddr 0 0x10000
>   Reading 65536 byte(s) (32 page(s)) at offset 0x00000000
>   Read speed: 254kiB/s
>
> In the spi-nand case 'io_op.len' is not always the same as 'len', thus
> we are using the wrong amount of data to derive the speed.
>
> The patch also updates the help for 'mtd read' command to show the presence
> of the benchmark option. The help for the 'mtd write' command help has not
> been changed to fit 80 character width restriction.

I would put this into a second commit, and anyway adapt the write
command as well. I do not think the 80 chars rule is so important here.

> Fixes: d246e70cf81d0 ("cmd: mtd: Enable speed benchmarking")
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> ---
>  cmd/mtd.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> Changes v2:
>  * update commit message
>  * 'Fixes:' tag was added
>  * calculate speed a bit more precisely using a 64-bit math

This should have been done in a separate patch. In general we prefer
atomic changes. It's okay as the diff is small, but as an example I'd
have instead done that with 3 patches:
- Fix the calculation (sf/io_op.len/len)
- Change the size because you fear 1h is not enough
- Document the commands

Anyway,

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

Thanks,
Miquèl