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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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.