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

[Miquel Raynal]

Hi Rafał,

zajec5 at gmail.com wrote on Mon, 28 Mar 2022 10:29:15 +0200:

> On 28.03.2022 09:27, Richard Weinberger wrote:
> > ----- 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?  
> 
> With --skip-all-ffs we want to write block if it contains data.
> 
> In other words this check is equal to:
> if (contains_data || write_all_block)
> 
> 
> >> 			/* 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?  
> 
> If mtd_write() returns error we didn't actualy write anything.
> 
> 
> >> +				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.  
> 
> I thought it's about changing / breaking user interface:
> 
> [2022-03-25] [11:40:53 CET] <derRichard> mraynal: i think we should make --skip-all-ffs default in nandwrite
> [2022-03-25] [11:40:56 CET] <derRichard> what do you think?
> [2022-03-25] [11:42:00 CET] <rmilecki> i was preparing a patch with warning if any 0xff block has been written
> [2022-03-25] [11:42:33 CET] <rmilecki> i didn't know defaulting to --skip-all-ffs can be done
> [2022-03-25] [11:47:05 CET] <mraynal> well, that would break the user interface

Indeed I raised this issue but I am not opposed to this change if
everybody agrees that it's a good move. In particular, the NAND
sublayer will give the same "empty" data to userspace whether it
programmed empty pages or skipped them. So I guess we are fine against
direct regressions (but we might break clever scripts doing strange
things).

I would go for this solution:
- Bring support for the double flag -[no-]skip-ffs
- Make the use of -skip-ffs the default
So that it is easy for scripting people to ensure their way is fine?

Thanks,
Miquèl