| 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: <OFDCB9EA90.C6F8EA4C-ON48258401.000981AF-48258401.000ED713@mxic.com.tw>
Date: Tue, 21 May 2019 10:42:06 +0800
From: masonccyang@...c.com.tw
To: "Miquel Raynal" <miquel.raynal@...tlin.com>
Cc: bbrezillon@...nel.org, computersforpeace@...il.com,
dwmw2@...radead.org, frieder.schrempf@...tron.de,
juliensu@...c.com.tw, linux-kernel@...r.kernel.org,
linux-mtd@...ts.infradead.org, marek.vasut@...il.com,
richard@....at, vigneshr@...com, zhengxunli@...c.com.tw
Subject: Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support
Hi Miquel,
> > Add support for Macronix NAND read retry.
> >
> > Macronix NANDs support specific read operation for data recovery,
> > which can be enabled/disabled with a SET/GET_FEATURE.
> > Driver checks byte 167 of Vendor Blocks in ONFI parameter page table
> > to see if this high-reliability function is supported.
> >
> > Signed-off-by: Mason Yang <masonccyang@...c.com.tw>
> > ---
> > drivers/mtd/nand/raw/nand_macronix.c | 57
++++++++++++++++++++++++++++++++++++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_macronix.c
b/drivers/mtd/nand/raw/
> nand_macronix.c
> > index e287e71..1a4dc92 100644
> > --- a/drivers/mtd/nand/raw/nand_macronix.c
> > +++ b/drivers/mtd/nand/raw/nand_macronix.c
> > @@ -17,6 +17,62 @@
> >
> > #include "internals.h"
> >
> > +#define MACRONIX_READ_RETRY_BIT BIT(0)
> > +#define MACRONIX_READ_RETRY_MODE 6
>
> Can you name this define MACRONIX_NUM_READ_RETRY_MODES?
okay, will fix.
>
> > +
> > +struct nand_onfi_vendor_macronix {
> > + u8 reserved[1];
>
> Do you need this "[1]" ?
okay, just u8 reserved;
>
> > + u8 reliability_func;
> > +} __packed;
> > +
> > +/*
> > + * Macronix NANDs support using SET/GET_FEATURES to enter/exit read
retry mode
> > + */
> > +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int
mode)
> > +{
> > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
> > + int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY;
> > +
> > + if (chip->parameters.supports_set_get_features &&
> > + test_bit(feature_addr, chip->parameters.set_feature_list) &&
> > + test_bit(feature_addr, chip->parameters.get_feature_list)) {
> > + feature[0] = mode;
> > + ret = nand_set_features(chip, feature_addr, feature);
> > + if (ret)
> > + pr_err("Failed to set read retry moded:%d\n", mode);
>
> Do you have to call nand_get_features() on error?
okay
>
> > +
> > + ret = nand_get_features(chip, feature_addr, feature);
> > + if (ret || feature[0] != mode)
> > + pr_err("Failed to verify read retry moded:%d(%d)\n",
> > + mode, feature[0]);
>
> if ret == 0 but feature[0] != mode, shouldn't you return an error?
okay, will fix.
>
> > + }
> > +
> > + return ret;
>
> This will produce a Warning at compile time (ret may be used
> uninitialized). Have you tested it?
Tool chain I used is "gcc-arm-linux-gnueabi" and no Warning at compile
time.
Patch it to:
----------------------------------------------------------------------------->
static int macronix_nand_setup_read_retry(struct nand_chip *chip, int
mode)
{
u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY;
if (chip->parameters.supports_set_get_features &&
test_bit(feature_addr, chip->parameters.set_feature_list) &&
test_bit(feature_addr, chip->parameters.get_feature_list)) {
feature[0] = mode;
ret = nand_set_features(chip, feature_addr, feature);
if (ret) {
pr_err("Failed to set read retry moded:%d\n",
mode);
goto err_out;
}
ret = nand_get_features(chip, feature_addr, feature);
if (ret) {
pr_err("Failed to get read retry moded:%d\n",
mode);
goto err_out;
} else if (feature[0] != mode) {
pr_err("Failed to verify read retry
moded:%d(%d)\n",
mode, feature[0]);
return -EIO;
}
}
err_out:
return ret;
}
-----------------------------------------------------------------------------<
thanks & best regards,
Mason
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.
Macronix International Co., Ltd.
=====================================================================
============================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.
Macronix International Co., Ltd.
=====================================================================
Powered by blists - more mailing lists