Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650972 > unrolled thread

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

Started byDarwin Dingel <Darwin.Dingel@alliedtelesis.co.nz>
First post2017-05-26 01:40 +0200
Last post2017-06-01 00:20 +0200
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

Contents

  Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages Darwin Dingel <Darwin.Dingel@alliedtelesis.co.nz> - 2017-05-26 01:40 +0200
    Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-05-29 10:50 +0200
      Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages Pavel Machek <pavel@ucw.cz> - 2017-06-01 00:20 +0200
    Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages Pavel Machek <pavel@ucw.cz> - 2017-05-31 22:50 +0200
      Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages Darwin Dingel <Darwin.Dingel@alliedtelesis.co.nz> - 2017-06-01 00:00 +0200
        Re: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages Pavel Machek <pavel@ucw.cz> - 2017-06-01 00:20 +0200

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

FromDarwin Dingel <Darwin.Dingel@alliedtelesis.co.nz>
Date2017-05-26 01:40 +0200
SubjectRe: mtd: nand: fsl_ifc: fix handing of bit flips in erased pages
Message-ID<tLawi-6pJ-17@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1652306

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-05-29 10:50 +0200
Message-ID<tMoxc-5JZ-15@gated-at.bofh.it>
In reply to#1650972
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

[toc] | [prev] | [next] | [standalone]

#1654673

FromPavel Machek <pavel@ucw.cz>
Date2017-06-01 00:20 +0200
Message-ID<tNk89-2vg-1@gated-at.bofh.it>
In reply to#1652306

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1654612

FromPavel Machek <pavel@ucw.cz>
Date2017-05-31 22:50 +0200
Message-ID<tNiJ3-1iM-3@gated-at.bofh.it>
In reply to#1650972

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1654664

FromDarwin Dingel <Darwin.Dingel@alliedtelesis.co.nz>
Date2017-06-01 00:00 +0200
Message-ID<tNjOO-28K-39@gated-at.bofh.it>
In reply to#1654612
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

[toc] | [prev] | [next] | [standalone]

#1654674

FromPavel Machek <pavel@ucw.cz>
Date2017-06-01 00:20 +0200
Message-ID<tNk89-2vg-3@gated-at.bofh.it>
In reply to#1654664

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web