[PATCH mtd-utils] nandwrite: warn about writing 0xff blocks

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Rafał Miłecki" <zajec5 at gmail.com>
> An: "Miquel Raynal" <miquel.raynal at bootlin.com>, "richard" <richard at nod.at>
> CC: "linux-mtd" <linux-mtd at lists.infradead.org>, "Rafał Miłecki" <rafal at milecki.pl>
> Gesendet: Freitag, 25. März 2022 13:00:25
> Betreff: [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks

> From: Rafał Miłecki <rafal at milecki.pl>
> 
> Such blocks may be incorrectly treated as empty (even though they may
> have non-erase OOB). Warn about it so people may start suing
> --skip-all-ffs .
> 
> Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
> ---
> nand-utils/nandwrite.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
> index e8a210c..cd53a17 100644
> --- a/nand-utils/nandwrite.c
> +++ b/nand-utils/nandwrite.c
> @@ -280,6 +280,7 @@ int main(int argc, char * const argv[])
> 	libmtd_t mtd_desc;
> 	int ebsize_aligned;
> 	uint8_t write_mode;
> +	size_t all_ffs_cnt = 0;
> 
> 	process_options(argc, argv);
> 
> @@ -417,6 +418,8 @@ int main(int argc, char * const argv[])
> 	 */
> 	while ((imglen > 0 || writebuf < filebuf + filebuf_len)
> 		&& mtdoffset < mtd.size) {
> +		bool allffs;
> +
> 		/*
> 		 * New eraseblock, check for bad block(s)
> 		 * Stay in the loop to be sure that, if mtdoffset changes because
> @@ -555,7 +558,8 @@ int main(int argc, char * const argv[])
> 		}
> 
> 		ret = 0;
> -		if (!skipallffs || !buffer_check_pattern(writebuf, mtd.min_io_size, 0xff)) {
> +		allffs = buffer_check_pattern(writebuf, mtd.min_io_size, 0xff);
> +		if (!allffs || !skipallffs) {

Why is checking for allffs needed here?

> 			/* Write out data */
> 			ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size,
> 					mtdoffset % mtd.eb_size,
> @@ -564,6 +568,8 @@ int main(int argc, char * const argv[])
> 					writeoob ? oobbuf : NULL,
> 					writeoob ? mtd.oob_size : 0,
> 					write_mode);
> +			if (!ret && allffs)

Why checking for !ret?

> +				all_ffs_cnt++;
> 		}
> 
> 		if (ret) {
> @@ -615,6 +621,11 @@ closeall:
> 		   || (writebuf < filebuf + filebuf_len))
> 		sys_errmsg_die("Data was only partially written due to error");
> 
> +	if (all_ffs_cnt) {
> +		fprintf(stderr, "Written %zu blocks containing only 0xff bytes\n",
> all_ffs_cnt);
> +		fprintf(stderr, "Those block may be incorrectly treated as empty!\n");
> +	}
> +

While I like the patch I'm still not so convinced why we can't make skipallffs=true by default.

Thanks,
//richard