Re: [RESEND PATCH V2 2/2] mtd: core: NAND filling block

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Bean,

"Bean Huo (beanhuo)" <beanhuo@xxxxxxxxxx> wrote on Fri, 18 Jan 2019
22:12:04 +0000:

> On some legacy planar 2D Micron NAND devices when a
> block erase command is issued, occasionally even
> though a block erase operation successfully completes
> and returns a pass status, the flash block may not be
> completely erased. Subsequent operations to this block
> on very rare cases can result in subtle failures or
> corruption. These extremely rare cases should nevertheless
> be considered.
> 
> These rare occurrences have been observed on partially
> written blocks. Partially written blocks are not uncommon
> with UBI/UBIFS.
> 
> To avoid this rare occurrence, we make sure that at least
> 15 pages have been programmed to a block before it is erased.
> In case we find that less than 15 pages have been programmed,
> additional pages are programmed in the block. The observation
> is that additional pages rarely need to be written

I would stop the commit message here and remove the end of the sentence
which, I believe, is inaccurate.

> and most of
> the time UBI/UBIFS erases blocks that contain more programmed
> pages.
> 
> Signed-off-by: Bean Huo <beanhuo@xxxxxxxxxx>
> Reviewed-by: ZOLTAN SZUBBOCSEV <zszubbocsev@xxxxxxxxxx>

Could you write this name in usual upper/lower case like "Zoltan
Szubbocsev"?

> ---
>  drivers/mtd/nand/raw/nand_micron.c | 119 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index b85e1c1..f52e072 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -541,8 +541,127 @@ static void micron_fixup_onfi_param_page(struct nand_chip *chip,
>  		p->revision = cpu_to_le16(ONFI_VERSION_1_0);
>  }
>  
> +static int check_page_if_emtpy(struct nand_chip *chip, char *data)

s/if/is
s/emtpy/empty/
s/char *data/void *data/

I would rename this function "nand_check_erased_page" to follow the
current naming and move it to nand_base.c right after
nand_check_erased_ecc_chunk(). Please also add kernel doc following the
other functions pattern.

> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	int ret, i;
> +	void *databuf, *eccbuf;
> +	int max_bitflips;
> +	struct mtd_oob_region oobregion;
> +
> +	mtd_ooblayout_ecc(mtd, 0, &oobregion);
> +	eccbuf = chip->oob_poi + oobregion.offset;
> +	databuf = data;
> +	max_bitflips = 0;
> +
> +	for (i = 0; i < chip->ecc.steps; i++) {
> +		ret = nand_check_erased_ecc_chunk(data,
> +						  chip->ecc.size,
> +						  eccbuf,
> +						  chip->ecc.bytes,
> +						  NULL, 0,
> +						  chip->ecc.strength);
> +		if (ret >= 0)
> +			max_bitflips = max(ret, max_bitflips);
> +		else
> +			return false;

		if (ret < 0)
			return false

		max_bitflips = max(ret, max_bitflips);

> +
> +		databuf += chip->ecc.size;
> +		eccbuf += chip->ecc.bytes;
> +
> +	}
> +	/*
> +	 * As for the empty/erased page, bitflips number should be zero or
> +	 * at least less than the bitflip_threshold.
> +	 */
> +	if (max_bitflips > mtd->bitflip_threshold)
> +		return false;
> +	else
> +		return true;

Why no just:

	return max_bitflips < mtd->bitflip_threshold;

Mind that currently, the check for returning -EUCLEAN to the upper
layer (meaning, there are too much bitflips) is "max_bitflips >=
mtd->bitflip_threshold", hence the use of '<' in my proposal.

> +}
> +
> +#define LAST_CHECKPU_PAGE 13

I would place this at the top of the micron_nand.c file

> +
> +static int before_erase_checkup(struct nand_chip *chip, int page)

Can you prefix this function with "micron_nand_" and use the same name
as what you will choose for the callback entry in nand_chip?

> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	u8 *data_buf;

"u8 *data" is enough

> +	int ret;
> +	uint32_t empty_page_mask = 0;

use u32 instead of uint32_t
> +
> +	 /* Only for legacy planar 2D parallels NAND. */

Extra space before /*, please run checkpatch.pl --strict and fix all
the warnings/checks that it reports.

> +	if ((mtd->type != MTD_NANDFLASH) || (chip->bits_per_cell != 1))
> +		return 0;
> +
> +	data_buf =  kmalloc(mtd->writesize, GFP_KERNEL);

Extra space before kmalloc.

You allocate just enough for storing in-band data (mtd->writesize) and
then you pass data_buf to the function checking for the page being
empty, which will then check behind the end of the allocated area. You
want to check if the page is empty, so you must allocate the oobsize
too.

However, I agree with Boris, why not just writing the pages directly?

> +	if (!data_buf)
> +		return -ENOMEM;
> +
> +	memset(data_buf, 0xFF, mtd->writesize);

Is this really needed?

> +	/*
> +	 * pgs[] contains pages need to be checked. We firstly check
                         ^the  ^that                   ^first
> +	 * page13,because, for the most of cases, PEB being erased is
> +	 * not partially programmed. If page13 contains data, we directly
> +	 * return since it is not a partially programmed PEB. Otherwise,
> +	 * then we check page0. And if page0 is programmed, and page13
> +	 * is not programmed, then we start to check from page11, page9,
> +	 * page7, page5, page3 respectively since the pages of PEB are
> +	 * programmed sequentially. We olny check odd page.
> +	 */
> +

Is this a hard rule that pages are programmed sequentially?

Why only odd pages?

[...]

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/