diff mbox series

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

Message ID 20220325120025.17931-1-zajec5@gmail.com
State Accepted
Delegated to: David Oberhollenzer
Headers show
Series [mtd-utils] nandwrite: warn about writing 0xff blocks | expand

Commit Message

Rafał Miłecki March 25, 2022, noon UTC 
From: Rafał Miłecki <rafal@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@milecki.pl>
---
 nand-utils/nandwrite.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

David Oberhollenzer March 28, 2022, 6:51 a.m. UTC | #1 
Applied to mtd-utils.git master.

Thanks,

David
Richard Weinberger March 28, 2022, 7:27 a.m. UTC | #2 
----- Ursprüngliche Mail -----
> Von: "Rafał Miłecki" <zajec5@gmail.com>
> An: "Miquel Raynal" <miquel.raynal@bootlin.com>, "richard" <richard@nod.at>
> CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "Rafał Miłecki" <rafal@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@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@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
Richard Weinberger March 28, 2022, 7:28 a.m. UTC | #3 
----- Ursprüngliche Mail -----
> Von: "david oberhollenzer" <david.oberhollenzer@sigma-star.at>
> An: "Rafał Miłecki" <zajec5@gmail.com>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "richard" <richard@nod.at>
> CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "Rafał Miłecki" <rafal@milecki.pl>
> Gesendet: Montag, 28. März 2022 08:51:38
> Betreff: Re: [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks

> Applied to mtd-utils.git master.

Please slow down a little. :)

Thanks,
//richard
Rafał Miłecki March 28, 2022, 8:29 a.m. UTC | #4 
On 28.03.2022 09:27, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Rafał Miłecki" <zajec5@gmail.com>
>> An: "Miquel Raynal" <miquel.raynal@bootlin.com>, "richard" <richard@nod.at>
>> CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "Rafał Miłecki" <rafal@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@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@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
Miquel Raynal March 28, 2022, 8:45 a.m. UTC | #5 
Hi Rafał,

zajec5@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@gmail.com>
> >> An: "Miquel Raynal" <miquel.raynal@bootlin.com>, "richard" <richard@nod.at>
> >> CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "Rafał Miłecki" <rafal@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@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@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
Richard Weinberger March 28, 2022, 8:51 a.m. UTC | #6 
----- Ursprüngliche Mail -----
> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
>> > 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?

Also emit a warning that a 0xff page is being skipped if the user
no not specify anything at the command line.
Then users will notice that the default has changed.

Thanks,
//richard
diff mbox series

Patch

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) {
 			/* 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)
+				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");
+	}
+
 	/* Return happy */
 	return EXIT_SUCCESS;
 }