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

Bean Huo (beanhuo)
2019-01-24T15:47:50Z

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@micron.com>
>> Reviewed-by: ZOLTAN SZUBBOCSEV <zszubbocsev@micron.com>
>
>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