linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode
@ 2020-05-04  9:42 Álvaro Fernández Rojas
  2020-05-04 10:32 ` Boris Brezillon
  0 siblings, 1 reply; 7+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-04  9:42 UTC (permalink / raw)
  To: miquel.raynal, richard, vigneshr, s.hauer, masonccyang,
	boris.brezillon, christophe.kerello, stefan, piotrs, devik, tglx,
	linux-mtd, linux-kernel
  Cc: Álvaro Fernández Rojas

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@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);
 	else
 		status = chip->ecc.write_oob(chip, page & chip->pagemask);
-- 
2.26.2


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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode
  2020-05-04  9:42 [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode Álvaro Fernández Rojas
@ 2020-05-04 10:32 ` Boris Brezillon
  2020-05-11 16:29   ` Miquel Raynal
  0 siblings, 1 reply; 7+ messages in thread
From: Boris Brezillon @ 2020-05-04 10:32 UTC (permalink / raw)
  To: Álvaro Fernández Rojas
  Cc: christophe.kerello, vigneshr, richard, s.hauer, devik,
	linux-kernel, stefan, linux-mtd, miquel.raynal, tglx,
	masonccyang, piotrs

On Mon,  4 May 2020 11:42:53 +0200
Álvaro Fernández Rojas <noltari@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 <noltari@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".

>  	else
>  		status = chip->ecc.write_oob(chip, page & chip->pagemask);


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode
  2020-05-04 10:32 ` Boris Brezillon
@ 2020-05-11 16:29   ` Miquel Raynal
  2020-05-12  8:36     ` Álvaro Fernández Rojas
  0 siblings, 1 reply; 7+ messages in thread
From: Miquel Raynal @ 2020-05-11 16:29 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Álvaro Fernández Rojas, christophe.kerello, vigneshr,
	richard, s.hauer, devik, linux-kernel, stefan, linux-mtd, tglx,
	masonccyang, piotrs

Hello,

Boris Brezillon <boris.brezillon@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 <noltari@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 <noltari@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?

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode
  2020-05-11 16:29   ` Miquel Raynal
@ 2020-05-12  8:36     ` Álvaro Fernández Rojas
  2020-05-12  8:44       ` Miquel Raynal
  0 siblings, 1 reply; 7+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12  8:36 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: christophe.kerello, vigneshr, richard, s.hauer, devik,
	linux-kernel, stefan, Boris Brezillon, linux-mtd, tglx,
	masonccyang, piotrs

Hi,

> El 11 may 2020, a las 18:29, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> 
> Hello,
> 
> Boris Brezillon <boris.brezillon@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 <noltari@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 <noltari@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?

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.

Best regards,
Álvaro.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode
  2020-05-12  8:36     ` Álvaro Fernández Rojas
@ 2020-05-12  8:44       ` Miquel Raynal
  2020-05-12  8:54         ` Boris Brezillon
  0 siblings, 1 reply; 7+ messages in thread
From: Miquel Raynal @ 2020-05-12  8:44 UTC (permalink / raw)
  To: Álvaro Fernández Rojas
  Cc: christophe.kerello, vigneshr, richard, s.hauer, devik,
	linux-kernel, stefan, Boris Brezillon, linux-mtd, tglx,
	masonccyang, piotrs

Hello,

Richard, maybe you'll have an idea to fix the situation here?

Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
10:36:25 +0200:

> Hi,
> 
> > El 11 may 2020, a las 18:29, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> > 
> > Hello,
> > 
> > Boris Brezillon <boris.brezillon@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 <noltari@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 <noltari@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.


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode
  2020-05-12  8:44       ` Miquel Raynal
@ 2020-05-12  8:54         ` Boris Brezillon
  2020-06-05 17:09           ` Álvaro Fernández Rojas
  0 siblings, 1 reply; 7+ messages in thread
From: Boris Brezillon @ 2020-05-12  8:54 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Álvaro Fernández Rojas, christophe.kerello, vigneshr,
	richard, s.hauer, devik, linux-kernel, stefan, linux-mtd, tglx,
	masonccyang, piotrs

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

> Hello,
> 
> Richard, maybe you'll have an idea to fix the situation here?
> 
> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> 10:36:25 +0200:
> 
> > Hi,
> >   
> > > El 11 may 2020, a las 18:29, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> > > 
> > > Hello,
> > > 
> > > Boris Brezillon <boris.brezillon@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 <noltari@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 <noltari@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.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode
  2020-05-12  8:54         ` Boris Brezillon
@ 2020-06-05 17:09           ` Álvaro Fernández Rojas
  0 siblings, 0 replies; 7+ messages in thread
From: Álvaro Fernández Rojas @ 2020-06-05 17:09 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: christophe.kerello, vigneshr, richard, s.hauer, devik,
	linux-kernel, stefan, linux-mtd, Miquel Raynal, tglx,
	masonccyang, piotrs

I’ve now sent another patch “mtd: rawnand: brcmnand: force raw OOB writes” as an alternative solution to this issue.

Best regards,
Álvaro.

> El 12 may 2020, a las 10:54, Boris Brezillon <boris.brezillon@collabora.com> escribió:
> 
> On Tue, 12 May 2020 10:44:22 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
>> Hello,
>> 
>> Richard, maybe you'll have an idea to fix the situation here?
>> 
>> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
>> 10:36:25 +0200:
>> 
>>> Hi,
>>> 
>>>> El 11 may 2020, a las 18:29, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
>>>> 
>>>> Hello,
>>>> 
>>>> Boris Brezillon <boris.brezillon@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 <noltari@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 <noltari@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.


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-06-05 17:09 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-04  9:42 [PATCH] nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode Álvaro Fernández Rojas
2020-05-04 10:32 ` Boris Brezillon
2020-05-11 16:29   ` Miquel Raynal
2020-05-12  8:36     ` Álvaro Fernández Rojas
2020-05-12  8:44       ` Miquel Raynal
2020-05-12  8:54         ` Boris Brezillon
2020-06-05 17:09           ` Álvaro Fernández Rojas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).