Re: [EXT] [PATCH v2 3/3] mtd: rawnand: micron: Address the shallow erase issue

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Wed, 6 May 2020 08:28:43 +0000
"Bean Huo (beanhuo)" <beanhuo@xxxxxxxxxx> wrote:

> Hi, Miquel
> I have two questions about your patch, please help me. 
> 
> > +	 */
> > +	for (eb = first_eb; eb < first_eb + nb_eb; eb++) {
> > +		/* Il all the first pages are not written yet, do it */
> > +		if (micron->writtenp[eb] != MICRON_PAGE_MASK_TRIGGER)
> > +			micron_nand_avoid_shallow_erase(chip, eb);
> > +
> > +		micron->writtenp[eb] = 0;
> > +	}  
> 
> 
> Here, if the power loss happens before erasing this block, for the next time boot up,
> What will happen from FS layer in case FS detect this filled data?

Most likely ECC errors will be returned, but that doesn't matter since
this block was about to be erased. You have pretty much the same
problem for partially erase blocks already, and that should be handled
by the wear-leveling/FS, if not, that would be bug (note that it's
properly handled by UBI, which just considers the block as invalid and
schedules an erase).

> 
> > +
> > +	return nand_erase_nand(chip, instr, allowbbt); 
> >+ }  
> 
>  static int
> > +micron_nand_write_oob(struct nand_chip *chip, loff_t to,
> > +				 struct mtd_oob_ops *ops)
> > +{
> > +	struct micron_nand *micron = nand_get_manufacturer_data(chip);
> > +	unsigned int eb_sz = nanddev_eraseblock_size(&chip->base);
> > +	unsigned int p_sz = nanddev_page_size(&chip->base);
> > +	unsigned int ppeb = nanddev_pages_per_eraseblock(&chip->base);
> > +	unsigned int nb_p_tot = ops->len / p_sz;
> > +	unsigned int first_eb = DIV_ROUND_DOWN_ULL(to, eb_sz);
> > +	unsigned int first_p = DIV_ROUND_UP_ULL(to - (first_eb * eb_sz), p_sz);
> > +	unsigned int nb_eb = DIV_ROUND_UP_ULL(first_p + nb_p_tot, ppeb);
> > +	unsigned int remaining_p, eb, nb_p;
> > +	int ret;
> > +
> > +	ret = nand_write_oob_nand(chip, to, ops);
> > +	if (ret || (ops->len != ops->retlen))
> > +		return ret;
> > +
> > +	/* Mark the last pages of the first erase block to write */
> > +	nb_p = min(nb_p_tot, ppeb - first_p);
> > +	micron->writtenp[first_eb] |= GENMASK(first_p + nb_p, first_p) &
> > +				      MICRON_PAGE_MASK_TRIGGER;
> > +	remaining_p = nb_p_tot - nb_p;
> > +
> > +	/* Mark all the pages of all "in-the-middle" erase blocks */
> > +	for (eb = first_eb + 1; eb < first_eb + nb_eb - 1; eb++) {
> > +		micron->writtenp[eb] |= MICRON_PAGE_MASK_TRIGGER;
> > +		remaining_p -= ppeb;
> > +	}
> > +
> > +	/* Mark the first pages of the last erase block to write */
> > +	if (remaining_p)
> > +		micron->writtenp[eb] |= GENMASK(remaining_p - 1, 0) &
> > +					MICRON_PAGE_MASK_TRIGGER;
> > +  
> 
> 
> This micron->written is stored in the system memory, once power cut,  for the next time
> Boot up, will it be reinstated or it will be 0x00?

Yep, and that shouldn't be a problem, it just means we might have
unneeded page writes if the pages were already written, but, other than
the perf penalty it incurs, it should work fine.

We can optimize that a bit by adding a ->post_read_page() hook
so we can flag already read pages as written/erased and avoid those
unneeded writes in some situations.

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