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

[Boris Brezillon <bbrezillon@xxxxxxxxxx>]

On Mon, 21 Jan 2019 12:28:17 +0000
"Bean Huo (beanhuo)" <beanhuo@xxxxxxxxxx> wrote:

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

Can you point me to docs/papers describing this effect? It's still
something I do not understand so I'd like to have more background on
what this effect is and what makes it more likely to happen on modern
SLC NANDs.
 
> 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.

Again, the impact will vary greatly depending on how efficient the
controller is WRT to data transfer on the bus, so saying the impact is
negligible is a lie. I do agree on the "reliability is more important
than perf" aspect though.

> This will simplify maintenance and minimize the risk of mistakes with parts requiring
> the patch not getting it.

That's also true.

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

Why only odd pages? You did not reply to this question yet, and I
really want to understand 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.

I do agree with that, except you're doing exactly the opposite: assuming
UBI is used and corrupting the first 2 pages is enough. That's just
wrong, and it's a typical example of UBI behavior leaking into the lower
layers of the MTD stack. To make it clear, incomplete erase can happen,
and wear-leveling/FS layers should be able to deal with that without
expecting the NAND driver to corrupt the first 2 pages of the block.

> 
> //Bean


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