Re: Pass -EUCLEN to userspace?

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Mon, 25 Apr 2016 11:26:16 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> > > 
> > > Regarding the maximum number of bitflips per chunk, maybe we can make it
> > > part of the ioctl request instead of saving the statistics at the MTD
> > > level.
> > > 
> > > How about creating a new ioctl taking a pointer to this struct as a
> > > parameter:
> > > 
> > > struct mtd_extended_read_ops {
> > > 	/* Existing params */
> > > 	unsigned int mode;
> > > 	size_t len;
> > > 	size_t retlen;
> > > 	size_t ooblen;
> > > 	size_t oobretlen;
> > > 	uint32_t ooboffs;
> > > 	void *datbuf;
> > > 	void *oobbuf;
> > > 
> > > 	/*
> > > 	 * Param containing the maximum number of bitflips for this
> > > 	 * read request.
> > > 	 */
> > > 	unsigned int max_bitflips;
> > > };    
> > 
> > Not sure how this ioctl exactly should look like, but this would solve
> > the problem.  
> 
> Let me design a quick prototype, I'll let you follow up with the patch
> submission process...

Below is an untested patch adding a new ioctl returning the ECC/read stats.
Feel free to debug/enhance this implementation and submit it to the MTD
ML.


--->8---
From 2c07a2a015e6c0bdc2f9d91c9a1b971903da0493 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@free-electrons.com>
Date: Mon, 25 Apr 2016 16:05:06 +0200
Subject: [PATCH] mtd: add a the MEMREAD ioctl to attach ECC stats to the read
 request

TODO: add proper commit message + split changes into several patches.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/devices/docg3.c        | 10 +++++
 drivers/mtd/mtdchar.c              | 76 ++++++++++++++++++++++++++++++++++++++
 drivers/mtd/mtdcore.c              |  9 +++++
 drivers/mtd/nand/nand_base.c       | 10 +++++
 drivers/mtd/onenand/onenand_base.c | 11 ++++++
 include/linux/mtd/mtd.h            |  7 ++++
 include/uapi/mtd/mtd-abi.h         | 50 +++++++++++++++++++++++++
 7 files changed, 173 insertions(+)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index b833e6c..965c97a 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -885,6 +885,8 @@ 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;
+	struct mtd_req_stats *reqstats = ops->stats;
+	struct mtd_ecc_stats stats;
 	int max_bitflips = 0;
 
 	if (buf)
@@ -912,6 +914,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	ret = 0;
 	skip = from % DOC_LAYOUT_PAGE_SIZE;
 	mutex_lock(&docg3->cascade->lock);
+	stats = mtd->ecc_stats;
 	while (ret >= 0 && (len > 0 || ooblen > 0)) {
 		calc_block_sector(from - skip, &block0, &block1, &page, &ofs,
 			docg3->reliable);
@@ -983,6 +986,13 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	}
 
 out:
+	if (reqstats) {
+		restats->corrected_bitflips += mtd->ecc_stats.corrected -
+					       stats.corrected;
+		restats->uncorrectable_errors += mtd->ecc_stats.failed -
+						 stats.failed;
+	}
+
 	mutex_unlock(&docg3->cascade->lock);
 	return ret;
 err_in_read:
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 2a47a3f..708ecee 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -656,6 +656,75 @@ static int mtdchar_write_ioctl(struct mtd_info *mtd,
 	return ret;
 }
 
