Re: [PATCH v2] mtd: mtd_oobtest: generate consitent data for verification

[Lee Jones <lee.jones@linaro.org>]

> mtd_oobtest writes OOB, read it back and verify.  The verification is
> not correctly done if oobsize is not multiple of 4.  Although the data
> to be written and the data to be compared are generated by several
> prandom_byte_state() calls starting with the same seed, these two are
> generated with the different size and different number of calls.
> 
> Due to the implementation of prandom_byte_state() if the size on each
> call is not multiple of 4, the resulting data is not always same.
> 
> This fixes it by just calling prandom_byte_state() once and using
> correct range instead of calling it multiple times for each.
> 
> Reported-by: George Cherian <george.cherian@ti.com>
> Reported-by: Lothar Waßmann <LW@KARO-electronics.de>
> Cc: George Cherian <george.cherian@ti.com>
> Cc: Lothar Waßmann <LW@KARO-electronics.de>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: linux-mtd@lists.infradead.org
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> v2: remove a dependency on unsubmitted patch
> 
>  drivers/mtd/tests/oobtest.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/tests/oobtest.c b/drivers/mtd/tests/oobtest.c
> index 2e9e2d1..c5ea177 100644
> --- a/drivers/mtd/tests/oobtest.c
> +++ b/drivers/mtd/tests/oobtest.c
> @@ -69,8 +69,8 @@ static int write_eraseblock(int ebnum)
>  	int err = 0;
>  	loff_t addr = ebnum * mtd->erasesize;
>  
> +	prandom_bytes_state(&rnd_state, writebuf, use_len_max * pgcnt);
>  	for (i = 0; i < pgcnt; ++i, addr += mtd->writesize) {
> -		prandom_bytes_state(&rnd_state, writebuf, use_len);
>  		ops.mode      = MTD_OPS_AUTO_OOB;
>  		ops.len       = 0;
>  		ops.retlen    = 0;
> @@ -78,7 +78,7 @@ static int write_eraseblock(int ebnum)
>  		ops.oobretlen = 0;
>  		ops.ooboffs   = use_offset;
>  		ops.datbuf    = NULL;
> -		ops.oobbuf    = writebuf;
> +		ops.oobbuf    = writebuf + use_len_max * i + use_offset;

Can you bracket-up the new complicated math for clarity please?

>  		err = mtd_write_oob(mtd, addr, &ops);
>  		if (err || ops.oobretlen != use_len) {
>  			pr_err("error: writeoob failed at %#llx\n",
> @@ -122,8 +122,8 @@ static int verify_eraseblock(int ebnum)
>  	int err = 0;
>  	loff_t addr = ebnum * mtd->erasesize;
>  
> +	prandom_bytes_state(&rnd_state, writebuf, use_len_max * pgcnt);
>  	for (i = 0; i < pgcnt; ++i, addr += mtd->writesize) {
> -		prandom_bytes_state(&rnd_state, writebuf, use_len);
>  		ops.mode      = MTD_OPS_AUTO_OOB;
>  		ops.len       = 0;
>  		ops.retlen    = 0;
> @@ -139,7 +139,8 @@ static int verify_eraseblock(int ebnum)
>  			errcnt += 1;
>  			return err ? err : -1;
>  		}
> -		if (memcmp(readbuf, writebuf, use_len)) {
> +		if (memcmp(readbuf, writebuf + use_len_max * i + use_offset,

Likewise, and beyond.

> +			   use_len)) {
>  			pr_err("error: verify failed at %#llx\n",
>  			       (long long)addr);
>  			errcnt += 1;
> @@ -166,7 +167,9 @@ static int verify_eraseblock(int ebnum)
>  				errcnt += 1;
>  				return err ? err : -1;
>  			}
> -			if (memcmp(readbuf + use_offset, writebuf, use_len)) {
> +			if (memcmp(readbuf + use_offset,
> +				   writebuf + use_len_max * i + use_offset,
> +				   use_len)) {
>  				pr_err("error: verify failed at %#llx\n",
>  						(long long)addr);
>  				errcnt += 1;
> @@ -566,8 +569,8 @@ static int __init mtd_oobtest_init(void)
>  		if (bbt[i] || bbt[i + 1])
>  			continue;
>  		addr = (i + 1) * mtd->erasesize - mtd->writesize;
> +		prandom_bytes_state(&rnd_state, writebuf, sz * cnt);
>  		for (pg = 0; pg < cnt; ++pg) {
> -			prandom_bytes_state(&rnd_state, writebuf, sz);
>  			ops.mode      = MTD_OPS_AUTO_OOB;
>  			ops.len       = 0;
>  			ops.retlen    = 0;
> @@ -575,7 +578,7 @@ static int __init mtd_oobtest_init(void)
>  			ops.oobretlen = 0;
>  			ops.ooboffs   = 0;
>  			ops.datbuf    = NULL;
> -			ops.oobbuf    = writebuf;
> +			ops.oobbuf    = writebuf + pg * sz;
>  			err = mtd_write_oob(mtd, addr, &ops);
>  			if (err)
>  				goto out;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog