| 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: <CRI01R.KF0NPTKK5WYV1@crapouillou.net>
Date: Fri, 15 Oct 2021 10:38:00 +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()
Hi,
Le ven., oct. 15 2021 at 11:35:15 +0200, Miquel Raynal
<miquel.raynal@...tlin.com> a écrit :
> Hi Paul,
>
>> >> */
>> >> >> >> /* 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.
>
> If the ooblayout prevents the BBM to be smatched I'm fine and this
> sentence should disappear because it's misleading.
>
>> >> >> >> + */
>> >> >> +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.
>
> read_oob and read_page both end up sending READ0 and READSTART so
> they do request the chip to perform an internal read twice. You
> need this only once. The call to nand_read_page_op() should be a
> nand_change_read_column() with no data requested.
OK.
>
>> >> >> /**
>> >> >> * 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.
>
> Why do you want to put it there is there is only one user?
But there are two users: davinci_nand.c and (with patch [3/3])
ingenic/ingenic_nand_drv.c.
-Paul
Powered by blists - more mailing lists