Re: [PATCH] nandwrite: Factor out buffer checking code

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

Hi Marek,

On Thu,  8 Dec 2016 05:13:42 +0100
Marek Vasut <marex@denx.de> wrote:

> Pull the buffer content checking code into separate function and
> simplify the code invoking it slightly.

Thanks for taking the time to prepare and send this patch.

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> Cc: Kees Trommel <ctrommel@aimvalley.nl>
> ---
> NOTE: This is only compile-tested.
> ---
>  nand-utils/nandwrite.c | 42 ++++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
> index b376869..c5bd1c2 100644
> --- a/nand-utils/nandwrite.c
> +++ b/nand-utils/nandwrite.c
> @@ -231,6 +231,31 @@ static void erase_buffer(void *buffer, size_t size)
>  		memset(buffer, kEraseByte, size);
>  }
>  
> +/* Check whether buffer is filled with character 'pattern' */
> +static int buffer_check_pattern(unsigned char *buffer, size_t size,
> +				unsigned char pattern)
> +{
> +	/* Invalid input */
> +	if (!buffer || (size == 0))
> +		return 0;
> +
> +	/* No match on first byte */
> +	if (*buffer != pattern)
> +		return 0;
> +
> +	/* First byte matched and buffer is 1 byte long, OK. */
> +	if (size == 1)
> +		return 1;
> +
> +	/*
> +	 * Check buffer longer than 1 byte. We already know that buffer[0]
> +	 * matches the pattern, so the test below only checks whether the
> +	 * buffer[0...size-2] == buffer[1...size-1] , which is a test for
> +	 * whether the buffer is filled with constant value.
> +	 */
> +	return !memcmp(buffer, buffer + 1, size - 1);
> +}
> +

Can we put that in include/common.h as an inline function so that other
tools/libs can re-use it?
I just grepped for the 0xff/0xFF pattern and found several places that
could use this function instead of open-coding it.

Looks good otherwise.

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

>  /*
>   * Main program
>   */
> @@ -523,18 +548,8 @@ int main(int argc, char * const argv[])
>  			}
>  		}
>  
> -		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) {
> +		ret = 0;
> +		if (!skipallffs || !buffer_check_pattern(writebuf, mtd.min_io_size, 0xff)) {
>  			/* Write out data */
>  			ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size,
>  					mtdoffset % mtd.eb_size,
> @@ -543,9 +558,8 @@ int main(int argc, char * const argv[])
>  					writeoob ? oobbuf : NULL,
>  					writeoob ? mtd.oob_size : 0,
>  					write_mode);
> -		} else  {
> -			ret = 0;
>  		}
> +
>  		if (ret) {
>  			long long i;
>  			if (errno != EIO) {