[PATCH] nand: brcmnand: fix OOB R/W with Hamming ECC

[PATCH] nand: brcmnand: fix OOB R/W with Hamming ECC

[Álvaro Fernández Rojas]

Hamming ECC doesn't cover the OOB data, so reading or writing OOB shall
always be done without ECC enabled.
This is a problem when adding JFFS2 cleanmarkers to erased blocks. If JFFS2
clenmarkers are added to the OOB with ECC enabled, OOB bytes will be changed
from ff ff ff to 00 00 00, reporting incorrect ECC errors.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 659eaa6f0980..5ff4291380c5 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -2688,6 +2688,12 @@ static int brcmnand_attach_chip(struct nand_chip *chip)
 
 	ret = brcmstb_choose_ecc_layout(host);
 
+	/* If OOB is written with ECC enabled it will cause ECC errors */
+	if (is_hamming_ecc(host->ctrl, &host->hwcfg)) {
+		chip->ecc.write_oob = brcmnand_write_oob_raw;
+		chip->ecc.read_oob = brcmnand_read_oob_raw;
+	}
+
 	return ret;
 }
 
-- 
2.20.1

Re: [PATCH] nand: brcmnand: fix OOB R/W with Hamming ECC

[Florian Fainelli]

On 2/22/2021 12:16 PM, Álvaro Fernández Rojas wrote:
> Hamming ECC doesn't cover the OOB data, so reading or writing OOB shall
> always be done without ECC enabled.
> This is a problem when adding JFFS2 cleanmarkers to erased blocks. If JFFS2
> clenmarkers are added to the OOB with ECC enabled, OOB bytes will be changed
> from ff ff ff to 00 00 00, reporting incorrect ECC errors.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Should there be a Fixes: tag provided here for back porting to stable trees?
-- 
Florian

Re: [PATCH] nand: brcmnand: fix OOB R/W with Hamming ECC

[Álvaro Fernández Rojas]

Hi Florian,

> El 24 feb 2021, a las 4:46, Florian Fainelli <f.fainelli@gmail.com> escribió:
> 
> 
> 
> On 2/22/2021 12:16 PM, Álvaro Fernández Rojas wrote:
>> Hamming ECC doesn't cover the OOB data, so reading or writing OOB shall
>> always be done without ECC enabled.
>> This is a problem when adding JFFS2 cleanmarkers to erased blocks. If JFFS2
>> clenmarkers are added to the OOB with ECC enabled, OOB bytes will be changed
>> from ff ff ff to 00 00 00, reporting incorrect ECC errors.
>> 
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> 
> Should there be a Fixes: tag provided here for back porting to stable trees?

I think so, but the fixed commit would be the first one, right?
27c5b17cd1b10564fa36f8f51e4b4b41436ecc32

> -- 
> Florian

Best regards,
Álvaro.

Re: [PATCH] nand: brcmnand: fix OOB R/W with Hamming ECC

[Miquel Raynal]

Hi Álvaro,

Álvaro Fernández Rojas <noltari@gmail.com> wrote on Wed, 24 Feb 2021
08:16:58 +0100:

> Hi Florian,
> 
> > El 24 feb 2021, a las 4:46, Florian Fainelli <f.fainelli@gmail.com> escribió:
> > 
> > 
> > 
> > On 2/22/2021 12:16 PM, Álvaro Fernández Rojas wrote:
> >> Hamming ECC doesn't cover the OOB data, so reading or writing OOB shall
> >> always be done without ECC enabled.
> >> This is a problem when adding JFFS2 cleanmarkers to erased blocks. If JFFS2
> >> clenmarkers are added to the OOB with ECC enabled, OOB bytes will be changed
> >> from ff ff ff to 00 00 00, reporting incorrect ECC errors.
> >> 
> >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > 
> > Should there be a Fixes: tag provided here for back porting to stable trees?
> 
> I think so, but the fixed commit would be the first one, right?
> 27c5b17cd1b10564fa36f8f51e4b4b41436ecc32

Yep, shouldn't be a problem.

Thanks,
Miquèl

[PATCH v2] nand: brcmnand: fix OOB R/W with Hamming ECC

[Álvaro Fernández Rojas]

Hamming ECC doesn't cover the OOB data, so reading or writing OOB shall
always be done without ECC enabled.
This is a problem when adding JFFS2 cleanmarkers to erased blocks. If JFFS2
clenmarkers are added to the OOB with ECC enabled, OOB bytes will be changed
from ff ff ff to 00 00 00, reporting incorrect ECC errors.

Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v2: Add fixed tag.

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 659eaa6f0980..5ff4291380c5 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -2688,6 +2688,12 @@ static int brcmnand_attach_chip(struct nand_chip *chip)
 
 	ret = brcmstb_choose_ecc_layout(host);
 
