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

[Rafał Miłecki]

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