Skip to contentSkip to conversation

nand: brcmnand: fix OOB R/W with Hamming ECC

PATCH Superseded Álvaro Fernández Rojas 4 participants 10 messages Feb 22, 2021 – Mar 2, 2021

Subsystems (3)
Memory technology devices (MTD) NAND flash Broadcom stb NAND flash driver
lore
Share & tools
lore

Álvaro Fernández Rojas

author
#0 lore raw

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

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 <[email protected]>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +6 -0
index 659eaa6f0980..5ff4291380c5 100644
@@ -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;
 }
Signature · 1 line
2.20.1
#1 lore raw
Quoted 7 lines · Á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 <[email protected]>

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

Signature · 5 lines
Florian

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
author
#2 lore raw

Hi Florian,

Quoted 11 lines · Florian Fainelli
Quoted 6 lines · Á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.

Quoted 3 lines
Quoted 1 line
Signed-off-by Álvaro Fernández Rojas <[email protected]>

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

Quoted 2 lines
Signature · 1 line
Florian

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

maintainer
#3 lore raw

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Wed, 24 Feb 2021 08:16:58 +0100:

Quoted 13 lines

Hi Florian,

Quoted 11 lines · Florian Fainelli
Quoted 6 lines · Á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.

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Álvaro Fernández Rojas <[email protected]>

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

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

author
#4 lore raw

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

In reply to Á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 <[email protected]>

---
v2: Add fixed tag.

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +6 -0
index 659eaa6f0980..5ff4291380c5 100644
@@ -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;
 }
Signature · 6 lines
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
maintainer Acked-by on v2 1/1
#5 lore raw
Quoted 1 line · Álvaro Fernández Rojas
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 <[email protected]>
maintainer
#6 lore raw

Hi Álvaro,

Brian Norris <[email protected]> wrote on Wed, 24 Feb 2021 13:01:13 -0800:

Quoted 7 lines
Quoted 1 line · Álvaro Fernández Rojas
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

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

author
#7 lore raw

Hi Miquel,

Quoted 9 lines · Miquel Raynal

Hi Álvaro,

Brian Norris <[email protected]> wrote on Wed, 24 Feb 2021 13:01:13 -0800:

Quoted 2 lines

On Wed, Feb 24, 2021 at 12:02 AM Álvaro Fernández Rojas <[email protected]> wrote:

Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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

Quoted 3 lines

Thanks,
Miquèl

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

maintainer
#8 lore raw

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Thu, 25 Feb 2021 08:54:09 +0100:

Quoted 17 lines

Hi Miquel,

Quoted 15 lines · Miquel Raynal

Hi Álvaro,

Brian Norris <[email protected]> wrote on Wed, 24 Feb 2021 13:01:13 -0800:

Quoted 7 lines
Quoted 1 line · Álvaro Fernández Rojas
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.

Quoted 6 lines
Quoted 2 lines

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.

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

maintainer Applied
#9 lore raw
In reply to Álvaro Fernández Rojas
Quoted 9 lines · On Wed, 2021-02-24 at 08:02:10 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_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 <[email protected]>
Acked-by Brian Norris <[email protected]>

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

Miquel