Re: [V2, 1/2] mtd: brcmnand: Add check for erased page bitflips

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

On Wed,  8 Jun 2016 17:01:20 -0400
Kamal Dasu <kdasu.kdev@gmail.com> wrote:

> Check for erased page bitflips in a page. And if well within
> threshold return data as all 0xff. Apply sw check for controller
> version < 7.2. Controller vesion >= 7.2 has hw support.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
> V2 changes 
>  Added use of nand_check_erased_ecc_chunk
>  Restrict change to older controller < 7.2
> ---
>  drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index b76ad7c..212acb4 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -1545,6 +1545,55 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
>  	return ret;
>  }
>  
> +/*
> + * Check a page to see if it is erased (w/ bitflips) after an uncorrectable ECC
> + * error
> + *
> + * Because the HW ECC signals an ECC error if an erase paged has even a single
> + * bitflip, we must check each ECC error to see if it is actually an erased
> + * page with bitflips, not a truly corrupted page.
> + *
> + * On a real error, return a negative error code (-EBADMSG for ECC error), and
> + * buf will contain raw data.
> + * Otherwise, buf gets filled with 0xffs and return the maximum number of
> + * bitflips-per-ECC-sector to the caller.
> + *
> + */
> +static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
> +		  struct nand_chip *chip, void *buf, u64 addr)
> +{
> +	int i, sas;
> +	void *oob = chip->oob_poi;
> +	int bitflips = 0;
> +	int page = addr >> chip->page_shift;
> +	int ret;
> +
> +	if (!buf) {
> +		buf = chip->buffers->databuf;
> +		/* Invalidate page cache */
> +		chip->pagebuf = -1;

Yes, I know it's ugly.

I'm seriously considering droping the page caching feature, not only
because it complicates usage of the databuf buffer, but also because in
some cases you might want to really read the same page several times
without the cache in the way (for example when you want to check the
read-disturb effect by reading over and over a single NAND page).

Actually, I still don't see the usefulness of it, since upper layers
(mainly FS layers) are already relying the VFS cache...

Brian, is there any good reason to keep it?

> +	}
> +
> +	sas = mtd->oobsize / chip->ecc.steps;
> +
> +	/* read without ecc for verification */
> +	chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
> +	ret = chip->ecc.read_page_raw(mtd, chip, buf, true, page);

Ok, so you have to read the whole page again :(. Are you sure the
controller does not leave the data untouched when it sees uncorrectable
errors? This way you would only have to read the OOB bytes for the
faulty sectors and not the whole page.

The rest looks good to me.

> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
> +		bitflips = nand_check_erased_ecc_chunk(buf, chip->ecc.size,
> +						       oob, sas, NULL, 0,
> +						       chip->ecc.strength);
> +		/* Too many bitflips */
> +		if (bitflips < 0)
> +			break;
> +	}
> +
> +	return bitflips;
> +}
> +
>  static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
>  			 u64 addr, unsigned int trans, u32 *buf, u8 *oob)
>  {
> @@ -1575,6 +1624,18 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
>  	}
>  
>  	if (mtd_is_eccerr(err)) {
> +		/*
> +		 * Controller version 7.2 has hw encoder to detect erased page
> +		 * bitflips, apply sw verification for older controllers only
> +		 */
> +		if (ctrl->nand_version < 0x0702) {
> +			err = brcmstb_nand_verify_erased_page(mtd, chip, buf,
> +							      addr);
> +			/* erased page bitflips corrected */
> +			if (err > 0)
> +				return err;
> +		}
> +
>  		dev_dbg(ctrl->dev, "uncorrectable error at 0x%llx\n",
>  			(unsigned long long)err_addr);
>  		mtd->ecc_stats.failed++;



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