Re: [RESEND PATCH v3 3/5] mtd: nand: use nand_check_erased_ecc_chunk in default ECC read functions

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Brian,

On Mon, 21 Sep 2015 16:45:09 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> + others
> 
> On Thu, Sep 03, 2015 at 06:03:40PM +0200, Boris Brezillon wrote:
> > The default NAND read functions are relying on an underlying controller
> > to correct bitflips, but some of those controller cannot properly fix
> > bitflips in erased pages.
> > In case of ECC failures, check if the page of subpage is empty before
> > reporting an ECC failure.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> I'd really want some test results for this before opting everyone in. If
> I remember your last response correctly, you're just testing sunxi-nand,
> which uses a different implementation, right?

Right, which means I didn't test those changes on a real platform. I
could test it on atmel an board though.

> 
> Potential different strategy: if we can get one or two drivers to test
> this, we could flip it around into an opt-in flag (this would
> modify/eliminate patch 3). I know this has downsides for
> less-actively-developed drivers, which may never get fixed up to support
> erased-page ECC checks, but then, it also likely has less benefits for
> those cases too.
> 
> Thoughts? I could be convinced another way if I we can get reasonable
> backing from others who can test this.

I don't have any strong opinion, as long as automatic 'bitflips in
erased pages' detection can be switched off in case NAND controller
drivers don't need it. 

> 
> One more comment below.
> 
> > ---
> >  drivers/mtd/nand/nand_base.c | 50 +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 43 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index a2687ea..9a109a5 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -1419,6 +1419,15 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
> >  
> >  		stat = chip->ecc.correct(mtd, p,
> >  			&chip->buffers->ecccode[i], &chip->buffers->ecccalc[i]);
> > +		if (stat == -EBADMSG) {
> > +			/* check for empty pages with bitflips */
> > +			stat = nand_check_erased_ecc_chunk(p, chip->ecc.size,
> > +						&chip->buffers->ecccode[i],
> > +						chip->ecc.bytes,
> > +						NULL, 0,
> > +						chip->ecc.strength);
> > +		}
> > +
> >  		if (stat < 0) {
> >  			mtd->ecc_stats.failed++;
> >  		} else {
> > @@ -1468,6 +1477,14 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
> >  		int stat;
> >  
> >  		stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
> > +		if (stat == -EBADMSG) {
> > +			/* check for empty pages with bitflips */
> > +			stat = nand_check_erased_ecc_chunk(p, eccsize,
> > +						&ecc_code[i], eccbytes,
> > +						NULL, 0,
> > +						chip->ecc.strength);
> > +		}
> > +
> >  		if (stat < 0) {
> >  			mtd->ecc_stats.failed++;
> >  		} else {
> > @@ -1520,6 +1537,14 @@ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd,
> >  		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> >  
> >  		stat = chip->ecc.correct(mtd, p, &ecc_code[i], NULL);
> > +		if (stat == -EBADMSG) {
> > +			/* check for empty pages with bitflips */
> > +			stat = nand_check_erased_ecc_chunk(p, eccsize,
> > +						&ecc_code[i], eccbytes,
> > +						NULL, 0,
> > +						chip->ecc.strength);
> > +		}
> > +
> >  		if (stat < 0) {
> >  			mtd->ecc_stats.failed++;
> >  		} else {
> > @@ -1547,6 +1572,8 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
> >  	int i, eccsize = chip->ecc.size;
> >  	int eccbytes = chip->ecc.bytes;
> >  	int eccsteps = chip->ecc.steps;
> > +	int eccstepsize = eccsize + eccbytes + chip->ecc.prepad +
> > +			  chip->ecc.postpad;
> 
> Hmm, is this correct? I think you shouldn't be adding in eccsize, if
> you're looking for just the length of the ECC OOB region. But I could be
> wrong.

Nope, should be 

	int eccstepsize = eccbytes + chip->ecc.prepad + chip->ecc.postpad;

I'll fix that.

Thanks,

Boris




-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com