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

[Steve deRosier <derosier@xxxxxxxxx>]

Boris and Miquel,

On Sun, May 3, 2020 at 9:36 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> 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.
>


I apologize.  And to clarify - I am not calling you (nor Boris, nor
the MTD community) lazy. As a long-time member of this list, I know
that's far from the case. Maybe more in this case Micron deserves to
be called lazy now that I understand the history. Though that I know
is an oversimplification too. In any case, perhaps _I_ was lazy with
my phrasing and by not looking up the history here.

Again, I apologize.

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

>From your description then, there's enough denial in Micron within
Micron that my statement does apply - that at least all future Micron
parts will have this problem. Simple logic -> if engineering won't
admit there is a problem, they're not going to get funding to fix the
problem. And it's not just going to go away in the next part, because
we all know that new part designs usually start by borrowing the old
part design and since you can't admit there's an issue it's likely the
problem will come along in the next part.

> I am open to alternatives.
>

This may sound extreme, but if the point here is to force the
manufacturer to give us the information that is required to support
their devices, I'd even suggest we pull the support and either drop
the driver entirely, move it to staging, or some other extremely
obvious move that says, "we are unable to properly support this
without the manufacturers help and the manufacturer doesn't care about
it's customers." Personally, when I upgrade my kernel, I want the
kernel to SHOUT to me that there's a problem with this driver rather
than have a performance issue that I might spend the next six months
trying to figure out why.

When I start a new project, one of the first things I do is check to
see if the parts that we want to use are supported by Linux. I'd like
it when I check if a Micron SLC NAND part is supported that it is
clear there's a big issue with that part. It showing up in staging
instead in the main tree would be a clear signal.

A message to Micron and other chip vendors:  Do not attempt to hide
your bugs, we will find out and hiding this stuff simply pisses us
off. Bugs happen, we get it, expect it, and can work around or fix
them. Responsible companies do not dismiss their users and will
quickly and actively investigate reported problems and then disclose
via errata. This used to be common, even as little as 10 or 20 years
ago - there was rarely a chip that I'd use that didn't have a few
errata documents associated with it. Now, good luck getting them (even
if you can get a datasheet).

We in the embedded Linux community want to work with the vendors to
get good solid upstream support. And vendors, seriously, why the hell
are you resisting it? You've got a large number of people not on your
payroll that are invested in spending time in making you succeed
because it makes things better for them too. And note the use of the
word "upstream" - I no longer use vendor drivers. They're generally of
horribly low quality and either make me stuck on a particular 8 year
old kernel or require me to rewrite them for current versions anyway.

Tomorrow I'm calling my CMs and will start the process to remove
Micron as an approved vendor and will start qualifying alternatives.
While AFAIK (and I intend to find out for sure), we don't have a
problem with the parts we've been using, I don't want to work with a
company that is technically dishonest like Micron.

To Miquel - I appreciate your work in finding and working around the
issue.  And I appreciate the pragmatic approach to fixing it.  But
perhaps it's time to say enough is enough.

I withdraw my NAK (for what it's worth), but can we find something to
very clearly mark "you may be using a broken device, but we don't know
because Micro won't tell us and so we've conservitavely setup a
workaround for all devices that affects performance"?

I'll suggest several alternative things, from most forceful to least:
1. Drop the driver.
2. Move the driver to staging.
3. Make it a kconfig option that is on by default, with strong wording
along the way of "if you know you're using a device without this
problem, you can turn this off".
4. Shove a printk of warning level in there so it's clear in the logs
for those of use who look at the logs when we upgrade.
5. Keep it on by default, and add a white-list of known devices that
are OK to not use it. Add a big comment of why (which is there) and
instructions of how to test if your device has the problem and that if
you test and know it's clean how to send a patch to add to the
whitelist.

At a minimum, I'd like to see a combo of #3 and #4.

Thanks,
- Steve

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