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

["Bean Huo (beanhuo)" <beanhuo@xxxxxxxxxx>]

Hi, Miquel

>
>> 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.
>
I don't understand where 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"?
>

Sorry for this typo, fixed in next version.

.....
>>
>> +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.
>
Thanks, will add in next version.

>> +{
>> +	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.
>


Ok, will add.

>> +}
>> +
>> +#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?
>

Will fix.

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