Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages

Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages

[Darwin Dingel]

Hi,

We are also having the same problem where the IFC (nand flash) was 
reporting ECC uncorrectable errors on single bitflips with erased pages. 
Applying this patch with some minor modifications seems to solve our 
issue. We are still doing more testing but recent results looks promising.

Our kernel is 4.4.6 so we have to modify it a bit to fit the old ECC 
layout structure. We just have a few comments about the patch:

 > -				if (!is_blank(mtd, bufnum))
 > -					ctrl->nand_stat |=
 > -						IFC_NAND_EVTER_STAT_ECCER;
 > -				break;
 > +				ctrl->nand_stat |= IFC_NAND_EVTER_STAT_ECCER;

Added 'error = 0' after setting the flag since no error was actually 
corrected.

 > -	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER)
 > -		dev_err(priv->dev, "NAND Flash ECC Uncorrectable Error\n");
 > -
 >  	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC)
 >  		mtd->ecc_stats.failed++;
 > +
 > +	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER) {
 > +		int res;
 > +
 > +		if (!oob_required)
 > +			fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
 > +
 > +		res = check_erased_page(chip, buf);
 > +		return res;
 > +	}

We have to do the check IFC_NAND_EVTER_STAT_ECCER first because the 
condition (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC) will never be 
true since IFC always sets IFC_NAND_EVTER_STAT_ECCER on empty pages. 
Incrementing failed stats first before doing check_erased_page() makes 
nand_read() report ECC error all time.

Our exact modification was:
	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER) {
		if (!oob_required)
			fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);

		return check_erased_page(chip, buf);
	}

	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC)
		mtd->ecc_stats.failed++;

Because check_erased_page() will be updating the failed stat anyway.


Cheers,
Darwin

Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages

[Boris Brezillon]

Hi,

On Thu, 25 May 2017 23:33:43 +0000
Darwin Dingel <Darwin.Dingel@alliedtelesis.co.nz> wrote:

> Hi,
> 
> We are also having the same problem where the IFC (nand flash) was 
> reporting ECC uncorrectable errors on single bitflips with erased pages. 
> Applying this patch with some minor modifications seems to solve our 
> issue. We are still doing more testing but recent results looks promising.

Pavel, can you send a v2 fixing these problems?

Thanks,

Boris

> 
> Our kernel is 4.4.6 so we have to modify it a bit to fit the old ECC 
> layout structure. We just have a few comments about the patch:
> 
>  > -				if (!is_blank(mtd, bufnum))
>  > -					ctrl->nand_stat |=
>  > -						IFC_NAND_EVTER_STAT_ECCER;
>  > -				break;
>  > +				ctrl->nand_stat |= IFC_NAND_EVTER_STAT_ECCER;  
> 
> Added 'error = 0' after setting the flag since no error was actually 
> corrected.
> 
>  > -	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER)
>  > -		dev_err(priv->dev, "NAND Flash ECC Uncorrectable Error\n");
>  > -
>  >  	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC)
>  >  		mtd->ecc_stats.failed++;
>  > +
>  > +	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER) {
>  > +		int res;
>  > +
>  > +		if (!oob_required)
>  > +			fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>  > +
>  > +		res = check_erased_page(chip, buf);
>  > +		return res;
>  > +	}  
> 
> We have to do the check IFC_NAND_EVTER_STAT_ECCER first because the 
> condition (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC) will never be 
> true since IFC always sets IFC_NAND_EVTER_STAT_ECCER on empty pages. 
> Incrementing failed stats first before doing check_erased_page() makes 
> nand_read() report ECC error all time.
> 
> Our exact modification was:
> 	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER) {
> 		if (!oob_required)
> 			fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
> 
> 		return check_erased_page(chip, buf);
> 	}
> 
> 	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC)
> 		mtd->ecc_stats.failed++;
> 
> Because check_erased_page() will be updating the failed stat anyway.
> 
> 
> Cheers,
> Darwin

Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Mon 2017-05-29 10:48:38, Boris Brezillon wrote:
> Hi,
> 
> On Thu, 25 May 2017 23:33:43 +0000
> Darwin Dingel <Darwin.Dingel@alliedtelesis.co.nz> wrote:
> 
> > Hi,
> > 
> > We are also having the same problem where the IFC (nand flash) was 
> > reporting ECC uncorrectable errors on single bitflips with erased pages. 
> > Applying this patch with some minor modifications seems to solve our 
> > issue. We are still doing more testing but recent results looks promising.
> 
> Pavel, can you send a v2 fixing these problems?

You should have v2 in your inbox by now.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]

Hi!

> We are also having the same problem where the IFC (nand flash) was 
> reporting ECC uncorrectable errors on single bitflips with erased pages. 
> Applying this patch with some minor modifications seems to solve our 
> issue. We are still doing more testing but recent results looks
> promising.

First, thanks for letting me know.

> Our kernel is 4.4.6 so we have to modify it a bit to fit the old ECC 
> layout structure. We just have a few comments about the patch:
> 
>  > -				if (!is_blank(mtd, bufnum))
>  > -					ctrl->nand_stat |=
>  > -						IFC_NAND_EVTER_STAT_ECCER;
>  > -				break;
>  > +				ctrl->nand_stat |= IFC_NAND_EVTER_STAT_ECCER;
> 
> Added 'error = 0' after setting the flag since no error was actually 
> corrected.

You meen "errors = 0"? Does that actually make a difference? It is a
local variable, and continue makes sure the value is not used:

                for (i = sector; i <= sector_end; i++) {
                        errors = check_read_ecc(mtd, ctrl, eccstat, i);

                        if (errors == 15) {
				/* 
                                 * Uncorrectable error. 
                                 * We'll check for blank pages later. 
                                 * 
                                 * We disable ECCER reporting due to... 
                                 * erratum IFC-A002770 -- so report it now if we 
                                 * see an uncorrectable error in ECCSTAT. 
                                 */
                                ctrl->nand_stat |= IFC_NAND_EVTER_STAT_ECCER;
                                continue;
                        }


>  > -	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER)
>  > -		dev_err(priv->dev, "NAND Flash ECC Uncorrectable Error\n");
>  > -
>  >  	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC)
>  >  		mtd->ecc_stats.failed++;
>  > +
>  > +	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER) {
>  > +		int res;
>  > +
>  > +		if (!oob_required)
>  > +			fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>  > +
>  > +		res = check_erased_page(chip, buf);
>  > +		return res;
>  > +	}
> 
> We have to do the check IFC_NAND_EVTER_STAT_ECCER first because the 
> condition (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC) will never be 
> true since IFC always sets IFC_NAND_EVTER_STAT_ECCER on empty pages. 
> Incrementing failed stats first before doing check_erased_page() makes 
> nand_read() report ECC error all time.

Yes, you are right; I overlooked that one. Thanks a lot!

> Our exact modification was:
> 	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER) {
> 		if (!oob_required)
> 			fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
> 
> 		return check_erased_page(chip, buf);
> 	}
> 
> 	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC)
> 		mtd->ecc_stats.failed++;
> 
> Because check_erased_page() will be updating the failed stat anyway.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages

[Darwin Dingel]

Hi,

On 01/06/17 08:59, Pavel Machek wrote:

> 
> You meen "errors = 0"? Does that actually make a difference? It is a
> local variable, and continue makes sure the value is not used:
> 

I missed the 'continue' statement there. In that case we don't need to 
reset 'error'.




Cheers,
Darwin

Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

Hi!

> > You meen "errors = 0"? Does that actually make a difference? It is a
> > local variable, and continue makes sure the value is not used:
> > 
> 
> I missed the 'continue' statement there. In that case we don't need to 
> reset 'error'.

Yes, thanks, I wanted to double-check. If you could take a look at
PATCHv2 -- it should have the problems fixed. Your Acked-by would be
nice ;-).

Thanks a lot,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages

[Darwin Dingel]

Hi Pavel,

Looks like uboot has similar IFC driver. Do you have plans to apply this change to uboot as well?


Cheers,
Darwin
​

From: Pavel Machek <pavel@ucw.cz>
Sent: Thursday, 1 June 2017 10:15 a.m.
To: Darwin Dingel
Cc: Boris Brezillon; richard@nod.at; dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com; cyrille.pitchen@atmel.com; linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org; mark.marshall@omicronenergy.com; b44839@freescale.com;  prabhakar@freescale.com
Subject: Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages
    
Hi!

> > You meen "errors = 0"? Does that actually make a difference? It is a
> > local variable, and continue makes sure the value is not used:
> > 
> 
> I missed the 'continue' statement there. In that case we don't need to 
> reset 'error'.

Yes, thanks, I wanted to double-check. If you could take a look at
PATCHv2 -- it should have the problems fixed. Your Acked-by would be
nice ;-).

Thanks a lot,
                                                                        Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)  http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
    

Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

Hi!

> 
> Looks like uboot has similar IFC driver. Do you have plans to apply this change to uboot as well?
> 

First, sorry for the delay. You are right that u-boot needs similar
changes, but we do not hit the problem in u-boot so it was not
priority.

Thanks for handling it.

									Pavel
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]