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

[Pavel Machek <pavel@ucw.cz>]

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