| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <973f8019-77ce-7b5e-0990-1492521ad4b3@kontron.de>
Date: Thu, 21 Mar 2019 09:04:23 +0000
From: Schrempf Frieder <frieder.schrempf@...tron.de>
To: Miquel Raynal <miquel.raynal@...tlin.com>
CC: "bbrezillon@...nel.org" <bbrezillon@...nel.org>,
"richard@....at" <richard@....at>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
David Woodhouse <dwmw2@...radead.org>,
"Brian Norris" <computersforpeace@...il.com>,
Marek Vasut <marek.vasut@...il.com>
Subject: Re: [PATCH v4 5/7] mtd: rawnand: Support bad block markers in first,
second or last page
On 04.03.19 12:21, Miquel Raynal wrote:
> Hi Schrempf,
>
> Schrempf Frieder <frieder.schrempf@...tron.de> wrote on Mon, 18 Feb
> 2019 10:42:45 +0000:
>
>> From: Frieder Schrempf <frieder.schrempf@...tron.de>
>>
>> Currently supported bad block marker positions within the block are:
>> * in first page only
>> * in last page only
>> * in first or second page
>>
>> Some ESMT NANDs are known to have been shipped by the manufacturer
>> with bad block markers in the first or last page, instead of the
>> first or second page.
>>
>> Also the datasheets for Cypress/Spansion/AMD NANDs claim that the
>> first, second *and* last page needs to be checked.
>>
>> Therefore we make it possible to set NAND_BBM_FIRSTPAGE,
>> NAND_BBM_SECONDPAGE and NAND_BBM_LASTPAGE independently in any
>> combination.
>>
>> To simplify the code, the logic to evaluate the flags is moved to a
>> a new function nand_bbm_get_next_page().
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@...tron.de>
>> Reviewed-by: Boris Brezillon <bbrezillon@...nel.org>
>> ---
>> drivers/mtd/nand/raw/internals.h | 1 +
>> drivers/mtd/nand/raw/nand_base.c | 62 ++++++++++++++++++++++++-----------
>> drivers/mtd/nand/raw/nand_bbt.c | 29 +++++++---------
>> 3 files changed, 55 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
>> index fbf6ca015cd7..97ae67e009d5 100644
>> --- a/drivers/mtd/nand/raw/internals.h
>> +++ b/drivers/mtd/nand/raw/internals.h
>> @@ -76,6 +76,7 @@ extern const struct nand_manufacturer_ops toshiba_nand_manuf_ops;
>>
>> /* Core functions */
>> const struct nand_manufacturer *nand_get_manufacturer(u8 id);
>> +int nand_bbm_get_next_page(struct nand_chip *chip, int page);
>> int nand_markbad_bbm(struct nand_chip *chip, loff_t ofs);
>> int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
>> int allowbbt);
>> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
>> index 9ef7b86cdc42..7bc20c1fe23c 100644
>> --- a/drivers/mtd/nand/raw/nand_base.c
>> +++ b/drivers/mtd/nand/raw/nand_base.c
>> @@ -285,6 +285,31 @@ static void nand_release_device(struct nand_chip *chip)
>> spin_unlock(&chip->controller->lock);
>> }
>>
>> +/**
>> + * nand_bbm_get_next_page - Get the next page for bad block markers
>> + * @chip: NAND chip object
>> + * @index: Current page, only pages beyond this will be considered
>
> Why pages *beyond*? Can't you change the logic to use the page from
> where to start searching? I am not sure such a change would be
> clearer in the rest of the code but I don't like the tests against
> "-1". Maybe you could just check negative values instead.
I guess I can change to use the page from where to start searching as a
parameter. If that's what you mean (inclusive instead of exclusive)?
Then the initial call would look like "nand_bbm_get_next_page(chip, 0)"
instead of "nand_bbm_get_next_page(chip, -1)" which seems a bit clearer.
>> + *
>> + * Returns an integer that corresponds to the page offset within a block, for
>> + * a page that is used to store bad block markers. If no more pages are
>> + * available, -1 is returned.
>> + */
>> +int nand_bbm_get_next_page(struct nand_chip *chip, int page)
>> +{
>> + struct mtd_info *mtd = nand_to_mtd(chip);
>> + int last_page = ((mtd->erasesize - mtd->writesize) >>
>> + chip->page_shift) & chip->pagemask;
>> +
>> + if (page < 0 && chip->options & NAND_BBM_FIRSTPAGE)
>> + return 0;
>> + else if (page < 1 && chip->options & NAND_BBM_SECONDPAGE)
>> + return 1;
>> + else if (page < last_page && chip->options & NAND_BBM_LASTPAGE)
>> + return last_page;
>> +
>> + return -1;
>
> I would prefer a named value (like -EINVAL) and checks against a
> negative value (not -1).
Right, makes sense.
>> +}
>> +
>> /**
>> * nand_block_bad - [DEFAULT] Read bad block marker from the chip
>> * @chip: NAND chip object
>> @@ -294,19 +319,14 @@ static void nand_release_device(struct nand_chip *chip)
>> */
>> static int nand_block_bad(struct nand_chip *chip, loff_t ofs)
>> {
>> - struct mtd_info *mtd = nand_to_mtd(chip);
>> - int page, page_end, res;
>> + int page_offset;
>> + int res, first_page = (int)(ofs >> chip->page_shift) & chip->pagemask;
>
> Maybe:
>
> int first_page, page_offset;
> int res;
> u8 bad;
>
> first_page = (int)(ofs >> chip->page_shift) & chip->pagemask;
> page_offset = nand_bbm_get_next_page(chip, -1);
Looks better.
>> u8 bad;
>>
>> - if (chip->options & NAND_BBM_LASTPAGE)
>> - ofs += mtd->erasesize - mtd->writesize;
>> + page_offset = nand_bbm_get_next_page(chip, -1);
>>
>> - page = (int)(ofs >> chip->page_shift) & chip->pagemask;
>> - page_end = page + (((chip->options & NAND_BBM_FIRSTPAGE) &&
>> - (chip->options & NAND_BBM_SECONDPAGE)) ? 2 : 1);
>> -
>> - for (; page < page_end; page++) {
>> - res = chip->ecc.read_oob(chip, page);
>> + while (page_offset != -1) {
>> + res = chip->ecc.read_oob(chip, first_page + page_offset);
>> if (res < 0)
>> return res;
>>
>> @@ -318,6 +338,8 @@ static int nand_block_bad(struct nand_chip *chip, loff_t ofs)
>> res = hweight8(bad) < chip->badblockbits;
>> if (res)
>> return res;
>> +
>> + page_offset = nand_bbm_get_next_page(chip, page_offset);
>> }
>>
>> return 0;
>> @@ -528,7 +550,7 @@ static int nand_default_block_markbad(struct nand_chip *chip, loff_t ofs)
>> struct mtd_info *mtd = nand_to_mtd(chip);
>> struct mtd_oob_ops ops;
>> uint8_t buf[2] = { 0, 0 };
>> - int ret = 0, res, i = 0;
>> + int ret = 0, res, page_offset;
>>
>> memset(&ops, 0, sizeof(ops));
>> ops.oobbuf = buf;
>> @@ -541,18 +563,18 @@ static int nand_default_block_markbad(struct nand_chip *chip, loff_t ofs)
>> }
>> ops.mode = MTD_OPS_PLACE_OOB;
>>
>> - /* Write to first/last page(s) if necessary */
>> - if (chip->options & NAND_BBM_LASTPAGE)
>> - ofs += mtd->erasesize - mtd->writesize;
>> - do {
>> - res = nand_do_write_oob(chip, ofs, &ops);
>> + page_offset = nand_bbm_get_next_page(chip, -1);
>> +
>> + while (page_offset != -1) {
>> + res = nand_do_write_oob(chip,
>> + ofs + page_offset * mtd->writesize,
>> + &ops);
>> +
>> if (!ret)
>> ret = res;
>>
>> - i++;
>> - ofs += mtd->writesize;
>> - } while ((chip->options & NAND_BBM_FIRSTPAGE) &&
>> - (chip->options & NAND_BBM_SECONDPAGE) && i < 2);
>> + page_offset = nand_bbm_get_next_page(chip, page_offset);
>> + }
>>
>> return ret;
>> }
>> diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c
>> index 7463afddc7ac..09603c502931 100644
>> --- a/drivers/mtd/nand/raw/nand_bbt.c
>> +++ b/drivers/mtd/nand/raw/nand_bbt.c
>> @@ -415,11 +415,12 @@ static void read_abs_bbts(struct nand_chip *this, uint8_t *buf,
>>
>> /* Scan a given block partially */
>> static int scan_block_fast(struct nand_chip *this, struct nand_bbt_descr *bd,
>> - loff_t offs, uint8_t *buf, int numpages)
>> + loff_t offs, uint8_t *buf)
>> {
>> struct mtd_info *mtd = nand_to_mtd(this);
>> +
>> struct mtd_oob_ops ops;
>> - int j, ret;
>> + int ret, page_offset;
>>
>> ops.ooblen = mtd->oobsize;
>> ops.oobbuf = buf;
>> @@ -427,12 +428,15 @@ static int scan_block_fast(struct nand_chip *this, struct nand_bbt_descr *bd,
>> ops.datbuf = NULL;
>> ops.mode = MTD_OPS_PLACE_OOB;
>>
>> - for (j = 0; j < numpages; j++) {
>> + page_offset = nand_bbm_get_next_page(this, -1);
>> +
>> + while (page_offset != -1) {
>> /*
>> * Read the full oob until read_oob is fixed to handle single
>> * byte reads for 16 bit buswidth.
>> */
>> - ret = mtd_read_oob(mtd, offs, &ops);
>> + ret = mtd_read_oob(mtd, offs + page_offset * mtd->writesize,
>
> Can you add '(' & ')' please?
Sure
>> + &ops);
>> /* Ignore ECC errors when checking for BBM */
>> if (ret && !mtd_is_bitflip_or_eccerr(ret))
>> return ret;
>> @@ -440,8 +444,9 @@ static int scan_block_fast(struct nand_chip *this, struct nand_bbt_descr *bd,
>> if (check_short_pattern(buf, bd))
>> return 1;
>>
>> - offs += mtd->writesize;
>> + page_offset = nand_bbm_get_next_page(this, page_offset);
>> }
>> +
>> return 0;
>> }
>>
>> @@ -460,18 +465,11 @@ static int create_bbt(struct nand_chip *this, uint8_t *buf,
>> struct nand_bbt_descr *bd, int chip)
>> {
>> struct mtd_info *mtd = nand_to_mtd(this);
>> - int i, numblocks, numpages;
>> - int startblock;
>> + int i, numblocks, startblock;
>> loff_t from;
>>
>> pr_info("Scanning device for bad blocks\n");
>>
>> - if ((this->options & NAND_BBM_FIRSTPAGE) &&
>> - (this->options & NAND_BBM_SECONDPAGE))
>> - numpages = 2;
>> - else
>> - numpages = 1;
>> -
>> if (chip == -1) {
>> numblocks = mtd->size >> this->bbt_erase_shift;
>> startblock = 0;
>> @@ -488,15 +486,12 @@ static int create_bbt(struct nand_chip *this, uint8_t *buf,
>> from = (loff_t)startblock << this->bbt_erase_shift;
>> }
>>
>> - if (this->options & NAND_BBM_LASTPAGE)
>> - from += mtd->erasesize - (mtd->writesize * numpages);
>> -
>> for (i = startblock; i < numblocks; i++) {
>> int ret;
>>
>> BUG_ON(bd->options & NAND_BBT_NO_OOB);
>>
>> - ret = scan_block_fast(this, bd, from, buf, numpages);
>> + ret = scan_block_fast(this, bd, from, buf);
>> if (ret < 0)
>> return ret;
>>
>
>
>
>
> Thanks,
> Miquèl
>
Powered by blists - more mailing lists