Re: [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode

[Boris Brezillon]

On Tue, 12 May 2020 10:44:22 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Hello,
> 
> Richard, maybe you'll have an idea to fix the situation here?
> 
> Ãlvaro FernÃndez Rojas <noltari@xxxxxxxxx> wrote on Tue, 12 May 2020
> 10:36:25 +0200:
> 
> > Hi,
> >   
> > > El 11 may 2020, a las 18:29, Miquel Raynal <miquel.raynal@xxxxxxxxxxx> escribiÃ:
> > > 
> > > Hello,
> > > 
> > > Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Mon, 4 May
> > > 2020 12:32:37 +0200:
> > >     
> > >> On Mon,  4 May 2020 11:42:53 +0200
> > >> Ãlvaro FernÃndez Rojas <noltari@xxxxxxxxx> 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 <noltari@xxxxxxxxx>
> > >>> ---
> > >>> 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.