[PATCH 0/7] mtd: Change meaning of -EUCLEAN return code on reads

[PATCH 0/7] mtd: Change meaning of -EUCLEAN return code on reads

[Mike Dunn]

This is the latest attempt to address this issue (full description below).  I
didn't give it a version number because the approach has changed and these are
more than just reworked earlier patches.  The fundamental change is that
bitflips are now considered at the finer granularity of one region comprising an
ecc step, instead of a full page (writesize region).  Less significant issues
that were brought up in the course of discussion are also addressed.

This patchset addresses the problem of insufficient information being returned
by mtd_read() with regard to bit error corrections.  Currently -EUCLEAN is
returned if one or more bit errors were corrected during the course of the read
operation.  Higher layers like UBI use this return code as an indication that
the erase block may be degrading and should be considered as a candidate for
being marked as a bad block.  The problem is that high bit error rates are
common on more recent NAND flash devices with strong ecc algorithms.  Frequent
(but entirely normal) bit error corrections on these devices result in blocks
being incorrectly marked as bad.  On some devices, ubi/ubifs can not be reliably
used because of this.

This problem was discussed a while back [1][2][3], and a consensus of sorts was
reached for a solution, which these patches implement.  The recent addition of
the mtd api entry functions now make this solution practical (thanks Artem!).  A
quick description of each patch will provide a synopsis of the set:

(1) Fix how mtd->ecc_strength is calculated from nand->ecc.strength to reflect
    the fact that granularity is now at ecc step level (the two values are now
    the same).
(2) Fix a couple incorrect nand->ecc.strength values.
(3) Expose mtd->ecc_strength through sysfs (read-only).
(4) mtd->bitflip_threshold is added and exposed as read/write through sysfs.
    The drivers can initialize it, otherwise mtd initializes it to the default
    value of ecc_strength.  Users can change it through sysfs.
(5) The nand driver method _read_page() returns max_bitflips to the nand
    infrastructure code.  This is the maximum number of bitflip corrections that
    were made on any one region comprising an ecc step.
(6) Sanity checks added to nand_scan_tail() to ensure that drivers needing to
    set ecc strength do so.  (This includes all drivers using hardware ecc.)
(7) Absent a hard error, all drivers' mtd->_read() methods now return
    max_bitflips to mtd, and mtd_read() makes the decision of whether to return
    -EUCLEAN or 0. If max_bitflips >= bitflip_threshold, -EUCLEAN is returned.
    If bitflip_threshold == 0 (no ecc), make no comparison and return 0.

Of course reviews greatly appreciated.  Thanks!

P.S. It looks like some of these patches may conflict with others that may be
queued up, so let me know if they need to be reworked.

[1] http://lists.infradead.org/pipermail/linux-mtd/2011-December/038755.html
[2] http://lists.infradead.org/pipermail/linux-mtd/2011-December/038688.html
[3] http://lists.infradead.org/pipermail/linux-mtd/2011-December/038828.html

Mike Dunn (7):
  mtd; ecc_strength is at ecc step granularity
  mtd: nand: fix incorrect ecc strength values
  mtd: expose ecc_strength through sysfs
  mtd: bitflip threshold added to mtd_info and sysfs
  mtd: nand: read_page() returns max_bitflips
  mtd: nand: sanity checks of ecc strength in nand_scan_tail()
  mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

 Documentation/ABI/testing/sysfs-class-mtd |   48 +++++++++++++++
 drivers/mtd/devices/docg3.c               |    4 +-
 drivers/mtd/mtdcore.c                     |   57 +++++++++++++++++-
 drivers/mtd/mtdpart.c                     |   12 ++--
 drivers/mtd/nand/alauda.c                 |    4 +-
 drivers/mtd/nand/atmel_nand.c             |    9 ++-
 drivers/mtd/nand/bcm_umi_bch.c            |    4 +-
 drivers/mtd/nand/bcm_umi_nand.c           |    7 +--
 drivers/mtd/nand/cafe_nand.c              |    4 +-
 drivers/mtd/nand/denali.c                 |   10 ++-
 drivers/mtd/nand/docg4.c                  |    5 +-
 drivers/mtd/nand/fsl_elbc_nand.c          |   21 ++++--
 drivers/mtd/nand/fsl_ifc_nand.c           |   10 +++-
 drivers/mtd/nand/fsmc_nand.c              |    9 ++-
 drivers/mtd/nand/jz4740_nand.c            |    6 +--
 drivers/mtd/nand/nand_base.c              |   95 ++++++++++++++++++++--------
 drivers/mtd/nand/sh_flctl.c               |    2 +-
 drivers/mtd/onenand/onenand_base.c        |    6 +-
 include/linux/mtd/mtd.h                   |   11 +++-
 include/linux/mtd/nand.h                  |    3 +
 20 files changed, 254 insertions(+), 73 deletions(-)

-- 
1.7.3.4

[PATCH 1/7] mtd: ecc_strength is at ecc step granularity

[Mike Dunn]

ecc_strength element of mtd_info will be the strength of one ecc step, not of
the entire writesize, as was previously planned.  This is the appropriate way
because, as was pointed out [1], bit errors in excess of the strength of one
step can cause a hard error if they all occur within the same ecc region.

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040313.html

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/nand/nand_base.c |    2 +-
 include/linux/mtd/mtd.h      |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index eb88d8b..c2d2e93 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3484,7 +3484,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 
 	/* propagate ecc info to mtd_info */
 	mtd->ecclayout = chip->ecc.layout;
-	mtd->ecc_strength = chip->ecc.strength * chip->ecc.steps;
+	mtd->ecc_strength = chip->ecc.strength;
 
 	/* Check, if we should skip the bad block table scan */
 	if (chip->options & NAND_SKIP_BBTSCAN)
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index cf5ea8c..cd0119d 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -164,7 +164,7 @@ struct mtd_info {
 	/* ECC layout structure pointer - read only! */
 	struct nand_ecclayout *ecclayout;
 
-	/* max number of correctible bit errors per writesize */
+	/* max number of correctible bit errors per ecc step */
 	unsigned int ecc_strength;
 
 	/* Data for variable erase regions. If numeraseregions is zero,
-- 
1.7.3.4

[PATCH 2/7] mtd: nand: fix incorrect ecc strength values

[Mike Dunn]

This fixes a couple of ecc strength values for which I earlier made conservative
guesses, but whose correct values were later determined [1] (thanks Ivan).  Also
sets strength for fsl_ifc_nand, which was merged to mainline after the original
patch that set the strength for all drivers.

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040325.html

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/nand/bcm_umi_nand.c |    7 +------
 drivers/mtd/nand/fsl_ifc_nand.c |    1 +
 drivers/mtd/nand/jz4740_nand.c  |    6 +-----
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/bcm_umi_nand.c b/drivers/mtd/nand/bcm_umi_nand.c
index 6908cdd..7134adf 100644
--- a/drivers/mtd/nand/bcm_umi_nand.c
+++ b/drivers/mtd/nand/bcm_umi_nand.c
@@ -476,12 +476,7 @@ static int __devinit bcm_umi_nand_probe(struct platform_device *pdev)
 		this->badblock_pattern = &largepage_bbt;
 	}
 
-	/*
-	 * FIXME: ecc strength value of 6 bits per 512 bytes of data is a
-	 * conservative guess, given 13 ecc bytes and using bch alg.
-	 * (Assume Galois field order m=15 to allow a margin of error.)
-	 */
-	this->ecc.strength = 6;
+	this->ecc.strength = 8;
 
 #endif
 
diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 5387cec..872cc96 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -821,6 +821,7 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
 	/* Hardware generates ECC per 512 Bytes */
 	chip->ecc.size = 512;
 	chip->ecc.bytes = 8;
+	chip->ecc.strength = 4;
 
 	switch (csor & CSOR_NAND_PGS_MASK) {
 	case CSOR_NAND_PGS_512:
diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c
index e4147e8..a6fa884 100644
--- a/drivers/mtd/nand/jz4740_nand.c
+++ b/drivers/mtd/nand/jz4740_nand.c
@@ -332,11 +332,7 @@ static int __devinit jz_nand_probe(struct platform_device *pdev)
 	chip->ecc.mode		= NAND_ECC_HW_OOB_FIRST;
 	chip->ecc.size		= 512;
 	chip->ecc.bytes		= 9;
-	chip->ecc.strength	= 2;
-	/*
-	 * FIXME: ecc_strength value of 2 bits per 512 bytes of data is a
-	 * conservative guess, given 9 ecc bytes and reed-solomon alg.
-	 */
+	chip->ecc.strength	= 4;
 
 	if (pdata)
 		chip->ecc.layout = pdata->ecc_layout;
-- 
1.7.3.4

Re: [PATCH 2/7] mtd: nand: fix incorrect ecc strength values

[Jiandong Zheng]

On 4/24/2012 12:18 PM, Mike Dunn wrote:
> This fixes a couple of ecc strength values for which I earlier made conservative
> guesses, but whose correct values were later determined [1] (thanks Ivan).  Also
> sets strength for fsl_ifc_nand, which was merged to mainline after the original
> patch that set the strength for all drivers.
>
> [1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040325.html
>
> Signed-off-by: Mike Dunn<mikedunn@newsguy.com>
> ---
>   drivers/mtd/nand/bcm_umi_nand.c |    7 +------
>   drivers/mtd/nand/fsl_ifc_nand.c |    1 +
>   drivers/mtd/nand/jz4740_nand.c  |    6 +-----
>   3 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mtd/nand/bcm_umi_nand.c b/drivers/mtd/nand/bcm_umi_nand.c
> index 6908cdd..7134adf 100644
> --- a/drivers/mtd/nand/bcm_umi_nand.c
> +++ b/drivers/mtd/nand/bcm_umi_nand.c
> @@ -476,12 +476,7 @@ static int __devinit bcm_umi_nand_probe(struct platform_device *pdev)
>   		this->badblock_pattern =&largepage_bbt;
>   	}
>
> -	/*
> -	 * FIXME: ecc strength value of 6 bits per 512 bytes of data is a
> -	 * conservative guess, given 13 ecc bytes and using bch alg.
> -	 * (Assume Galois field order m=15 to allow a margin of error.)
> -	 */
> -	this->ecc.strength = 6;
> +	this->ecc.strength = 8;
>
>   #endif
>
>
Acked-by: Jiandong Zheng <jdzheng@broadcom.com>

[PATCH 3/7] mtd: expose ecc_strength through sysfs

[Mike Dunn]

ecc_strength element of struct mtd_info is exposed as a read-only variable in
sysfs.  

This patch is unchanged from its earlier version, except for the wording in the
documentation to reflect the fact that it now applies to each ecc step.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 Documentation/ABI/testing/sysfs-class-mtd |   12 ++++++++++++
 drivers/mtd/mtdcore.c                     |   10 ++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd
index 4d55a18..43d1818 100644
--- a/Documentation/ABI/testing/sysfs-class-mtd
+++ b/Documentation/ABI/testing/sysfs-class-mtd
@@ -123,3 +123,15 @@ Description:
 		half page, or a quarter page).
 
 		In the case of ECC NOR, it is the ECC block size.
+
+What:		/sys/class/mtd/mtdX/ecc_strength
+Date:		April 2012
+KernelVersion:	3.4
+Contact:	linux-mtd@lists.infradead.org
+Description:
+		Maximum number of bit errors that the device is capable of
+		correcting within each region covering an ecc step.  This will
+		always be a non-negative integer.  Note that some devices will
+		have multiple ecc steps within each writesize region.
+
+		In the case of devices lacking any ECC capability, it is 0.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index c837507..090e849 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -250,6 +250,15 @@ static ssize_t mtd_name_show(struct device *dev,
 }
 static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
 
+static ssize_t mtd_ecc_strength_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", mtd->ecc_strength);
+}
+static DEVICE_ATTR(ecc_strength, S_IRUGO, mtd_ecc_strength_show, NULL);
+
 static struct attribute *mtd_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_flags.attr,
