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(-)
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
On 5/4/2020 2:29 AM, Álvaro Fernández 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 the ECC aren't 0xff.
>
> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
Can you provide a Fixes: tag for this change?
> ---
> 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;
>
--
Florian
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(-)
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
Hi Álvaro,
Álvaro Fernández Rojas <[email protected]> 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 <[email protected]>
> ---
> 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
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(-)
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
Hi Álvaro,
Álvaro Fernández Rojas <[email protected]> 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 <[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(-)
>
> 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
Hi Miquèl
> El 12 may 2020, a las 9:16, Miquel Raynal <[email protected]> escribió:
>
> Hi Álvaro,
>
> Álvaro Fernández Rojas <[email protected]> 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 <[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(-)
>>
>> 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.
Hi Álvaro,
Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020
09:24:32 +0200:
> Hi Miquèl
>
> > El 12 may 2020, a las 9:16, Miquel Raynal <[email protected]> escribió:
> >
> > Hi Álvaro,
> >
> > Álvaro Fernández Rojas <[email protected]> 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 <[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(-)
> >>
> >> 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
Hi Miquèl,
El mar., 12 may. 2020 a las 9:34, Miquel Raynal
(<[email protected]>) escribió:
>
> Hi Álvaro,
>
> Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020
> 09:24:32 +0200:
>
> > Hi Miquèl
> >
> > > El 12 may 2020, a las 9:16, Miquel Raynal <[email protected]> escribió:
> > >
> > > Hi Álvaro,
> > >
> > > Álvaro Fernández Rojas <[email protected]> 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 <[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(-)
> > >>
> > >> 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.
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(-)
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
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 <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel