Re: [PATCH] mtd: fsl_ifc_nand: Recover corrupted empty page for preventing read-only mount in UBIFS

[Scott Wood <scottwood@freescale.com>]

On Tue, 2014-04-01 at 01:49 +0000, Eunbong Song wrote:
> Even if the meaning of EUCLEAN was changed by commit edbc4540.
> There is still possibility of read-only mount in UBIFS with ubifs_scan() "corrupt empty space at LEB".
> So i made this patch for fix that problem.

Please elaborate on the nature of the problem.

> This patch do as follow.
>  - If there are ecc errors which is equal to or less than chip->ecc.strength in page.
>  - Check that page has how many zero bits, and if zero bits are equal to or less than
>    chip->ecc.strength then overwrite 1 to zero bits in buf.

This is difficult to parse, with no mention in this sentence that you're
talking about corrupted empty pages.

> ubifs_scan() cannot detect corrupted empty space because buf is recovered by this patch.
> And this is safe because ecc controller can correct up to chip->ecc.strength bits.

So the concern is that is_blank is failing to report a page that has not
been written to but has errors that would have been correctable if the
page had been written?

Do most drivers handle this?

> Signed-off-by: Eunbong Song <eunb.song@samsung.com>
> ---
>  drivers/mtd/nand/fsl_ifc_nand.c |   41 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 41 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index 90ca7e7..2129c39 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -277,6 +277,42 @@ static int is_blank(struct mtd_info *mtd, unsigned int bufnum)
>  	return 1;
>  }
>  
> +static int num_zero_bits(uint8_t val)
> +{
> +	int i, ret=0;
> +
> +	for(i=7; i>=0 ; i--)
> +		if(!(0x1 & (val >> i)))
> +			ret++;

Whitespace (here and elsewhere)

Also, use hweight8(~val) instead of reimplementing it.  Or better, use
hweight64() and process the data in larger chunks.

> +	return ret;
> +}
> +
> +static int is_corrupted_blank(struct mtd_info *mtd, uint8_t * buf)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	int i;
> +	int zero_bits = 0;
> +
> +	for (i = 0; i < mtd->writesize ; i++) {
> +		if(buf[i] != 0xff) {
> +			zero_bits += num_zero_bits(buf[i]);	
> +		}
> +	}
> +
> +	if(zero_bits && (zero_bits <= chip->ecc.strength)){
> +		return 1;
> +	}
> +
> +	return 0;
> +}

What if it's a page that legitimately has only a handful of zero bits?
You need to count zero bits in the ECC as well.

Also, this could be combined with is_blank().

> +static void recover_corrupted_blank(struct mtd_info *mtd, uint8_t * buf)
> +{
> +	memset(buf, 0xff, mtd->writesize);
> +	return;
> +}
> +
>  /* returns nonzero if entire page is blank */
>  static int check_read_ecc(struct mtd_info *mtd, struct fsl_ifc_ctrl *ctrl,
>  			  u32 *eccstat, unsigned int bufnum)
> @@ -760,6 +796,11 @@ static int fsl_ifc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC)
>  		mtd->ecc_stats.failed++;
>  
> +	if(nctrl->max_bitflips && (nctrl->max_bitflips <= chip->ecc.strength)){
> +		if(is_corrupted_blank(mtd, buf))
> +			recover_corrupted_blank(mtd, buf);
> +	}

If the page is blank except for errors, most likely max_bitflips will be
zero because fsl_ifc_run_command() already considered it an
uncorrectable error and set ECCER instead.  Moving corrupted blank page
detection into is_blank() wouldn't have this problem.

How did you test this patch?

-Scott