* [PATCH] nand: brcmnand: correctly verify erased pages @ 2020-05-04 9:29 Álvaro Fernández Rojas 2020-05-04 21:43 ` Florian Fainelli 2020-05-05 8:20 ` [PATCH v2] " Álvaro Fernández Rojas 0 siblings, 2 replies; 11+ messages in thread From: Álvaro Fernández Rojas @ 2020-05-04 9:29 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. 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] nand: brcmnand: correctly verify erased pages 2020-05-04 9:29 [PATCH] nand: brcmnand: correctly verify erased pages Álvaro Fernández Rojas @ 2020-05-04 21:43 ` Florian Fainelli 2020-05-05 8:24 ` Álvaro Fernández Rojas 2020-05-05 8:20 ` [PATCH v2] " Álvaro Fernández Rojas 1 sibling, 1 reply; 11+ messages in thread From: Florian Fainelli @ 2020-05-04 21:43 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 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] nand: brcmnand: correctly verify erased pages 2020-05-04 21:43 ` Florian Fainelli @ 2020-05-05 8:24 ` Álvaro Fernández Rojas 0 siblings, 0 replies; 11+ messages in thread From: Álvaro Fernández Rojas @ 2020-05-05 8:24 UTC (permalink / raw) To: Florian Fainelli Cc: vigneshr, kdasu.kdev, richard, linux-kernel, dri-devel, linaro-mm-sig, linux-mtd, miquel.raynal, bcm-kernel-feedback-list, computersforpeace, linux-media [-- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] nand: brcmnand: correctly verify erased pages 2020-05-04 9:29 [PATCH] nand: brcmnand: correctly verify erased pages Álvaro Fernández Rojas 2020-05-04 21:43 ` Florian Fainelli @ 2020-05-05 8:20 ` Álvaro Fernández Rojas 2020-05-12 6:51 ` [PATCH v3] mtd: rawnand: " Álvaro Fernández Rojas 1 sibling, 1 reply; 11+ 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] 11+ messages in thread
* [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages 2020-05-05 8:20 ` [PATCH v2] " Álvaro Fernández Rojas @ 2020-05-12 6:51 ` Álvaro Fernández Rojas 2020-05-12 7:16 ` Miquel Raynal 2020-05-12 8:24 ` [PATCH v4] " Álvaro Fernández Rojas 0 siblings, 2 replies; 11+ 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] 11+ messages in thread
* Re: [PATCH v3] mtd: rawnand: brcmnand: correctly verify erased pages 2020-05-12 6:51 ` [PATCH v3] mtd: rawnand: " Álvaro Fernández Rojas @ 2020-05-12 7:16 ` Miquel Raynal 2020-05-12 7:24 ` Álvaro Fernández Rojas 2020-05-12 8:24 ` [PATCH v4] " Álvaro Fernández Rojas 1 sibling, 1 reply; 11+ 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] 11+ 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 2020-05-12 7:34 ` Miquel Raynal 0 siblings, 1 reply; 11+ 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] 11+ 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 2020-05-12 8:24 ` Álvaro Fernández Rojas 0 siblings, 1 reply; 11+ 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] 11+ 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 0 siblings, 0 replies; 11+ 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] 11+ messages in thread
* [PATCH v4] mtd: rawnand: brcmnand: correctly verify erased pages 2020-05-12 6:51 ` [PATCH v3] mtd: rawnand: " Álvaro Fernández Rojas 2020-05-12 7:16 ` Miquel Raynal @ 2020-05-12 8:24 ` Álvaro Fernández Rojas 2020-05-24 19:16 ` Miquel Raynal 1 sibling, 1 reply; 11+ 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] 11+ messages in thread
* Re: [PATCH v4] mtd: rawnand: brcmnand: correctly verify erased pages 2020-05-12 8:24 ` [PATCH v4] " Álvaro Fernández Rojas @ 2020-05-24 19:16 ` Miquel Raynal 0 siblings, 0 replies; 11+ 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] 11+ messages in thread
end of thread, other threads:[~2020-05-25 7:22 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-04 9:29 [PATCH] nand: brcmnand: correctly verify erased pages Álvaro Fernández Rojas 2020-05-04 21:43 ` Florian Fainelli 2020-05-05 8:24 ` Álvaro Fernández Rojas 2020-05-05 8:20 ` [PATCH v2] " Álvaro Fernández Rojas 2020-05-12 6:51 ` [PATCH v3] mtd: rawnand: " Álvaro Fernández Rojas 2020-05-12 7:16 ` Miquel Raynal 2020-05-12 7:24 ` Álvaro Fernández Rojas 2020-05-12 7:34 ` Miquel Raynal 2020-05-12 8:24 ` Álvaro Fernández Rojas 2020-05-12 8:24 ` [PATCH v4] " Álvaro Fernández Rojas 2020-05-24 19:16 ` Miquel Raynal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).