* [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 ^ 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: miquel.raynal, richard, vigneshr, s.hauer, masonccyang, christophe.kerello, stefan, piotrs, devik, tglx, linux-mtd, linux-kernel 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); ^ 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, richard, vigneshr, s.hauer, masonccyang, christophe.kerello, stefan, piotrs, devik, tglx, linux-mtd, linux-kernel 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? ^ 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: Boris Brezillon, richard, vigneshr, s.hauer, masonccyang, christophe.kerello, stefan, piotrs, devik, tglx, linux-mtd, linux-kernel 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. ^ 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: Boris Brezillon, richard, vigneshr, s.hauer, masonccyang, christophe.kerello, stefan, piotrs, devik, tglx, linux-mtd, linux-kernel 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. ^ 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, richard, vigneshr, s.hauer, masonccyang, christophe.kerello, stefan, piotrs, devik, tglx, linux-mtd, linux-kernel 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. ^ 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: Miquel Raynal, richard, vigneshr, s.hauer, masonccyang, christophe.kerello, stefan, piotrs, devik, tglx, linux-mtd, linux-kernel 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. ^ 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).