Re: [RESEND PATCH v3 5/5] mtd: nand: remove custom 'erased check' implementation

Re: [RESEND PATCH v3 5/5] mtd: nand: remove custom 'erased check' implementation

[Brian Norris]

+ others

On Thu, Sep 03, 2015 at 06:03:42PM +0200, Boris Brezillon wrote:
> Some drivers are manually checking for 'erased pages' while correcting
> ECC bytes.
> This logic is now done by the core infrastructure, and can thus be removed
> from those drivers.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |  8 --------
>  drivers/mtd/nand/diskonchip.c   | 32 ++------------------------------
>  drivers/mtd/nand/jz4740_nand.c  | 18 ------------------
>  3 files changed, 2 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 816ef53..0d6adbe 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -316,14 +316,6 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd,
>  	unsigned num_errors, corrected;
>  	unsigned long timeo;
>  
> -	/* All bytes 0xff?  It's an erased page; ignore its ECC. */
> -	for (i = 0; i < 10; i++) {
> -		if (ecc_code[i] != 0xff)
> -			goto compare;
> -	}
> -	return 0;
> -
> -compare:
>  	/* Unpack ten bytes into eight 10 bit values.  We know we're
>  	 * little-endian, and use type punning for less shifting/masking.
>  	 */
> diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c
> index 7da266a..eb65769 100644
> --- a/drivers/mtd/nand/diskonchip.c
> +++ b/drivers/mtd/nand/diskonchip.c
> @@ -936,37 +936,9 @@ static int doc200x_correct_data(struct mtd_info *mtd, u_char *dat,
>  				calc_ecc[i] = ReadDOC_(docptr, DoC_Mplus_ECCSyndrome0 + i);
>  			else
>  				calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i);
> -			if (calc_ecc[i] != empty_read_syndrome[i])
> -				emptymatch = 0;
>  		}
> -		/* If emptymatch=1, the read syndrome is consistent with an
> -		   all-0xff data and stored ecc block.  Check the stored ecc. */
> -		if (emptymatch) {
> -			for (i = 0; i < 6; i++) {
> -				if (read_ecc[i] == 0xff)
> -					continue;
> -				emptymatch = 0;
> -				break;
> -			}
> -		}
> -		/* If emptymatch still =1, check the data block. */
> -		if (emptymatch) {
> -			/* Note: this somewhat expensive test should not be triggered
> -			   often.  It could be optimized away by examining the data in
> -			   the readbuf routine, and remembering the result. */
> -			for (i = 0; i < 512; i++) {
> -				if (dat[i] == 0xff)
> -					continue;
> -				emptymatch = 0;
> -				break;
> -			}
> -		}
> -		/* If emptymatch still =1, this is almost certainly a freshly-
> -		   erased block, in which case the ECC will not come out right.
> -		   We'll suppress the error and tell the caller everything's
> -		   OK.  Because it is. */
> -		if (!emptymatch)
> -			ret = doc_ecc_decode(rs_decoder, dat, calc_ecc);
> +
> +		ret = doc_ecc_decode(rs_decoder, dat, calc_ecc);
>  		if (ret > 0)
>  			printk(KERN_ERR "doc200x_correct_data corrected %d errors\n", ret);
>  	}

I see new warnings:

drivers/mtd/nand/diskonchip.c: At top level:
drivers/mtd/nand/diskonchip.c: In function ‘doc200x_correct_data’:
drivers/mtd/nand/diskonchip.c:915:6: warning: unused variable ‘emptymatch’ [-Wunused-variable]
  int emptymatch = 1;
      ^
drivers/mtd/nand/diskonchip.c:79:15: warning: ‘empty_read_syndrome’ defined but not used [-Wunused-variable]
 static u_char empty_read_syndrome[6] = { 0x26, 0xff, 0x6d, 0x47, 0x73, 0x7a };
               ^

I'd also like test results for these drivers before doing this. And I'm
not sure how to find good testers for these drivers, even it users still
exist.

> diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c
> index ba44af3..4d73276 100644
> --- a/drivers/mtd/nand/jz4740_nand.c
> +++ b/drivers/mtd/nand/jz4740_nand.c
> @@ -224,24 +224,6 @@ static int jz_nand_correct_ecc_rs(struct mtd_info *mtd, uint8_t *dat,
>  	uint32_t t;
>  	unsigned int timeout = 1000;
>  
> -	t = read_ecc[0];
> -
> -	if (t == 0xff) {
> -		for (i = 1; i < 9; ++i)
> -			t &= read_ecc[i];
> -
> -		t &= dat[0];
> -		t &= dat[nand->chip.ecc.size / 2];
> -		t &= dat[nand->chip.ecc.size - 1];
> -
> -		if (t == 0xff) {
> -			for (i = 1; i < nand->chip.ecc.size - 1; ++i)
> -				t &= dat[i];
> -			if (t == 0xff)
> -				return 0;
> -		}
> -	}
> -
>  	for (i = 0; i < 9; ++i)
>  		writeb(read_ecc[i], nand->base + JZ_REG_NAND_PAR0 + i);
>  
> -- 
> 1.9.1
> 

Re: [RESEND PATCH v3 5/5] mtd: nand: remove custom 'erased check' implementation

[Boris Brezillon]

Hi Brian,

On Mon, 21 Sep 2015 16:28:32 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> + others
> 
> On Thu, Sep 03, 2015 at 06:03:42PM +0200, Boris Brezillon wrote:
> > Some drivers are manually checking for 'erased pages' while correcting
> > ECC bytes.
> > This logic is now done by the core infrastructure, and can thus be removed
> > from those drivers.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/nand/davinci_nand.c |  8 --------
> >  drivers/mtd/nand/diskonchip.c   | 32 ++------------------------------
> >  drivers/mtd/nand/jz4740_nand.c  | 18 ------------------
> >  3 files changed, 2 insertions(+), 56 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> > index 816ef53..0d6adbe 100644
> > --- a/drivers/mtd/nand/davinci_nand.c
> > +++ b/drivers/mtd/nand/davinci_nand.c
> > @@ -316,14 +316,6 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd,
> >  	unsigned num_errors, corrected;
> >  	unsigned long timeo;
> >  
> > -	/* All bytes 0xff?  It's an erased page; ignore its ECC. */
> > -	for (i = 0; i < 10; i++) {
> > -		if (ecc_code[i] != 0xff)
> > -			goto compare;
> > -	}
> > -	return 0;
> > -
> > -compare:
> >  	/* Unpack ten bytes into eight 10 bit values.  We know we're
> >  	 * little-endian, and use type punning for less shifting/masking.
> >  	 */
> > diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c
> > index 7da266a..eb65769 100644
> > --- a/drivers/mtd/nand/diskonchip.c
> > +++ b/drivers/mtd/nand/diskonchip.c
> > @@ -936,37 +936,9 @@ static int doc200x_correct_data(struct mtd_info *mtd, u_char *dat,
> >  				calc_ecc[i] = ReadDOC_(docptr, DoC_Mplus_ECCSyndrome0 + i);
> >  			else
> >  				calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i);
> > -			if (calc_ecc[i] != empty_read_syndrome[i])
> > -				emptymatch = 0;
> >  		}
> > -		/* If emptymatch=1, the read syndrome is consistent with an
> > -		   all-0xff data and stored ecc block.  Check the stored ecc. */
> > -		if (emptymatch) {
> > -			for (i = 0; i < 6; i++) {
> > -				if (read_ecc[i] == 0xff)
> > -					continue;
> > -				emptymatch = 0;
> > -				break;
> > -			}
> > -		}
> > -		/* If emptymatch still =1, check the data block. */
> > -		if (emptymatch) {
> > -			/* Note: this somewhat expensive test should not be triggered
> > -			   often.  It could be optimized away by examining the data in
> > -			   the readbuf routine, and remembering the result. */
> > -			for (i = 0; i < 512; i++) {
> > -				if (dat[i] == 0xff)
> > -					continue;
> > -				emptymatch = 0;
> > -				break;
> > -			}
> > -		}
> > -		/* If emptymatch still =1, this is almost certainly a freshly-
> > -		   erased block, in which case the ECC will not come out right.
> > -		   We'll suppress the error and tell the caller everything's
> > -		   OK.  Because it is. */
> > -		if (!emptymatch)
> > -			ret = doc_ecc_decode(rs_decoder, dat, calc_ecc);
> > +
> > +		ret = doc_ecc_decode(rs_decoder, dat, calc_ecc);
> >  		if (ret > 0)
> >  			printk(KERN_ERR "doc200x_correct_data corrected %d errors\n", ret);
> >  	}
> 
> I see new warnings:
> 
> drivers/mtd/nand/diskonchip.c: At top level:
> drivers/mtd/nand/diskonchip.c: In function ‘doc200x_correct_data’:
> drivers/mtd/nand/diskonchip.c:915:6: warning: unused variable ‘emptymatch’ [-Wunused-variable]
>   int emptymatch = 1;
>       ^
> drivers/mtd/nand/diskonchip.c:79:15: warning: ‘empty_read_syndrome’ defined but not used [-Wunused-variable]
>  static u_char empty_read_syndrome[6] = { 0x26, 0xff, 0x6d, 0x47, 0x73, 0x7a };
>                ^

Oops, I'll fix those warnings.

> 
> I'd also like test results for these drivers before doing this. And I'm
> not sure how to find good testers for these drivers, even it users still
> exist.

Yep, the goal of this patch was to let people owning boards embedding
those chips test the changes.
If we don't get anyone to test it, maybe we should leave those drivers
unchanged at the expense of doing twice the test in case of bitflips in
erased pages or uncorrectable bitflips (which is really unlikely).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com