On Tue, 12 May 2020 10:44:22 +0200 Miquel Raynal <miquel.ray...@bootlin.com> wrote:
> 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. > Totally agree with Miquel on that one: let's fix the write/read/ecc-layout semantics instead of adding more obscure modes.