+static int mtdchar_read_ioctl(struct mtd_info *mtd,
+			      struct mtd_read_req __user *argp)
+{
+	struct mtd_read_req req;
+	struct mtd_req_stats stats;
+	struct mtd_oob_ops ops = { .stats = &stats };
+	void __user *usr_data, *usr_oob;
+	int ret;
+
+	if (copy_from_user(&req, argp, sizeof(req)))
+		return -EFAULT;
+
+	usr_data = (void __user *)(uintptr_t)req.usr_data;
+	usr_oob = (void __user *)(uintptr_t)req.usr_oob;
+	if ((usr_data && !access_ok(VERIFY_WRITE, usr_data, req.len)) ||
+	    (usr_oob && !access_ok(VERIFY_WRITE, usr_oob, req.ooblen)))
+		return -EFAULT;
+
+	if (!mtd->_read_oob)
+		return -EOPNOTSUPP;
+
+	ops.mode = req.mode;
+	ops.len = (size_t)req.len;
+	ops.ooblen = (size_t)req.ooblen;
+	ops.ooboffs = 0;
+
+	if (usr_data) {
+		ops.datbuf = kzalloc(ops.len, GFP_KERNEL);
+		if (!ops.datbuf)
+			return -ENOMEM;
+	}
+
+	if (usr_oob) {
+		ops.oobbuf = kzalloc(ops.ooblen, GFP_KERNEL);
+		if (!ops.oobbuf) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	}
+
+	ret = mtd_read_oob(mtd, (loff_t)req.start, &ops);
+	if (ret)
+		goto out;
+
+	if (usr_data && copy_to_user(ops.datbuf, usr_data, ops.retlen)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	if (usr_oob && copy_to_user(ops.oobbuf, usr_oob, ops.oobretlen)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	req.len = ops.retlen;
+	req.ooblen = ops.oobretlen;
+	req.stats.max_bitflips = stats.max_bitflips;
+	req.stats.corrected_bitflips = stats.corrected_bitflips;
+	req.stats.uncorrectable_errors = stats.uncorrectable_errors;
+	if (copy_to_user(&req, argp, sizeof(req)))
+		ret = -EFAULT;
+
+out:
+	kfree(ops.datbuf);
+	kfree(ops.oobbuf);
+
+	return ret;
+}
+
 static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 {
 	struct mtd_file_info *mfi = file->private_data;
@@ -850,6 +919,13 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 		break;
 	}
 
+	case MEMREAD:
+	{
+		ret = mtdchar_read_ioctl(mtd,
+		      (struct mtd_read_req __user *)arg);
+		break;
+	}
+
 	case MEMLOCK:
 	{
 		struct erase_info_user einfo;
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index e3936b8..ba6488f 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -988,6 +988,11 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
 		return -EOPNOTSUPP;
 
 	ledtrig_mtd_activity();
+
+	/* Make sure all counters are initialized to zero. */
+	if (ops->stats)
+		memset(ops->stats, 0, sizeof(*ops->stats));
+
 	/*
 	 * In cases where ops->datbuf != NULL, mtd->_read_oob() has semantics
 	 * similar to mtd->_read(), returning a non-negative integer
@@ -999,6 +1004,10 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
 		return ret_code;
 	if (mtd->ecc_strength == 0)
 		return 0;	/* device lacks ecc */
+
+	if (ops->stats)
+		ops->stats->max_bitflips = ret_code;
+
 	return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
 }
 EXPORT_SYMBOL_GPL(mtd_read_oob);
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7bc37b4..25cab31 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2162,6 +2162,8 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 			 struct mtd_oob_ops *ops)
 {
+	struct mtd_req_stats *reqstats = ops->stats;
+	struct mtd_ecc_stats stats;
 	int ret = -ENOTSUPP;
 
 	ops->retlen = 0;
@@ -2185,11 +2187,19 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 		goto out;
 	}
 
+	stats = mtd->ecc_stats;
 	if (!ops->datbuf)
 		ret = nand_do_read_oob(mtd, from, ops);
 	else
 		ret = nand_do_read_ops(mtd, from, ops);
 
+	if (reqstats) {
+		reqstats->uncorrectable_errors += mtd->ecc_stats.failed -
+						  stats.failed;
+		reqstats->corrected_bitflips += mtd->ecc_stats.corrected -
+						stats.corrected;
+	}
+
 out:
 	nand_release_device(mtd);
 	return ret;
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index a4b029a..2f700eb 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1491,6 +1491,8 @@ static int onenand_read_oob(struct mtd_info *mtd, loff_t from,
 			    struct mtd_oob_ops *ops)
 {
 	struct onenand_chip *this = mtd->priv;
+	struct mtd_req_stats *reqstats = ops->stats;
+	struct mtd_ecc_stats stats;
 	int ret;
 
 	switch (ops->mode) {
@@ -1504,12 +1506,21 @@ static int onenand_read_oob(struct mtd_info *mtd, loff_t from,
 	}
 
 	onenand_get_device(mtd, FL_READING);
+	stats = mtd->ecc_stats;
+
 	if (ops->datbuf)
 		ret = ONENAND_IS_4KB_PAGE(this) ?
 			onenand_mlc_read_ops_nolock(mtd, from, ops) :
 			onenand_read_ops_nolock(mtd, from, ops);
 	else
 		ret = onenand_read_oob_nolock(mtd, from, ops);
+
+	if (reqstats) {
+		reqstats->corrected_bitflips += mtd->ecc_stats.corrected -
+						stats.corrected;
+		reqstats->uncorrectable_errors += mtd->ecc_stats.failed -
+						  stats.failed;
+	}
 	onenand_release_device(mtd);
 
 	return ret;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 29a1706..bd5e8a1 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -64,6 +64,12 @@ struct mtd_erase_region_info {
 	unsigned long *lockmap;		/* If keeping bitmap of locks */
 };
 
+struct mtd_req_stats {
+	unsigned int max_bitflips;
+	unsigned int corrected_bitflips;
+	unsigned int uncorrectable_errors;
+};
+
 /**
  * struct mtd_oob_ops - oob operation operands
  * @mode:	operation mode
@@ -92,6 +98,7 @@ struct mtd_oob_ops {
 	uint32_t	ooboffs;
 	uint8_t		*datbuf;
 	uint8_t		*oobbuf;
+	struct mtd_req_stats *stats;
 };
 
 #define MTD_MAX_OOBFREE_ENTRIES_LARGE	32
diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
index 0ec1da2..a61a042 100644
--- a/include/uapi/mtd/mtd-abi.h
+++ b/include/uapi/mtd/mtd-abi.h
@@ -90,6 +90,52 @@ struct mtd_write_req {
 	__u8 padding[7];
 };
 
+/**
+ * struct mtd_read_req_stats - statistics attached to a read request
+ *
+ * @max_bitflips: the maximum number of bitflips found in the read portion.
+ *		  This information can be used to decide when the data stored
+ *		  on a specific portion should be moved somewhere else to
+ *		  avoid data loss.
+ * @corrected_bitflips: the number of bitflips corrected during the read
+ *			operation
+ * @uncorrectable_errors: the number of uncorrectable errors that happened
+ *			  during the read operation
+ */
+struct mtd_read_req_stats {
+	__u32 max_bitflips;
+	__u32 corrected_bitflips;
+	__u32 uncorrectable_errors;
+};
+
+/**
+ * struct mtd_read_req - data structure for requesting a write operation
+ *
+ * @start:	start address
+ * @len:	length of data buffer
+ * @ooblen:	length of OOB buffer
+ * @usr_data:	user-provided data buffer
+ * @usr_oob:	user-provided OOB buffer
+ * @mode:	MTD mode (see "MTD operation modes")
+ * @padding:	reserved, must be set to 0
+ * @stats:	statistics attached to the read request
+ *
+ * This structure supports ioctl(MEMWRITE) operations, allowing data and/or OOB
+ * writes in various modes. To write to OOB-only, set @usr_data == NULL, and to
+ * write data-only, set @usr_oob == NULL. However, setting both @usr_data and
+ * @usr_oob to NULL is not allowed.
+ */
+struct mtd_read_req {
+	__u64 start;
+	__u64 len;
+	__u64 ooblen;
+	__u64 usr_data;
+	__u64 usr_oob;
+	__u8 mode;
+	__u8 padding[7];
+	struct mtd_read_req_stats stats;
+};
+
 #define MTD_ABSENT		0
 #define MTD_RAM			1
 #define MTD_ROM			2
@@ -203,6 +249,10 @@ struct otp_info {
  * without OOB, e.g., NOR flash.
  */
 #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
+/*
+ * Same as for MEMWRITE but for read operations.
+ */
+#define MEMREAD			_IOWR('M', 25, struct mtd_read_req)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace