| Message ID | 20200504094253.2741109-1-noltari@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | nand: raw: use write_oob_raw for MTD_OPS_AUTO_OOB mode | expand |
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);
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?
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.
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.
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.
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.
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);
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(-)