Skip to contentSkip to conversation

nand: brcmnand: correctly verify erased pages

PATCH Superseded Álvaro Fernández Rojas 3 participants 12 messages May 4, 2020 – May 24, 2020

Subsystems (4)
Media (V4L/DVB) 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: correctly verify erased pages

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that the ECC aren't 0xff.

Signed-off-by Álvaro Fernández Rojas <[email protected]>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +18 -4
index e4e3ceeac38f..546f0807b887 100644
@@ -2018,6 +2018,7 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
+	struct mtd_oob_region oobecc;
 	int i, sas;
 	void *oob = chip->oob_poi;
 	int bitflips = 0;

@@ -2035,11 +2036,24 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 	if (ret)
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  NULL, 0, NULL, 0,
+						  chip->ecc.strength);
+		if (ret < 0)
+			return ret;
+
+		bitflips = max(bitflips, ret);
+	}
+
+	for (i = 0; mtd->ooblayout->ecc(mtd, i, &oobecc) != -ERANGE; i++)
+	{
+		ret = nand_check_erased_ecc_chunk(NULL, 0,
+						  oob + oobecc.offset,
+						  oobecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);
 		if (ret < 0)
 			return ret;
Signature · 1 line
2.26.2
#1 lore raw
Quoted 6 lines · Álvaro Fernández Rojas

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that the ECC aren't 0xff.

Signed-off-by Álvaro Fernández Rojas <[email protected]>

Can you provide a Fixes: tag for this change?

Quoted 46 lines
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +18 -4
index e4e3ceeac38f..546f0807b887 100644
@@ -2018,6 +2018,7 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
+	struct mtd_oob_region oobecc;
 	int i, sas;
 	void *oob = chip->oob_poi;
 	int bitflips = 0;

@@ -2035,11 +2036,24 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 	if (ret)
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  NULL, 0, NULL, 0,
+						  chip->ecc.strength);
+		if (ret < 0)
+			return ret;
+
+		bitflips = max(bitflips, ret);
+	}
+
+	for (i = 0; mtd->ooblayout->ecc(mtd, i, &oobecc) != -ERANGE; i++)
+	{
+		ret = nand_check_erased_ecc_chunk(NULL, 0,
+						  oob + oobecc.offset,
+						  oobecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);
 		if (ret < 0)
 			return ret;
Signature · 1 line
Florian
author
#2 lore raw

Hi Florian,

I added it on v2.

Best regards,
Álvaro.

Quoted 52 lines · Florian Fainelli
Quoted 6 lines · Álvaro Fernández Rojas

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that the ECC aren't 0xff.

Signed-off-by Álvaro Fernández Rojas <[email protected] <mailto:[email protected]>>

Can you provide a Fixes: tag for this change?

Quoted 38 lines

---
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 22 ++++++++++++++++++----

1 file changed, 18 insertions(+), 4 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +14 -4
index e4e3ceeac38f..546f0807b887 100644
@@ -2018,6 +2018,7 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
+	struct mtd_oob_region oobecc;
 	int i, sas;
 	void *oob = chip->oob_poi;
 	int bitflips = 0;

@@ -2035,11 +2036,24 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 	if (ret)
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  NULL, 0, NULL, 0,
+						  chip->ecc.strength);
+		if (ret < 0)
+			return ret;
+
+		bitflips = max(bitflips, ret);
+	}
+
+	for (i = 0; mtd->ooblayout->ecc(mtd, i, &oobecc) != -ERANGE; i++)
+	{
Quoted 11 lines
Quoted 8 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +4 -0
+		ret = nand_check_erased_ecc_chunk(NULL, 0,
+						  oob + oobecc.offset,
+						  oobecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);
 		if (ret < 0)
 			return ret;
Signature · 1 line
Florian

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#3 lore raw

PATCH v2 1/1 nand: brcmnand: correctly verify erased pages

In reply to Álvaro Fernández Rojas

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that the ECC aren't 0xff.

Fixes 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips")
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v2: Add Fixes tag

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +18 -4
index e4e3ceeac38f..546f0807b887 100644
@@ -2018,6 +2018,7 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
+	struct mtd_oob_region oobecc;
 	int i, sas;
 	void *oob = chip->oob_poi;
 	int bitflips = 0;

@@ -2035,11 +2036,24 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 	if (ret)
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  NULL, 0, NULL, 0,
+						  chip->ecc.strength);
+		if (ret < 0)
+			return ret;
+
+		bitflips = max(bitflips, ret);
+	}
+
+	for (i = 0; mtd->ooblayout->ecc(mtd, i, &oobecc) != -ERANGE; i++)
+	{
+		ret = nand_check_erased_ecc_chunk(NULL, 0,
+						  oob + oobecc.offset,
+						  oobecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);
 		if (ret < 0)
 			return ret;
Signature · 1 line
2.26.2
maintainer
#4 lore raw

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 5 May 2020 10:20:55 +0200:

Quoted 7 lines

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that the ECC aren't 0xff.

Fixes 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips")

This extra space between the Fixes tag and your SoB should be removed

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

---
v2: Add Fixes tag

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +11 -4
index e4e3ceeac38f..546f0807b887 100644
@@ -2018,6 +2018,7 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
+	struct mtd_oob_region oobecc;
 	int i, sas;
 	void *oob = chip->oob_poi;
 	int bitflips = 0;

@@ -2035,11 +2036,24 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 	if (ret)
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  NULL, 0, NULL, 0,
+						  chip->ecc.strength);
+		if (ret < 0)
+			return ret;
+
+		bitflips = max(bitflips, ret);
+	}

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2059

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +7 -0
+
+	for (i = 0; mtd->ooblayout->ecc(mtd, i, &oobecc) != -ERANGE; i++)
+	{
+		ret = nand_check_erased_ecc_chunk(NULL, 0,
+						  oob + oobecc.offset,
+						  oobecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);
 		if (ret < 0)
 			return

If I understand correctly, the cleanmarker is in the "available OOB area", which is somewhere in the OOB area between the bad block marker and the ECC bytes. I think that checking the data buffer and the ECC area only is enough and we can probably leave the remaining spare OOB area.

But instead of calling nand_check_erased_ecc_chunk twice, just call:

nand_check_erased_ecc_chunk(data, datalen, ecc, ecclen, NULL, 0,
strength);

And also please clarify your commit log: you are not "just checking the ECC bytes" but you are checking both the main area and the ECC bytes.

Thanks,
Miquèl

author
#5 lore raw

PATCH v3 1/1 mtd: rawnand: brcmnand: correctly verify erased pages

In reply to Álvaro Fernández Rojas

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that data and ECC bytes aren't 0xff.

Fixes 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips")
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: Fix commit log and merge nand_check_erased_ecc_chunk calls.
v2: Add Fixes tag

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +14 -5
index e4e3ceeac38f..80fe01f03516 100644
@@ -2018,8 +2018,9 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
+	struct mtd_oob_region oobecc;
 	int i, sas;
-	void *oob = chip->oob_poi;
+	void *oob;
 	int bitflips = 0;
 	int page = addr >> chip->page_shift;
 	int ret;

@@ -2035,11 +2036,19 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 	if (ret)
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		if (mtd->ooblayout->ecc(mtd, i, &oobecc)) {
+			oob = NULL;
+			oobecc.length = 0;
+		} else {
+			oob = chip->oob_poi + oobecc.offset;
+		}
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  oob, oobecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);
 		if (ret < 0)
 			return ret;
Signature · 1 line
2.26.2
maintainer
#6 lore raw

Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 08:51:11 +0200:

Quoted 31 lines

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that data and ECC bytes aren't 0xff.

Fixes 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips")
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: Fix commit log and merge nand_check_erased_ecc_chunk calls.
v2: Add Fixes tag

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -1
index e4e3ceeac38f..80fe01f03516 100644
@@ -2018,8 +2018,9 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
+	struct mtd_oob_region oobecc;
 	int i, sas;
-	void *oob = chip->oob_poi;
+	void *oob;
 	int bitflips = 0;
 	int page = addr >> chip->page_shift;
 	int ret;

@@ -2035,11 +2036,19 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 	if (ret)

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2042

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -4
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		if (mtd->ooblayout->ecc(mtd, i, &oobecc)) {

Please use the mtdcore.c's helpers
(mtd_ooblayout_set/get_data/free/ecc/bytes).

Also, what are you trying to discriminate with the return code of the function? Shouldn't this function "always" work?

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2052

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +9 -0
+			oob = NULL;
+			oobecc.length = 0;
+		} else {
+			oob = chip->oob_poi + oobecc.offset;
+		}
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  oob, oobecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);

As I told you, this helper takes "maid data" then "spare area" then "ecc bytes". The names are pretty important here as you want to avoid checking the spare OOB bytes on purpose, so maybe you could have more meaningful names and call "ecc" instead of "oob" the ecc region?

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2054

Quoted 2 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
 		if (ret < 0)
 			return ret;

Thanks,
Miquèl

author
#7 lore raw

Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages

Hi Miquèl

Quoted 52 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 08:51:11 +0200:

Quoted 31 lines

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that data and ECC bytes aren't 0xff.

Fixes 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips")
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: Fix commit log and merge nand_check_erased_ecc_chunk calls.
v2: Add Fixes tag

drivers/mtd/nand/raw/brcmnand/brcmnand.c | 19 ++++++++++++++-----

1 file changed, 14 insertions(+), 5 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -1
index e4e3ceeac38f..80fe01f03516 100644
@@ -2018,8 +2018,9 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
+	struct mtd_oob_region oobecc;
 	int i, sas;
-	void *oob = chip->oob_poi;
+	void *oob;
 	int bitflips = 0;
 	int page = addr >> chip->page_shift;
 	int ret;

@@ -2035,11 +2036,19 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 	if (ret)

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2042

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -4
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		if (mtd->ooblayout->ecc(mtd, i, &oobecc)) {

Please use the mtdcore.c's helpers
(mtd_ooblayout_set/get_data/free/ecc/bytes).

Quoted 2 lines

Also, what are you trying to discriminate with the return code of the function? Shouldn't this function "always" work?

Just making sure it doesn’t return an ERANGE in case chip->ecc.size doesn’t match the sections from mtd->ooblayout->ecc, which shouldn’t happen, so I think we can remove that...

Quoted 16 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2052

Quoted 11 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +9 -0
+			oob = NULL;
+			oobecc.length = 0;
+		} else {
+			oob = chip->oob_poi + oobecc.offset;
+		}
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  oob, oobecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);

As I told you, this helper takes "maid data" then "spare area" then "ecc bytes". The names are pretty important here as you want to avoid checking the spare OOB bytes on purpose, so maybe you could have more meaningful names and call "ecc" instead of "oob" the ecc region?

Actually I thought you meant the commit log, not the code itself...

Quoted 7 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2054

Quoted 3 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
 		if (ret < 0)
 			return ret;

Thanks,
Miquèl

Regards,
Álvaro.

maintainer
#8 lore raw

Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 09:24:32 +0200:

Quoted 54 lines

Hi Miquèl

Quoted 52 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 08:51:11 +0200:

Quoted 31 lines

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that data and ECC bytes aren't 0xff.

Fixes 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips")
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: Fix commit log and merge nand_check_erased_ecc_chunk calls.
v2: Add Fixes tag

drivers/mtd/nand/raw/brcmnand/brcmnand.c | 19 ++++++++++++++-----

1 file changed, 14 insertions(+), 5 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -1
index e4e3ceeac38f..80fe01f03516 100644
@@ -2018,8 +2018,9 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
+	struct mtd_oob_region oobecc;
 	int i, sas;
-	void *oob = chip->oob_poi;
+	void *oob;
 	int bitflips = 0;
 	int page = addr >> chip->page_shift;
 	int ret;

@@ -2035,11 +2036,19 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 	if (ret)

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2042

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -4
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		if (mtd->ooblayout->ecc(mtd, i, &oobecc)) {

Please use the mtdcore.c's helpers
(mtd_ooblayout_set/get_data/free/ecc/bytes).

Quoted 4 lines
Quoted 2 lines

Also, what are you trying to discriminate with the return code of the function? Shouldn't this function "always" work?

Just making sure it doesn’t return an ERANGE in case chip->ecc.size doesn’t match the sections from mtd->ooblayout->ecc, which shouldn’t happen, so I think we can remove that...

The style we prefer for error checking is:

	ret = function();
	if (ret)
		do someting;

instead of:

	if (function())

Anyway, I really don't know if it can happen or not. I suppose it does. What I don't understand is your "oob = chip->oob_poi + oobecc.offset". If you expect an error, then you should not update this pointer, right?

Don't you need to use 2 * i instead of i here? Following your other contribution, sections are distributed like "data/ecc/data/ecc/etc".

Quoted 18 lines
Quoted 17 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2052

Quoted 12 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +9 -0
+			oob = NULL;
+			oobecc.length = 0;
+		} else {
+			oob = chip->oob_poi + oobecc.offset;
+		}
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  oob, oobecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);

As I told you, this helper takes "maid data" then "spare area" then "ecc bytes". The names are pretty important here as you want to avoid checking the spare OOB bytes on purpose, so maybe you could have more meaningful names and call "ecc" instead of "oob" the ecc region?

Quoted 1 line

Actually I thought you meant the commit log, not the code itself...

No problem ;) I meant both actually, And I think you should name the oob pointer ecc_bytes.

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
 		if (ret < 0)
 			return ret;
Quoted 6 lines
Quoted 2 lines

Thanks,
Miquèl

Regards,
Álvaro.

Thanks,
Miquèl

author
#9 lore raw

Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages

Hi Miquèl,

Quoted 59 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 09:24:32 +0200:

Quoted 53 lines

Hi Miquèl

Quoted 51 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 08:51:11 +0200:

Quoted 31 lines

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that data and ECC bytes aren't 0xff.

Fixes 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips")
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: Fix commit log and merge nand_check_erased_ecc_chunk calls.
v2: Add Fixes tag

drivers/mtd/nand/raw/brcmnand/brcmnand.c | 19 ++++++++++++++-----

1 file changed, 14 insertions(+), 5 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -1
index e4e3ceeac38f..80fe01f03516 100644
@@ -2018,8 +2018,9 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
             struct nand_chip *chip, void *buf, u64 addr)
 {
+  struct mtd_oob_region oobecc;
   int i, sas;
-  void *oob = chip->oob_poi;
+  void *oob;
   int bitflips = 0;
   int page = addr >> chip->page_shift;
   int ret;

@@ -2035,11 +2036,19 @@ static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
   if (ret)

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd, · line 2042

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -4
           return ret;
 
-  for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+  for (i = 0; i < chip->ecc.steps; i++) {
           ecc_chunk = buf + chip->ecc.size * i;
-          ret = nand_check_erased_ecc_chunk(ecc_chunk,
-                                            chip->ecc.size,
-                                            oob, sas, NULL, 0,
+
+          if (mtd->ooblayout->ecc(mtd, i, &oobecc)) {

Please use the mtdcore.c's helpers
(mtd_ooblayout_set/get_data/free/ecc/bytes).

Ok, I will use mtd_ooblayout_ecc function.

Quoted 12 lines
Quoted 5 lines
Quoted 3 lines

Also, what are you trying to discriminate with the return code of the function? Shouldn't this function "always" work?

Just making sure it doesn’t return an ERANGE in case chip->ecc.size doesn’t match the sections from mtd->ooblayout->ecc, which shouldn’t happen, so I think we can remove that...

The style we prefer for error checking is:

        ret = function();
        if (ret)
                do someting;
Quoted 7 lines

instead of:

        if (function())

Anyway, I really don't know if it can happen or not. I suppose it does. What I don't understand is your "oob = chip->oob_poi + oobecc.offset". If you expect an error, then you should not update this pointer, right?

After switching to mtd_ooblayout_ecc, error checking isn't needed anymore.

Quoted 3 lines

Don't you need to use 2 * i instead of i here? Following your other contribution, sections are distributed like "data/ecc/data/ecc/etc".

No, we're checking ECC bytes in the OOB, not about usable bytes in the OOB area, which is what my other patch changes.

Quoted 40 lines
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +9 -0
+                  oob = NULL;
+                  oobecc.length = 0;
+          } else {
+                  oob = chip->oob_poi + oobecc.offset;
+          }
+
+          ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+                                            oob, oobecc.length,
+                                            NULL, 0,
                                             chip->ecc.strength);
Quoted 6 lines
Quoted 4 lines

As I told you, this helper takes "maid data" then "spare area" then "ecc bytes". The names are pretty important here as you want to avoid checking the spare OOB bytes on purpose, so maybe you could have more meaningful names and call "ecc" instead of "oob" the ecc region?

Actually I thought you meant the commit log, not the code itself...

No problem ;) I meant both actually, And I think you should name the oob pointer ecc_bytes.

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
           if (ret < 0)
                   return ret;
Quoted 6 lines
Quoted 2 lines

Thanks,
Miquèl

Regards,
Álvaro.

Quoted 2 lines

Thanks,
Miquèl

Regards,
Álvaro.

author
#10 lore raw

PATCH v4 1/1 mtd: rawnand: brcmnand: correctly verify erased pages

In reply to Álvaro Fernández Rojas

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that data and ECC bytes aren't 0xff.

Fixes 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips")
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v4: Use mtd_ooblayout_ecc function, rename oob to ecc_bytes and remove unused
sas variable.
v3: Fix commit log and merge nand_check_erased_ecc_chunk calls.
v2: Add Fixes tag

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +11 -8
index e4e3ceeac38f..a001483b3614 100644
@@ -2018,28 +2018,31 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 static int brcmstb_nand_verify_erased_page(struct mtd_info *mtd,
 		  struct nand_chip *chip, void *buf, u64 addr)
 {
-	int i, sas;
-	void *oob = chip->oob_poi;
+	struct mtd_oob_region ecc;
+	int i;
 	int bitflips = 0;
 	int page = addr >> chip->page_shift;
 	int ret;
+	void *ecc_bytes;
 	void *ecc_chunk;
 
 	if (!buf)
 		buf = nand_get_data_buf(chip);
 
-	sas = mtd->oobsize / chip->ecc.steps;
-
 	/* read without ecc for verification */
 	ret = chip->ecc.read_page_raw(chip, buf, true, page);
 	if (ret)
 		return ret;
 
-	for (i = 0; i < chip->ecc.steps; i++, oob += sas) {
+	for (i = 0; i < chip->ecc.steps; i++) {
 		ecc_chunk = buf + chip->ecc.size * i;
-		ret = nand_check_erased_ecc_chunk(ecc_chunk,
-						  chip->ecc.size,
-						  oob, sas, NULL, 0,
+
+		mtd_ooblayout_ecc(mtd, i, &ecc);
+		ecc_bytes = chip->oob_poi + ecc.offset;
+
+		ret = nand_check_erased_ecc_chunk(ecc_chunk, chip->ecc.size,
+						  ecc_bytes, ecc.length,
+						  NULL, 0,
 						  chip->ecc.strength);
 		if (ret < 0)
 			return ret;
Signature · 1 line
2.26.2
maintainer Applied
#11 lore raw

Re: [PATCH v4] mtd: rawnand: brcmnand: correctly verify erased pages

Quoted 7 lines · On Tue, 2020-05-12 at 08:24:51 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?…

The current code checks that the whole OOB area is erased.
This is a problem when JFFS2 cleanmarkers are added to the OOB, since it will fail due to the usable OOB bytes not being 0xff.
Correct this by only checking that data and ECC bytes aren't 0xff.

Fixes 02b88eea9f9c ("mtd: brcmnand: Add check for erased page bitflips")
Signed-off-by Álvaro Fernández Rojas <[email protected]>

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

Miquel