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@xxxxxxxxxxxxxxxxxxx> 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