@@ -260,6 +269,7 @@ static struct attribute *mtd_attrs[] = {
 	&dev_attr_oobsize.attr,
 	&dev_attr_numeraseregions.attr,
 	&dev_attr_name.attr,
+	&dev_attr_ecc_strength.attr,
 	NULL,
 };
 
-- 
1.7.3.4

[PATCH 4/7] mtd: bitflip threshold added to mtd_info and sysfs

[Mike Dunn]

An element 'bitflip_threshold' is added to struct mtd_info, and also exposed as
a read/write variable in sysfs.  This will be used to determine whether or not
mtd_read() returns -EUCLEAN or 0 (absent a hard error).  If the driver leaves it
as zero, mtd will set it to a default value of ecc_strength.  

This patch is unchanged from its earlier version, except for the wording in the
documentation to reflect the fact that it now applies to each ecc step, as well
as changes suggested during discussion [1].

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040331.html

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 Documentation/ABI/testing/sysfs-class-mtd |   36 +++++++++++++++++++++++++++++
 drivers/mtd/mtdcore.c                     |   33 ++++++++++++++++++++++++++
 include/linux/mtd/mtd.h                   |    9 +++++++
 3 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd
index 43d1818..7883508 100644
--- a/Documentation/ABI/testing/sysfs-class-mtd
+++ b/Documentation/ABI/testing/sysfs-class-mtd
@@ -135,3 +135,39 @@ Description:
 		have multiple ecc steps within each writesize region.
 
 		In the case of devices lacking any ECC capability, it is 0.
+
+What:		/sys/class/mtd/mtdX/bitflip_threshold
+Date:		April 2012
+KernelVersion:	3.4
+Contact:	linux-mtd@lists.infradead.org
+Description:
+		This allows the user to examine and adjust the criteria by which
+		mtd returns -EUCLEAN from mtd_read().  If the maximum number of
+		bit errors that were corrected on any single region comprising
+		an ecc step (as reported by the driver) equals or exceeds this
+		value, -EUCLEAN is returned.  Otherwise, absent an error, 0 is
+		returned.  Higher layers (e.g., UBI) use this return code as an
+		indication that an erase block may be degrading and should be
+		scrutinized as a candidate for being marked as bad.
+
+		The initial value may be specified by the flash device driver.
+		If not, then the default value is ecc_strength.
+
+		The introduction of this feature brings a subtle change to the
+		meaning of the -EUCLEAN return code.  Previously, it was
+		interpreted to mean simply "one or more bit errors were
+		corrected".  Its new interpretation can be phrased as "a
+		dangerously high number of bit errors were corrected on one or
+		more regions comprising an ecc step".  The precise definition of
+		"dangerously high" can be adjusted by the user with
+		bitflip_threshold.  Users are discouraged from doing this,
+		however, unless they know what they are doing and have intimate
+		knowledge of the properties of their device.  Broadly speaking,
+		bitflip_threshold should be low enough to detect genuine erase
+		block degradation, but high enough to avoid the consequences of
+		a persistent return value of -EUCLEAN on devices where sticky
+		bitflips occur.  Note that if bitflip_threshold exceeds
+		ecc_strength, -EUCLEAN is never returned by the read functions.
+
+		This is generally applicable only to NAND flash devices with ECC
+		capability.  It is ignored on devices lacking ECC capability.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 090e849..6a7cba1 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -259,6 +259,34 @@ static ssize_t mtd_ecc_strength_show(struct device *dev,
 }
 static DEVICE_ATTR(ecc_strength, S_IRUGO, mtd_ecc_strength_show, NULL);
 
+static ssize_t mtd_bitflip_threshold_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", mtd->bitflip_threshold);
+}
+
+static ssize_t mtd_bitflip_threshold_store(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t count)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	unsigned int bitflip_threshold;
+	int retval;
+
+	retval = kstrtouint(buf, 0, &bitflip_threshold);
+	if (retval)
+		return retval;
+
+	mtd->bitflip_threshold = bitflip_threshold;
+	return count;
+}
+static DEVICE_ATTR(bitflip_threshold, S_IRUGO | S_IWUSR,
+		   mtd_bitflip_threshold_show,
+		   mtd_bitflip_threshold_store);
+
 static struct attribute *mtd_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_flags.attr,
@@ -270,6 +298,7 @@ static struct attribute *mtd_attrs[] = {
 	&dev_attr_numeraseregions.attr,
 	&dev_attr_name.attr,
 	&dev_attr_ecc_strength.attr,
+	&dev_attr_bitflip_threshold.attr,
 	NULL,
 };
 
@@ -332,6 +361,10 @@ int add_mtd_device(struct mtd_info *mtd)
 	mtd->index = i;
 	mtd->usecount = 0;
 
