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 at 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