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

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

Hi, Boris

>> +
>> +	 /* Only for legacy planar 2D parallels NAND. */
>> +	if ((mtd->type != MTD_NANDFLASH) || (chip->bits_per_cell != 1))
>> +		return 0;
>
>Are you implying that all your SLC chips are impacted by this issue, really?
>

In principle, all SLC NAND devices can show a sensitivity of the erase operation 
to block filling prior to erase. On certain devices it is almost undetectable. 
Nonetheless, given the expected improvement in robustness and the negligible impact
of the patch on performance, we recommend it to be applied to all SLC NAND devices. 
This will simplify maintenance and minimize the risk of mistakes with parts requiring
the patch not getting it.

>> +
>> +	data_buf =  kmalloc(mtd->writesize, GFP_KERNEL);
>> +	if (!data_buf)
>> +		return -ENOMEM;
>> +
>> +	memset(data_buf, 0xFF, mtd->writesize);
>> +	/*
>> +	 * pgs[] contains pages need to be checked. We firstly check
>> +	 * 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.
>
>So now you say the minimum amount of written pages is 14 not 15? In your
>previous patch, the magic number was 15 (and the commit log says 15 too).
>
it is still 15 pages, page0~page14. But it is now odd pages, there will one less odd pages than
even pages.

>> 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.
>
>Looks overly complex for only a small gain. Did you try writing X (X being 14 in this
>case) pages all the time? If you did, how does it compare to this version (perf-
>wise). I suspect that reading pages before potentially overwriting them will
>actually take more time than blindly overwriting 14 pages with 0x00.
>
>> We olny check odd page.
>
>Why?!
>
>> +	 */
>> +
>> +	char pgs[] = {13, 0, 11, 9, 7, 5, 3};
>> +	int  c = ARRAY_SIZE(pgs) - 1;
>> +	int i;
>> +
>> +	for (i = 0 ; i <=  c; i++) {
>> +		ret = nand_read_page_raw(chip, data_buf, 1, page + pgs[i]);
>> +		if (ret)
>> +			continue; /* Read error, we just skip it now. */
>> +		if (check_page_if_emtpy(chip, data_buf) == true) {
>> +			if (pgs[i] == 0)
>> +			/* page0 is not programmed. */
>> +				goto free_buf;
>> +
>> +			/* Mark page need to program later */
>> +			empty_page_mask |= (1 << pgs[i]);
>> +		} else {
>> +			if (pgs[i] == LAST_CHECKPU_PAGE)
>> +			/*
>> +			 * If page13 is programmed, this block has
>> +			 * met the requirement of robust erase.
>> +			 */
>> +				goto free_buf;
>> +		}
>> +	}
>> +	/* Corrupt page0 and page1, in order to simulate an
>> +	 * uncompleted eraseing scenario. Just for case of
>> +	 * power loss hits while below programming. in this
>> +	 * way, the PEB will be re-erased again.
>
>Looks like the "erase the first 2 pages" thing is here to corrupt UBI's VID and EC
>headers. Is this really needed? We should definitely not assume the NAND user is
>UBI, and that's exactly what you're doing here.
>
We must deal with all sorts of power loss, if other NAND based file systems
Have the same behavior with UBIFS to deal with un-completed erasing.
This is not just for UBIFS.

//Bean

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