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

[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 --]