Add --skip-all-ffs option to mtd-utils nandwrite

Add --skip-all-ffs option to mtd-utils nandwrite

[Kees Trommel]

[-- Attachment #1: Type: text/plain, Size: 881 bytes --]

All,

Would it be possible to add an option --skip-all-ffs option to mtd-utils 
nandwrite?

With this option pages that contain only 0xFF bytes will be skipped and 
not written to flash.

This option is useful when you want to write using nandwrite an UBI 
image to NAND devices with a HW ECC. Without this option the OOB of a 
page that is written with all 0xFFs is no longer erased because the HW 
adds an non 0xFFs ECC to the OOB. If the data of the page contains only 
0xFFs then UBI/UBIFS assumes that the page is erased and writes to it 
without erasing the page first. This causes that a read of this page 
fails with unrecoverable ECC errors and a subsequent corruption of the 
UBIFS.

Attached is a patch file that adds the --skip-all-ffs option. A 
nandwrite of UBI image with this option will be successful while it 
fails without this option.

Regards,

Kees Trommel



[-- Attachment #2: 0001-skip-all-ff-pages-option.patch --]
[-- Type: text/x-patch, Size: 3222 bytes --]

>From f5dd8f13b45a37f0375f01a05a3ed41b39b50beb Mon Sep 17 00:00:00 2001
From: Kees Trommel <ctrommel@linvm302.aimsys.nl>
Date: Mon, 5 Dec 2016 16:37:11 +0100
Subject: [PATCH] skip-all-ff-pages-option

Signed-off-by: Kees Trommel <ctrommel@linvm302.aimsys.nl>
---
 nandwrite.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index 9c3fe8f..722b1bd 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -49,6 +49,7 @@ static void display_help(int status)
 "Writes to the specified MTD device.\n"
 "\n"
 "  -a, --autoplace         Use auto OOB layout\n"
+"  -k, --skip-all-ffs      Skip pages that contain only ff bytes\n"
 "  -m, --markbad           Mark blocks bad if write fails\n"
 "  -n, --noecc             Write without ecc\n"
 "  -N, --noskipbad         Write without bad block skipping\n"
@@ -93,6 +94,7 @@ static bool		onlyoob = false;
 static bool		markbad = false;
 static bool		noecc = false;
 static bool		autoplace = false;
+static bool		skipallffs = false;
 static bool		noskipbad = false;
 static bool		pad = false;
 static int		blockalign = 1; /* default to using actual block size */
@@ -103,7 +105,7 @@ static void process_options(int argc, char * const argv[])
 
 	for (;;) {
 		int option_index = 0;
-		static const char short_options[] = "hb:mnNoOpqs:a";
+		static const char short_options[] = "hb:mnNoOpqs:ak";
 		static const struct option long_options[] = {
 			/* Order of these args with val==0 matters; see option_index. */
 			{"version", no_argument, 0, 0},
@@ -120,6 +122,7 @@ static void process_options(int argc, char * const argv[])
 			{"quiet", no_argument, 0, 'q'},
 			{"start", required_argument, 0, 's'},
 			{"autoplace", no_argument, 0, 'a'},
+			{"skip-all-ffs", no_argument, 0, 'k'},
 			{0, 0, 0, 0},
 		};
 
@@ -173,6 +176,9 @@ static void process_options(int argc, char * const argv[])
 		case 'a':
 			autoplace = true;
 			break;
+		case 'k':
+			skipallffs = true;
+			break;
 		case 'h':
 			display_help(EXIT_SUCCESS);
 			break;
@@ -231,6 +237,8 @@ static void erase_buffer(void *buffer, size_t size)
  */
 int main(int argc, char * const argv[])
 {
+	int allffs;
+	int ii;
 	int fd = -1;
 	int ifd = -1;
 	int pagelen;
@@ -516,14 +524,29 @@ int main(int argc, char * const argv[])
 			}
 		}
 
-		/* Write out data */
-		ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size,
-				mtdoffset % mtd.eb_size,
-				onlyoob ? NULL : writebuf,
-				onlyoob ? 0 : mtd.min_io_size,
-				writeoob ? oobbuf : NULL,
-				writeoob ? mtd.oob_size : 0,
-				write_mode);
+		allffs = 0;
+		if (skipallffs) 
+		{
+			for (ii = 0; ii < mtd.min_io_size; ii += sizeof(uint32_t))
+			{
+				if (*(uint32_t*)(writebuf + ii) != 0xffffffff)
+					break;
+			}
+			if (ii == mtd.min_io_size)
+				allffs = 1;
+		}
+		if (!allffs) {
+			/* Write out data */
+			ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size,
+					mtdoffset % mtd.eb_size,
+					onlyoob ? NULL : writebuf,
+					onlyoob ? 0 : mtd.min_io_size,
+					writeoob ? oobbuf : NULL,
+					writeoob ? mtd.oob_size : 0,
+					write_mode);
+		} else  {
+			ret = 0;
+		}
 		if (ret) {
 			long long i;
 			if (errno != EIO) {
-- 
2.5.5

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[David Oberhollenzer]

Hi,


On 12/05/2016 05:14 PM, Kees Trommel wrote:
> Would it be possible to add an option --skip-all-ffs option to mtd-utils nandwrite?
> 
> With this option pages that contain only 0xFF bytes will be skipped and not written to flash.
> 
> This option is useful when you want to write using nandwrite an UBI image to NAND devices with a HW ECC. Without this option the OOB of a page that is written with all 0xFFs is no longer erased because the HW adds an non 0xFFs ECC to the OOB. If the data of the page contains only 0xFFs then UBI/UBIFS assumes that the page is erased and writes to it without erasing the page first. This causes that a read of this page fails with unrecoverable ECC errors and a subsequent corruption of the UBIFS.

Yes, that sounds reasonable.
 
> Attached is a patch file that adds the --skip-all-ffs option. A nandwrite of UBI image with this option will be successful while it fails without this option.

Your patch looks good to me, however even after fixing the paths to nandwrite.c (the
source tree got restructured over a year ago, see commit 7d81790ced) it doesn't apply
to the current mtd-utils master branch. Could you please rebase your patch onto the
current mtd-utils master branch (git://git.infradead.org/mtd-utils.git) and resubmit it?


Thanks,

David

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[Kees Trommel]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

David,

Attached a rebased patch file.

Kees.

On 12/05/16 20:13, David Oberhollenzer wrote:
> Hi,
>
>
> On 12/05/2016 05:14 PM, Kees Trommel wrote:
>> Would it be possible to add an option --skip-all-ffs option to mtd-utils nandwrite?
>>
>> With this option pages that contain only 0xFF bytes will be skipped and not written to flash.
>>
>> This option is useful when you want to write using nandwrite an UBI image to NAND devices with a HW ECC. Without this option the OOB of a page that is written with all 0xFFs is no longer erased because the HW adds an non 0xFFs ECC to the OOB. If the data of the page contains only 0xFFs then UBI/UBIFS assumes that the page is erased and writes to it without erasing the page first. This causes that a read of this page fails with unrecoverable ECC errors and a subsequent corruption of the UBIFS.
> Yes, that sounds reasonable.
>   
>> Attached is a patch file that adds the --skip-all-ffs option. A nandwrite of UBI image with this option will be successful while it fails without this option.
> Your patch looks good to me, however even after fixing the paths to nandwrite.c (the
> source tree got restructured over a year ago, see commit 7d81790ced) it doesn't apply
> to the current mtd-utils master branch. Could you please rebase your patch onto the
> current mtd-utils master branch (git://git.infradead.org/mtd-utils.git) and resubmit it?
>
>
> Thanks,
>
> David
>
>


[-- Attachment #2: 0001-skip-all-ff-pages-option.patch --]
[-- Type: text/x-patch, Size: 3282 bytes --]

>From 8bf3ad295aee71fdcee48fe462908503ac82ff95 Mon Sep 17 00:00:00 2001
From: Kees Trommel <ctrommel@linvm302.aimsys.nl>
Date: Tue, 6 Dec 2016 09:21:19 +0100
Subject: [PATCH] skip-all-ff-pages-option

Signed-off-by: Kees Trommel <ctrommel@linvm302.aimsys.nl>
---
 nand-utils/nandwrite.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
index 1c00cdf..b376869 100644
--- a/nand-utils/nandwrite.c
+++ b/nand-utils/nandwrite.c
@@ -49,6 +49,7 @@ static void display_help(int status)
 "Writes to the specified MTD device.\n"
 "\n"
 "  -a, --autoplace         Use auto OOB layout\n"
+"  -k, --skip-all-ffs      Skip pages that contain only 0xff bytes\n"
 "  -m, --markbad           Mark blocks bad if write fails\n"
 "  -n, --noecc             Write without ecc\n"
 "  -N, --noskipbad         Write without bad block skipping\n"
@@ -92,6 +93,7 @@ static bool		onlyoob = false;
 static bool		markbad = false;
 static bool		noecc = false;
 static bool		autoplace = false;
+static bool		skipallffs = false;
 static bool		noskipbad = false;
 static bool		pad = false;
 static int		blockalign = 1; /* default to using actual block size */
@@ -102,7 +104,7 @@ static void process_options(int argc, char * const argv[])
 
 	for (;;) {
 		int option_index = 0;
-		static const char short_options[] = "hb:mnNoOpqs:aV";
+		static const char short_options[] = "hb:mnNoOpqs:akV";
 		static const struct option long_options[] = {
 			/* Order of these args with val==0 matters; see option_index. */
 			{"version", no_argument, 0, 'V'},
@@ -119,6 +121,7 @@ static void process_options(int argc, char * const argv[])
 			{"quiet", no_argument, 0, 'q'},
 			{"start", required_argument, 0, 's'},
 			{"autoplace", no_argument, 0, 'a'},
+			{"skip-all-ffs", no_argument, 0, 'k'},
 			{0, 0, 0, 0},
 		};
 
@@ -172,6 +175,9 @@ static void process_options(int argc, char * const argv[])
 		case 'a':
 			autoplace = true;
 			break;
+		case 'k':
+			skipallffs = true;
+			break;
 		case 'h':
 			display_help(EXIT_SUCCESS);
 			break;
@@ -230,6 +236,8 @@ static void erase_buffer(void *buffer, size_t size)
  */
 int main(int argc, char * const argv[])
 {
+	int allffs;
+	int ii;
 	int fd = -1;
 	int ifd = -1;
 	int pagelen;
@@ -515,14 +523,29 @@ int main(int argc, char * const argv[])
 			}
 		}
 
-		/* Write out data */
-		ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size,
-				mtdoffset % mtd.eb_size,
-				onlyoob ? NULL : writebuf,
-				onlyoob ? 0 : mtd.min_io_size,
-				writeoob ? oobbuf : NULL,
-				writeoob ? mtd.oob_size : 0,
-				write_mode);
+		allffs = 0;
+		if (skipallffs) 
+		{
+			for (ii = 0; ii < mtd.min_io_size; ii += sizeof(uint32_t))
+			{
+				if (*(uint32_t*)(writebuf + ii) != 0xffffffff)
+					break;
+			}
+			if (ii == mtd.min_io_size)
+				allffs = 1;
+		}
+		if (!allffs) {
+			/* Write out data */
+			ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size,
+					mtdoffset % mtd.eb_size,
+					onlyoob ? NULL : writebuf,
+					onlyoob ? 0 : mtd.min_io_size,
+					writeoob ? oobbuf : NULL,
+					writeoob ? mtd.oob_size : 0,
+					write_mode);
+		} else  {
+			ret = 0;
+		}
 		if (ret) {
 			long long i;
 			if (errno != EIO) {
-- 
2.5.5

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[David Oberhollenzer]

On 12/06/2016 09:29 AM, Kees Trommel wrote:
> David,
> 
> Attached a rebased patch file.
> 
> Kees.
Applied to mtd-utils.git.


Thanks,

David

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[Marek Vasut]

On 12/06/2016 02:19 PM, David Oberhollenzer wrote:
> On 12/06/2016 09:29 AM, Kees Trommel wrote:
>> David,
>>
>> Attached a rebased patch file.
>>
>> Kees.
> Applied to mtd-utils.git.

It'd be great to have the patch posted to the list with git send-email ,
otherwise it's not possible to review it :-(

Also, the code doesn't look awesome, see below:

+               if (skipallffs)
+               {
+                       for (ii = 0; ii < mtd.min_io_size; ii +=
sizeof(uint32_t))
+                       {
+                               if (*(uint32_t*)(writebuf + ii) !=
0xffffffff)
+                                       break;

Is this memcmp()-alike function ?

+                       }
+                       if (ii == mtd.min_io_size)
+                               allffs = 1;
+               }
+               if (!allffs) {

This could be simply turned to:

ret = 0;
if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) {
    ret = mtd_write(....);
}

And then you don't need to introduce any new vars (which have weird
names like 'ii' ) either.

+                       /* Write out data */
+                       ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset /
mtd.eb_size,
+                                       mtdoffset % mtd.eb_size,
+                                       onlyoob ? NULL : writebuf,
+                                       onlyoob ? 0 : mtd.min_io_size,
+                                       writeoob ? oobbuf : NULL,
+                                       writeoob ? mtd.oob_size : 0,
+                                       write_mode);
+               } else  {
+                       ret = 0;
+               }

-- 
Best regards,
Marek Vasut

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[David Oberhollenzer]

On 12/07/2016 06:01 AM, Marek Vasut wrote:
> +               if (skipallffs)
> +               {
> +                       for (ii = 0; ii < mtd.min_io_size; ii +=
> sizeof(uint32_t))
> +                       {
> +                               if (*(uint32_t*)(writebuf + ii) !=
> 0xffffffff)
> +                                       break;
> 
> Is this memcmp()-alike function ?
> 
> +                       }
> +                       if (ii == mtd.min_io_size)
> +                               allffs = 1;
> +               }
> +               if (!allffs) {
The point of the patch is not to write a page that is filled with 0xFF bytes.
A feature that is turned off by default and can be activated via a command
line switch.

The code you are trying to replace tries to figure out if an entire block of
memory is set to 0xFF. The code path should be conditional, as this feature
is turned off unless explicitly requested.

> This could be simply turned to:
> 
> ret = 0;
> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) {
>     ret = mtd_write(....);
> }
This does something completely different.

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[Marek Vasut]

On 12/07/2016 09:21 AM, David Oberhollenzer wrote:
> On 12/07/2016 06:01 AM, Marek Vasut wrote:
>> +               if (skipallffs)
>> +               {
>> +                       for (ii = 0; ii < mtd.min_io_size; ii +=
>> sizeof(uint32_t))
>> +                       {
>> +                               if (*(uint32_t*)(writebuf + ii) !=
>> 0xffffffff)
>> +                                       break;
>>
>> Is this memcmp()-alike function ?
>>
>> +                       }
>> +                       if (ii == mtd.min_io_size)
>> +                               allffs = 1;
>> +               }
>> +               if (!allffs) {
> The point of the patch is not to write a page that is filled with 0xFF bytes.
> A feature that is turned off by default and can be activated via a command
> line switch.

That much I figured out

> The code you are trying to replace tries to figure out if an entire block of
> memory is set to 0xFF. The code path should be conditional, as this feature
> is turned off unless explicitly requested.

And to do that, we need to open-code it like above ?
Looks pretty crappy.

>> This could be simply turned to:
>>
>> ret = 0;
>> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) {
>>     ret = mtd_write(....);
>> }
> This does something completely different.

It checks whether the buffer if full of the same bytes, yes ?
Add additional conditions as needed (check whether user provided
the arg and whether byte 0 in the buffer is 0xff and you should
be done).

-- 
Best regards,
Marek Vasut

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[Boris Brezillon]

On Wed, 7 Dec 2016 15:09:49 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 12/07/2016 09:21 AM, David Oberhollenzer wrote:
> > On 12/07/2016 06:01 AM, Marek Vasut wrote:  
> >> +               if (skipallffs)
> >> +               {
> >> +                       for (ii = 0; ii < mtd.min_io_size; ii +=
> >> sizeof(uint32_t))
> >> +                       {
> >> +                               if (*(uint32_t*)(writebuf + ii) !=
> >> 0xffffffff)
> >> +                                       break;
> >>
> >> Is this memcmp()-alike function ?
> >>
> >> +                       }
> >> +                       if (ii == mtd.min_io_size)
> >> +                               allffs = 1;
> >> +               }
> >> +               if (!allffs) {  
> > The point of the patch is not to write a page that is filled with 0xFF bytes.
> > A feature that is turned off by default and can be activated via a command
> > line switch.  
> 
> That much I figured out
> 
> > The code you are trying to replace tries to figure out if an entire block of
> > memory is set to 0xFF. The code path should be conditional, as this feature
> > is turned off unless explicitly requested.  
> 
> And to do that, we need to open-code it like above ?
> Looks pretty crappy.

I agree with Marek on one point: we're likely to need the 0xff-pattern
check in other places, so how about implementing a helper (or several
helpers) in libmtd?

> 
> >> This could be simply turned to:
> >>
> >> ret = 0;
> >> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) {
> >>     ret = mtd_write(....);
> >> }  

Oh, nice trick! It works with and additional writebuf[0] == 0xff test.
However I'm concerned about readability (can be addressed with a comment
explaining what you're doing), and perfs (memcmp is likely to be
optimized for aligned accesses, and you defeat that with your writebuf
+ 1).

Here's a proposal for the pattern comparison API:

struct pattern_buf {
	unsigned int size;
	void *buf;
}

const struct pattern_buf *mtd_create_pattern_buf(int size, u8 pattern);
void mtd_free_pattern_buf(const struct pattern_buf *pbuf);
bool mtd_check_pattern(const struct pattern_buf *pbuf, void *buf,
		       int size);

This way you can allocate a pattern buffer of pagesize and re-use it for
each new page you want to check. This also solves the unaligned
memcmp() problem.

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[Marek Vasut]

On 12/07/2016 05:07 PM, Boris Brezillon wrote:
> On Wed, 7 Dec 2016 15:09:49 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 12/07/2016 09:21 AM, David Oberhollenzer wrote:
>>> On 12/07/2016 06:01 AM, Marek Vasut wrote:  
>>>> +               if (skipallffs)
>>>> +               {
>>>> +                       for (ii = 0; ii < mtd.min_io_size; ii +=
>>>> sizeof(uint32_t))
>>>> +                       {
>>>> +                               if (*(uint32_t*)(writebuf + ii) !=
>>>> 0xffffffff)
>>>> +                                       break;
>>>>
>>>> Is this memcmp()-alike function ?
>>>>
>>>> +                       }
>>>> +                       if (ii == mtd.min_io_size)
>>>> +                               allffs = 1;
>>>> +               }
>>>> +               if (!allffs) {  
>>> The point of the patch is not to write a page that is filled with 0xFF bytes.
>>> A feature that is turned off by default and can be activated via a command
>>> line switch.  
>>
>> That much I figured out
>>
>>> The code you are trying to replace tries to figure out if an entire block of
>>> memory is set to 0xFF. The code path should be conditional, as this feature
>>> is turned off unless explicitly requested.  
>>
>> And to do that, we need to open-code it like above ?
>> Looks pretty crappy.
> 
> I agree with Marek on one point: we're likely to need the 0xff-pattern
> check in other places, so how about implementing a helper (or several
> helpers) in libmtd?

Helper would be great, yeah.

>>>> This could be simply turned to:
>>>>
>>>> ret = 0;
>>>> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) {
>>>>     ret = mtd_write(....);
>>>> }  
> 
> Oh, nice trick! It works with and additional writebuf[0] == 0xff test.
> However I'm concerned about readability (can be addressed with a comment
> explaining what you're doing)

Yeah, it's not exactly obvious. You need the writebuf[0] == pattern test
and bufsize >= 2 test.

>, and perfs (memcmp is likely to be
> optimized for aligned accesses, and you defeat that with your writebuf
> + 1).

That's up to libc optimization, really. memcpy() implementation in glibc
and musl at least does a lot of magic to optimize copying regardless of
alignment.

> Here's a proposal for the pattern comparison API:
> 
> struct pattern_buf {
> 	unsigned int size;
> 	void *buf;
> }
> 
> const struct pattern_buf *mtd_create_pattern_buf(int size, u8 pattern);
> void mtd_free_pattern_buf(const struct pattern_buf *pbuf);
> bool mtd_check_pattern(const struct pattern_buf *pbuf, void *buf,
> 		       int size);
> 
> This way you can allocate a pattern buffer of pagesize and re-use it for
> each new page you want to check. This also solves the unaligned
> memcmp() problem.

Here you're filling memory with stuff, which for large buffer will
trigger IO to main memory, which is slow. This is unnecessary since
you already have such data available to you (it's your own buffer and
it's in cache), just use it.

And the implementation would be:

int mtd_buffer_is_filled_with_byte(void *buf, size_t size, u8 pattern)
{
  int match;

  if (size == 0)
    return 0;

  match = (*buf == pattern);

  return (size == 1) ? match : !memcmp(buf, buf + 1, size - 1)
}

-- 
Best regards,
Marek Vasut

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[Boris Brezillon]

On Wed, 7 Dec 2016 17:31:04 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 12/07/2016 05:07 PM, Boris Brezillon wrote:
> > On Wed, 7 Dec 2016 15:09:49 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 12/07/2016 09:21 AM, David Oberhollenzer wrote:  
> >>> On 12/07/2016 06:01 AM, Marek Vasut wrote:    
> >>>> +               if (skipallffs)
> >>>> +               {
> >>>> +                       for (ii = 0; ii < mtd.min_io_size; ii +=
> >>>> sizeof(uint32_t))
> >>>> +                       {
> >>>> +                               if (*(uint32_t*)(writebuf + ii) !=
> >>>> 0xffffffff)
> >>>> +                                       break;
> >>>>
> >>>> Is this memcmp()-alike function ?
> >>>>
> >>>> +                       }
> >>>> +                       if (ii == mtd.min_io_size)
> >>>> +                               allffs = 1;
> >>>> +               }
> >>>> +               if (!allffs) {    
> >>> The point of the patch is not to write a page that is filled with 0xFF bytes.
> >>> A feature that is turned off by default and can be activated via a command
> >>> line switch.    
> >>
> >> That much I figured out
> >>  
> >>> The code you are trying to replace tries to figure out if an entire block of
> >>> memory is set to 0xFF. The code path should be conditional, as this feature
> >>> is turned off unless explicitly requested.    
> >>
> >> And to do that, we need to open-code it like above ?
> >> Looks pretty crappy.  
> > 
> > I agree with Marek on one point: we're likely to need the 0xff-pattern
> > check in other places, so how about implementing a helper (or several
> > helpers) in libmtd?  
> 
> Helper would be great, yeah.
> 
> >>>> This could be simply turned to:
> >>>>
> >>>> ret = 0;
> >>>> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) {
> >>>>     ret = mtd_write(....);
> >>>> }    
> > 
> > Oh, nice trick! It works with and additional writebuf[0] == 0xff test.
> > However I'm concerned about readability (can be addressed with a comment
> > explaining what you're doing)  
> 
> Yeah, it's not exactly obvious. You need the writebuf[0] == pattern test
> and bufsize >= 2 test.
> 
> >, and perfs (memcmp is likely to be
> > optimized for aligned accesses, and you defeat that with your writebuf
> > + 1).  
> 
> That's up to libc optimization, really. memcpy() implementation in glibc
> and musl at least does a lot of magic to optimize copying regardless of
> alignment.
> 
> > Here's a proposal for the pattern comparison API:
> > 
> > struct pattern_buf {
> > 	unsigned int size;
> > 	void *buf;
> > }
> > 
> > const struct pattern_buf *mtd_create_pattern_buf(int size, u8 pattern);
> > void mtd_free_pattern_buf(const struct pattern_buf *pbuf);
> > bool mtd_check_pattern(const struct pattern_buf *pbuf, void *buf,
> > 		       int size);
> > 
> > This way you can allocate a pattern buffer of pagesize and re-use it for
> > each new page you want to check. This also solves the unaligned
> > memcmp() problem.  
> 
> Here you're filling memory with stuff, which for large buffer will
> trigger IO to main memory, which is slow. This is unnecessary since
> you already have such data available to you (it's your own buffer and
> it's in cache), just use it.

Indeed, I didn't consider caches, and as you said, memcmp is likely to
be optimized for the unaligned case, and even if it's not, I'm not sure
we really care about perfs here. 

> 
> And the implementation would be:
> 
> int mtd_buffer_is_filled_with_byte(void *buf, size_t size, u8 pattern)
> {
>   int match;
> 
>   if (size == 0)
>     return 0;
> 
>   match = (*buf == pattern);
> 
>   return (size == 1) ? match : !memcmp(buf, buf + 1, size - 1)
> }
> 

Looks good to me. Feel free to post a patch ;-).

Re: Add --skip-all-ffs option to mtd-utils nandwrite

[Marek Vasut]

On 12/07/2016 05:59 PM, Boris Brezillon wrote:
> On Wed, 7 Dec 2016 17:31:04 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 12/07/2016 05:07 PM, Boris Brezillon wrote:
>>> On Wed, 7 Dec 2016 15:09:49 +0100
>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>   
>>>> On 12/07/2016 09:21 AM, David Oberhollenzer wrote:  
>>>>> On 12/07/2016 06:01 AM, Marek Vasut wrote:    
>>>>>> +               if (skipallffs)
>>>>>> +               {
>>>>>> +                       for (ii = 0; ii < mtd.min_io_size; ii +=
>>>>>> sizeof(uint32_t))
>>>>>> +                       {
>>>>>> +                               if (*(uint32_t*)(writebuf + ii) !=
>>>>>> 0xffffffff)
>>>>>> +                                       break;
>>>>>>
>>>>>> Is this memcmp()-alike function ?
>>>>>>
>>>>>> +                       }
>>>>>> +                       if (ii == mtd.min_io_size)
>>>>>> +                               allffs = 1;
>>>>>> +               }
>>>>>> +               if (!allffs) {    
>>>>> The point of the patch is not to write a page that is filled with 0xFF bytes.
>>>>> A feature that is turned off by default and can be activated via a command
>>>>> line switch.    
>>>>
>>>> That much I figured out
>>>>  
>>>>> The code you are trying to replace tries to figure out if an entire block of
>>>>> memory is set to 0xFF. The code path should be conditional, as this feature
>>>>> is turned off unless explicitly requested.    
>>>>
>>>> And to do that, we need to open-code it like above ?
>>>> Looks pretty crappy.  
>>>
>>> I agree with Marek on one point: we're likely to need the 0xff-pattern
>>> check in other places, so how about implementing a helper (or several
>>> helpers) in libmtd?  
>>
>> Helper would be great, yeah.
>>
>>>>>> This could be simply turned to:
>>>>>>
>>>>>> ret = 0;
>>>>>> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) {
>>>>>>     ret = mtd_write(....);
>>>>>> }    
>>>
>>> Oh, nice trick! It works with and additional writebuf[0] == 0xff test.
>>> However I'm concerned about readability (can be addressed with a comment
>>> explaining what you're doing)  
>>
>> Yeah, it's not exactly obvious. You need the writebuf[0] == pattern test
>> and bufsize >= 2 test.
>>
>>> , and perfs (memcmp is likely to be
>>> optimized for aligned accesses, and you defeat that with your writebuf
>>> + 1).  
>>
>> That's up to libc optimization, really. memcpy() implementation in glibc
>> and musl at least does a lot of magic to optimize copying regardless of
>> alignment.
>>
>>> Here's a proposal for the pattern comparison API:
>>>
>>> struct pattern_buf {
>>> 	unsigned int size;
>>> 	void *buf;
>>> }
>>>
>>> const struct pattern_buf *mtd_create_pattern_buf(int size, u8 pattern);
>>> void mtd_free_pattern_buf(const struct pattern_buf *pbuf);
>>> bool mtd_check_pattern(const struct pattern_buf *pbuf, void *buf,
>>> 		       int size);
>>>
>>> This way you can allocate a pattern buffer of pagesize and re-use it for
>>> each new page you want to check. This also solves the unaligned
>>> memcmp() problem.  
>>
>> Here you're filling memory with stuff, which for large buffer will
>> trigger IO to main memory, which is slow. This is unnecessary since
>> you already have such data available to you (it's your own buffer and
>> it's in cache), just use it.
> 
> Indeed, I didn't consider caches, and as you said, memcmp is likely to
> be optimized for the unaligned case, and even if it's not, I'm not sure
> we really care about perfs here. 
> 
>>
>> And the implementation would be:
>>
>> int mtd_buffer_is_filled_with_byte(void *buf, size_t size, u8 pattern)
>> {
>>   int match;
>>
>>   if (size == 0)
>>     return 0;
>>
>>   match = (*buf == pattern);
>>
>>   return (size == 1) ? match : !memcmp(buf, buf + 1, size - 1)
>> }
>>
> 
> Looks good to me. Feel free to post a patch ;-).

Grumble ... done.

-- 
Best regards,
Marek Vasut