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

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Steve,

Steve deRosier <derosier@xxxxxxxxx> wrote on Sun, 3 May 2020 09:10:15
-0700:

> On Sun, May 3, 2020 at 4:41 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> >
> > With recent SLC NANDs, Micron admits that a "shallow erase" issue may
> > be observable. It is actually the chip itself not doing a correct
> > erase operation because of its internal machinery stating that the
> > pages have not been programmed. Micron told us that there is a way to
> > workaround this issue: ensure that all the odd pages in the 16 first
> > ones of each block to erase have been fully written.
> >
> > To avoid a very big performance drawback by re-writting all the pages
> > for each erase operation, the fix proposed here overloads the ->erase
> > and ->write_oob hooks to count the pages actually written at runtime
> > and avoid re-writting them if not needed.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > ---
> >  drivers/mtd/nand/raw/nand_micron.c | 121 +++++++++++++++++++++++++++++
> >  1 file changed, 121 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> > index 56654030ec7f..a9afd1b9a9e8 100644
> > --- a/drivers/mtd/nand/raw/nand_micron.c
> > +++ b/drivers/mtd/nand/raw/nand_micron.c
> > @@ -36,6 +36,15 @@
> >  #define NAND_ECC_STATUS_1_3_CORRECTED  BIT(4)
> >  #define NAND_ECC_STATUS_7_8_CORRECTED  (BIT(4) | BIT(3))
> >
> > +/*
> > + * Micron SLC chips are subject to a shallow erase issue: if the first
> > + * pages of a block have not enough bytes programmed, the internal
> > + * machinery might declare the block empty and skip the actual erase
> > + * operation. This is the number of pages we check by software.
> > + */
> > +#define MICRON_SHALLOW_ERASE_MIN_PAGE 16
> > +#define MICRON_PAGE_MASK_TRIGGER GENMASK(MICRON_SHALLOW_ERASE_MIN_PAGE, 0)
> > +
> >  struct nand_onfi_vendor_micron {
> >         u8 two_plane_read;
> >         u8 read_cache;
> > @@ -64,6 +73,7 @@ struct micron_on_die_ecc {
> >
> >  struct micron_nand {
> >         struct micron_on_die_ecc ecc;
> > +       u16 *writtenp;
> >  };
> >
> >  static int micron_nand_setup_read_retry(struct nand_chip *chip, int retry_mode)
> > @@ -429,6 +439,106 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
> >         return MICRON_ON_DIE_SUPPORTED;
> >  }
> >
> > +static int micron_nand_avoid_shallow_erase(struct nand_chip *chip,
> > +                                          unsigned int eb)
> > +{
> > +       struct micron_nand *micron = nand_get_manufacturer_data(chip);
> > +       unsigned int page = eb * nanddev_pages_per_eraseblock(&chip->base);
> > +       u8 *databuf = nand_get_data_buf(chip);
> > +       int ret, i;
> > +
> > +       memset(databuf, 0x00, nanddev_page_size(&chip->base));
> > +
> > +       /* Micron advises to only write the first 8 odd pages, counting from 1 */
> > +       for (i = 0; i < MICRON_SHALLOW_ERASE_MIN_PAGE; i += 2, page += 2) {
> > +               if (!(micron->writtenp[eb] & BIT(i))) {
> > +                       ret = nand_write_page_raw(chip, databuf, false, page);
> > +                       if (ret)
> > +                               return ret;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int micron_nand_erase(struct nand_chip *chip, struct erase_info *instr,
> > +                       int allowbbt)
> > +{
> > +       struct micron_nand *micron = nand_get_manufacturer_data(chip);
> > +       unsigned int eb_sz = nanddev_eraseblock_size(&chip->base);
> > +       unsigned int first_eb = DIV_ROUND_DOWN_ULL(instr->addr, eb_sz);
> > +       unsigned int nb_eb = DIV_ROUND_UP_ULL(instr->len, eb_sz);
> > +       unsigned int eb;
> > +
> > +       if (!micron)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * Check that enough pages have been written in each block.
> > +        * If not, write them before actually erasing.
> > +        */
> > +       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;
> > +       }
> > +
> > +       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;
> > +
> > +       return 0;
> > +}
> > +
> > +static bool micron_nand_with_shallow_erase_issue(struct nand_chip *chip)
> > +{
> > +       /*
> > +        * The shallow erase issue has been observed with MT29F*G*A
> > +        * parts but Micron suspects that the issue can happen with
> > +        * almost all recent SLC but at such a low probability that it
> > +        * is almost invisible. Nevertheless, as we mitigate the
> > +        * performance penalty at runtime by following the number of
> > +        * written pages in a block before erasing it, we may want to
> > +        * enable this fix by default.
> > +        */
> > +       return nand_is_slc(chip);
> > +}  
> 
> 
> Whoa, let's hold our horses here!  "almost all recent" would imply
> that older SLCs aren't affected. And the likelyhood that Micron will
> fix newer parts is high - because why  would they leave in a major bug
> like that in the next mask? So, what you're saying is when someone
> goes to upgrade their older device's Linux they're going to take a
> major filesystem performance hit without knowing it (because
> realistically who reads 10,000s of patches before upgrading) when
> their chip doesn't need it. Because we're too lazy to get the list
> from Micron and code that ugliness?

Well, that's where I strongly disagree. I know about this for
almost three years now. It took us this time to figure out:
1- what is impacted
2- why
3- what could work-around it

As you can see, we failed in #1 and trust me, we tried. By e-mail,
IRC, IRL. We tried hard. Bean and Zoltan in copy know about the
issue and they tried to minimize and hide it as much as they could,
lying shamelessly to us several times. Please do not call this
laziness.

> We put this in and the resulting discussions for embedded systems
> designers for the next decade are going to be one of two things:
> * Oh, you want to use that SLC NAND from Micron? Well then don't use
> Linux because it performs crappy on Micron SLC NANDs.
> OR
> * Oh, you want to use Linux? Well, don't use a Micron SLC NAND then
> because they perform crappy on Linux.
> 
> Let's get a list of all chip that have this bug (and let's be clear -
> it's a bug, not a "quirk") and enable it for those chips specifically.
> Even better if there was something in the chipinfo itself that made it
> obvious which ones had the problem (because realistically it's
> probably specific to a particular geometry). In any case, it's in the
> best interest of Micron to identify to us exactly which chips have or
> are likely to have this issue and for us to be specific on which get
> assigned this quirk. It is probably listed in an errata app-note, and
> if not it should be.
> 
> Strong NAK to defaulting all Micron SLC NANDs to this - unless it
> truly is the case that _all_ Micron SLC NANDs in the past and in the
> future likely have this problem.

I am open to alternatives.

Thanks,
Miquèl

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