| 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: <20180412084919.1ca7991d@xps13>
Date: Thu, 12 Apr 2018 08:49:19 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Abhishek Sahu <absahu@...eaurora.org>
Cc: Boris Brezillon <boris.brezillon@...e-electrons.com>,
Archit Taneja <architt@...eaurora.org>,
Richard Weinberger <richard@....at>,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
Marek Vasut <marek.vasut@...il.com>,
linux-mtd@...ts.infradead.org,
Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>,
Andy Gross <andy.gross@...aro.org>,
Brian Norris <computersforpeace@...il.com>,
David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH 3/9] mtd: nand: qcom: erased page detection for
uncorrectable errors only
Hi Abhishek,
On Thu, 12 Apr 2018 12:03:58 +0530, Abhishek Sahu
<absahu@...eaurora.org> wrote:
> On 2018-04-10 14:29, Miquel Raynal wrote:
> > Hi Abhishek,
> > > On Wed, 4 Apr 2018 18:12:19 +0530, Abhishek Sahu
> > <absahu@...eaurora.org> wrote:
> > >> The NAND flash controller generates ECC uncorrectable error
> >> first in case of completely erased page. Currently driver
> >> applies the erased page detection logic for other operation
> >> errors also so fix this and return EIO for other operational
> >> errors.
> > > I am sorry I don't understand very well what is the purpose of this
> > patch, could you please explain it again?
> > > Do you mean that you want to avoid having rising ECC errors when you
> > read erased pages?
> > Thanks Miquel for your review.
>
> QCOM NAND flash controller has in built erased page
> detection HW.
> Following is the flow in the HW if controller tries
> to read erased page
>
> 1. First ECC uncorrectable error will be generated from
> ECC engine since ECC engine first calculates the ECC with
> all 0xff and match the calculated ECC with ECC code in OOB
> (which is again all 0xff).
> 2. After getting ECC error, erased CW detection HW checks if
> all the bytes in page are 0xff and then it updates the
> status in separate register NAND_ERASED_CW_DETECT_STATUS
>
> So the erased CW detect status should be checked only if
> ECC engine generated the uncorrectable error.
>
> Currently for all other operational errors also (like TIMEOUT,
> MPU errors etc), the erased CW detect register was being
> checked.
This is very clear, thanks. I don't know very much this controller so I
think you can add this information in the commit message for future
reference.
>
> >> >> Signed-off-by: Abhishek Sahu <absahu@...eaurora.org>
> >> ---
> >> drivers/mtd/nand/qcom_nandc.c | 8 +++++++-
> >> 1 file changed, 7 insertions(+), 1 deletion(-)
> >> >> diff --git a/drivers/mtd/nand/qcom_nandc.c >> b/drivers/mtd/nand/qcom_nandc.c
> >> index 17321fc..57c16a6 100644
> >> --- a/drivers/mtd/nand/qcom_nandc.c
> >> +++ b/drivers/mtd/nand/qcom_nandc.c
> >> @@ -1578,6 +1578,7 @@ static int parse_read_errors(struct >> qcom_nand_host *host, u8 *data_buf,
> >> struct nand_ecc_ctrl *ecc = &chip->ecc;
> >> unsigned int max_bitflips = 0;
> >> struct read_stats *buf;
> >> + bool flash_op_err = false;
> >> int i;
> >> >> buf = (struct read_stats *)nandc->reg_read_buf;
> >> @@ -1599,7 +1600,7 @@ static int parse_read_errors(struct >> qcom_nand_host *host, u8 *data_buf,
> >> buffer = le32_to_cpu(buf->buffer);
> >> erased_cw = le32_to_cpu(buf->erased_cw);
> >> >> - if (flash & (FS_OP_ERR | FS_MPU_ERR)) {
> >> + if ((flash & FS_OP_ERR) && (buffer & BS_UNCORRECTABLE_BIT)) {
> > > And later you have another "if (buffer & BS_UNCORRECTABLE_BIT)" which
> > is then redundant, unless that is not what you actually want to do?
>
> Yes. That check seems to be redundant. I will fix that.
>
> > > Maybe you can add comments before the if ()/ else if () to explain in
> > which case you enter each branch.
>
> Sure. That would be better. Will add the same in next patch set.
>
> > >> bool erased;
> >> >> /* ignore erased codeword errors */
> >> @@ -1641,6 +1642,8 @@ static int parse_read_errors(struct >> qcom_nand_host *host, u8 *data_buf,
> >> max_t(unsigned int, max_bitflips, ret);
> >> }
> >> }
> >> + } else if (flash & (FS_OP_ERR | FS_MPU_ERR)) {
> >> + flash_op_err = true;
> >> } else {
> >> unsigned int stat;
> >> >> @@ -1654,6 +1657,9 @@ static int parse_read_errors(struct >> qcom_nand_host *host, u8 *data_buf,
> >> oob_buf += oob_len + ecc->bytes;
> >> }
> >> >> + if (flash_op_err)
> >> + return -EIO;
> >> +
> > > In you are propagating an error related to the controller, this is
> > fine, but I think you just want to raise the fact that a NAND
> > uncorrectable error occurred, in this case you should just increment
> > mtd->ecc_stats.failed and return 0 (returning max_bitflips here would > be
> > fine too has it would be 0 too).
>
> The flash_op_err will be for other operational errors only (like timeout,
> MPU error, device failure etc). For correctable errors,
>
> ret = nand_check_erased_ecc_chunk(data_buf,
> data_len, eccbuf, ecclen, oob_buf,
> extraooblen, ecc->strength);
Why do you need nand_check_erased_ecc_chunk() if the blank page check
is done in hw?
Thanks,
Miquèl
> if (ret < 0) {
> mtd->ecc_stats.failed++;
> } else {
> mtd->ecc_stats.corrected += ret;
>
> Already, it is incrementing mtd->ecc_stats.failed
>
> Thanks,
> Abhishek
--
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists