[PATCH v4 0/4] mtd: Preparation patches for the SPI NAND framework

[PATCH v4 0/4] mtd: Preparation patches for the SPI NAND framework

[Boris Brezillon]

Hello,

Here is a set of patches preparing the introduction of the generic
NAND and SPI frameworks.

If you want to see the overall picture of the SPI NAND framework you can
have a look at this branch [2].

Regards,

Boris

Change in v4:
- add "mtd: mtdpart: Make ECC stat handling consistent" and
- integrate "mtd: Remove duplicate checks on mtd_oob_ops parameter"
  into this series
- drop "mtd: Stop directly calling master ->_xxx() hooks from mtdpart
  code"

Boris Brezillon (3):
  mtd: mtdpart: Make ECC stat handling consistent
  mtd: Fallback to ->_read/write_oob() when ->_read/write() is missing
  mtd: Remove duplicate checks on mtd_oob_ops parameter

 drivers/mtd/devices/docg3.c        | 70 ------------------------------
 drivers/mtd/mtdcore.c              | 31 +++++++++++++-
 drivers/mtd/mtdpart.c              | 42 +++++-------------
 drivers/mtd/nand/nand_base.c       | 87 --------------------------------------
 drivers/mtd/onenand/onenand_base.c | 81 -----------------------------------
 5 files changed, 40 insertions(+), 271 deletions(-)

-- 
2.11.0

[PATCH v4 1/3] mtd: mtdpart: Make ECC stat handling consistent

[Boris Brezillon]

part_read() and part_read_oob() where counting ECC failures and
bitflips differently. Adjust part_read_oob() to mimic what is done in
part_read(). This is in needed to use ->_read_oob() as a fallback when
when ->_read() is not implemented.

Note that bitflips and ECC failure accounting on MTD partitions is
broken by design, because nothing prevents concurrent accesses to the
underlying master MTD device between the moment we save the stats in a
local variable and the moment master->_read[_oob]() returns. It's not
something that can easily be fixed, so leave it like that for now.

Suggested-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Changes in v4:
- new patch
---
 drivers/mtd/mtdpart.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index be088bccd593..283e8526713b 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -105,6 +105,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 		struct mtd_oob_ops *ops)
 {
 	struct mtd_part *part = mtd_to_part(mtd);
+	struct mtd_ecc_stats stats;
 	int res;
 
 	if (from >= mtd->size)
@@ -127,12 +128,12 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 	}
 
 	res = part->parent->_read_oob(part->parent, from + part->offset, ops);
-	if (unlikely(res)) {
-		if (mtd_is_bitflip(res))
-			mtd->ecc_stats.corrected++;
-		if (mtd_is_eccerr(res))
-			mtd->ecc_stats.failed++;
-	}
+	if (unlikely(mtd_is_eccerr(res)))
+		mtd->ecc_stats.failed +=
+			part->parent->ecc_stats.failed - stats.failed;
+	else
+		mtd->ecc_stats.corrected +=
+			part->parent->ecc_stats.corrected - stats.corrected;
 	return res;
 }
 
-- 
2.11.0

[PATCH v4 2/3] mtd: Fallback to ->_read/write_oob() when ->_read/write() is missing

[Boris Brezillon]

Some MTD sublayers/drivers are implementing ->_read/write_oob() and
provide dummy wrappers for their ->_read/write() implementations.
Let the core handle this case instead of duplicating the logic.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
Acked-by: Brian Norris <computersforpeace@gmail.com>
Reviewed-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
Changes in v4:
- Update mtdpart.c to assign slave->_read/_write() only if the master
  has ->_read/_write != NULL

Changes in v3:
- none

Changes in v2:
- none
---
 drivers/mtd/devices/docg3.c        | 65 --------------------------------------
 drivers/mtd/mtdcore.c              | 31 ++++++++++++++++--
 drivers/mtd/mtdpart.c              |  6 ++--
 drivers/mtd/nand/nand_base.c       | 56 --------------------------------
 drivers/mtd/onenand/onenand_base.c | 63 ------------------------------------
 5 files changed, 33 insertions(+), 188 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 0806f72102c0..5fb5e93d1547 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -990,36 +990,6 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	goto out;
 }
 
-/**
- * doc_read - Read bytes from flash
- * @mtd: the device
- * @from: the offset from first block and first page, in bytes, aligned on page
- *        size
- * @len: the number of bytes to read (must be a multiple of 4)
- * @retlen: the number of bytes actually read
- * @buf: the filled in buffer
- *
- * Reads flash memory pages. This function does not read the OOB chunk, but only
- * the page data.
- *
- * Returns 0 if read successful, of -EIO, -EINVAL if an error occurred
- */
-static int doc_read(struct mtd_info *mtd, loff_t from, size_t len,
-	     size_t *retlen, u_char *buf)
-{
-	struct mtd_oob_ops ops;
-	size_t ret;
-
-	memset(&ops, 0, sizeof(ops));
-	ops.datbuf = buf;
-	ops.len = len;
-	ops.mode = MTD_OPS_AUTO_OOB;
-
-	ret = doc_read_oob(mtd, from, &ops);
-	*retlen = ops.retlen;
-	return ret;
-}
-
 static int doc_reload_bbt(struct docg3 *docg3)
 {
 	int block = DOC_LAYOUT_BLOCK_BBT;
@@ -1513,39 +1483,6 @@ static int doc_write_oob(struct mtd_info *mtd, loff_t ofs,
 	return ret;
 }
 
-/**
- * doc_write - Write a buffer to the chip
- * @mtd: the device
- * @to: the offset from first block and first page, in bytes, aligned on page
- *      size
- * @len: the number of bytes to write (must be a full page size, ie. 512)
- * @retlen: the number of bytes actually written (0 or 512)
- * @buf: the buffer to get bytes from
- *
- * Writes data to the chip.
- *
- * Returns 0 if write successful, -EIO if write error
- */
-static int doc_write(struct mtd_info *mtd, loff_t to, size_t len,
-		     size_t *retlen, const u_char *buf)
-{
-	struct docg3 *docg3 = mtd->priv;
-	int ret;
-	struct mtd_oob_ops ops;
-
-	doc_dbg("doc_write(to=%lld, len=%zu)\n", to, len);
-	ops.datbuf = (char *)buf;
-	ops.len = len;
-	ops.mode = MTD_OPS_PLACE_OOB;
-	ops.oobbuf = NULL;
-	ops.ooblen = 0;
-	ops.ooboffs = 0;
-
-	ret = doc_write_oob(mtd, to, &ops);
-	*retlen = ops.retlen;
-	return ret;
-}
-
 static struct docg3 *sysfs_dev2docg3(struct device *dev,
 				     struct device_attribute *attr)
 {
@@ -1866,8 +1803,6 @@ static int __init doc_set_driver_info(int chip_id, struct mtd_info *mtd)
 	mtd->writebufsize = mtd->writesize = DOC_LAYOUT_PAGE_SIZE;
 	mtd->oobsize = DOC_LAYOUT_OOB_SIZE;
 	mtd->_erase = doc_erase;
-	mtd->_read = doc_read;
-	mtd->_write = doc_write;
 	mtd->_read_oob = doc_read_oob;
 	mtd->_write_oob = doc_write_oob;
 	mtd->_block_isbad = doc_block_isbad;
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 642c35dde686..d7ab091b36b2 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1058,7 +1058,20 @@ int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
 	 * 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 (mtd->_read) {
+		ret_code = mtd->_read(mtd, from, len, retlen, buf);
+	} else if (mtd->_read_oob) {
+		struct mtd_oob_ops ops = {
+			.len = len,
+			.datbuf = buf,
+		};
+
+		ret_code = mtd->_read_oob(mtd, from, &ops);
+		*retlen = ops.retlen;
+	} else {
+		return -ENOTSUPP;
+	}
+
 	if (unlikely(ret_code < 0))
 		return ret_code;
 	if (mtd->ecc_strength == 0)
@@ -1073,11 +1086,25 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 	*retlen = 0;
 	if (to < 0 || to >= mtd->size || len > mtd->size - to)
 		return -EINVAL;
-	if (!mtd->_write || !(mtd->flags & MTD_WRITEABLE))
+	if ((!mtd->_write && !mtd->_write_oob) ||
+	    !(mtd->flags & MTD_WRITEABLE))
 		return -EROFS;
 	if (!len)
 		return 0;
 	ledtrig_mtd_activity();
+
+	if (!mtd->_write) {
+		struct mtd_oob_ops ops = {
+			.len = len,
+			.datbuf = (u8 *)buf,
+		};
+		int ret;
+
+		ret = mtd->_write_oob(mtd, to, &ops);
+		*retlen = ops.retlen;
+		return ret;
+	}
+
 	return mtd->_write(mtd, to, len, retlen, buf);
 }
 EXPORT_SYMBOL_GPL(mtd_write);
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 283e8526713b..07ef168ded98 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -436,8 +436,10 @@ static struct mtd_part *allocate_partition(struct mtd_info *parent,
 				parent->dev.parent;
 	slave->mtd.dev.of_node = part->of_node;
 
-	slave->mtd._read = part_read;
-	slave->mtd._write = part_write;
+	if (parent->_read)
+		slave->mtd._read = part_read;
+	if (parent->_write)
+		slave->mtd._write = part_write;
 
 	if (parent->_panic_write)
 		slave->mtd._panic_write = part_panic_write;
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 6135d007a068..889ceadbf607 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2027,33 +2027,6 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 }
 
 /**
- * nand_read - [MTD Interface] MTD compatibility function for nand_do_read_ecc
- * @mtd: MTD device structure
- * @from: offset to read from
- * @len: number of bytes to read
- * @retlen: pointer to variable to store the number of read bytes
- * @buf: the databuffer to put data
- *
- * Get hold of the chip and call nand_do_read.
- */
-static int nand_read(struct mtd_info *mtd, loff_t from, size_t len,
-		     size_t *retlen, uint8_t *buf)
-{
-	struct mtd_oob_ops ops;
-	int ret;
-
-	nand_get_device(mtd, FL_READING);
-	memset(&ops, 0, sizeof(ops));
-	ops.len = len;
-	ops.datbuf = buf;
-	ops.mode = MTD_OPS_PLACE_OOB;
-	ret = nand_do_read_ops(mtd, from, &ops);
-	*retlen = ops.retlen;
-	nand_release_device(mtd);
-	return ret;
-}
-
-/**
  * nand_read_oob_std - [REPLACEABLE] the most common OOB data read function
  * @mtd: mtd info structure
  * @chip: nand chip info structure
@@ -2822,33 +2795,6 @@ static int panic_nand_write(struct mtd_info *mtd, loff_t to, size_t len,
 }
 
 /**
- * nand_write - [MTD Interface] NAND write with ECC
- * @mtd: MTD device structure
- * @to: offset to write to
- * @len: number of bytes to write
- * @retlen: pointer to variable to store the number of written bytes
- * @buf: the data to write
- *
- * NAND write with ECC.
- */
-static int nand_write(struct mtd_info *mtd, loff_t to, size_t len,
-			  size_t *retlen, const uint8_t *buf)
-{
-	struct mtd_oob_ops ops;
-	int ret;
-
-	nand_get_device(mtd, FL_WRITING);
-	memset(&ops, 0, sizeof(ops));
-	ops.len = len;
-	ops.datbuf = (uint8_t *)buf;
-	ops.mode = MTD_OPS_PLACE_OOB;
-	ret = nand_do_write_ops(mtd, to, &ops);
-	*retlen = ops.retlen;
-	nand_release_device(mtd);
-	return ret;
-}
-
-/**
  * nand_do_write_oob - [MTD Interface] NAND write out-of-band
  * @mtd: MTD device structure
  * @to: offset to write to
@@ -4917,8 +4863,6 @@ int nand_scan_tail(struct mtd_info *mtd)
 	mtd->_erase = nand_erase;
 	mtd->_point = NULL;
 	mtd->_unpoint = NULL;
-	mtd->_read = nand_read;
-	mtd->_write = nand_write;
 	mtd->_panic_write = panic_nand_write;
 	mtd->_read_oob = nand_read_oob;
 	mtd->_write_oob = nand_write_oob;
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index 1a6d0e367b89..050ba8a87543 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1448,38 +1448,6 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
 }
 
 /**
- * onenand_read - [MTD Interface] Read data from flash
- * @param mtd		MTD device structure
- * @param from		offset to read from
- * @param len		number of bytes to read
- * @param retlen	pointer to variable to store the number of read bytes
- * @param buf		the databuffer to put data
- *
- * Read with ecc
-*/
-static int onenand_read(struct mtd_info *mtd, loff_t from, size_t len,
-	size_t *retlen, u_char *buf)
-{
-	struct onenand_chip *this = mtd->priv;
-	struct mtd_oob_ops ops = {
-		.len	= len,
-		.ooblen	= 0,
-		.datbuf	= buf,
-		.oobbuf	= NULL,
-	};
-	int ret;
-
-	onenand_get_device(mtd, FL_READING);
-	ret = ONENAND_IS_4KB_PAGE(this) ?
-		onenand_mlc_read_ops_nolock(mtd, from, &ops) :
-		onenand_read_ops_nolock(mtd, from, &ops);
-	onenand_release_device(mtd);
-
-	*retlen = ops.retlen;
-	return ret;
-}
-
-/**
  * onenand_read_oob - [MTD Interface] Read main and/or out-of-band
  * @param mtd:		MTD device structure
  * @param from:		offset to read from
@@ -2129,35 +2097,6 @@ static int onenand_write_oob_nolock(struct mtd_info *mtd, loff_t to,
 }
 
 /**
- * onenand_write - [MTD Interface] write buffer to FLASH
- * @param mtd		MTD device structure
- * @param to		offset to write to
- * @param len		number of bytes to write
- * @param retlen	pointer to variable to store the number of written bytes
- * @param buf		the data to write
- *
- * Write with ECC
- */
-static int onenand_write(struct mtd_info *mtd, loff_t to, size_t len,
-	size_t *retlen, const u_char *buf)
-{
-	struct mtd_oob_ops ops = {
-		.len	= len,
-		.ooblen	= 0,
-		.datbuf	= (u_char *) buf,
-		.oobbuf	= NULL,
-	};
-	int ret;
-
-	onenand_get_device(mtd, FL_WRITING);
-	ret = onenand_write_ops_nolock(mtd, to, &ops);
-	onenand_release_device(mtd);
-
-	*retlen = ops.retlen;
-	return ret;
-}
-
-/**
  * onenand_write_oob - [MTD Interface] NAND write data and/or out-of-band
  * @param mtd:		MTD device structure
  * @param to:		offset to write
@@ -4038,8 +3977,6 @@ int onenand_scan(struct mtd_info *mtd, int maxchips)
 	mtd->_erase = onenand_erase;
 	mtd->_point = NULL;
 	mtd->_unpoint = NULL;
-	mtd->_read = onenand_read;
-	mtd->_write = onenand_write;
 	mtd->_read_oob = onenand_read_oob;
 	mtd->_write_oob = onenand_write_oob;
 	mtd->_panic_write = onenand_panic_write;
-- 
2.11.0

[PATCH v4 3/3] mtd: Remove duplicate checks on mtd_oob_ops parameter

[Boris Brezillon]

Some of the check done in custom ->_read/write_oob() implementation are
already done by the core (in mtd_check_oob_ops()).

Suggested-by: Peter Pan <peterpansjtu@gmail.com>
[Remove redundant checks done in mtdpart.c]
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Changes in v4:
- integrated into the SPI NAND preparation patchset

Changes in v3:
- Rebased on mtd/next after "mtd: Stop directly calling master ->_xxx()
  hooks from mtdpart code" had been dropped

Changes in v2:
- Merge "mtd: Remove unneeded checks in part_{read,write}_oob()" into
  this commit
---
 drivers/mtd/devices/docg3.c        |  5 -----
 drivers/mtd/mtdpart.c              | 23 -----------------------
 drivers/mtd/nand/nand_base.c       | 31 -------------------------------
 drivers/mtd/onenand/onenand_base.c | 18 ------------------
 4 files changed, 77 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 5fb5e93d1547..a85af236b44d 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -904,9 +904,6 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	if (ooblen % DOC_LAYOUT_OOB_SIZE)
 		return -EINVAL;
 
-	if (from + len > mtd->size)
-		return -EINVAL;
-
 	ops->oobretlen = 0;
 	ops->retlen = 0;
 	ret = 0;
@@ -1441,8 +1438,6 @@ static int doc_write_oob(struct mtd_info *mtd, loff_t ofs,
 	if (len && ooblen &&
 	    (len / DOC_LAYOUT_PAGE_SIZE) != (ooblen / oobdelta))
 		return -EINVAL;
-	if (ofs + len > mtd->size)
-		return -EINVAL;
 
 	ops->oobretlen = 0;
 	ops->retlen = 0;
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 07ef168ded98..131bb1d52871 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -108,25 +108,6 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 	struct mtd_ecc_stats stats;
 	int res;
 
-	if (from >= mtd->size)
-		return -EINVAL;
-	if (ops->datbuf && from + ops->len > mtd->size)
-		return -EINVAL;
-
-	/*
-	 * If OOB is also requested, make sure that we do not read past the end
-	 * of this partition.
-	 */
-	if (ops->oobbuf) {
-		size_t len, pages;
-
-		len = mtd_oobavail(mtd, ops);
-		pages = mtd_div_by_ws(mtd->size, mtd);
-		pages -= mtd_div_by_ws(from, mtd);
-		if (ops->ooboffs + ops->ooblen > pages * len)
-			return -EINVAL;
-	}
-
 	res = part->parent->_read_oob(part->parent, from + part->offset, ops);
 	if (unlikely(mtd_is_eccerr(res)))
 		mtd->ecc_stats.failed +=
@@ -190,10 +171,6 @@ static int part_write_oob(struct mtd_info *mtd, loff_t to,
 {
 	struct mtd_part *part = mtd_to_part(mtd);
 
-	if (to >= mtd->size)
-		return -EINVAL;
-	if (ops->datbuf && to + ops->len > mtd->size)
-		return -EINVAL;
 	return part->parent->_write_oob(part->parent, to + part->offset, ops);
 }
 
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 889ceadbf607..61eeac233683 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2187,21 +2187,6 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 
 	len = mtd_oobavail(mtd, ops);
 
-	if (unlikely(ops->ooboffs >= len)) {
-		pr_debug("%s: attempt to start read outside oob\n",
-				__func__);
-		return -EINVAL;
-	}
-
-	/* Do not allow reads past end of device */
-	if (unlikely(from >= mtd->size ||
-		     ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
-					(from >> chip->page_shift)) * len)) {
-		pr_debug("%s: attempt to read beyond end of device\n",
-				__func__);
-		return -EINVAL;
-	}
-
 	chipnr = (int)(from >> chip->chip_shift);
 	chip->select_chip(mtd, chipnr);
 
@@ -2820,22 +2805,6 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 		return -EINVAL;
 	}
 
-	if (unlikely(ops->ooboffs >= len)) {
-		pr_debug("%s: attempt to start write outside oob\n",
-				__func__);
-		return -EINVAL;
-	}
-
-	/* Do not allow write past end of device */
-	if (unlikely(to >= mtd->size ||
-		     ops->ooboffs + ops->ooblen >
-			((mtd->size >> chip->page_shift) -
-			 (to >> chip->page_shift)) * len)) {
-		pr_debug("%s: attempt to write beyond end of device\n",
-				__func__);
-		return -EINVAL;
-	}
-
 	chipnr = (int)(to >> chip->chip_shift);
 
 	/*
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index 050ba8a87543..979f4031f23c 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1383,15 +1383,6 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
 		return -EINVAL;
 	}
 
-	/* Do not allow reads past end of device */
-	if (unlikely(from >= mtd->size ||
-		     column + len > ((mtd->size >> this->page_shift) -
-				     (from >> this->page_shift)) * oobsize)) {
-		printk(KERN_ERR "%s: Attempted to read beyond end of device\n",
-			__func__);
-		return -EINVAL;
-	}
-
 	stats = mtd->ecc_stats;
 
 	readcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_READ : ONENAND_CMD_READOOB;
@@ -2024,15 +2015,6 @@ static int onenand_write_oob_nolock(struct mtd_info *mtd, loff_t to,
 		return -EINVAL;
 	}
 
