Hello, Richard, maybe you'll have an idea to fix the situation here?
Álvaro Fernández Rojas <nolt...@gmail.com> wrote on Tue, 12 May 2020 10:36:25 +0200: > Hi, > > > El 11 may 2020, a las 18:29, Miquel Raynal <miquel.ray...@bootlin.com> > > escribió: > > > > Hello, > > > > Boris Brezillon <boris.brezil...@collabora.com> wrote on Mon, 4 May > > 2020 12:32:37 +0200: > > > >> On Mon, 4 May 2020 11:42:53 +0200 > >> Álvaro Fernández Rojas <nolt...@gmail.com> wrote: > >> > >>> Some NAND controllers change the ECC bytes when OOB is written with ECC > >>> enabled. > >>> This is a problem in brcmnand, since adding JFFS2 cleanmarkers after the > >>> page > >>> has been erased will change the ECC bytes to 0 and the controller will > >>> think > >>> the block is bad. > >>> It can be fixed by using write_oob_raw, which ensures ECC is disabled. > >>> > >>> Signed-off-by: Álvaro Fernández Rojas <nolt...@gmail.com> > >>> --- > >>> drivers/mtd/nand/raw/nand_base.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/mtd/nand/raw/nand_base.c > >>> b/drivers/mtd/nand/raw/nand_base.c > >>> index c24e5e2ba130..755d25200520 100644 > >>> --- a/drivers/mtd/nand/raw/nand_base.c > >>> +++ b/drivers/mtd/nand/raw/nand_base.c > >>> @@ -488,7 +488,7 @@ static int nand_do_write_oob(struct nand_chip *chip, > >>> loff_t to, > >>> > >>> nand_fill_oob(chip, ops->oobbuf, ops->ooblen, ops); > >>> > >>> - if (ops->mode == MTD_OPS_RAW) > >>> + if (ops->mode == MTD_OPS_AUTO_OOB || ops->mode == MTD_OPS_RAW) > >>> status = chip->ecc.write_oob_raw(chip, page & chip->pagemask); > >>> > >> > >> The doc says: > >> > >> @MTD_OPS_PLACE_OOB: OOB data are placed at the given offset (default) > >> @MTD_OPS_AUTO_OOB: OOB data are automatically placed at the free areas > >> which are defined by the internal ecclayout > >> @MTD_OPS_RAW: data are transferred as-is, with no error > >> correction; this mode implies %MTD_OPS_PLACE_OOB > >> > >> To me, that means MTD_OPS_PLACE_OOB and MTD_OPS_AUTO_OOB do not imply > >> MTD_OPS_RAW. Anyway those modes are just too vague. We really should > >> separate the ECC-disabled/ECC-enabled concept (AKA raw vs non-raw mode) > >> from the OOB placement scheme. IIRC, Miquel had a patchset doing that. > >> > >> We also should have the concept of protected OOB-region vs > >> unprotected-OOB-region if we want JFFS2 to work with controllers that > >> protect part of the OOB region. Once we have that we can patch JFFS2 > >> to write things with "ECC-disabled"+"auto-OOB-placement-on-unprotected > >> area". > > > > I see the problem but as Boris said the fix is not valid as-is. > > Problem is: I don't have a better proposal yet. > > > > Is forcing JFFS2 to write cleanmarkers in raw mode only an option? > > The doc says that for MTD_OPS_AUTO_OOB "the data is automatically placed at > the free areas which are defined by the internal ecclayout”. > So, if we’re placing this data in the free OOB area left by the ECC bytes it > means that this automatically placed data won’t be error correctable, since > it’s in the OOB, and the OOB data isn’t error corrected, right? No, free bytes sometimes are and sometimes are not covered by the ECC engine. It depends on the controller. > The problem is that "flash_erase -j” uses MTD_OPS_AUTO_OOB to write the OOB > JFFS2 clean markers and if this is written with ECC enabled the NAND > controller will change the ECC bytes to an invalid value (or at least > brcmnand controller). > > Another option could be adding another mode, something like > MTD_OPS_AUTO_OOB_RAW and using it in mtd-utils and JFFS2. No, these modes already are completely wrong, I must resend my series fixing them.