+	/* default value if not set by driver */
+	if (mtd->bitflip_threshold == 0)
+		mtd->bitflip_threshold = mtd->ecc_strength;
+
 	if (is_power_of_2(mtd->erasesize))
 		mtd->erasesize_shift = ffs(mtd->erasesize) - 1;
 	else
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index cd0119d..63dadc0 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -157,6 +157,15 @@ struct mtd_info {
 	unsigned int erasesize_mask;
 	unsigned int writesize_mask;
 
+	/*
+	 * read ops return -EUCLEAN if max number of bitflips corrected on any
+	 * one region comprising an ecc step equals or exceeds this value.
+	 * Settable by driver, else defaults to ecc_strength.  User can override
+	 * in sysfs.  N.B. The meaning of the -EUCLEAN return code has changed;
+	 * see Documentation/ABI/testing/sysfs-class-mtd for more detail.
+	 */
+	unsigned int bitflip_threshold;
+
 	// Kernel-only stuff starts here.
 	const char *name;
 	int index;
-- 
1.7.3.4

[PATCH 5/7] mtd: nand: read_page() returns max_bitflips

[Mike Dunn]

The ecc.read_page() method for nand drivers is changed to return the maximum
number of bitflips that were corrected on any one region covering an ecc step,
This patch doesn't change what the nand code returns to mtd.

This patch wasn't necessary in the earlier attempt at this effort because when
max_bitflips referred to the entire page, the number of corrected bitflips could
be obtained from the ecc_stats by the nand infrastructure code.

This basically follows Schmulik's suggested approach [1].  It was tested only on
the docg4, but all drivers were compile-tested with the appropriate
cross-compiler.

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040343.html

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---

Most of the changes to the drivers are straightforward, but a few really need
review.  The changes to the freescale controllers (fsl_elbc_nand and
fsl_ifc_nand) are a little more involved.  I am least confident about the denali
driver... I confess I am totally confused by the "multi connected nand support"
patch (commit 08b9ab9996c7e582f86da319f43d2dcb8ff55993).

 drivers/mtd/nand/atmel_nand.c    |    9 +++++--
 drivers/mtd/nand/bcm_umi_bch.c   |    4 ++-
 drivers/mtd/nand/cafe_nand.c     |    4 ++-
 drivers/mtd/nand/denali.c        |   10 +++++--
 drivers/mtd/nand/docg4.c         |    5 ++-
 drivers/mtd/nand/fsl_elbc_nand.c |   21 +++++++++++-----
 drivers/mtd/nand/fsl_ifc_nand.c  |    9 ++++++-
 drivers/mtd/nand/fsmc_nand.c     |    9 +++++--
 drivers/mtd/nand/nand_base.c     |   47 +++++++++++++++++++++++++-------------
 drivers/mtd/nand/sh_flctl.c      |    2 +-
 10 files changed, 82 insertions(+), 38 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 2165576..9a7876e 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -335,6 +335,7 @@ static int atmel_nand_read_page(struct mtd_info *mtd,
 	uint8_t *oob = chip->oob_poi;
 	uint8_t *ecc_pos;
 	int stat;
+	unsigned int max_bitflips = 0;
 
 	/*
 	 * Errata: ALE is incorrectly wired up to the ECC controller
@@ -371,10 +372,12 @@ static int atmel_nand_read_page(struct mtd_info *mtd,
 	/* check if there's an error */
 	stat = chip->ecc.correct(mtd, p, oob, NULL);
 
-	if (stat < 0)
+	if (stat < 0) {
 		mtd->ecc_stats.failed++;
-	else
+	} else {
 		mtd->ecc_stats.corrected += stat;
+		max_bitflips = max_t(unsigned int, max_bitflips, stat);
+	}
 
 	/* get back to oob start (end of page) */
 	chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize, -1);
@@ -382,7 +385,7 @@ static int atmel_nand_read_page(struct mtd_info *mtd,
 	/* read the oob */
 	chip->read_buf(mtd, oob, mtd->oobsize);
 
-	return 0;
+	return max_bitflips;
 }
 
 /*
diff --git a/drivers/mtd/nand/bcm_umi_bch.c b/drivers/mtd/nand/bcm_umi_bch.c
index a930666..f8472b4 100644
--- a/drivers/mtd/nand/bcm_umi_bch.c
+++ b/drivers/mtd/nand/bcm_umi_bch.c
@@ -116,6 +116,7 @@ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd,
 	uint8_t eccCalc[NAND_ECC_NUM_BYTES];
 	int sectorOobSize = mtd->oobsize / eccsteps;
 	int stat;
+	unsigned int max_bitflips = 0;
 
 	for (sectorIdx = 0; sectorIdx < eccsteps;
 			sectorIdx++, datap += eccsize) {
@@ -177,9 +178,10 @@ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd,
 			}
 #endif
 			mtd->ecc_stats.corrected += stat;
+			max_bitflips = max_t(unsigned int, max_bitflips, stat);
 		}
 	}
-	return 0;
+	return max_bitflips;
 }
 
 /****************************************************************************
diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
index 2973d97..886ff3a 100644
--- a/drivers/mtd/nand/cafe_nand.c
+++ b/drivers/mtd/nand/cafe_nand.c
@@ -383,6 +383,7 @@ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 			       uint8_t *buf, int page)
 {
 	struct cafe_priv *cafe = mtd->priv;
+	unsigned int max_bitflips = 0;
 
 	cafe_dev_dbg(&cafe->pdev->dev, "ECC result %08x SYN1,2 %08x\n",
 		     cafe_readl(cafe, NAND_ECC_RESULT),
@@ -449,10 +450,11 @@ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 		} else {
 			dev_dbg(&cafe->pdev->dev, "Corrected %d symbol errors\n", n);
 			mtd->ecc_stats.corrected += n;
+			max_bitflips = max_t(unsigned int, max_bitflips, n);
 		}
 	}
 
-	return 0;
+	return max_bitflips;
 }
 
 static struct nand_ecclayout cafe_oobinfo_2048 = {
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index a1048c7..1b34647 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -924,9 +924,10 @@ bool is_erased(uint8_t *buf, int len)
 #define ECC_LAST_ERR(x)		((x) & ERR_CORRECTION_INFO__LAST_ERR_INFO)
 
 static bool handle_ecc(struct denali_nand_info *denali, uint8_t *buf,
-					uint32_t irq_status)
+		       uint32_t irq_status, unsigned int *max_bitflips)
 {
 	bool check_erased_page = false;
+	unsigned int bitflips = 0;
 
 	if (irq_status & INTR_STATUS__ECC_ERR) {
 		/* read the ECC errors. we'll ignore them for now */
@@ -965,6 +966,7 @@ static bool handle_ecc(struct denali_nand_info *denali, uint8_t *buf,
 					/* correct the ECC error */
 					buf[offset] ^= err_correction_value;
 					denali->mtd.ecc_stats.corrected++;
+					bitflips++;
 				}
 			} else {
 				/* if the error is not correctable, need to
@@ -984,6 +986,7 @@ static bool handle_ecc(struct denali_nand_info *denali, uint8_t *buf,
 		clear_interrupts(denali);
 		denali_set_intr_modes(denali, true);
 	}
+	*max_bitflips = bitflips;
 	return check_erased_page;
 }
 
@@ -1121,6 +1124,7 @@ static int denali_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 			    uint8_t *buf, int page)
 {
+	unsigned int max_bitflips;
 	struct denali_nand_info *denali = mtd_to_denali(mtd);
 
 	dma_addr_t addr = denali->buf.dma_buf;
@@ -1153,7 +1157,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 
 	memcpy(buf, denali->buf.buf, mtd->writesize);
 
-	check_erased_page = handle_ecc(denali, buf, irq_status);
+	check_erased_page = handle_ecc(denali, buf, irq_status, &max_bitflips);
 	denali_enable_dma(denali, false);
 
 	if (check_erased_page) {
@@ -1167,7 +1171,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 				denali->mtd.ecc_stats.failed++;
 		}
 	}
-	return 0;
+	return max_bitflips;
 }
 
 static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c
index b082026..8e7da01 100644
--- a/drivers/mtd/nand/docg4.c
+++ b/drivers/mtd/nand/docg4.c
@@ -720,6 +720,7 @@ static int read_page(struct mtd_info *mtd, struct nand_chip *nand,
 	struct docg4_priv *doc = nand->priv;
 	void __iomem *docptr = doc->virtadr;
 	uint16_t status, edc_err, *buf16;
+	int bits_corrected = 0;
 
 	dev_dbg(doc->dev, "%s: page %08x\n", __func__, page);
 
@@ -772,7 +773,7 @@ static int read_page(struct mtd_info *mtd, struct nand_chip *nand,
 
 		/* If bitflips are reported, attempt to correct with ecc */
 		if (edc_err & DOC_ECCCONF1_BCH_SYNDROM_ERR) {
-			int bits_corrected = correct_data(mtd, buf, page);
+			bits_corrected = correct_data(mtd, buf, page);
 			if (bits_corrected == -EBADMSG)
 				mtd->ecc_stats.failed++;
 			else
@@ -781,7 +782,7 @@ static int read_page(struct mtd_info *mtd, struct nand_chip *nand,
 	}
 
 	writew(0, docptr + DOC_DATAEND);
-	return 0;
+	return bits_corrected;
 }
 
 
diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
index 80b5264..8638b5e 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
@@ -75,6 +75,7 @@ struct fsl_elbc_fcm_ctrl {
 	unsigned int use_mdr;    /* Non zero if the MDR is to be set      */
 	unsigned int oob;        /* Non zero if operating on OOB data     */
 	unsigned int counter;	 /* counter for the initializations	  */
+	unsigned int max_bitflips;  /* Saved during READ0 cmd		  */
 };
 
 /* These map to the positions used by the FCM hardware ECC generator */
@@ -253,6 +254,8 @@ static int fsl_elbc_run_command(struct mtd_info *mtd)
 	if (chip->ecc.mode != NAND_ECC_HW)
 		return 0;
 
+	elbc_fcm_ctrl->max_bitflips = 0;
+
 	if (elbc_fcm_ctrl->read_bytes == mtd->writesize + mtd->oobsize) {
 		uint32_t lteccr = in_be32(&lbc->lteccr);
 		/*
@@ -262,11 +265,16 @@ static int fsl_elbc_run_command(struct mtd_info *mtd)
 		 * bits 28-31 are uncorrectable errors, marked elsewhere.
 		 * for small page nand only 1 bit is used.
 		 * if the ELBC doesn't have the lteccr register it reads 0
+		 * FIXME: 4 bits can be corrected on NANDs with 2k pages, so
+		 * count the number of sub-pages with bitflips and update
+		 * ecc_stats.corrected accordingly.
 		 */
 		if (lteccr & 0x000F000F)
 			out_be32(&lbc->lteccr, 0x000F000F); /* clear lteccr */
-		if (lteccr & 0x000F0000)
+		if (lteccr & 0x000F0000) {
 			mtd->ecc_stats.corrected++;
+			elbc_fcm_ctrl->max_bitflips = 1;
+		}
 	}
 
 	return 0;
@@ -743,13 +751,17 @@ static int fsl_elbc_read_page(struct mtd_info *mtd,
 			      uint8_t *buf,
 			      int page)
 {
+	struct fsl_elbc_mtd *priv = chip->priv;
+	struct fsl_lbc_ctrl *ctrl = priv->ctrl;
+	struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = ctrl->nand;
+
 	fsl_elbc_read_buf(mtd, buf, mtd->writesize);
 	fsl_elbc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
 
 	if (fsl_elbc_wait(mtd, chip) & NAND_STATUS_FAIL)
 		mtd->ecc_stats.failed++;
 
-	return 0;
+	return elbc_fcm_ctrl->max_bitflips;
 }
 
 /* ECC will be calculated automatically, and errors will be detected in
@@ -814,11 +826,6 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
 		chip->ecc.size = 512;
 		chip->ecc.bytes = 3;
 		chip->ecc.strength = 1;
-		/*
-		 * FIXME: can hardware ecc correct 4 bitflips if page size is
-		 * 2k?  Then does hardware report number of corrections for this
-		 * case?  If so, ecc_stats reporting needs to be fixed as well.
-		 */
 	} else {
 		/* otherwise fall back to default software ECC */
 		chip->ecc.mode = NAND_ECC_SOFT;
diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 872cc96..df36037 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -63,6 +63,7 @@ struct fsl_ifc_nand_ctrl {
 	unsigned int oob;	/* Non zero if operating on OOB data	*/
 	unsigned int eccread;	/* Non zero for a full-page ECC read	*/
 	unsigned int counter;	/* counter for the initializations	*/
+	unsigned int max_bitflips;  /* Saved during READ0 cmd		*/
 };
 
 static struct fsl_ifc_nand_ctrl *ifc_nand_ctrl;
@@ -268,6 +269,8 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
 		int sector = bufnum * chip->ecc.steps;
 		int sector_end = sector + chip->ecc.steps - 1;
 
+		nctrl->max_bitflips = 0;
+
 		for (i = sector / 4; i <= sector_end / 4; i++)
 			eccstat[i] = in_be32(&ifc->ifc_nand.nand_eccstat[i]);
 
@@ -290,6 +293,9 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
 			}
 
 			mtd->ecc_stats.corrected += errors;
