[PATCH] nand: brcmnand: correctly verify erased pages

[PATCH] nand: brcmnand: correctly verify erased pages

[Álvaro Fernández Rojas]

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

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 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

Re: [PATCH] nand: brcmnand: correctly verify erased pages

[Florian Fainelli]

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 <noltari@gmail.com>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] nand: brcmnand: correctly verify erased pages

[Á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

Re: [PATCH] nand: brcmnand: correctly verify erased pages

[Álvaro Fernández Rojas]

[-- Attachment #1.1: Type: text/plain, Size: 2292 bytes --]

Hi Florian,

I added it on v2.

Best regards,
Álvaro.

> El 4 may 2020, a las 23:43, Florian Fainelli <f.fainelli@gmail.com> escribió:
> 
> 
> 
> 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 <noltari@gmail.com <mailto:noltari@gmail.com>>
> 
> 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


[-- Attachment #1.2: Type: text/html, Size: 13914 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Á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

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

[Miquel Raynal]

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

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

[Álvaro Fernández Rojas]

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

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

[Miquel Raynal]

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

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

[Álvaro Fernández Rojas]

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

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

[Á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

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

[Miquel Raynal]

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