| 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: <89I01R.QTBARVYLTBT02@crapouillou.net>
Date: Fri, 15 Oct 2021 10:27:08 +0100
From: Paul Cercueil <paul@...pouillou.net>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: Richard Weinberger <richard@....at>,
Vignesh Raghavendra <vigneshr@...com>,
Harvey Hunt <harveyhuntnexus@...il.com>, list@...ndingux.net,
linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-mips@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()
Le ven., oct. 15 2021 at 10:51:46 +0200, Miquel Raynal
<miquel.raynal@...tlin.com> a écrit :
> Hi Paul,
>
> paul@...pouillou.net wrote on Fri, 15 Oct 2021 09:38:31 +0100:
>
>> Hi Miquel,
>>
>> Le ven., oct. 15 2021 at 08:13:13 +0200, Miquel Raynal
>> <miquel.raynal@...tlin.com> a écrit :
>> > Hi Paul,
>> >
>> > paul@...pouillou.net wrote on Sat, 9 Oct 2021 20:49:51 +0200:
>> >
>> >> Move the function nand_read_page_hwecc_oob_first() (previously
>> >> nand_davinci_read_page_hwecc_oob_first()) to nand_base.c, and
>> >> export it
>> >> as a GPL symbol, so that it can be used by more modules.
>> >> >> Cc: <stable@...r.kernel.org> # v5.2
>> >> Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for
>> the >> JZ4740")
>> >> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>> >> ---
>> >> drivers/mtd/nand/raw/davinci_nand.c | 70 >>
>> +----------------------------
>> >> drivers/mtd/nand/raw/nand_base.c | 69 >>
>> ++++++++++++++++++++++++++++
>> >> include/linux/mtd/rawnand.h | 2 +
>> >> 3 files changed, 72 insertions(+), 69 deletions(-)
>> >> >> diff --git a/drivers/mtd/nand/raw/davinci_nand.c >>
>> b/drivers/mtd/nand/raw/davinci_nand.c
>> >> index 89de24d3bb7a..45fec8c192ab 100644
>> >> --- a/drivers/mtd/nand/raw/davinci_nand.c
>> >> +++ b/drivers/mtd/nand/raw/davinci_nand.c
>> >> @@ -371,74 +371,6 @@ static int
>> nand_davinci_correct_4bit(struct >> nand_chip *chip, u_char *data,
>> >> return corrected;
>> >> }
>> >> >> -/**
>> >> - * nand_read_page_hwecc_oob_first - hw ecc, read oob first
>> >> - * @chip: nand chip info structure
>> >> - * @buf: buffer to store read data
>> >> - * @oob_required: caller requires OOB data read to
>> chip->oob_poi
>> >> - * @page: page number to read
>> >> - *
>> >> - * Hardware ECC for large page chips, require OOB to be read
>> >> first. For this
>> >> - * ECC mode, the write_page method is re-used from ECC_HW.
>> These >> methods
>> >> - * read/write ECC from the OOB area, unlike the
>> ECC_HW_SYNDROME >> support with
>> >> - * multiple ECC steps, follows the "infix ECC" scheme and >>
>> reads/writes ECC from
>> >> - * the data area, by overwriting the NAND manufacturer bad
>> block >> markings.
>> >> - */
>> >> -static int nand_davinci_read_page_hwecc_oob_first(struct
>> nand_chip >> *chip,
>> >> - uint8_t *buf,
>> >> - int oob_required, int page)
>> >> -{
>> >> - struct mtd_info *mtd = nand_to_mtd(chip);
>> >> - int i, eccsize = chip->ecc.size, ret;
>> >> - int eccbytes = chip->ecc.bytes;
>> >> - int eccsteps = chip->ecc.steps;
>> >> - uint8_t *p = buf;
>> >> - uint8_t *ecc_code = chip->ecc.code_buf;
>> >> - unsigned int max_bitflips = 0;
>> >> -
>> >> - /* Read the OOB area first */
>> >> - ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >>
>> mtd->oobsize);
>> >> - if (ret)
>> >> - return ret;
>> >> -
>> >> - ret = nand_read_page_op(chip, page, 0, NULL, 0);
>> >> - if (ret)
>> >> - return ret;
>> >> -
>> >> - ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code,
>> chip->oob_poi, 0,
>> >> - chip->ecc.total);
>> >> - if (ret)
>> >> - return ret;
>> >> -
>> >> - for (i = 0; eccsteps; eccsteps--, i += eccbytes, p +=
>> eccsize) {
>> >> - int stat;
>> >> -
>> >> - chip->ecc.hwctl(chip, NAND_ECC_READ);
>> >> -
>> >> - ret = nand_read_data_op(chip, p, eccsize, false, false);
>> >> - if (ret)
>> >> - return ret;
>> >> -
>> >> - stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
>> >> - if (stat == -EBADMSG &&
>> >> - (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
>> >> - /* check for empty pages with bitflips */
>> >> - stat = nand_check_erased_ecc_chunk(p, eccsize,
>> >> - &ecc_code[i],
>> >> - eccbytes, NULL, 0,
>> >> - chip->ecc.strength);
>> >> - }
>> >> -
>> >> - if (stat < 0) {
>> >> - mtd->ecc_stats.failed++;
>> >> - } else {
>> >> - mtd->ecc_stats.corrected += stat;
>> >> - max_bitflips = max_t(unsigned int, max_bitflips, stat);
>> >> - }
>> >> - }
>> >> - return max_bitflips;
>> >> -}
>> >> -
>> >> >>
>> /*----------------------------------------------------------------------
>> */
>> >> >> /* An ECC layout for using 4-bit ECC with small-page flash,
>> storing
>> >> @@ -648,7 +580,7 @@ static int davinci_nand_attach_chip(struct
>> >> nand_chip *chip)
>> >> } else if (chunks == 4 || chunks == 8) {
>> >> mtd_set_ooblayout(mtd,
>> >> nand_get_large_page_ooblayout());
>> >> - chip->ecc.read_page =
>> nand_davinci_read_page_hwecc_oob_first;
>> >> + chip->ecc.read_page = nand_read_page_hwecc_oob_first;
>> >> } else {
>> >> return -EIO;
>> >> }
>> >> diff --git a/drivers/mtd/nand/raw/nand_base.c >>
>> b/drivers/mtd/nand/raw/nand_base.c
>> >> index 3d6c6e880520..cb5f343b9fa2 100644
>> >> --- a/drivers/mtd/nand/raw/nand_base.c
>> >> +++ b/drivers/mtd/nand/raw/nand_base.c
>> >> @@ -3160,6 +3160,75 @@ static int nand_read_page_hwecc(struct
>> >> nand_chip *chip, uint8_t *buf,
>> >> return max_bitflips;
>> >> }
>> >> >> +/**
>> >> + * nand_read_page_hwecc_oob_first - Hardware ECC page read
>> with ECC
>> >> + * data read from OOB area
>> >> + * @chip: nand chip info structure
>> >> + * @buf: buffer to store read data
>> >> + * @oob_required: caller requires OOB data read to
>> chip->oob_poi
>> >> + * @page: page number to read
>> >> + *
>> >> + * Hardware ECC for large page chips, require OOB to be read
>> >> first. For this
>> >
>> > requires
>> >
>> > With this ECC configuration?
>> >
>> >> + * ECC mode, the write_page method is re-used from ECC_HW.
>> These >> methods
>> >
>> > I do not understand this sentence nor the next one about
>> syndrome. I
>> > believe it is related to your engine and should not leak into the
>> > core.
>> >
>> >> + * read/write ECC from the OOB area, unlike the
>> ECC_HW_SYNDROME >> support with
>> >> + * multiple ECC steps, follows the "infix ECC" scheme and >>
>> reads/writes ECC from
>> >> + * the data area, by overwriting the NAND manufacturer bad
>> block >> markings.
>> >
>> > That's a sentence I don't like. What do you mean exactly?
>> >
>> > What "Infix ECC" scheme is?
>> >
>> > Do you mean that unlike the syndrome mode it *does not*
>> overwrite the
>> > BBM ?
>>
>> I don't mean anything. I did not write that comment. I just moved
>> the function verbatim with no changes. If something needs to be
>> fixed, then it needs to be fixed before/after this patch.
>
> Well, this comment should be adapted because as-is I don't think it's
> wise to move it around.
OK.
I think it says that BBM can be overwritten with this configuration,
but that would be if the OOB layout covers the BBM area.
>>
>> >> + */
>> >> +int nand_read_page_hwecc_oob_first(struct nand_chip *chip,
>> uint8_t >> *buf,
>> >> + int oob_required, int page)
>> >> +{
>> >> + struct mtd_info *mtd = nand_to_mtd(chip);
>> >> + int i, eccsize = chip->ecc.size, ret;
>> >> + int eccbytes = chip->ecc.bytes;
>> >> + int eccsteps = chip->ecc.steps;
>> >> + uint8_t *p = buf;
>> >> + uint8_t *ecc_code = chip->ecc.code_buf;
>> >> + unsigned int max_bitflips = 0;
>> >> +
>> >> + /* Read the OOB area first */
>> >> + ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >>
>> mtd->oobsize);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + ret = nand_read_page_op(chip, page, 0, NULL, 0);
>> >
>> > Definitely not, your are requesting the chip to do the read_page
>> > operation twice. You only need a nand_change_read_column I
>> believe.
>>
>> Again, this code is just being moved around - don't shoot the
>> messenger :)
>
> haha
>
> Well, now you touch the core, so I need to be more careful, and the
> code is definitely wrong, so even if we don't move that code off, you
> definitely want to fix it in order to improve your performances.
I don't see the read_page being done twice?
There's one read_oob, one read_page, then read_data in the loop.
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code,
>> chip->oob_poi, 0,
>> >> + chip->ecc.total);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p +=
>> eccsize) {
>> >> + int stat;
>> >> +
>> >> + chip->ecc.hwctl(chip, NAND_ECC_READ);
>> >> +
>> >> + ret = nand_read_data_op(chip, p, eccsize, false, false);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
>> >> + if (stat == -EBADMSG &&
>> >> + (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
>> >> + /* check for empty pages with bitflips */
>> >> + stat = nand_check_erased_ecc_chunk(p, eccsize,
>> >> + &ecc_code[i],
>> >> + eccbytes, NULL, 0,
>> >> + chip->ecc.strength);
>> >> + }
>> >> +
>> >> + if (stat < 0) {
>> >> + mtd->ecc_stats.failed++;
>> >> + } else {
>> >> + mtd->ecc_stats.corrected += stat;
>> >> + max_bitflips = max_t(unsigned int, max_bitflips, stat);
>> >> + }
>> >> + }
>> >> + return max_bitflips;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(nand_read_page_hwecc_oob_first);
>> >> +
>> >> /**
>> >> * nand_read_page_syndrome - [REPLACEABLE] hardware ECC
>> syndrome >> based page read
>> >> * @chip: nand chip info structure
>> >> diff --git a/include/linux/mtd/rawnand.h >>
>> b/include/linux/mtd/rawnand.h
>> >> index b2f9dd3cbd69..5b88cd51fadb 100644
>> >> --- a/include/linux/mtd/rawnand.h
>> >> +++ b/include/linux/mtd/rawnand.h
>> >> @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct nand_chip
>> *chip, >> void *buf, unsigned int len,
>> >> bool force_8bit, bool check_only);
>> >> int nand_write_data_op(struct nand_chip *chip, const void *buf,
>> >> unsigned int len, bool force_8bit);
>> >> +int nand_read_page_hwecc_oob_first(struct nand_chip *chip,
>> uint8_t >> *buf,
>> >> + int oob_required, int page);
>> >
>> > You certainly want to add this symbol closer to the other
>> read/write
>> > page helpers?
>>
>> Where would that be? The other read/write page helpers are all
>> "static" so they don't appear in any header.
>
> I believe we should keep this header local as long as there are no
> other users.
I'll move it to internal.h then.
Cheers,
-Paul
Powered by blists - more mailing lists