-	/* Do not allow reads past end of device */
-	if (unlikely(to >= mtd->size ||
-		     column + len > ((mtd->size >> this->page_shift) -
-				     (to >> this->page_shift)) * oobsize)) {
-		printk(KERN_ERR "%s: Attempted to write past end of device\n",
-		       __func__);
-		return -EINVAL;
-	}
-
 	oobbuf = this->oob_buf;
 
 	oobcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_PROG : ONENAND_CMD_PROGOOB;
-- 
2.11.0

Re: [PATCH v4 0/4] mtd: Preparation patches for the SPI NAND framework

[Boris Brezillon]

Should be [PATCH v4 *0/3*].

On Mon,  8 Jan 2018 22:15:39 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hello,
> 
> Here is a set of patches preparing the introduction of the generic
> NAND and SPI frameworks.
> 
> If you want to see the overall picture of the SPI NAND framework you can
> have a look at this branch [2].
> 
> Regards,
> 
> Boris
> 
> Change in v4:
> - add "mtd: mtdpart: Make ECC stat handling consistent" and
> - integrate "mtd: Remove duplicate checks on mtd_oob_ops parameter"
>   into this series
> - drop "mtd: Stop directly calling master ->_xxx() hooks from mtdpart
>   code"
> 
> Boris Brezillon (3):
>   mtd: mtdpart: Make ECC stat handling consistent
>   mtd: Fallback to ->_read/write_oob() when ->_read/write() is missing
>   mtd: Remove duplicate checks on mtd_oob_ops parameter
> 
>  drivers/mtd/devices/docg3.c        | 70 ------------------------------
>  drivers/mtd/mtdcore.c              | 31 +++++++++++++-
>  drivers/mtd/mtdpart.c              | 42 +++++-------------
>  drivers/mtd/nand/nand_base.c       | 87 --------------------------------------
>  drivers/mtd/onenand/onenand_base.c | 81 -----------------------------------
>  5 files changed, 40 insertions(+), 271 deletions(-)
> 

Re: [PATCH v4 1/3] mtd: mtdpart: Make ECC stat handling consistent

[Boris Brezillon]

On Mon,  8 Jan 2018 22:15:40 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> part_read() and part_read_oob() where counting ECC failures and
> bitflips differently. Adjust part_read_oob() to mimic what is done in
> part_read(). This is in needed to use ->_read_oob() as a fallback when
> when ->_read() is not implemented.
> 
> Note that bitflips and ECC failure accounting on MTD partitions is
> broken by design, because nothing prevents concurrent accesses to the
> underlying master MTD device between the moment we save the stats in a
> local variable and the moment master->_read[_oob]() returns. It's not
> something that can easily be fixed, so leave it like that for now.
> 
> Suggested-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Changes in v4:
> - new patch
> ---
>  drivers/mtd/mtdpart.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index be088bccd593..283e8526713b 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -105,6 +105,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
>  		struct mtd_oob_ops *ops)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> +	struct mtd_ecc_stats stats;
>  	int res;
>  
>  	if (from >= mtd->size)
> @@ -127,12 +128,12 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
>  	}
>  

Missing

	stats = part->parent->ecc_stats;

here.

I'll send a new version. Sorry for the inconvenience.

>  	res = part->parent->_read_oob(part->parent, from + part->offset, ops);
> -	if (unlikely(res)) {
> -		if (mtd_is_bitflip(res))
> -			mtd->ecc_stats.corrected++;
> -		if (mtd_is_eccerr(res))
> -			mtd->ecc_stats.failed++;
> -	}
> +	if (unlikely(mtd_is_eccerr(res)))
> +		mtd->ecc_stats.failed +=
> +			part->parent->ecc_stats.failed - stats.failed;
> +	else
> +		mtd->ecc_stats.corrected +=
> +			part->parent->ecc_stats.corrected - stats.corrected;
>  	return res;
>  }
>  

Re: [PATCH v4 0/4] mtd: Preparation patches for the SPI NAND framework

[Ladislav Michl]

On Mon, Jan 08, 2018 at 10:15:39PM +0100, Boris Brezillon wrote:
> Hello,
> 
> Here is a set of patches preparing the introduction of the generic
> NAND and SPI frameworks.

Tested on IGEPv2 Rev. B (TI OMAP3530) with Muxed OneNAND(DDP) 512MB 1.8V
16-bit (0x58) after fixing PATCH v4 1/3.

Tested-by: Ladislav Michl <ladis@linux-mips.org>

> If you want to see the overall picture of the SPI NAND framework you can
> have a look at this branch [2].
> 
> Regards,
> 
> Boris
> 
> Change in v4:
> - add "mtd: mtdpart: Make ECC stat handling consistent" and
> - integrate "mtd: Remove duplicate checks on mtd_oob_ops parameter"
>   into this series
> - drop "mtd: Stop directly calling master ->_xxx() hooks from mtdpart
>   code"
> 
> Boris Brezillon (3):
>   mtd: mtdpart: Make ECC stat handling consistent
>   mtd: Fallback to ->_read/write_oob() when ->_read/write() is missing
>   mtd: Remove duplicate checks on mtd_oob_ops parameter
> 
>  drivers/mtd/devices/docg3.c        | 70 ------------------------------
>  drivers/mtd/mtdcore.c              | 31 +++++++++++++-
>  drivers/mtd/mtdpart.c              | 42 +++++-------------
>  drivers/mtd/nand/nand_base.c       | 87 --------------------------------------
>  drivers/mtd/onenand/onenand_base.c | 81 -----------------------------------
>  5 files changed, 40 insertions(+), 271 deletions(-)
> 
> -- 
> 2.11.0

Re: [PATCH v4 3/3] mtd: Remove duplicate checks on mtd_oob_ops parameter

[Miquel RAYNAL]

Hello Boris,

On Mon,  8 Jan 2018 22:15:42 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Some of the check done in custom ->_read/write_oob() implementation
> are already done by the core (in mtd_check_oob_ops()).

Not sure this is relevant here as your series introduces changes for
the SPI NAND framework, but there are other places where these checks
are, IMHO, also redundant and could be removed. The "past end" string
when grepped in the MTD folder core returns a few more hits.

In the NAND core:
  - nand_do_read_oob()
  - nand_read_oob()
  - nand_do_write_oob()
  - nand_write_oob()
Maybe also in onenand_base.c, but I am less confident for this one:
  - onenand_bbt_read_oob()

What do you think?

Thanks,
Miquèl

> 
> Suggested-by: Peter Pan <peterpansjtu@gmail.com>
> [Remove redundant checks done in mtdpart.c]
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Changes in v4:
> - integrated into the SPI NAND preparation patchset
> 
> Changes in v3:
> - Rebased on mtd/next after "mtd: Stop directly calling master
> ->_xxx() hooks from mtdpart code" had been dropped
> 
> Changes in v2:
> - Merge "mtd: Remove unneeded checks in part_{read,write}_oob()" into
>   this commit
> ---
>  drivers/mtd/devices/docg3.c        |  5 -----
>  drivers/mtd/mtdpart.c              | 23 -----------------------
>  drivers/mtd/nand/nand_base.c       | 31
> ------------------------------- drivers/mtd/onenand/onenand_base.c |
> 18 ------------------ 4 files changed, 77 deletions(-)
> 
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index 5fb5e93d1547..a85af236b44d 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -904,9 +904,6 @@ static int doc_read_oob(struct mtd_info *mtd,
> loff_t from, if (ooblen % DOC_LAYOUT_OOB_SIZE)
>  		return -EINVAL;
>  
> -	if (from + len > mtd->size)
> -		return -EINVAL;
> -
>  	ops->oobretlen = 0;
>  	ops->retlen = 0;
>  	ret = 0;
> @@ -1441,8 +1438,6 @@ static int doc_write_oob(struct mtd_info *mtd,
> loff_t ofs, if (len && ooblen &&
>  	    (len / DOC_LAYOUT_PAGE_SIZE) != (ooblen / oobdelta))
>  		return -EINVAL;
> -	if (ofs + len > mtd->size)
> -		return -EINVAL;
>  
>  	ops->oobretlen = 0;
>  	ops->retlen = 0;
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 07ef168ded98..131bb1d52871 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -108,25 +108,6 @@ static int part_read_oob(struct mtd_info *mtd,
> loff_t from, struct mtd_ecc_stats stats;
>  	int res;
>  
> -	if (from >= mtd->size)
> -		return -EINVAL;
> -	if (ops->datbuf && from + ops->len > mtd->size)
> -		return -EINVAL;
> -
> -	/*
> -	 * If OOB is also requested, make sure that we do not read
> past the end
> -	 * of this partition.
> -	 */
> -	if (ops->oobbuf) {
> -		size_t len, pages;
> -
> -		len = mtd_oobavail(mtd, ops);
> -		pages = mtd_div_by_ws(mtd->size, mtd);
> -		pages -= mtd_div_by_ws(from, mtd);
> -		if (ops->ooboffs + ops->ooblen > pages * len)
> -			return -EINVAL;
> -	}
> -
>  	res = part->parent->_read_oob(part->parent, from +
> part->offset, ops); if (unlikely(mtd_is_eccerr(res)))
>  		mtd->ecc_stats.failed +=
> @@ -190,10 +171,6 @@ static int part_write_oob(struct mtd_info *mtd,
> loff_t to, {
>  	struct mtd_part *part = mtd_to_part(mtd);
>  
> -	if (to >= mtd->size)
> -		return -EINVAL;
> -	if (ops->datbuf && to + ops->len > mtd->size)
> -		return -EINVAL;
>  	return part->parent->_write_oob(part->parent, to +
> part->offset, ops); }
>  
> diff --git a/drivers/mtd/nand/nand_base.c
> b/drivers/mtd/nand/nand_base.c index 889ceadbf607..61eeac233683 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2187,21 +2187,6 @@ static int nand_do_read_oob(struct mtd_info
> *mtd, loff_t from, 
>  	len = mtd_oobavail(mtd, ops);
>  
> -	if (unlikely(ops->ooboffs >= len)) {
> -		pr_debug("%s: attempt to start read outside oob\n",
> -				__func__);
> -		return -EINVAL;
> -	}
> -
> -	/* Do not allow reads past end of device */
> -	if (unlikely(from >= mtd->size ||
> -		     ops->ooboffs + readlen > ((mtd->size >>
> chip->page_shift) -
> -					(from >> chip->page_shift))
> * len)) {
> -		pr_debug("%s: attempt to read beyond end of
> device\n",
> -				__func__);
> -		return -EINVAL;
> -	}
> -
>  	chipnr = (int)(from >> chip->chip_shift);
>  	chip->select_chip(mtd, chipnr);
>  
> @@ -2820,22 +2805,6 @@ static int nand_do_write_oob(struct mtd_info
> *mtd, loff_t to, return -EINVAL;
>  	}
>  
> -	if (unlikely(ops->ooboffs >= len)) {
> -		pr_debug("%s: attempt to start write outside oob\n",
> -				__func__);
> -		return -EINVAL;
> -	}
> -
> -	/* Do not allow write past end of device */
> -	if (unlikely(to >= mtd->size ||
> -		     ops->ooboffs + ops->ooblen >
> -			((mtd->size >> chip->page_shift) -
> -			 (to >> chip->page_shift)) * len)) {
> -		pr_debug("%s: attempt to write beyond end of
> device\n",
> -				__func__);
> -		return -EINVAL;
> -	}
> -
>  	chipnr = (int)(to >> chip->chip_shift);
>  
>  	/*
> diff --git a/drivers/mtd/onenand/onenand_base.c
> b/drivers/mtd/onenand/onenand_base.c index 050ba8a87543..979f4031f23c
> 100644 --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -1383,15 +1383,6 @@ static int onenand_read_oob_nolock(struct
> mtd_info *mtd, loff_t from, return -EINVAL;
>  	}
>  
> -	/* Do not allow reads past end of device */
> -	if (unlikely(from >= mtd->size ||
> -		     column + len > ((mtd->size >> this->page_shift)
> -
> -				     (from >> this->page_shift)) *
> oobsize)) {
> -		printk(KERN_ERR "%s: Attempted to read beyond end of
> device\n",
> -			__func__);
> -		return -EINVAL;
> -	}
> -
>  	stats = mtd->ecc_stats;
>  
>  	readcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_READ :
> ONENAND_CMD_READOOB; @@ -2024,15 +2015,6 @@ static int
> onenand_write_oob_nolock(struct mtd_info *mtd, loff_t to, return
> -EINVAL; }
>  
> -	/* Do not allow reads past end of device */
> -	if (unlikely(to >= mtd->size ||
> -		     column + len > ((mtd->size >> this->page_shift)
> -
> -				     (to >> this->page_shift)) *
> oobsize)) {
> -		printk(KERN_ERR "%s: Attempted to write past end of
> device\n",
> -		       __func__);
> -		return -EINVAL;
> -	}
> -
>  	oobbuf = this->oob_buf;
>  
>  	oobcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_PROG :
> ONENAND_CMD_PROGOOB;



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 3/3] mtd: Remove duplicate checks on mtd_oob_ops parameter

[Boris Brezillon]

On Mon, 8 Jan 2018 23:04:55 +0100
Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:

> Hello Boris,
> 
> On Mon,  8 Jan 2018 22:15:42 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > Some of the check done in custom ->_read/write_oob() implementation
> > are already done by the core (in mtd_check_oob_ops()).  
> 
> Not sure this is relevant here as your series introduces changes for
> the SPI NAND framework, but there are other places where these checks
> are, IMHO, also redundant and could be removed. The "past end" string
> when grepped in the MTD folder core returns a few more hits.
> 
> In the NAND core:
>   - nand_do_read_oob()
>   - nand_read_oob()
>   - nand_do_write_oob()
>   - nand_write_oob()

That's true for nand_read/write_oob(). The one in nand_do_write_oob()
is still needed because mtd_check_oob_ops() allows OOB writes crossing a
page boundary. Finally, I don't see any boundary checks in
nand_do_read_oob().

> Maybe also in onenand_base.c, but I am less confident for this one:
>   - onenand_bbt_read_oob()

Unfortunately no, this function is directly called from onenand_bbt.c,
which means the MTD layer layer is completely bypassed. I also found
boundary checks in onenand_mlc_read_ops_nolock(), but again, this
function is called from do_otp_read() which is not going through
mtd_check_oob_ops().

> 
> What do you think?

I'll remove the extra checks in nand_read/write_oob(). For the other
ones, one solution would be to expose mtd_check_oob_ops(), but I'll keep
that for later.

Re: [PATCH v4 3/3] mtd: Remove duplicate checks on mtd_oob_ops parameter

[Miquel RAYNAL]

Hello Boris,

On Mon, 8 Jan 2018 23:30:10 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Mon, 8 Jan 2018 23:04:55 +0100
> Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:
> 
> > Hello Boris,
> > 
> > On Mon,  8 Jan 2018 22:15:42 +0100
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >   
> > > Some of the check done in custom ->_read/write_oob()
> > > implementation are already done by the core (in
> > > mtd_check_oob_ops()).    
> > 
> > Not sure this is relevant here as your series introduces changes for
> > the SPI NAND framework, but there are other places where these
> > checks are, IMHO, also redundant and could be removed. The "past
> > end" string when grepped in the MTD folder core returns a few more
> > hits.
> > 
> > In the NAND core:
> >   - nand_do_read_oob()
> >   - nand_read_oob()
> >   - nand_do_write_oob()
> >   - nand_write_oob()  
> 
> That's true for nand_read/write_oob(). The one in nand_do_write_oob()
> is still needed because mtd_check_oob_ops() allows OOB writes
> crossing a page boundary. Finally, I don't see any boundary checks in
> nand_do_read_oob().

I forgot that crossing page boundaries was not a use case of
mtd_check_oob_ops(), thanks for pointing it. However in
nand_do_read/write_oob(), the comment and the code really state the
checked boundary is the end of the device. So are you sure these two
checks are needed?

[1]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2226
[2]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2886

> 
> > Maybe also in onenand_base.c, but I am less confident for this one:
> >   - onenand_bbt_read_oob()  
> 
> Unfortunately no, this function is directly called from onenand_bbt.c,
> which means the MTD layer layer is completely bypassed. I also found
> boundary checks in onenand_mlc_read_ops_nolock(), but again, this
> function is called from do_otp_read() which is not going through
> mtd_check_oob_ops().
> 
> > 
> > What do you think?  
> 
> I'll remove the extra checks in nand_read/write_oob(). For the other
> ones, one solution would be to expose mtd_check_oob_ops(), but I'll
> keep that for later.

Ok.

Thanks,
Miquèl

Re: [PATCH v4 3/3] mtd: Remove duplicate checks on mtd_oob_ops parameter

[Boris Brezillon]

On Tue, 9 Jan 2018 08:19:53 +0100
Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:

> Hello Boris,
> 
> On Mon, 8 Jan 2018 23:30:10 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Mon, 8 Jan 2018 23:04:55 +0100
> > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:
> >   
> > > Hello Boris,
> > > 
> > > On Mon,  8 Jan 2018 22:15:42 +0100
> > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > >     
> > > > Some of the check done in custom ->_read/write_oob()
> > > > implementation are already done by the core (in
> > > > mtd_check_oob_ops()).      
> > > 
> > > Not sure this is relevant here as your series introduces changes for
> > > the SPI NAND framework, but there are other places where these
> > > checks are, IMHO, also redundant and could be removed. The "past
> > > end" string when grepped in the MTD folder core returns a few more
> > > hits.
> > > 
> > > In the NAND core:
> > >   - nand_do_read_oob()
> > >   - nand_read_oob()
> > >   - nand_do_write_oob()
> > >   - nand_write_oob()    
> > 
> > That's true for nand_read/write_oob(). The one in nand_do_write_oob()
> > is still needed because mtd_check_oob_ops() allows OOB writes
> > crossing a page boundary. Finally, I don't see any boundary checks in
> > nand_do_read_oob().  
> 
> I forgot that crossing page boundaries was not a use case of
> mtd_check_oob_ops(), thanks for pointing it. However in
> nand_do_read/write_oob(), the comment and the code really state the
> checked boundary is the end of the device. So are you sure these two
> checks are needed?
> 
> [1]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2226
> [2]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2886

You mean, the lines I remove in this patch? :P

> 
> >   
> > > Maybe also in onenand_base.c, but I am less confident for this one:
> > >   - onenand_bbt_read_oob()    
> > 
> > Unfortunately no, this function is directly called from onenand_bbt.c,
> > which means the MTD layer layer is completely bypassed. I also found
> > boundary checks in onenand_mlc_read_ops_nolock(), but again, this
> > function is called from do_otp_read() which is not going through
> > mtd_check_oob_ops().
> >   
> > > 
> > > What do you think?    
> > 
> > I'll remove the extra checks in nand_read/write_oob(). For the other
> > ones, one solution would be to expose mtd_check_oob_ops(), but I'll
> > keep that for later.  
> 
> Ok.
> 
> Thanks,
> Miquèl

Re: [PATCH v4 3/3] mtd: Remove duplicate checks on mtd_oob_ops parameter

[Miquel RAYNAL]

On Tue, 9 Jan 2018 09:11:21 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Tue, 9 Jan 2018 08:19:53 +0100
> Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:
> 
> > Hello Boris,
> > 
> > On Mon, 8 Jan 2018 23:30:10 +0100
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >   
> > > On Mon, 8 Jan 2018 23:04:55 +0100
> > > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:
> > >     
> > > > Hello Boris,
> > > > 
> > > > On Mon,  8 Jan 2018 22:15:42 +0100
> > > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > > >       
> > > > > Some of the check done in custom ->_read/write_oob()
> > > > > implementation are already done by the core (in
> > > > > mtd_check_oob_ops()).        
> > > > 
> > > > Not sure this is relevant here as your series introduces
> > > > changes for the SPI NAND framework, but there are other places
> > > > where these checks are, IMHO, also redundant and could be
> > > > removed. The "past end" string when grepped in the MTD folder
> > > > core returns a few more hits.
> > > > 
> > > > In the NAND core:
> > > >   - nand_do_read_oob()
> > > >   - nand_read_oob()
> > > >   - nand_do_write_oob()
> > > >   - nand_write_oob()      
> > > 
> > > That's true for nand_read/write_oob(). The one in
> > > nand_do_write_oob() is still needed because mtd_check_oob_ops()
> > > allows OOB writes crossing a page boundary. Finally, I don't see
> > > any boundary checks in nand_do_read_oob().    
> > 
> > I forgot that crossing page boundaries was not a use case of
> > mtd_check_oob_ops(), thanks for pointing it. However in
> > nand_do_read/write_oob(), the comment and the code really state the
> > checked boundary is the end of the device. So are you sure these two
> > checks are needed?
> > 
> > [1]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2226
> > [2]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/nand_base.c#L2886  
> 
> You mean, the lines I remove in this patch? :P

Oops, sorry for the noise then!

Thanks,
Miquèl

Re: [PATCH v4 1/3] mtd: mtdpart: Make ECC stat handling consistent

[Robert Jarzmik]

Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> part_read() and part_read_oob() where counting ECC failures and
Typo: s/where/were.

> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index be088bccd593..283e8526713b 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -105,6 +105,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
>  		struct mtd_oob_ops *ops)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> +	struct mtd_ecc_stats stats;
>  	int res;
>  
>  	if (from >= mtd->size)
> @@ -127,12 +128,12 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
>  	}
>  
>  	res = part->parent->_read_oob(part->parent, from + part->offset, ops);
> -	if (unlikely(res)) {
> -		if (mtd_is_bitflip(res))
> -			mtd->ecc_stats.corrected++;
> -		if (mtd_is_eccerr(res))
> -			mtd->ecc_stats.failed++;
> -	}
> +	if (unlikely(mtd_is_eccerr(res)))
> +		mtd->ecc_stats.failed +=
> +			part->parent->ecc_stats.failed - stats.failed;
stats is used uninitialized, but I suppose you've already caught that.

Cheers.

-- 
Robert

Re: [PATCH v4 1/3] mtd: mtdpart: Make ECC stat handling consistent

[Boris Brezillon]

On Wed, 10 Jan 2018 21:27:34 +0100
Robert Jarzmik <robert.jarzmik@free.fr> wrote:

> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> 
> > part_read() and part_read_oob() where counting ECC failures and  
> Typo: s/where/were.

I'll fix the typo.

> 
> > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> > index be088bccd593..283e8526713b 100644
> > --- a/drivers/mtd/mtdpart.c
> > +++ b/drivers/mtd/mtdpart.c
> > @@ -105,6 +105,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
> >  		struct mtd_oob_ops *ops)
> >  {
> >  	struct mtd_part *part = mtd_to_part(mtd);
> > +	struct mtd_ecc_stats stats;
> >  	int res;
> >  
> >  	if (from >= mtd->size)
> > @@ -127,12 +128,12 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
> >  	}
> >  
> >  	res = part->parent->_read_oob(part->parent, from + part->offset, ops);
> > -	if (unlikely(res)) {
> > -		if (mtd_is_bitflip(res))
> > -			mtd->ecc_stats.corrected++;
> > -		if (mtd_is_eccerr(res))
> > -			mtd->ecc_stats.failed++;
> > -	}
> > +	if (unlikely(mtd_is_eccerr(res)))
> > +		mtd->ecc_stats.failed +=
> > +			part->parent->ecc_stats.failed - stats.failed;  
> stats is used uninitialized, but I suppose you've already caught that.

Yep, already fixed in v5.

Thanks,

Boris