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

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

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 ;-).