+	/* If OOB is written with ECC enabled it will cause ECC errors */
+	if (is_hamming_ecc(host->ctrl, &host->hwcfg)) {
+		chip->ecc.write_oob = brcmnand_write_oob_raw;
+		chip->ecc.read_oob = brcmnand_read_oob_raw;
+	}
+
 	return ret;
 }
 
-- 
2.20.1

Re: [PATCH v2] nand: brcmnand: fix OOB R/W with Hamming ECC

[Brian Norris]

On Wed, Feb 24, 2021 at 12:02 AM Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")

FWIW, I could believe this was broken. We weren't testing Hamming ECC
(nor JFFS2) at the time, so it could easily have obvious bugs like
this.

And since I got this far, and I'm still in MAINTAINERS, I guess:

Acked-by: Brian Norris <computersforpeace@gmail.com>

Re: [PATCH v2] nand: brcmnand: fix OOB R/W with Hamming ECC

[Miquel Raynal]

Hi Álvaro,

Brian Norris <computersforpeace@gmail.com> wrote on Wed, 24 Feb 2021
13:01:13 -0800:

> On Wed, Feb 24, 2021 at 12:02 AM Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
> > Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")  
> 
> FWIW, I could believe this was broken. We weren't testing Hamming ECC
> (nor JFFS2) at the time, so it could easily have obvious bugs like
> this.

Right, you should probably limit the backport to the time when raw
accessors got introduced/fixed.

Thanks,
Miquèl

Re: [PATCH v2] nand: brcmnand: fix OOB R/W with Hamming ECC

[Álvaro Fernández Rojas]

Hi Miquel,

> El 25 feb 2021, a las 8:48, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> 
> Hi Álvaro,
> 
> Brian Norris <computersforpeace@gmail.com> wrote on Wed, 24 Feb 2021
> 13:01:13 -0800:
> 
>> On Wed, Feb 24, 2021 at 12:02 AM Álvaro Fernández Rojas
>> <noltari@gmail.com> wrote:
>>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")  
>> 
>> FWIW, I could believe this was broken. We weren't testing Hamming ECC
>> (nor JFFS2) at the time, so it could easily have obvious bugs like
>> this.
> 
> Right, you should probably limit the backport to the time when raw
> accessors got introduced/fixed.

What do you mean?
Those accessors have been there since the first commit (27c5b17cd1b10564fa36f8f51e4b4b41436ecc32):
https://github.com/torvalds/linux/blob/27c5b17cd1b10564fa36f8f51e4b4b41436ecc32/drivers/mtd/nand/brcmnand/brcmnand.c#L1896-L1899

> 
> Thanks,
> Miquèl

Best regards,
Álvaro.

Re: [PATCH v2] nand: brcmnand: fix OOB R/W with Hamming ECC

[Miquel Raynal]

Hi Álvaro,

Álvaro Fernández Rojas <noltari@gmail.com> wrote on Thu, 25 Feb 2021
08:54:09 +0100:

> Hi Miquel,
> 
> > El 25 feb 2021, a las 8:48, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> > 
> > Hi Álvaro,
> > 
> > Brian Norris <computersforpeace@gmail.com> wrote on Wed, 24 Feb 2021
> > 13:01:13 -0800:
> >   
> >> On Wed, Feb 24, 2021 at 12:02 AM Álvaro Fernández Rojas
> >> <noltari@gmail.com> wrote:  
> >>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")    
> >> 
> >> FWIW, I could believe this was broken. We weren't testing Hamming ECC
> >> (nor JFFS2) at the time, so it could easily have obvious bugs like
> >> this.  
> > 
> > Right, you should probably limit the backport to the time when raw
> > accessors got introduced/fixed.  
> 
> What do you mean?
> Those accessors have been there since the first commit (27c5b17cd1b10564fa36f8f51e4b4b41436ecc32):
> https://github.com/torvalds/linux/blob/27c5b17cd1b10564fa36f8f51e4b4b41436ecc32/drivers/mtd/nand/brcmnand/brcmnand.c#L1896-L1899

I misunderstood Brian's answer. This commit is not that old and looks
legit.

Re: [PATCH v2] nand: brcmnand: fix OOB R/W with Hamming ECC

[Miquel Raynal]

On Wed, 2021-02-24 at 08:02:10 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?= wrote:
> Hamming ECC doesn't cover the OOB data, so reading or writing OOB shall
> always be done without ECC enabled.
> This is a problem when adding JFFS2 cleanmarkers to erased blocks. If JFFS2
> clenmarkers are added to the OOB with ECC enabled, OOB bytes will be changed
> from ff ff ff to 00 00 00, reporting incorrect ECC errors.
> 
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Acked-by: Brian Norris <computersforpeace@gmail.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel