* [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks
@ 2022-03-25 12:00 Rafał Miłecki
2022-03-28 6:51 ` David Oberhollenzer
2022-03-28 7:27 ` Richard Weinberger
0 siblings, 2 replies; 7+ messages in thread
From: Rafał Miłecki @ 2022-03-25 12:00 UTC (permalink / raw)
To: Miquel Raynal, Richard Weinberger; +Cc: linux-mtd, Rafał Miłecki
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) {
/* 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;
}
--
2.34.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks
2022-03-25 12:00 [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks Rafał Miłecki
@ 2022-03-28 6:51 ` David Oberhollenzer
2022-03-28 7:28 ` Richard Weinberger
2022-03-28 7:27 ` Richard Weinberger
1 sibling, 1 reply; 7+ messages in thread
From: David Oberhollenzer @ 2022-03-28 6:51 UTC (permalink / raw)
To: Rafał Miłecki, Miquel Raynal, Richard Weinberger
Cc: linux-mtd, Rafał Miłecki
Applied to mtd-utils.git master.
Thanks,
David
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks
2022-03-28 6:51 ` David Oberhollenzer
@ 2022-03-28 7:28 ` Richard Weinberger
0 siblings, 0 replies; 7+ messages in thread
From: Richard Weinberger @ 2022-03-28 7:28 UTC (permalink / raw)
To: david oberhollenzer
Cc: Rafał Miłecki, Miquel Raynal, linux-mtd,
Rafał Miłecki
----- 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks
2022-03-25 12:00 [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks Rafał Miłecki
2022-03-28 6:51 ` David Oberhollenzer
@ 2022-03-28 7:27 ` Richard Weinberger
2022-03-28 8:29 ` Rafał Miłecki
1 sibling, 1 reply; 7+ messages in thread
From: Richard Weinberger @ 2022-03-28 7:27 UTC (permalink / raw)
To: Rafał Miłecki; +Cc: Miquel Raynal, linux-mtd, Rafał Miłecki
----- 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks
2022-03-28 7:27 ` Richard Weinberger
@ 2022-03-28 8:29 ` Rafał Miłecki
2022-03-28 8:45 ` Miquel Raynal
0 siblings, 1 reply; 7+ messages in thread
From: Rafał Miłecki @ 2022-03-28 8:29 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Miquel Raynal, linux-mtd, Rafał Miłecki
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks
2022-03-28 8:29 ` Rafał Miłecki
@ 2022-03-28 8:45 ` Miquel Raynal
2022-03-28 8:51 ` Richard Weinberger
0 siblings, 1 reply; 7+ messages in thread
From: Miquel Raynal @ 2022-03-28 8:45 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Richard Weinberger, linux-mtd, Rafał Miłecki
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks
2022-03-28 8:45 ` Miquel Raynal
@ 2022-03-28 8:51 ` Richard Weinberger
0 siblings, 0 replies; 7+ messages in thread
From: Richard Weinberger @ 2022-03-28 8:51 UTC (permalink / raw)
To: Miquel Raynal; +Cc: Rafał Miłecki, linux-mtd, Rafał Miłecki
----- 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-28 8:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-25 12:00 [PATCH mtd-utils] nandwrite: warn about writing 0xff blocks Rafał Miłecki
2022-03-28 6:51 ` David Oberhollenzer
2022-03-28 7:28 ` Richard Weinberger
2022-03-28 7:27 ` Richard Weinberger
2022-03-28 8:29 ` Rafał Miłecki
2022-03-28 8:45 ` Miquel Raynal
2022-03-28 8:51 ` Richard Weinberger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox