* [PATCH v2] nand: brcmnand: correctly verify erased pages
@ 2020-05-05 8:20 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-05 8:20 UTC (permalink / raw)
To: computersforpeace, kdasu.kdev, miquel.raynal, richard, vigneshr,
sumit.semwal, linux-mtd, bcm-kernel-feedback-list, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
Cc: Á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 <noltari@gmail.com>
---
v2: Add Fixes tag
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e4e3ceeac38f..546f0807b887 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -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;
--
2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v2] nand: brcmnand: correctly verify erased pages
@ 2020-05-05 8:20 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-05 8:20 UTC (permalink / raw)
To: computersforpeace, kdasu.kdev, miquel.raynal, richard, vigneshr,
sumit.semwal, linux-mtd, bcm-kernel-feedback-list, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
Cc: Á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 <noltari@gmail.com>
---
v2: Add Fixes tag
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e4e3ceeac38f..546f0807b887 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -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;
--
2.26.2
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2] nand: brcmnand: correctly verify erased pages
2020-05-05 8:20 ` Álvaro Fernández Rojas
@ 2020-05-11 16:43 ` Miquel Raynal
-1 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-11 16:43 UTC (permalink / raw)
To: Álvaro Fernández Rojas
Cc: computersforpeace, kdasu.kdev, richard, vigneshr, sumit.semwal,
linux-mtd, bcm-kernel-feedback-list, linux-kernel, linux-media,
dri-devel, linaro-mm-sig
Hi Álvaro,
Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 5 May 2020
10:20:55 +0200:
> 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
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
> v2: Add Fixes tag
>
> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index e4e3ceeac38f..546f0807b887 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -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
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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2] nand: brcmnand: correctly verify erased pages
@ 2020-05-11 16:43 ` Miquel Raynal
0 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-11 16:43 UTC (permalink / raw)
To: Álvaro Fernández Rojas
Cc: vigneshr, kdasu.kdev, richard, linux-kernel, dri-devel,
linaro-mm-sig, linux-mtd, bcm-kernel-feedback-list,
computersforpeace, sumit.semwal, linux-media
Hi Álvaro,
Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 5 May 2020
10:20:55 +0200:
> 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
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
> v2: Add Fixes tag
>
> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index e4e3ceeac38f..546f0807b887 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -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
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
2020-05-05 8:20 ` Álvaro Fernández Rojas
(?)
@ 2020-05-12 6:51 ` Álvaro Fernández Rojas
-1 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 6:51 UTC (permalink / raw)
To: computersforpeace, kdasu.kdev, miquel.raynal, richard, vigneshr,
sumit.semwal, linux-mtd, bcm-kernel-feedback-list, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
Cc: Á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 <noltari@gmail.com>
---
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(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e4e3ceeac38f..80fe01f03516 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -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;
--
2.26.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 6:51 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 6:51 UTC (permalink / raw)
To: computersforpeace, kdasu.kdev, miquel.raynal, richard, vigneshr,
sumit.semwal, linux-mtd, bcm-kernel-feedback-list, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
Cc: Á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 <noltari@gmail.com>
---
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(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e4e3ceeac38f..80fe01f03516 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -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;
--
2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 6:51 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 6:51 UTC (permalink / raw)
To: computersforpeace, kdasu.kdev, miquel.raynal, richard, vigneshr,
sumit.semwal, linux-mtd, bcm-kernel-feedback-list, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
Cc: Á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 <noltari@gmail.com>
---
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(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e4e3ceeac38f..80fe01f03516 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -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;
--
2.26.2
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
2020-05-12 6:51 ` Álvaro Fernández Rojas
(?)
@ 2020-05-12 7:16 ` Miquel Raynal
-1 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-12 7:16 UTC (permalink / raw)
To: Álvaro Fernández Rojas
Cc: computersforpeace, kdasu.kdev, richard, vigneshr, sumit.semwal,
linux-mtd, bcm-kernel-feedback-list, linux-kernel, linux-media,
dri-devel, linaro-mm-sig
Hi Álvaro,
Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
08:51:11 +0200:
> 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 <noltari@gmail.com>
> ---
> 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(-)
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index e4e3ceeac38f..80fe01f03516 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -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)) {
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?
> + 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?
> if (ret < 0)
> return ret;
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 7:16 ` Miquel Raynal
0 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-12 7:16 UTC (permalink / raw)
To: Álvaro Fernández Rojas
Cc: vigneshr, kdasu.kdev, richard, linux-kernel, dri-devel,
linaro-mm-sig, linux-mtd, bcm-kernel-feedback-list,
computersforpeace, linux-media
Hi Álvaro,
Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
08:51:11 +0200:
> 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 <noltari@gmail.com>
> ---
> 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(-)
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index e4e3ceeac38f..80fe01f03516 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -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)) {
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?
> + 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?
> if (ret < 0)
> return ret;
Thanks,
Miquèl
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 7:16 ` Miquel Raynal
0 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-12 7:16 UTC (permalink / raw)
To: Álvaro Fernández Rojas
Cc: vigneshr, kdasu.kdev, richard, linux-kernel, dri-devel,
linaro-mm-sig, linux-mtd, bcm-kernel-feedback-list,
computersforpeace, sumit.semwal, linux-media
Hi Álvaro,
Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
08:51:11 +0200:
> 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 <noltari@gmail.com>
> ---
> 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(-)
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index e4e3ceeac38f..80fe01f03516 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -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)) {
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?
> + 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?
> if (ret < 0)
> return ret;
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
2020-05-12 7:16 ` Miquel Raynal
(?)
@ 2020-05-12 7:24 ` Álvaro Fernández Rojas
-1 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 7:24 UTC (permalink / raw)
To: Miquel Raynal
Cc: computersforpeace, kdasu.kdev, richard, vigneshr, sumit.semwal,
linux-mtd, bcm-kernel-feedback-list, linux-kernel, linux-media,
dri-devel, linaro-mm-sig
Hi Miquèl
> El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
>
> Hi Álvaro,
>
> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> 08:51:11 +0200:
>
>> 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 <noltari@gmail.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index e4e3ceeac38f..80fe01f03516 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -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)) {
>
> 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?
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...
>
>> + 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...
>
>> if (ret < 0)
>> return ret;
>
>
> Thanks,
> Miquèl
Regards,
Álvaro.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 7:24 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 7:24 UTC (permalink / raw)
To: Miquel Raynal
Cc: vigneshr, kdasu.kdev, richard, linux-kernel, dri-devel,
linaro-mm-sig, linux-mtd, bcm-kernel-feedback-list,
computersforpeace, linux-media
Hi Miquèl
> El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
>
> Hi Álvaro,
>
> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> 08:51:11 +0200:
>
>> 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 <noltari@gmail.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index e4e3ceeac38f..80fe01f03516 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -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)) {
>
> 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?
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...
>
>> + 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...
>
>> if (ret < 0)
>> return ret;
>
>
> Thanks,
> Miquèl
Regards,
Álvaro.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 7:24 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 7:24 UTC (permalink / raw)
To: Miquel Raynal
Cc: vigneshr, kdasu.kdev, richard, linux-kernel, dri-devel,
linaro-mm-sig, linux-mtd, bcm-kernel-feedback-list,
computersforpeace, sumit.semwal, linux-media
Hi Miquèl
> El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
>
> Hi Álvaro,
>
> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> 08:51:11 +0200:
>
>> 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 <noltari@gmail.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index e4e3ceeac38f..80fe01f03516 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -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)) {
>
> 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?
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...
>
>> + 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...
>
>> if (ret < 0)
>> return ret;
>
>
> Thanks,
> Miquèl
Regards,
Álvaro.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
2020-05-12 7:24 ` Álvaro Fernández Rojas
(?)
@ 2020-05-12 7:34 ` Miquel Raynal
-1 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-12 7:34 UTC (permalink / raw)
To: Álvaro Fernández Rojas
Cc: computersforpeace, kdasu.kdev, richard, vigneshr, sumit.semwal,
linux-mtd, bcm-kernel-feedback-list, linux-kernel, linux-media,
dri-devel, linaro-mm-sig
Hi Álvaro,
Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
09:24:32 +0200:
> Hi Miquèl
>
> > El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> >
> > Hi Álvaro,
> >
> > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> > 08:51:11 +0200:
> >
> >> 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 <noltari@gmail.com>
> >> ---
> >> 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(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> index e4e3ceeac38f..80fe01f03516 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> @@ -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)) {
> >
> > 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?
>
> 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".
>
> >
> >> + 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...
No problem ;) I meant both actually, And I think you should name the
oob pointer ecc_bytes.
>
> >
> >> if (ret < 0)
> >> return ret;
> >
> >
> > Thanks,
> > Miquèl
>
> Regards,
> Álvaro.
>
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 7:34 ` Miquel Raynal
0 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-12 7:34 UTC (permalink / raw)
To: Álvaro Fernández Rojas
Cc: vigneshr, kdasu.kdev, richard, linux-kernel, dri-devel,
linaro-mm-sig, linux-mtd, bcm-kernel-feedback-list,
computersforpeace, linux-media
Hi Álvaro,
Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
09:24:32 +0200:
> Hi Miquèl
>
> > El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> >
> > Hi Álvaro,
> >
> > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> > 08:51:11 +0200:
> >
> >> 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 <noltari@gmail.com>
> >> ---
> >> 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(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> index e4e3ceeac38f..80fe01f03516 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> @@ -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)) {
> >
> > 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?
>
> 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".
>
> >
> >> + 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...
No problem ;) I meant both actually, And I think you should name the
oob pointer ecc_bytes.
>
> >
> >> if (ret < 0)
> >> return ret;
> >
> >
> > Thanks,
> > Miquèl
>
> Regards,
> Álvaro.
>
Thanks,
Miquèl
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 7:34 ` Miquel Raynal
0 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-12 7:34 UTC (permalink / raw)
To: Álvaro Fernández Rojas
Cc: vigneshr, kdasu.kdev, richard, linux-kernel, dri-devel,
linaro-mm-sig, linux-mtd, bcm-kernel-feedback-list,
computersforpeace, sumit.semwal, linux-media
Hi Álvaro,
Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
09:24:32 +0200:
> Hi Miquèl
>
> > El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> >
> > Hi Álvaro,
> >
> > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> > 08:51:11 +0200:
> >
> >> 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 <noltari@gmail.com>
> >> ---
> >> 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(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> index e4e3ceeac38f..80fe01f03516 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> @@ -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)) {
> >
> > 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?
>
> 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".
>
> >
> >> + 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...
No problem ;) I meant both actually, And I think you should name the
oob pointer ecc_bytes.
>
> >
> >> if (ret < 0)
> >> return ret;
> >
> >
> > Thanks,
> > Miquèl
>
> Regards,
> Álvaro.
>
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
2020-05-12 7:34 ` Miquel Raynal
(?)
@ 2020-05-12 8:24 ` Álvaro Fernández Rojas
-1 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 8:24 UTC (permalink / raw)
To: Miquel Raynal
Cc: computersforpeace, kdasu.kdev, richard, Vignesh R, Sumit Semwal,
linux-mtd, bcm-kernel-feedback-list, Linux Kernel Mailing List,
linux-media, dri-devel, linaro-mm-sig
Hi Miquèl,
El mar., 12 may. 2020 a las 9:34, Miquel Raynal
(<miquel.raynal@bootlin.com>) escribió:
>
> Hi Álvaro,
>
> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> 09:24:32 +0200:
>
> > Hi Miquèl
> >
> > > El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> > >
> > > Hi Álvaro,
> > >
> > > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> > > 08:51:11 +0200:
> > >
> > >> 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 <noltari@gmail.com>
> > >> ---
> > >> 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(-)
> > >>
> > >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> index e4e3ceeac38f..80fe01f03516 100644
> > >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> @@ -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)) {
> > >
> > > Please use the mtdcore.c's helpers
> > > (mtd_ooblayout_set/get_data/free/ecc/bytes).
Ok, I will use mtd_ooblayout_ecc function.
> > >
> > > 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?
After switching to mtd_ooblayout_ecc, error checking isn't needed anymore.
>
> 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.
>
> >
> > >
> > >> + 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...
>
> No problem ;) I meant both actually, And I think you should name the
> oob pointer ecc_bytes.
>
> >
> > >
> > >> if (ret < 0)
> > >> return ret;
> > >
> > >
> > > Thanks,
> > > Miquèl
> >
> > Regards,
> > Álvaro.
> >
>
>
>
>
> Thanks,
> Miquèl
Regards,
Álvaro.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 8:24 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 8:24 UTC (permalink / raw)
To: Miquel Raynal
Cc: Vignesh R, kdasu.kdev, richard, Linux Kernel Mailing List,
dri-devel, linaro-mm-sig, linux-mtd, bcm-kernel-feedback-list,
computersforpeace, linux-media
Hi Miquèl,
El mar., 12 may. 2020 a las 9:34, Miquel Raynal
(<miquel.raynal@bootlin.com>) escribió:
>
> Hi Álvaro,
>
> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> 09:24:32 +0200:
>
> > Hi Miquèl
> >
> > > El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> > >
> > > Hi Álvaro,
> > >
> > > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> > > 08:51:11 +0200:
> > >
> > >> 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 <noltari@gmail.com>
> > >> ---
> > >> 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(-)
> > >>
> > >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> index e4e3ceeac38f..80fe01f03516 100644
> > >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> @@ -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)) {
> > >
> > > Please use the mtdcore.c's helpers
> > > (mtd_ooblayout_set/get_data/free/ecc/bytes).
Ok, I will use mtd_ooblayout_ecc function.
> > >
> > > 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?
After switching to mtd_ooblayout_ecc, error checking isn't needed anymore.
>
> 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.
>
> >
> > >
> > >> + 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...
>
> No problem ;) I meant both actually, And I think you should name the
> oob pointer ecc_bytes.
>
> >
> > >
> > >> if (ret < 0)
> > >> return ret;
> > >
> > >
> > > Thanks,
> > > Miquèl
> >
> > Regards,
> > Álvaro.
> >
>
>
>
>
> Thanks,
> Miquèl
Regards,
Álvaro.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 8:24 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 8:24 UTC (permalink / raw)
To: Miquel Raynal
Cc: Vignesh R, kdasu.kdev, richard, Linux Kernel Mailing List,
dri-devel, linaro-mm-sig, linux-mtd, bcm-kernel-feedback-list,
computersforpeace, Sumit Semwal, linux-media
Hi Miquèl,
El mar., 12 may. 2020 a las 9:34, Miquel Raynal
(<miquel.raynal@bootlin.com>) escribió:
>
> Hi Álvaro,
>
> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> 09:24:32 +0200:
>
> > Hi Miquèl
> >
> > > El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@bootlin.com> escribió:
> > >
> > > Hi Álvaro,
> > >
> > > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Tue, 12 May 2020
> > > 08:51:11 +0200:
> > >
> > >> 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 <noltari@gmail.com>
> > >> ---
> > >> 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(-)
> > >>
> > >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> index e4e3ceeac38f..80fe01f03516 100644
> > >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >> @@ -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)) {
> > >
> > > Please use the mtdcore.c's helpers
> > > (mtd_ooblayout_set/get_data/free/ecc/bytes).
Ok, I will use mtd_ooblayout_ecc function.
> > >
> > > 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?
After switching to mtd_ooblayout_ecc, error checking isn't needed anymore.
>
> 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.
>
> >
> > >
> > >> + 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...
>
> No problem ;) I meant both actually, And I think you should name the
> oob pointer ecc_bytes.
>
> >
> > >
> > >> if (ret < 0)
> > >> return ret;
> > >
> > >
> > > Thanks,
> > > Miquèl
> >
> > Regards,
> > Álvaro.
> >
>
>
>
>
> Thanks,
> Miquèl
Regards,
Álvaro.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v4] mtd: rawnand: brcmnand: correctly verify erased pages
2020-05-12 6:51 ` Álvaro Fernández Rojas
(?)
@ 2020-05-12 8:24 ` Álvaro Fernández Rojas
-1 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 8:24 UTC (permalink / raw)
To: computersforpeace, kdasu.kdev, miquel.raynal, richard, vigneshr,
sumit.semwal, linux-mtd, bcm-kernel-feedback-list, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
Cc: Á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 <noltari@gmail.com>
---
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(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e4e3ceeac38f..a001483b3614 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -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;
--
2.26.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 8:24 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 8:24 UTC (permalink / raw)
To: computersforpeace, kdasu.kdev, miquel.raynal, richard, vigneshr,
sumit.semwal, linux-mtd, bcm-kernel-feedback-list, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
Cc: Á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 <noltari@gmail.com>
---
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(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e4e3ceeac38f..a001483b3614 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -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;
--
2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-12 8:24 ` Álvaro Fernández Rojas
0 siblings, 0 replies; 33+ messages in thread
From: Álvaro Fernández Rojas @ 2020-05-12 8:24 UTC (permalink / raw)
To: computersforpeace, kdasu.kdev, miquel.raynal, richard, vigneshr,
sumit.semwal, linux-mtd, bcm-kernel-feedback-list, linux-kernel,
linux-media, dri-devel, linaro-mm-sig
Cc: Á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 <noltari@gmail.com>
---
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(-)
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index e4e3ceeac38f..a001483b3614 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -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;
--
2.26.2
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v4] mtd: rawnand: brcmnand: correctly verify erased pages
2020-05-12 8:24 ` Álvaro Fernández Rojas
(?)
@ 2020-05-24 19:16 ` Miquel Raynal
-1 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-24 19:16 UTC (permalink / raw)
To: Álvaro Fernández Rojas, computersforpeace, kdasu.kdev,
miquel.raynal, richard, vigneshr, sumit.semwal, linux-mtd,
bcm-kernel-feedback-list, linux-kernel, linux-media, dri-devel,
linaro-mm-sig
On Tue, 2020-05-12 at 08:24:51 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?= wrote:
> 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 <noltari@gmail.com>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v4] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-24 19:16 ` Miquel Raynal
0 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-24 19:16 UTC (permalink / raw)
To: Álvaro Fernández Rojas, computersforpeace, kdasu.kdev,
miquel.raynal, richard, vigneshr, sumit.semwal, linux-mtd,
bcm-kernel-feedback-list, linux-kernel, linux-media, dri-devel,
linaro-mm-sig
On Tue, 2020-05-12 at 08:24:51 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?= wrote:
> 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 <noltari@gmail.com>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v4] mtd: rawnand: brcmnand: correctly verify erased pages
@ 2020-05-24 19:16 ` Miquel Raynal
0 siblings, 0 replies; 33+ messages in thread
From: Miquel Raynal @ 2020-05-24 19:16 UTC (permalink / raw)
To: Álvaro Fernández Rojas, computersforpeace, kdasu.kdev,
miquel.raynal, richard, vigneshr, sumit.semwal, linux-mtd,
bcm-kernel-feedback-list, linux-kernel, linux-media, dri-devel,
linaro-mm-sig
On Tue, 2020-05-12 at 08:24:51 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?= wrote:
> 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 <noltari@gmail.com>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 33+ messages in thread