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 <[email protected]>
---
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
On Mon, 4 May 2020 11:42:53 +0200
Álvaro Fernández Rojas <[email protected]> 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 <[email protected]>
> ---
> 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);
Hello,
Boris Brezillon <[email protected]> wrote on Mon, 4 May
2020 12:32:37 +0200:
> On Mon, 4 May 2020 11:42:53 +0200
> Álvaro Fernández Rojas <[email protected]> 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 <[email protected]>
> > ---
> > 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?
Hi,
> El 11 may 2020, a las 18:29, Miquel Raynal <[email protected]> escribió:
>
> Hello,
>
> Boris Brezillon <[email protected]> wrote on Mon, 4 May
> 2020 12:32:37 +0200:
>
>> On Mon, 4 May 2020 11:42:53 +0200
>> Álvaro Fernández Rojas <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
Hello,
Richard, maybe you'll have an idea to fix the situation here?
Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020
10:36:25 +0200:
> Hi,
>
> > El 11 may 2020, a las 18:29, Miquel Raynal <[email protected]> escribió:
> >
> > Hello,
> >
> > Boris Brezillon <[email protected]> wrote on Mon, 4 May
> > 2020 12:32:37 +0200:
> >
> >> On Mon, 4 May 2020 11:42:53 +0200
> >> Álvaro Fernández Rojas <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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.
On Tue, 12 May 2020 10:44:22 +0200
Miquel Raynal <[email protected]> wrote:
> Hello,
>
> Richard, maybe you'll have an idea to fix the situation here?
>
> Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020
> 10:36:25 +0200:
>
> > Hi,
> >
> > > El 11 may 2020, a las 18:29, Miquel Raynal <[email protected]> escribió:
> > >
> > > Hello,
> > >
> > > Boris Brezillon <[email protected]> wrote on Mon, 4 May
> > > 2020 12:32:37 +0200:
> > >
> > >> On Mon, 4 May 2020 11:42:53 +0200
> > >> Álvaro Fernández Rojas <[email protected]> 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 <[email protected]>
> > >>> ---
> > >>> 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.
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 <[email protected]> escribió:
>
> On Tue, 12 May 2020 10:44:22 +0200
> Miquel Raynal <[email protected]> wrote:
>
>> Hello,
>>
>> Richard, maybe you'll have an idea to fix the situation here?
>>
>> Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020
>> 10:36:25 +0200:
>>
>>> Hi,
>>>
>>>> El 11 may 2020, a las 18:29, Miquel Raynal <[email protected]> escribió:
>>>>
>>>> Hello,
>>>>
>>>> Boris Brezillon <[email protected]> wrote on Mon, 4 May
>>>> 2020 12:32:37 +0200:
>>>>
>>>>> On Mon, 4 May 2020 11:42:53 +0200
>>>>> Álvaro Fernández Rojas <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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.