Hi Miquèl
> El 12 may 2020, a las 9:16, Miquel Raynal <miquel.raynal@xxxxxxxxxxx> escribió:
>
> Hi Álvaro,
>
> Álvaro Fernández Rojas <noltari@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