+			nctrl->max_bitflips = max_t(unsigned int,
+						    nctrl->max_bitflips,
+						    errors);
 		}
 
 		nctrl->eccread = 0;
@@ -698,6 +704,7 @@ static int fsl_ifc_read_page(struct mtd_info *mtd,
 {
 	struct fsl_ifc_mtd *priv = chip->priv;
 	struct fsl_ifc_ctrl *ctrl = priv->ctrl;
+	struct fsl_ifc_nand_ctrl *nctrl = ifc_nand_ctrl;
 
 	fsl_ifc_read_buf(mtd, buf, mtd->writesize);
 	fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
@@ -708,7 +715,7 @@ static int fsl_ifc_read_page(struct mtd_info *mtd,
 	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC)
 		mtd->ecc_stats.failed++;
 
-	return 0;
+	return nctrl->max_bitflips;
 }
 
 /* ECC will be calculated automatically, and errors will be detected in
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index 1b8330e..1bf7f80 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -720,6 +720,7 @@ static int fsmc_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 	 */
 	uint16_t ecc_oob[7];
 	uint8_t *oob = (uint8_t *)&ecc_oob[0];
+	unsigned int max_bitflips = 0;
 
 	for (i = 0, s = 0; s < eccsteps; s++, i += eccbytes, p += eccsize) {
 		chip->cmdfunc(mtd, NAND_CMD_READ0, s * eccsize, page);
@@ -748,13 +749,15 @@ static int fsmc_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
 
 		stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
-		if (stat < 0)
+		if (stat < 0) {
 			mtd->ecc_stats.failed++;
-		else
+		} else {
 			mtd->ecc_stats.corrected += stat;
+			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+		}
 	}
 
-	return 0;
+	return max_bitflips;
 }
 
 /*
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index c2d2e93..3137abd 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1138,6 +1138,7 @@ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip,
 	uint8_t *ecc_calc = chip->buffers->ecccalc;
 	uint8_t *ecc_code = chip->buffers->ecccode;
 	uint32_t *eccpos = chip->ecc.layout->eccpos;
+	unsigned int max_bitflips = 0;
 
 	chip->ecc.read_page_raw(mtd, chip, buf, page);
 
@@ -1154,12 +1155,14 @@ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip,
 		int stat;
 
 		stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
-		if (stat < 0)
+		if (stat < 0) {
 			mtd->ecc_stats.failed++;
-		else
+		} else {
 			mtd->ecc_stats.corrected += stat;
+			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+		}
 	}
-	return 0;
+	return max_bitflips;
 }
 
 /**
@@ -1180,6 +1183,7 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	int datafrag_len, eccfrag_len, aligned_len, aligned_pos;
 	int busw = (chip->options & NAND_BUSWIDTH_16) ? 2 : 1;
 	int index = 0;
+	unsigned int max_bitflips = 0;
 
 	/* Column address within the page aligned to ECC size (256bytes) */
 	start_step = data_offs / chip->ecc.size;
@@ -1244,12 +1248,14 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 
 		stat = chip->ecc.correct(mtd, p,
 			&chip->buffers->ecccode[i], &chip->buffers->ecccalc[i]);
-		if (stat < 0)
+		if (stat < 0) {
 			mtd->ecc_stats.failed++;
-		else
+		} else {
 			mtd->ecc_stats.corrected += stat;
+			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+		}
 	}
-	return 0;
+	return max_bitflips;
 }
 
 /**
@@ -1271,6 +1277,7 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 	uint8_t *ecc_calc = chip->buffers->ecccalc;
 	uint8_t *ecc_code = chip->buffers->ecccode;
 	uint32_t *eccpos = chip->ecc.layout->eccpos;
+	unsigned int max_bitflips = 0;
 
 	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
 		chip->ecc.hwctl(mtd, NAND_ECC_READ);
@@ -1289,12 +1296,14 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 		int stat;
 
 		stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
-		if (stat < 0)
+		if (stat < 0) {
 			mtd->ecc_stats.failed++;
-		else
+		} else {
 			mtd->ecc_stats.corrected += stat;
+			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+		}
 	}
-	return 0;
+	return max_bitflips;
 }
 
 /**
@@ -1320,6 +1329,7 @@ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd,
 	uint8_t *ecc_code = chip->buffers->ecccode;
 	uint32_t *eccpos = chip->ecc.layout->eccpos;
 	uint8_t *ecc_calc = chip->buffers->ecccalc;
+	unsigned int max_bitflips = 0;
 
 	/* Read the OOB area first */
 	chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
@@ -1337,12 +1347,14 @@ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd,
 		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
 
 		stat = chip->ecc.correct(mtd, p, &ecc_code[i], NULL);
-		if (stat < 0)
+		if (stat < 0) {
 			mtd->ecc_stats.failed++;
-		else
+		} else {
 			mtd->ecc_stats.corrected += stat;
+			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+		}
 	}
-	return 0;
+	return max_bitflips;
 }
 
 /**
@@ -1363,6 +1375,7 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
 	int eccsteps = chip->ecc.steps;
 	uint8_t *p = buf;
 	uint8_t *oob = chip->oob_poi;
+	unsigned int max_bitflips = 0;
 
 	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
 		int stat;
@@ -1379,10 +1392,12 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->read_buf(mtd, oob, eccbytes);
 		stat = chip->ecc.correct(mtd, p, oob, NULL);
 
-		if (stat < 0)
+		if (stat < 0) {
 			mtd->ecc_stats.failed++;
-		else
+		} else {
 			mtd->ecc_stats.corrected += stat;
+			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+		}
 
 		oob += eccbytes;
 
@@ -1397,7 +1412,7 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
 	if (i)
 		chip->read_buf(mtd, oob, i);
 
-	return 0;
+	return max_bitflips;
 }
 
 /**
@@ -1588,7 +1603,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	if (oob)
 		ops->oobretlen = ops->ooblen - oobreadlen;
 
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	if (mtd->ecc_stats.failed - stats.failed)
diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index e9b2b26..4ea3e20 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -359,7 +359,7 @@ static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 		if (flctl->hwecc_cant_correct[i])
 			mtd->ecc_stats.failed++;
 		else
-			mtd->ecc_stats.corrected += 0;
+			mtd->ecc_stats.corrected += 0; /* FIXME */
 	}
 
 	return 0;
-- 
1.7.3.4

Re: [PATCH 5/7] mtd: nand: read_page() returns max_bitflips

[Scott Wood]

On 04/24/2012 02:18 PM, Mike Dunn wrote:
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index 872cc96..df36037 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -63,6 +63,7 @@ struct fsl_ifc_nand_ctrl {
>  	unsigned int oob;	/* Non zero if operating on OOB data	*/
>  	unsigned int eccread;	/* Non zero for a full-page ECC read	*/
>  	unsigned int counter;	/* counter for the initializations	*/
> +	unsigned int max_bitflips;  /* Saved during READ0 cmd		*/
>  };
>  
>  static struct fsl_ifc_nand_ctrl *ifc_nand_ctrl;
> @@ -268,6 +269,8 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
>  		int sector = bufnum * chip->ecc.steps;
>  		int sector_end = sector + chip->ecc.steps - 1;
>  
> +		nctrl->max_bitflips = 0;
> +
>  		for (i = sector / 4; i <= sector_end / 4; i++)
>  			eccstat[i] = in_be32(&ifc->ifc_nand.nand_eccstat[i]);
>  

I'd prefer to clear max_bitflips before this if-block, so that we can
rely on max_bitflips being zero when ECC wasn't requested.

Otherwise ACK the eLBC and IFC changes.

-Scott

Re: [PATCH 5/7] mtd: nand: read_page() returns max_bitflips

[Mike Dunn]

On 04/24/2012 12:45 PM, Scott Wood wrote:
> 
> I'd prefer to clear max_bitflips before this if-block, so that we can
> rely on max_bitflips being zero when ECC wasn't requested.
> 
> Otherwise ACK the eLBC and IFC changes.


Thanks for the ACK Scott.  I'll include your preference in the next version of
the patches.

Mike

Re: [PATCH 5/7] mtd: nand: read_page() returns max_bitflips

[Shmulik Ladkani]

Hi Mike,

On Tue, 24 Apr 2012 12:18:23 -0700 Mike Dunn <mikedunn@newsguy.com> wrote:
> The ecc.read_page() method for nand drivers is changed to return the maximum
> number of bitflips that were corrected on any one region covering an ecc step,
> This patch doesn't change what the nand code returns to mtd.
> 
> This patch wasn't necessary in the earlier attempt at this effort because when
> max_bitflips referred to the entire page, the number of corrected bitflips could
> be obtained from the ecc_stats by the nand infrastructure code.
> 
> This basically follows Schmulik's suggested approach [1].  It was tested only on
> the docg4, but all drivers were compile-tested with the appropriate
> cross-compiler.
> 
> [1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040343.html
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>

Reviewed nand_base.c, looks okay.

(though I wish we could avoid the max_bitflips calculation code
repetition)

Regards,
Shmulik

[PATCH 6/7] mtd: nand: sanity checks of ecc strength in nand_scan_tail()

[Mike Dunn]

This patch adds sanity checks that ensure that drivers for controllers with
hardware ECC set the 'strength' element in struct nand_ecc_ctrl.  Also stylistic
changes to the line that calculates strength for software ECC.  Both suggested
during discussion [1].

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040467.html

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/nand/nand_base.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 3137abd..275c43f 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3302,10 +3302,21 @@ int nand_scan_tail(struct mtd_info *mtd)
 				   "hardware ECC not possible\n");
 			BUG();
 		}
+
+		if (!chip->ecc.strength) {
+			pr_warn("Driver must set ecc.strength when using hardware ECC\n");
+			BUG();
+		}
+
 		if (!chip->ecc.read_page)
 			chip->ecc.read_page = nand_read_page_hwecc_oob_first;
 
 	case NAND_ECC_HW:
+		if (!chip->ecc.strength) {
+			pr_warn("Driver must set ecc.strength when using hardware ECC\n");
+			BUG();
+		}
+
 		/* Use standard hwecc read page function? */
 		if (!chip->ecc.read_page)
 			chip->ecc.read_page = nand_read_page_hwecc;
@@ -3345,12 +3356,17 @@ int nand_scan_tail(struct mtd_info *mtd)
 		if (!chip->ecc.write_oob)
 			chip->ecc.write_oob = nand_write_oob_syndrome;
 
-		if (mtd->writesize >= chip->ecc.size)
-			break;
-		pr_warn("%d byte HW ECC not possible on "
-			   "%d byte page size, fallback to SW ECC\n",
-			   chip->ecc.size, mtd->writesize);
+		if (mtd->writesize >= chip->ecc.size) {
+			if (!chip->ecc.strength) {
+				pr_warn("Driver must set ecc.strength when using hardware ECC\n");
+				BUG();
+			}
+			break;	/* all's well */
+		}
+		pr_warn("%d byte HW ECC not possible on %d byte page size, fallback to SW ECC\n",
+			chip->ecc.size, mtd->writesize);
 		chip->ecc.mode = NAND_ECC_SOFT;
+		/* fall through */
 
 	case NAND_ECC_SOFT:
 		chip->ecc.calculate = nand_calculate_ecc;
@@ -3401,7 +3417,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 			BUG();
 		}
 		chip->ecc.strength =
-			chip->ecc.bytes*8 / fls(8*chip->ecc.size);
+			chip->ecc.bytes * 8 / fls(8 * chip->ecc.size);
 		break;
 
 	case NAND_ECC_NONE:
-- 
1.7.3.4

Re: [PATCH 6/7] mtd: nand: sanity checks of ecc strength in nand_scan_tail()

[Brian Norris]

Hi Mike,

On Tue, Apr 24, 2012 at 12:18 PM, Mike Dunn <mikedunn@newsguy.com> wrote:
> This patch adds sanity checks that ensure that drivers for controllers with
> hardware ECC set the 'strength' element in struct nand_ecc_ctrl.  Also stylistic
> changes to the line that calculates strength for software ECC.  Both suggested
> during discussion [1].
>
> [1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040467.html

Thanks for addressing these issues! Note my comment below though.

> ---
>  drivers/mtd/nand/nand_base.c |   28 ++++++++++++++++++++++------
>  1 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 3137abd..275c43f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3302,10 +3302,21 @@ int nand_scan_tail(struct mtd_info *mtd)
>                                   "hardware ECC not possible\n");
>                        BUG();
>                }
> +
> +               if (!chip->ecc.strength) {
> +                       pr_warn("Driver must set ecc.strength when using hardware ECC\n");
> +                       BUG();
> +               }
> +
>                if (!chip->ecc.read_page)
>                        chip->ecc.read_page = nand_read_page_hwecc_oob_first;
>
>        case NAND_ECC_HW:
> +               if (!chip->ecc.strength) {
> +                       pr_warn("Driver must set ecc.strength when using hardware ECC\n");
> +                       BUG();
> +               }
> +
>                /* Use standard hwecc read page function? */
>                if (!chip->ecc.read_page)
>                        chip->ecc.read_page = nand_read_page_hwecc;
> @@ -3345,12 +3356,17 @@ int nand_scan_tail(struct mtd_info *mtd)
>                if (!chip->ecc.write_oob)
>                        chip->ecc.write_oob = nand_write_oob_syndrome;
>
> -               if (mtd->writesize >= chip->ecc.size)
> -                       break;
> -               pr_warn("%d byte HW ECC not possible on "
> -                          "%d byte page size, fallback to SW ECC\n",
> -                          chip->ecc.size, mtd->writesize);
> +               if (mtd->writesize >= chip->ecc.size) {
> +                       if (!chip->ecc.strength) {
> +                               pr_warn("Driver must set ecc.strength when using hardware ECC\n");
> +                               BUG();
> +                       }
> +                       break;  /* all's well */
> +               }
> +               pr_warn("%d byte HW ECC not possible on %d byte page size, fallback to SW ECC\n",
> +                       chip->ecc.size, mtd->writesize);
>                chip->ecc.mode = NAND_ECC_SOFT;
> +               /* fall through */
>
>        case NAND_ECC_SOFT:
>                chip->ecc.calculate = nand_calculate_ecc;

I think you only need this last check (under ECC_HW_SYNDROME), since
the ECC_HW and ECC_HW_OOB_FIRST cases necessarily fall through to
ECC_HW_SYNDROME case. (Note the lack of 'break;' statements in the
first two 'case's.)

Brian

Re: [PATCH 6/7] mtd: nand: sanity checks of ecc strength in nand_scan_tail()

[Mike Dunn]

On 04/24/2012 09:09 PM, Brian Norris wrote:
> 
> I think you only need this last check (under ECC_HW_SYNDROME), since
> the ECC_HW and ECC_HW_OOB_FIRST cases necessarily fall through to
> ECC_HW_SYNDROME case. (Note the lack of 'break;' statements in the
> first two 'case's.)


Oops... you're right, I did miss that.  This should have been a much simpler
patch.  Thanks!

Mike

[PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Mike Dunn]

The drivers _read() method, absent an error, return a non-negative integer
indicating the maximum number of bit errors that were corrected in any one
region comprising an ecc step.  MTD returns -EUCLEAN if this is >=
bitflip_threshold, 0 otherwise.  If bitflip_threshold is zero, the comparison is
not made since these devices lack ECC and always return zero in the non-error
case (thanks Brian) [1].  Note that this is a subtle change to the driver
interface.

This and the preceeding patches in this set were tested with ubi on top of the
nandsim and docg4 devices, running the ubi test io_basic from mtd-utils.

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040468.html

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/devices/docg3.c        |    4 +++-
 drivers/mtd/mtdcore.c              |   14 +++++++++++++-
 drivers/mtd/mtdpart.c              |   12 ++++++------
 drivers/mtd/nand/alauda.c          |    4 ++--
 drivers/mtd/nand/nand_base.c       |   18 ++++++++++++++----
 drivers/mtd/onenand/onenand_base.c |    6 ++++--
 include/linux/mtd/nand.h           |    3 +++
 7 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 3414031..7644d59 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -862,6 +862,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	u8 *buf = ops->datbuf;
 	size_t len, ooblen, nbdata, nboob;
 	u8 hwecc[DOC_ECC_BCH_SIZE], eccconf1;
+	int max_bitflips = 0;
 
 	if (buf)
 		len = ops->len;
@@ -948,7 +949,8 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 			}
 			if (ret > 0) {
 				mtd->ecc_stats.corrected += ret;
-				ret = -EUCLEAN;
+				max_bitflips = max(max_bitflips, ret);
+				ret = max_bitflips;
 			}
 		}
 
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 6a7cba1..b5bb628 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -800,12 +800,24 @@ EXPORT_SYMBOL_GPL(mtd_get_unmapped_area);
 int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
 	     u_char *buf)
 {
+	int ret_code;
 	*retlen = 0;
 	if (from < 0 || from > mtd->size || len > mtd->size - from)
 		return -EINVAL;
 	if (!len)
 		return 0;
-	return mtd->_read(mtd, from, len, retlen, buf);
+
+	/*
+	 * In the absence of an error, drivers return a non-negative integer
+	 * representing the maximum number of bitflips that were corrected on
+	 * any one ecc region (if applicable; zero otherwise).
+	 */
+	ret_code = mtd->_read(mtd, from, len, retlen, buf);
+	if (unlikely(ret_code < 0))
+		return ret_code;
+	if (mtd->bitflip_threshold == 0)
+		return 0;	/* device lacks ecc */
+	return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
 }
 EXPORT_SYMBOL_GPL(mtd_read);
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 9651c06..d6321f6 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -67,12 +67,12 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
 	stats = part->master->ecc_stats;
 	res = part->master->_read(part->master, from + part->offset, len,
 				  retlen, buf);
-	if (unlikely(res)) {
-		if (mtd_is_bitflip(res))
-			mtd->ecc_stats.corrected += part->master->ecc_stats.corrected - stats.corrected;
-		if (mtd_is_eccerr(res))
-			mtd->ecc_stats.failed += part->master->ecc_stats.failed - stats.failed;
-	}
+	if (unlikely(mtd_is_eccerr(res)))
+		mtd->ecc_stats.failed +=
+			part->master->ecc_stats.failed - stats.failed;
+	else
+		mtd->ecc_stats.corrected +=
+			part->master->ecc_stats.corrected - stats.corrected;
 	return res;
 }
 
diff --git a/drivers/mtd/nand/alauda.c b/drivers/mtd/nand/alauda.c
index 4f20e1d..60a0dfd 100644
--- a/drivers/mtd/nand/alauda.c
+++ b/drivers/mtd/nand/alauda.c
@@ -414,7 +414,7 @@ static int alauda_bounce_read(struct mtd_info *mtd, loff_t from, size_t len,
 	}
 	err = 0;
 	if (corrected)
-		err = -EUCLEAN;
+		err = 1;	/* return max_bitflips per ecc step */
 	if (uncorrected)
 		err = -EBADMSG;
 out:
@@ -446,7 +446,7 @@ static int alauda_read(struct mtd_info *mtd, loff_t from, size_t len,
 	}
 	err = 0;
 	if (corrected)
-		err = -EUCLEAN;
+		err = 1;	/* return max_bitflips per ecc step */
 	if (uncorrected)
 		err = -EBADMSG;
 	return err;
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 275c43f..9f1b202 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1486,6 +1486,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 		mtd->oobavail : mtd->oobsize;
 
 	uint8_t *bufpoi, *oob, *buf;
+	unsigned int max_bitflips = 0;
 
 	stats = mtd->ecc_stats;
 
@@ -1513,7 +1514,10 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 				sndcmd = 0;
 			}
 
-			/* Now read the page into the buffer */
+			/*
+			 * Now read the page into the buffer.  Absent an error,
+			 * the read methods return max bitflips per ecc step.
+			 */
 			if (unlikely(ops->mode == MTD_OPS_RAW))
 				ret = chip->ecc.read_page_raw(mtd, chip,
 							      bufpoi, page);
@@ -1530,15 +1534,19 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 				break;
 			}
 
+			max_bitflips = max_t(unsigned int, max_bitflips, ret);
+
 			/* Transfer not aligned data */
 			if (!aligned) {
 				if (!NAND_SUBPAGE_READ(chip) && !oob &&
 				    !(mtd->ecc_stats.failed - stats.failed) &&
-				    (ops->mode != MTD_OPS_RAW))
+				    (ops->mode != MTD_OPS_RAW)) {
 					chip->pagebuf = realpage;
-				else
+					chip->pagebuf_bitflips = ret;
+				} else {
 					/* Invalidate page cache */
 					chip->pagebuf = -1;
+				}
 				memcpy(buf, chip->buffers->databuf + col, bytes);
 			}
 
@@ -1571,6 +1579,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 		} else {
 			memcpy(buf, chip->buffers->databuf + col, bytes);
 			buf += bytes;
+			max_bitflips = max_t(unsigned int, max_bitflips,
+					     chip->pagebuf_bitflips);
 		}
 
 		readlen -= bytes;
@@ -1609,7 +1619,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	if (mtd->ecc_stats.failed - stats.failed)
 		return -EBADMSG;
 
-	return  mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
+	return max_bitflips;
 }
 
 /**
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index b3ce12e..7153e0d 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1201,7 +1201,8 @@ static int onenand_mlc_read_ops_nolock(struct mtd_info *mtd, loff_t from,
 	if (mtd->ecc_stats.failed - stats.failed)
 		return -EBADMSG;
 
-	return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
+	/* return max bitflips per ecc step; ONENANDs correct 1 bit only */
+	return mtd->ecc_stats.corrected != stats.corrected ? 1 : 0;
 }
 
 /**
@@ -1333,7 +1334,8 @@ static int onenand_read_ops_nolock(struct mtd_info *mtd, loff_t from,
 	if (mtd->ecc_stats.failed - stats.failed)
 		return -EBADMSG;
 
-	return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
+	/* return max bitflips per ecc step; ONENANDs correct 1 bit only */
+	return mtd->ecc_stats.corrected != stats.corrected ? 1 : 0;
 }
 
 /**
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 1482340..2829e8b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -459,6 +459,8 @@ struct nand_buffers {
  * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1
  * @pagebuf:		[INTERN] holds the pagenumber which is currently in
  *			data_buf.
+ * @pagebuf_bitflips:	[INTERN] holds the bitflip count for the page which is
+ *			currently in data_buf.
  * @subpagesize:	[INTERN] holds the subpagesize
  * @onfi_version:	[INTERN] holds the chip ONFI version (BCD encoded),
  *			non 0 if ONFI supported.
@@ -519,6 +521,7 @@ struct nand_chip {
 	uint64_t chipsize;
 	int pagemask;
 	int pagebuf;
+	unsigned int pagebuf_bitflips;
 	int subpagesize;
 	uint8_t cellinfo;
 	int badblockpos;
-- 
1.7.3.4

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Shmulik Ladkani]

Hi Mike,

Reviewed mtdcore.c, mtdpart.c and nand_base.c.
Looks very good.

Minor comments below.

On Tue, 24 Apr 2012 12:18:25 -0700 Mike Dunn <mikedunn@newsguy.com> wrote:
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 9651c06..d6321f6 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -67,12 +67,12 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	stats = part->master->ecc_stats;
>  	res = part->master->_read(part->master, from + part->offset, len,
>  				  retlen, buf);
> -	if (unlikely(res)) {
> -		if (mtd_is_bitflip(res))
> -			mtd->ecc_stats.corrected += part->master->ecc_stats.corrected - stats.corrected;
> -		if (mtd_is_eccerr(res))
> -			mtd->ecc_stats.failed += part->master->ecc_stats.failed - stats.failed;
> -	}
> +	if (unlikely(mtd_is_eccerr(res)))
> +		mtd->ecc_stats.failed +=
> +			part->master->ecc_stats.failed - stats.failed;
> +	else
> +		mtd->ecc_stats.corrected +=
> +			part->master->ecc_stats.corrected - stats.corrected;
>  	return res;
>  }

Probably need {} around the statements within the conditions.

Also I think there's no reason to execute "corrected +=" in case 'res'
is zero (although no harm).

Regards,
Shmulik

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Robert Jarzmik]

Mike Dunn <mikedunn@newsguy.com> writes:

One little trick with docg3 patch part:
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index 3414031..7644d59 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -948,7 +949,8 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
>  			}
>  			if (ret > 0) {
>  				mtd->ecc_stats.corrected += ret;
> -				ret = -EUCLEAN;
> +				max_bitflips = max(max_bitflips, ret);
> +				ret = max_bitflips;
>  			}
>  		}

If you do set ret here, the next loop you'll do in the following statement
"	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
0.

I think you should change :
>- 	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
into
>- 	while (ret >= 0 && (len > 0 || ooblen > 0)) {".

With that change, please add my:
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

-- 
Robert

PS: It's really a great work that bitflip serie

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Mike Dunn]

Thanks for the review Robert.

On 04/25/2012 11:27 AM, Robert Jarzmik wrote:
> 
> I think you should change :
>> - 	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
> into
>> - 	while (ret >= 0 && (len > 0 || ooblen > 0)) {".
> 
> With that change, please add my:
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>


In my exuberance, I prematurely sent the next version of the whole set.  Artem,
Robert's requested change is in patch 7/7.  If no other problems come to light,
maybe you could consider merging the first 6?  Then I only need to prepare a
corrected version of patch 7.

Thanks,
Mike

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

On Wed, 2012-04-25 at 12:13 -0700, Mike Dunn wrote:
> Thanks for the review Robert.
> 
> On 04/25/2012 11:27 AM, Robert Jarzmik wrote:
> > 
> > I think you should change :
> >> - 	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
> > into
> >> - 	while (ret >= 0 && (len > 0 || ooblen > 0)) {".
> > 
> > With that change, please add my:
> > Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 
> 
> In my exuberance, I prematurely sent the next version of the whole set.  Artem,
> Robert's requested change is in patch 7/7.  If no other problems come to light,
> maybe you could consider merging the first 6?  Then I only need to prepare a
> corrected version of patch 7.

Pushed the series to l2-mtd.git, thanks! You can send the newer version
and I'll put it instead of the older one. Do not forget to add
Acked-by's when you re-send, please.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Mike Dunn]

On 04/29/2012 12:24 PM, Artem Bityutskiy wrote:
> On Wed, 2012-04-25 at 12:13 -0700, Mike Dunn wrote:
>> Thanks for the review Robert.
>>
>> On 04/25/2012 11:27 AM, Robert Jarzmik wrote:
>>>
>>> I think you should change :
>>>> - 	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
>>> into
>>>> - 	while (ret >= 0 && (len > 0 || ooblen > 0)) {".
>>>
>>> With that change, please add my:
>>> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
>>
>>
>> In my exuberance, I prematurely sent the next version of the whole set.  Artem,
>> Robert's requested change is in patch 7/7.  If no other problems come to light,
>> maybe you could consider merging the first 6?  Then I only need to prepare a
>> corrected version of patch 7.
> 
> Pushed the series to l2-mtd.git, thanks! You can send the newer version
> and I'll put it instead of the older one. Do not forget to add
> Acked-by's when you re-send, please.
> 


Thanks Artem.  Since you merged all 7, the only thing remaining is the fix to
docg3 that Robert describes above.  Robert, if you would rather prepare that
patch, I defer to you.  Otherwise, I'll submit it no later than tomorrow.

Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.

Mike

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Robert Jarzmik]

Mike Dunn <mikedunn@newsguy.com> writes:

> Thanks Artem.  Since you merged all 7, the only thing remaining is the fix to
> docg3 that Robert describes above.  Robert, if you would rather prepare that
> patch, I defer to you.  Otherwise, I'll submit it no later than tomorrow.
Oh no, go ahead, you have my blessing.

> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just
> merged.
That's your opportunity to add it :)

Cheers.

-- 
Robert

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On Mon, 2012-04-30 at 12:55 -0700, Mike Dunn wrote:
> Thanks Artem.  Since you merged all 7, the only thing remaining is the fix to
> docg3 that Robert describes above.  Robert, if you would rather prepare that
> patch, I defer to you.  Otherwise, I'll submit it no later than tomorrow.
> 
> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.

If you know that an Acked-by or Reviewed-by was missed, let me know and
I'll add the tag(s) to the patch(es). I think it is important to be
careful about the tags because people spent their time helping to
improve the patches.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Mike Dunn]

On 05/01/2012 05:20 AM, Artem Bityutskiy wrote:
> On Mon, 2012-04-30 at 12:55 -0700, Mike Dunn wrote:
>>
>> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.
> 
> If you know that an Acked-by or Reviewed-by was missed, let me know and
> I'll add the tag(s) to the patch(es). I think it is important to be
> careful about the tags because people spent their time helping to
> improve the patches.


Well, I owe a debt to several reviewers, among them Brian Norris (caught the bug
that would cause all non-ecc devices to return -EUCLEAN), Shmulik Ladkani, Ivan
Djelic (reviews and tests), Scott Wood, Robert Jarzmik, yourself, ...

As far as formal tags... maybe these, if they don't object:

mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN
5568eb3c3bb2816281b6a7c04db92434b72b1495
Tested-by: Ivan Djelic <ivan.djelic@parrot.com>

mtd: nand: read_page() returns max_bitflips
6bf87bf989bfdfc78fb2c5cd55de4bab9b572992
Acked-by (freescale changes): Scott Wood <scottwood@freescale.com>

If the others feel a tag credit is appropriate, I certainly don't mind.

Thanks,
Mike

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Shmulik Ladkani]

On Tue, 01 May 2012 10:27:14 -0700 Mike Dunn <mikedunn@newsguy.com> wrote:
> On 05/01/2012 05:20 AM, Artem Bityutskiy wrote:
> > On Mon, 2012-04-30 at 12:55 -0700, Mike Dunn wrote:
> >>
> >> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.
> > 
> > If you know that an Acked-by or Reviewed-by was missed, let me know and
> > I'll add the tag(s) to the patch(es). I think it is important to be
> > careful about the tags because people spent their time helping to
> > improve the patches.
> 
> 
> Well, I owe a debt to several reviewers, among them Brian Norris (caught the bug
> that would cause all non-ecc devices to return -EUCLEAN), Shmulik Ladkani, Ivan
> Djelic (reviews and tests), Scott Wood, Robert Jarzmik, yourself, ...
> 
> As far as formal tags... maybe these, if they don't object:
> 
> mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN
> 5568eb3c3bb2816281b6a7c04db92434b72b1495
> Tested-by: Ivan Djelic <ivan.djelic@parrot.com>
> 
> mtd: nand: read_page() returns max_bitflips
> 6bf87bf989bfdfc78fb2c5cd55de4bab9b572992
> Acked-by (freescale changes): Scott Wood <scottwood@freescale.com>
> 
> If the others feel a tag credit is appropriate, I certainly don't mind.

On a side note (sorry for the off-topic post):

It seems the use of the formal tags is somewhat limited.

AFAIU, Acked-by is supposed to be specified by the maintainer or major
contributor, the one in charge of the code affected, and it could
be specified to just a part of the patch.

OTOH Reviewed-by can be specified by any interested reviewer, as an
indication that the entire patch has been reviewed.

However when patches get partially reviewed by interested parties, no
formal tag is suitable.

Regards,
Shmulik

Re: [PATCH 7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

On Tue, 2012-05-01 at 10:27 -0700, Mike Dunn wrote:
> On 05/01/2012 05:20 AM, Artem Bityutskiy wrote:
> > On Mon, 2012-04-30 at 12:55 -0700, Mike Dunn wrote:
> >>
> >> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.
> > 
> > If you know that an Acked-by or Reviewed-by was missed, let me know and
> > I'll add the tag(s) to the patch(es). I think it is important to be
> > careful about the tags because people spent their time helping to
> > improve the patches.
> 
> 
> Well, I owe a debt to several reviewers, among them Brian Norris (caught the bug
> that would cause all non-ecc devices to return -EUCLEAN), Shmulik Ladkani, Ivan
> Djelic (reviews and tests), Scott Wood, Robert Jarzmik, yourself, ...
> 
> As far as formal tags... maybe these, if they don't object:

How it usually goes - if the person sends a "formal" acked-by or some
other tag which can be copy-pasted, then he cares and wants the tag to
go in with the patch. But if the person just comments on the patch,
reviews, but does not send a "formal" tag, he does not care. In this
case you may give the person a credit in free form in the commit message
optionally, if you feel very grateful.

IOW, if we missed someone's "formal" tag - let's add it - I shamelessly
rebase my tree anyway, under excuse of "I am not the maintainer" :-)

> mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN
> 5568eb3c3bb2816281b6a7c04db92434b72b1495
> Tested-by: Ivan Djelic <ivan.djelic@parrot.com>
> 
> mtd: nand: read_page() returns max_bitflips
> 6bf87bf989bfdfc78fb2c5cd55de4bab9b572992
> Acked-by (freescale changes): Scott Wood <scottwood@freescale.com>

OK, thanks.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/7] mtd: Change meaning of -EUCLEAN return code on reads

[Ivan Djelic]

On Tue, Apr 24, 2012 at 08:18:18PM +0100, Mike Dunn wrote:
> 
> This problem was discussed a while back [1][2][3], and a consensus of sorts was
> reached for a solution, which these patches implement.  The recent addition of
> the mtd api entry functions now make this solution practical (thanks Artem!).  A
> quick description of each patch will provide a synopsis of the set:
> 
> (1) Fix how mtd->ecc_strength is calculated from nand->ecc.strength to reflect
>     the fact that granularity is now at ecc step level (the two values are now
>     the same).
> (2) Fix a couple incorrect nand->ecc.strength values.
> (3) Expose mtd->ecc_strength through sysfs (read-only).
> (4) mtd->bitflip_threshold is added and exposed as read/write through sysfs.
>     The drivers can initialize it, otherwise mtd initializes it to the default
>     value of ecc_strength.  Users can change it through sysfs.
> (5) The nand driver method _read_page() returns max_bitflips to the nand
>     infrastructure code.  This is the maximum number of bitflip corrections that
>     were made on any one region comprising an ecc step.
> (6) Sanity checks added to nand_scan_tail() to ensure that drivers needing to
>     set ecc strength do so.  (This includes all drivers using hardware ecc.)
> (7) Absent a hard error, all drivers' mtd->_read() methods now return
>     max_bitflips to mtd, and mtd_read() makes the decision of whether to return
>     -EUCLEAN or 0. If max_bitflips >= bitflip_threshold, -EUCLEAN is returned.
>     If bitflip_threshold == 0 (no ecc), make no comparison and return 0.
> 
> Of course reviews greatly appreciated.  Thanks!

Hi Mike,

Thanks for this new version.
The whole series of patches looks OK to me, except for one small glitch:
'mtd->bitflip_threshold' can be customized by the driver, but in that case it is not
propagated to the slave partition devices (the same way 'ecc_strength' is propagated).
Something like this is missing:

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index d6321f6..d518e4d 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -517,6 +517,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 
        slave->mtd.ecclayout = master->ecclayout;
        slave->mtd.ecc_strength = master->ecc_strength;
+       slave->mtd.bitflip_threshold = master->bitflip_threshold;
+
        if (master->_block_isbad) {
                uint64_t offs = 0;

Apart from that, I was able to run a few tests on a BeagleBoard revC3 with simulated
bitflips, dropping my own error concealment code in favor of your patch. I did the
following checks:
 - when errors_corrected < bitflip_threshold, check that mtd_read() returns 0
 - when errors_corrected >= bitflip_threshold, check that mtd_read() returns -EUCLEAN
 - check if driver customization of bitflip_threshold works
 - check if per-partition customization of bitflip_threshold through sysfs works
Everything worked as expected.

BR,
--
Ivan

Re: [PATCH 0/7] mtd: Change meaning of -EUCLEAN return code on reads

[Mike Dunn]

On 04/25/2012 04:23 AM, Ivan Djelic wrote:
> 
> Thanks for this new version.
> The whole series of patches looks OK to me, except for one small glitch:
> 'mtd->bitflip_threshold' can be customized by the driver, but in that case it is not
> propagated to the slave partition devices (the same way 'ecc_strength' is propagated).
> Something like this is missing:
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index d6321f6..d518e4d 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -517,6 +517,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
>  
>         slave->mtd.ecclayout = master->ecclayout;
>         slave->mtd.ecc_strength = master->ecc_strength;
> +       slave->mtd.bitflip_threshold = master->bitflip_threshold;
> +
>         if (master->_block_isbad) {
>                 uint64_t offs = 0;
> 


Yes, you're right.  Without this it breaks if the driver sets bitflip_threshold.


> Apart from that, I was able to run a few tests on a BeagleBoard revC3 with simulated
> bitflips, dropping my own error concealment code in favor of your patch. I did the
> following checks:
>  - when errors_corrected < bitflip_threshold, check that mtd_read() returns 0
>  - when errors_corrected >= bitflip_threshold, check that mtd_read() returns -EUCLEAN
>  - check if driver customization of bitflip_threshold works


I guess I should have performed this test too :)


>  - check if per-partition customization of bitflip_threshold through sysfs works
> Everything worked as expected.


Thanks once again Ivan.  I'll follow up with a new version of the patch set in a
day or too.

Mike

[PATCH v2 0/7] mtd: Change meaning of -EUCLEAN return code on reads

[Mike Dunn]

This v2 of this set includes the following changes:
  - [Patch 4/7]: Propagate bitflip_threshold from master to slave partition.
  - [Patch 5/7]: Zero out max_bitflips outside of if-block in fsl_ifc_nand.
  - [Patch 6/7]: Simplify ecc strength sanity check.

This patchset addresses the problem of insufficient information being returned
by mtd_read() with regard to bit error corrections.  Currently -EUCLEAN is
returned if one or more bit errors were corrected during the course of the read
operation.  Higher layers like UBI use this return code as an indication that
the erase block may be degrading and should be considered as a candidate for
being marked as a bad block.  The problem is that high bit error rates are
common on more recent NAND flash devices with strong ecc algorithms.  Frequent
(but entirely normal) bit error corrections on these devices result in blocks
being incorrectly marked as bad.  On some devices, ubi/ubifs can not be reliably
used because of this.

This problem was discussed a while back [1][2][3], and a consensus of sorts was
reached for a solution, which these patches implement.  The recent addition of
the mtd api entry functions now make this solution practical (thanks Artem!).  A
quick description of each patch will provide a synopsis of the set:

(1) Fix how mtd->ecc_strength is calculated from nand->ecc.strength to reflect
    the fact that granularity is now at ecc step level (the two values are now
    the same).
(2) Fix a couple incorrect nand->ecc.strength values.
(3) Expose mtd->ecc_strength through sysfs (read-only).
(4) mtd->bitflip_threshold is added and exposed as read/write through sysfs.
    The drivers can initialize it, otherwise mtd initializes it to the default
    value of ecc_strength.  Users can change it through sysfs.
(5) The nand driver method _read_page() returns max_bitflips to the nand
    infrastructure code.  This is the maximum number of bitflip corrections that
    were made on any one region comprising an ecc step.
(6) Sanity checks added to nand_scan_tail() to ensure that drivers needing to
    set ecc strength do so.  (This includes all drivers using hardware ecc.)
(7) Absent a hard error, all drivers' mtd->_read() methods now return
    max_bitflips to mtd, and mtd_read() makes the decision of whether to return
    -EUCLEAN or 0. If max_bitflips >= bitflip_threshold, -EUCLEAN is returned.
    If bitflip_threshold == 0 (no ecc), make no comparison and return 0.

Of course reviews greatly appreciated.  Thanks!

P.S. It looks like some of these patches may conflict with others that may be
queued up, so let me know if they need to be reworked.

[1] http://lists.infradead.org/pipermail/linux-mtd/2011-December/038755.html
[2] http://lists.infradead.org/pipermail/linux-mtd/2011-December/038688.html
[3] http://lists.infradead.org/pipermail/linux-mtd/2011-December/038828.html

Mike Dunn (7):
  mtd; ecc_strength is at ecc step granularity
  mtd: nand: fix incorrect ecc strength values
  mtd: expose ecc_strength through sysfs
  mtd: bitflip threshold added to mtd_info and sysfs
  mtd: nand: read_page() returns max_bitflips
  mtd: nand: sanity checks of ecc strength in nand_scan_tail()
  mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

 Documentation/ABI/testing/sysfs-class-mtd |   48 +++++++++++++++
 drivers/mtd/devices/docg3.c               |    4 +-
 drivers/mtd/mtdcore.c                     |   57 +++++++++++++++++-
 drivers/mtd/mtdpart.c                     |   12 ++--
 drivers/mtd/nand/alauda.c                 |    4 +-
 drivers/mtd/nand/atmel_nand.c             |    9 ++-
 drivers/mtd/nand/bcm_umi_bch.c            |    4 +-
 drivers/mtd/nand/bcm_umi_nand.c           |    7 +--
 drivers/mtd/nand/cafe_nand.c              |    4 +-
 drivers/mtd/nand/denali.c                 |   10 ++-
 drivers/mtd/nand/docg4.c                  |    5 +-
 drivers/mtd/nand/fsl_elbc_nand.c          |   21 ++++--
 drivers/mtd/nand/fsl_ifc_nand.c           |   10 +++-
 drivers/mtd/nand/fsmc_nand.c              |    9 ++-
 drivers/mtd/nand/jz4740_nand.c            |    6 +--
 drivers/mtd/nand/nand_base.c              |   95 ++++++++++++++++++++--------
 drivers/mtd/nand/sh_flctl.c               |    2 +-
 drivers/mtd/onenand/onenand_base.c        |    6 +-
 include/linux/mtd/mtd.h                   |   11 +++-
 include/linux/mtd/nand.h                  |    3 +
 20 files changed, 254 insertions(+), 73 deletions(-)

-- 
1.7.3.4

[PATCH 2/7] mtd: nand: fix incorrect ecc strength values

[Mike Dunn]

This fixes a couple of ecc strength values for which I earlier made conservative
guesses, but whose correct values were later determined [1] (thanks Ivan).  Also
sets strength for fsl_ifc_nand, which was merged to mainline after the original
patch that set the strength for all drivers.

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040325.html

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---

This v2 is actually unchanged from the original set.

 drivers/mtd/nand/bcm_umi_nand.c |    7 +------
 drivers/mtd/nand/fsl_ifc_nand.c |    1 +
 drivers/mtd/nand/jz4740_nand.c  |    6 +-----
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/bcm_umi_nand.c b/drivers/mtd/nand/bcm_umi_nand.c
index 6908cdd..7134adf 100644
--- a/drivers/mtd/nand/bcm_umi_nand.c
+++ b/drivers/mtd/nand/bcm_umi_nand.c
@@ -476,12 +476,7 @@ static int __devinit bcm_umi_nand_probe(struct platform_device *pdev)
 		this->badblock_pattern = &largepage_bbt;
 	}
 
-	/*
-	 * FIXME: ecc strength value of 6 bits per 512 bytes of data is a
-	 * conservative guess, given 13 ecc bytes and using bch alg.
-	 * (Assume Galois field order m=15 to allow a margin of error.)
-	 */
-	this->ecc.strength = 6;
+	this->ecc.strength = 8;
 
 #endif
 
diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 5387cec..872cc96 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -821,6 +821,7 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
 	/* Hardware generates ECC per 512 Bytes */
 	chip->ecc.size = 512;
 	chip->ecc.bytes = 8;
+	chip->ecc.strength = 4;
 
 	switch (csor & CSOR_NAND_PGS_MASK) {
 	case CSOR_NAND_PGS_512:
diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c
index e4147e8..a6fa884 100644
--- a/drivers/mtd/nand/jz4740_nand.c
+++ b/drivers/mtd/nand/jz4740_nand.c
@@ -332,11 +332,7 @@ static int __devinit jz_nand_probe(struct platform_device *pdev)
 	chip->ecc.mode		= NAND_ECC_HW_OOB_FIRST;
 	chip->ecc.size		= 512;
 	chip->ecc.bytes		= 9;
-	chip->ecc.strength	= 2;
-	/*
-	 * FIXME: ecc_strength value of 2 bits per 512 bytes of data is a
-	 * conservative guess, given 9 ecc bytes and reed-solomon alg.
-	 */
+	chip->ecc.strength	= 4;
 
 	if (pdata)
 		chip->ecc.layout = pdata->ecc_layout;
-- 
1.7.3.4