| 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: <ee9e30c5c95e49a7b0a3692db7c85338@svr-chch-ex1.atlnz.lc>
Date: Wed, 20 Jun 2018 22:22:05 +0000
From: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To: Boris Brezillon <boris.brezillon@...tlin.com>
CC: "miquel.raynal@...tlin.com" <miquel.raynal@...tlin.com>,
"dwmw2@...radead.org" <dwmw2@...radead.org>,
"computersforpeace@...il.com" <computersforpeace@...il.com>,
"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Richard Weinberger <richard@....at>,
"Marek Vasut" <marek.vasut@...il.com>
Subject: Re: [PATCH v3 4/4] mtd: rawnand: micron: support 8/512 on-die ECC
On 20/06/18 20:02, Boris Brezillon wrote:
> On Wed, 20 Jun 2018 17:05:44 +1200
> Chris Packham <chris.packham@...iedtelesis.co.nz> wrote:
>
>> Micron MT29F1G08ABAFAWP-ITE:F supports an on-die ECC with 8 bits
>> per 512 bytes. Add support for this combination.
>>
>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
>> ---
>> Changes in v2:
>> - New
>> Changes in v3:
>> - Handle reporting of corrected errors that don't require a rewrite, expand
>> comment for the ECC status bits.
>>
>> drivers/mtd/nand/raw/nand_micron.c | 34 ++++++++++++++++++++++++------
>> 1 file changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
>> index 5cec79372181..0c2bde4411d7 100644
>> --- a/drivers/mtd/nand/raw/nand_micron.c
>> +++ b/drivers/mtd/nand/raw/nand_micron.c
>> @@ -18,10 +18,24 @@
>> #include <linux/mtd/rawnand.h>
>>
>> /*
>> - * Special Micron status bit that indicates when the block has been
>> - * corrected by on-die ECC and should be rewritten
>> + * Special Micron status bit 3 indicates that the block has been
>> + * corrected by on-die ECC and should be rewritten.
>> + *
>> + * On chips with 8-bit ECC and additional bit can be used to distinguish
>> + * cases where a errors were corrected without needing a rewrite
>> + *
>> + * Bit 4 Bit 3 Bit 0 Description
>> + * ----- ----- ----- -----------
>> + * 0 0 0 No Errors
>> + * 0 0 1 Multiple uncorrected errors
>> + * 0 1 0 4 - 6 errors corrected, recommend rewrite
>> + * 0 0 1 Reserved
>> + * 1 0 0 1 - 3 errors corrected
>> + * 1 0 1 Reserved
>> + * 1 1 0 7 - 8 errors corrected, recommend rewrite
>> */
>> #define NAND_STATUS_WRITE_RECOMMENDED BIT(3)
>> +#define NAND_STATUS_ERRORS_CORRECTED BIT(4)
>>
>> struct nand_onfi_vendor_micron {
>> u8 two_plane_read;
>> @@ -141,7 +155,7 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>> mtd->ecc_stats.failed++;
>>
>> /*
>> - * The internal ECC doesn't tell us the number of bitflips
>> + * The internal 4-bit ECC doesn't tell us the number of bitflips
>> * that have been corrected, but tells us if it recommends to
>> * rewrite the block. If it's the case, then we pretend we had
>> * a number of bitflips equal to the ECC strength, which will
>> @@ -149,6 +163,12 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>> */
>> else if (status & NAND_STATUS_WRITE_RECOMMENDED)
>> max_bitflips = chip->ecc.strength;
>> + /*
>> + * Chips with 8-bit internal ECC do tell us if errors 1 to 3 bit
>> + * errors have been corrected without recommending a rewrite.
>> + */
>> + else if (status & NAND_STATUS_ERRORS_CORRECTED)
>> + max_bitflips = 3;
>
> Why not masking bit 3, 4 and 0 and having a switch-case block?
Mainly because the existing code was just checking bit 3 and that
happened to worked for my use-case.
I'm happy to re-work it as you've suggested can anyone point me at the
datasheet for a 4/512 on-die chip (or just the part number I can lookup
on mircon's site).
>
> Also, you should update ecc_stats.corrected (see the patch I just sent
> [1]).
>
Will do. I'll pull in your patch and base v4 on top of that.
>>
>> ret = nand_read_data_op(chip, buf, mtd->writesize, false);
>> if (!ret && oob_required)
>> @@ -240,9 +260,9 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
>>
>> /*
>> * Some Micron NANDs have an on-die ECC of 4/512, some other
>> - * 8/512. We only support the former.
>> + * 8/512.
>> */
>> - if (chip->ecc_strength_ds != 4)
>> + if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
>> return MICRON_ON_DIE_UNSUPPORTED;
I was thinking about removing this. The original code excluded 8/512 due
to lack of access to a chip that implements this, which I now have.
> Given that our on-die-support detection procedure is not reliable, I'd
> recommend changing the way we do it and instead base this detection
> logic on the model name (in the ONFI param page) or the READ_ID bytes.
>
The problem is I don't know an exhaustive list of IDs that this applies
to. I guess having a list of known IDs and falling back to the current
detection is probably the best approach unless Micron get back to us
with some other method of detecting these and determining if it is
forceably enabled.
>>
>> return MICRON_ON_DIE_SUPPORTED;
>> @@ -274,9 +294,9 @@ static int micron_nand_init(struct nand_chip *chip)
>> return -EINVAL;
>> }
>>
>> - chip->ecc.bytes = 8;
>> + chip->ecc.bytes = chip->ecc_strength_ds * 2;
>> chip->ecc.size = 512;
>> - chip->ecc.strength = 4;
>> + chip->ecc.strength = chip->ecc_strength_ds;
>> chip->ecc.algo = NAND_ECC_BCH;
>> chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
>> chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
>
> [1]http://patchwork.ozlabs.org/patch/932006/
>
Powered by blists - more mailing lists