[RFC] UBI statistics and bitrot interface

[RFC] UBI statistics and bitrot interface

[Richard Weinberger]

This series adds new ioctl()s to UBI such that userspace can query
read/erase counters and trigger re-reads/scrubbing of PEBs.
Especially for MLC NAND flash where read disturb and data retention
is a big topic this interface can be used by applications to foster the NAND.
Parallel to this series at patch series against mtd-utils is sent that
implements a ubihealthd which makes use of that interface.

[PATCH 1/5] UBI: Introduce prepare_erase_work()
[PATCH 2/5] UBI: Introduce in_pq()
[PATCH 3/5] UBI: Expose the bitrot interface
[PATCH 4/5] UBI: Add basic read counter support
[PATCH 5/5] UBI: Expose UBI statistics interface

[PATCH 1/5] UBI: Introduce prepare_erase_work()

[Richard Weinberger]

Split ubi_work creation and scheduling code such that
we can re-use the creation code where we schedule ubi work
on our own.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/wl.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index eb4489f9..beaf3c4 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -569,29 +569,26 @@ static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
 			int shutdown);
 
 /**
- * schedule_erase - schedule an erase work.
+ * prepare_erase_work - prepare an erase work.
  * @ubi: UBI device description object
  * @e: the WL entry of the physical eraseblock to erase
  * @vol_id: the volume ID that last used this PEB
  * @lnum: the last used logical eraseblock number for the PEB
  * @torture: if the physical eraseblock has to be tortured
  *
- * This function returns zero in case of success and a %-ENOMEM in case of
- * failure.
+ * This function returns a struct ubi_work in case of success
+ * and an ERR_PTR(%-ENOMEM) in case of failure.
  */
-static int schedule_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
-			  int vol_id, int lnum, int torture)
+static struct ubi_work *prepare_erase_work(struct ubi_wl_entry *e, int vol_id,
+					   int lnum, int torture)
 {
 	struct ubi_work *wl_wrk;
 
 	ubi_assert(e);
 
-	dbg_wl("schedule erasure of PEB %d, EC %d, torture %d",
-	       e->pnum, e->ec, torture);
-
 	wl_wrk = kmalloc(sizeof(struct ubi_work), GFP_NOFS);
 	if (!wl_wrk)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	wl_wrk->func = &erase_worker;
 	wl_wrk->e = e;
@@ -599,6 +596,31 @@ static int schedule_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
 	wl_wrk->lnum = lnum;
 	wl_wrk->torture = torture;
 
+	return wl_wrk;
+}
+
+/**
+ * schedule_erase - schedule an erase work.
+ * @ubi: UBI device description object
+ * @e: the WL entry of the physical eraseblock to erase
+ * @vol_id: the volume ID that last used this PEB
+ * @lnum: the last used logical eraseblock number for the PEB
+ * @torture: if the physical eraseblock has to be tortured
+ *
+ * This function returns zero in case of success and a %-ENOMEM in case of
+ * failure.
+ */
+static int schedule_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
+			  int vol_id, int lnum, int torture)
+{
+	struct ubi_work *wl_wrk = prepare_erase_work(e, vol_id, lnum, torture);
+
+	dbg_wl("schedule erasure of PEB %d, EC %d, torture %d",
+	       e->pnum, e->ec, torture);
+
+	if (IS_ERR(wl_wrk))
+		return PTR_ERR(wl_wrk);
+
 	schedule_ubi_work(ubi, wl_wrk);
 	return 0;
 }
-- 
1.8.4.5

[PATCH 2/5] UBI: Introduce in_pq()

[Richard Weinberger]

This function works like in_wl_tree() but checks whether an ubi_wl_entry
is currently in the protection queue.
We need this function to query the current state of an ubi_wl_entry.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/wl.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index beaf3c4..a7b0543 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -278,6 +278,27 @@ static int in_wl_tree(struct ubi_wl_entry *e, struct rb_root *root)
 }
 
 /**
+ * in_pq - check if a wear-leveling entry is present in the protection queue.
+ * @ubi: UBI device description object
+ * @e: the wear-leveling entry to check
+ *
+ * This function returns non-zero if @e is in the protection queue and zero
+ * if it is not.
+ */
+static inline int in_pq(const struct ubi_device *ubi, struct ubi_wl_entry *e)
+{
+	struct ubi_wl_entry *p;
+	int i;
+
+	for (i = 0; i < UBI_PROT_QUEUE_LEN; ++i)
+		list_for_each_entry(p, &ubi->pq[i], u.list)
+			if (p == e)
+				return 1;
+
+	return 0;
+}
+
+/**
  * prot_queue_add - add physical eraseblock to the protection queue.
  * @ubi: UBI device description object
  * @e: the physical eraseblock to add
@@ -1758,16 +1779,11 @@ static int self_check_in_wl_tree(const struct ubi_device *ubi,
 static int self_check_in_pq(const struct ubi_device *ubi,
 			    struct ubi_wl_entry *e)
 {
-	struct ubi_wl_entry *p;
-	int i;
-
 	if (!ubi_dbg_chk_gen(ubi))
 		return 0;
 
-	for (i = 0; i < UBI_PROT_QUEUE_LEN; ++i)
-		list_for_each_entry(p, &ubi->pq[i], u.list)
-			if (p == e)
-				return 0;
+	if (in_pq(ubi, e))
+		return 0;
 
 	ubi_err(ubi, "self-check failed for PEB %d, EC %d, Protect queue",
 		e->pnum, e->ec);
-- 
1.8.4.5

[PATCH 3/5] UBI: Expose the bitrot interface

[Richard Weinberger]

Using UBI_IOCRPEB and UBI_IOCSPEB userspace can force
reading and scrubbing of PEBs.

In case of bitflips UBI will automatically take action
and move data to a different PEB.
This interface allows a daemon to foster your NAND.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/cdev.c      |  30 ++++++++++
 drivers/mtd/ubi/ubi.h       |   1 +
 drivers/mtd/ubi/wl.c        | 135 ++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/mtd/ubi-user.h |   3 +
 4 files changed, 169 insertions(+)

diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index d16fccf..f500451 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -963,6 +963,36 @@ static long ubi_cdev_ioctl(struct file *file, unsigned int cmd,
 		break;
 	}
 
+	/* Read a PEB */
+	case UBI_IOCRPEB:
+	{
+		int pnum;
+
+		err = get_user(pnum, (__user int32_t *)argp);
+		if (err) {
+			err = -EFAULT;
+			break;
+		}
+
+		err = ubi_bitrot_check(ubi, pnum, 0);
+		break;
+	}
+
+	/* Scrub a PEB */
+	case UBI_IOCSPEB:
+	{
+		int pnum;
+
+		err = get_user(pnum, (__user int32_t *)argp);
+		if (err) {
+			err = -EFAULT;
+			break;
+		}
+
+		err = ubi_bitrot_check(ubi, pnum, 1);
+		break;
+	}
+
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 2974b67..40c9eeb 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -859,6 +859,7 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct ubi_wl_entry *used_e,
 int ubi_is_erase_work(struct ubi_work *wrk);
 void ubi_refill_pools(struct ubi_device *ubi);
 int ubi_ensure_anchor_pebs(struct ubi_device *ubi);
+int ubi_bitrot_check(struct ubi_device *ubi, int pnum, int force_scrub);
 
 /* io.c */
 int ubi_io_read(const struct ubi_device *ubi, void *buf, int pnum, int offset,
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index a7b0543..ad02c46 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1351,6 +1351,141 @@ retry:
 	return ensure_wear_leveling(ubi, 0);
 }
 
+static int scrub_possible(struct ubi_device *ubi, struct ubi_wl_entry *e)
+{
+	if (in_wl_tree(e, &ubi->scrub))
+		return -EBUSY;
+	else if (in_wl_tree(e, &ubi->erroneous))
+		return -EBUSY;
+	else if (ubi->move_from == e)
+		return -EBUSY;
+	else if (ubi->move_to == e)
+		return -EBUSY;
+
+	return 0;
+}
+
+/**
+ * ubi_bitrot_check - Check an eraseblock for bitflips
+ * @ubi: UBI device description object
+ * @pnum: the physical eraseblock to schedule
+ * @force_scrub: force scrubbing if non-zero
+ *
+ * Returns:
+ * %EINVAL, PEB is out of range
+ * %ENOENT, PEB is no longer used by UBI
+ * %EBUSY, PEB cannot be checked now or a check is currently running on it
+ * %EAGAIN, bit flips happened but scrubbing is currently not possible
+ * %EUCLEAN, bit flips happened and PEB is scheduled for scrubbing
+ * %0, no bit flips detected
+ */
+int ubi_bitrot_check(struct ubi_device *ubi, int pnum, int force_scrub)
+{
+	int err;
+	struct ubi_wl_entry *e;
+	struct ubi_work *wl_wrk;
+
+	if (pnum < 0 || pnum >= ubi->peb_count) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * Pause all parallel work, otherwise it can happen that the
+	 * erase worker frees a wl entry under us.
+	 */
+	down_write(&ubi->work_sem);
+
+	/*
+	 * Make sure that the wl entry does not change state while
+	 * inspecting it.
+	 */
+	spin_lock(&ubi->wl_lock);
+	e = ubi->lookuptbl[pnum];
+	if (!e) {
+		spin_unlock(&ubi->wl_lock);
+		err = -ENOENT;
+		goto out_unlock;
+	}
+	/*
+	 * Does it make sense to check this PEB?
+	 * Maybe UBI is already inspecing it...
+	 */
+	err = scrub_possible(ubi, e);
+	spin_unlock(&ubi->wl_lock);
+	if (err)
+		goto out_unlock;
+
+	if (!force_scrub) {
+		mutex_lock(&ubi->buf_mutex);
+		err = ubi_io_read(ubi, ubi->peb_buf, e->pnum, 0, ubi->peb_size);
+		mutex_unlock(&ubi->buf_mutex);
+	}
+
+	if (err == UBI_IO_BITFLIPS || force_scrub) {
+		/*
+		 * Okay, bit flip happened, let figure what we can do.
+		 */
+		spin_lock(&ubi->wl_lock);
+
+		/*
+		 * Need to re-check state
+		 */
+		err = scrub_possible(ubi, e);
+		if (err) {
+			spin_unlock(&ubi->wl_lock);
+			goto out_unlock;
+		}
+
+		if (in_pq(ubi, e)) {
+			prot_queue_del(ubi, e->pnum);
+			wl_tree_add(e, &ubi->scrub);
+			spin_unlock(&ubi->wl_lock);
+
+			err = ensure_wear_leveling(ubi, 1);
+		} else if (in_wl_tree(e, &ubi->used)) {
+			rb_erase(&e->u.rb, &ubi->used);
+			wl_tree_add(e, &ubi->scrub);
+			spin_unlock(&ubi->wl_lock);
+
+			err = ensure_wear_leveling(ubi, 1);
+		} else if (in_wl_tree(e, &ubi->free)) {
+			rb_erase(&e->u.rb, &ubi->free);
+			spin_unlock(&ubi->wl_lock);
+
+			wl_wrk = prepare_erase_work(e, -1, -1, force_scrub ? 0 : 1);
+			if (IS_ERR(wl_wrk)) {
+				err = PTR_ERR(wl_wrk);
+				goto out_unlock;
+			}
+
+			__schedule_ubi_work(ubi, wl_wrk);
+		} else {
+			spin_unlock(&ubi->wl_lock);
+			up_write(&ubi->work_sem);
+			/*
+			 * e is owned by fastmap. We are not allowed to
+			 * move it as the on-flash fastmap data structure refers to it.
+			 * Let's schedule a new fastmap write
+			 * such that the said PEB can get released.
+			 */
+			ubi_update_fastmap(ubi);
+			err = -EAGAIN;
+			goto out;
+		}
+
+		if (!err && !force_scrub)
+			err = -EUCLEAN;
+	} else {
+		err = 0;
+	}
+
+out_unlock:
+	up_write(&ubi->work_sem);
+out:
+	return err;
+}
+
 /**
  * ubi_wl_flush - flush all pending works.
  * @ubi: UBI device description object
diff --git a/include/uapi/mtd/ubi-user.h b/include/uapi/mtd/ubi-user.h
index 1927b0d..6e1ec98 100644
--- a/include/uapi/mtd/ubi-user.h
+++ b/include/uapi/mtd/ubi-user.h
@@ -170,6 +170,9 @@
 /* Re-name volumes */
 #define UBI_IOCRNVOL _IOW(UBI_IOC_MAGIC, 3, struct ubi_rnvol_req)
 
+#define UBI_IOCRPEB _IOW(UBI_IOC_MAGIC, 4, __s32)
+#define UBI_IOCSPEB _IOW(UBI_IOC_MAGIC, 5, __s32)
+
 /* ioctl commands of the UBI control character device */
 
 #define UBI_CTRL_IOC_MAGIC 'o'
-- 
1.8.4.5

[PATCH 4/5] UBI: Add basic read counter support

[Richard Weinberger]

Read counters allow to deal better with read disturb.
If the thresholds are known, jeopardized PEBs can be
re-erased before data is lost.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/Kconfig | 12 ++++++++++++
 drivers/mtd/ubi/io.c    |  1 +
 drivers/mtd/ubi/ubi.h   |  5 +++++
 drivers/mtd/ubi/wl.c    | 37 +++++++++++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+)

diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig
index f0855ce..7a33cbf 100644
--- a/drivers/mtd/ubi/Kconfig
+++ b/drivers/mtd/ubi/Kconfig
@@ -103,4 +103,16 @@ config MTD_UBI_BLOCK
 
 	   If in doubt, say "N".
 
+config MTD_UBI_READ_COUNTER
+	bool "Maintain a per block read counter"
+	default n
+	help
+	   This option enables a per block read counter such that userspace can
+	   query how often a block has been read since erasure.
+	   Enable this if you plan to use UBI on MLC NAND in conjunction with
+	   ubihealthd. It will increase UBI's memory footprint by
+	   sizeof(unsigned int) * number of eraseblocks.
+
+	   If in doubt, say "N".
+
 endif # MTD_UBI
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 1fc23e4..2018bd1 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -164,6 +164,7 @@ int ubi_io_read(const struct ubi_device *ubi, void *buf, int pnum, int offset,
 
 	addr = (loff_t)pnum * ubi->peb_size + offset;
 retry:
+	ubi_wl_update_rc((struct ubi_device *)ubi, pnum);
 	err = mtd_read(ubi->mtd, addr, len, &read, buf);
 	if (err) {
 		const char *errstr = mtd_is_eccerr(err) ? " (ECC error)" : "";
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 40c9eeb..9bfede0 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -168,6 +168,7 @@ enum {
  * @u.list: link in the protection queue
  * @ec: erase counter
  * @pnum: physical eraseblock number
+ * @rc: number reads since last erasure
  *
  * This data structure is used in the WL sub-system. Each physical eraseblock
  * has a corresponding &struct wl_entry object which may be kept in different
@@ -180,6 +181,9 @@ struct ubi_wl_entry {
 	} u;
 	int ec;
 	int pnum;
+#ifdef CONFIG_MTD_UBI_READ_COUNTER
+	unsigned int rc;
+#endif
 };
 
 /**
@@ -860,6 +864,7 @@ int ubi_is_erase_work(struct ubi_work *wrk);
 void ubi_refill_pools(struct ubi_device *ubi);
 int ubi_ensure_anchor_pebs(struct ubi_device *ubi);
 int ubi_bitrot_check(struct ubi_device *ubi, int pnum, int force_scrub);
+void ubi_wl_update_rc(struct ubi_device *ubi, int pnum);
 
 /* io.c */
 int ubi_io_read(const struct ubi_device *ubi, void *buf, int pnum, int offset,
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index ad02c46..43466d4 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -447,6 +447,41 @@ static int prot_queue_del(struct ubi_device *ubi, int pnum)
 	return 0;
 }
 
+void ubi_wl_update_rc(struct ubi_device *ubi, int pnum)
+{
+#ifdef CONFIG_MTD_UBI_READ_COUNTER
+	struct ubi_wl_entry *e;
+
+	/*
+	 * WL not initialized yet.
+	 */
+	if (!ubi->lookuptbl)
+		return;
+
+	spin_lock(&ubi->wl_lock);
+	e = ubi->lookuptbl[pnum];
+	if (e)
+		e->rc++;
+	spin_unlock(&ubi->wl_lock);
+#endif
+}
+
+static void ubi_wl_clear_rc(struct ubi_wl_entry *e)
+{
+#ifdef CONFIG_MTD_UBI_READ_COUNTER
+	e->rc = 0;
+#endif
+}
+
+static void ubi_wl_get_rc(struct ubi_wl_entry *e, struct ubi_stats_entry *se)
+{
+#ifdef CONFIG_MTD_UBI_READ_COUNTER
+	se->rc = e->rc;
+#else
+	se->rc = -1;
+#endif
+}
+
 /**
  * sync_erase - synchronously erase a physical eraseblock.
  * @ubi: UBI device description object
@@ -477,6 +512,8 @@ static int sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
 	if (err < 0)
 		goto out_free;
 
+	ubi_wl_clear_rc(e);
+
 	ec += err;
 	if (ec > UBI_MAX_ERASECOUNTER) {
 		/*
-- 
1.8.4.5

[PATCH 5/5] UBI: Expose UBI statistics interface

[Richard Weinberger]

UBI_IOCSTATS allows userspace to query read and erase
counters and can take action.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/cdev.c      | 23 +++++++++++++++++
 drivers/mtd/ubi/ubi.h       |  1 +
 drivers/mtd/ubi/wl.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/mtd/ubi-user.h | 16 ++++++++++++
 4 files changed, 101 insertions(+)

diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index f500451..ab58c5f 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -992,6 +992,29 @@ static long ubi_cdev_ioctl(struct file *file, unsigned int cmd,
 		err = ubi_bitrot_check(ubi, pnum, 1);
 		break;
 	}
+	/* Get UBI stats */
+	case UBI_IOCSTATS:
+	{
+		struct ubi_stats_req *req;
+		struct ubi_stats_req __user *ureq = argp;
+
+		req = kmalloc(sizeof(struct ubi_stats_req), GFP_KERNEL);
+		if (!req) {
+			err = -ENOMEM;
+			break;
+		}
+
+		err = copy_from_user(req, argp, sizeof(struct ubi_stats_req));
+		if (err) {
+			kfree(req);
+			err = -EFAULT;
+			break;
+		}
+
+		err = ubi_wl_report_stats(ubi, req, ureq->stats);
+		kfree(req);
+		break;
+	}
 
 	default:
 		err = -ENOTTY;
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 9bfede0..c6c2964 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -865,6 +865,7 @@ void ubi_refill_pools(struct ubi_device *ubi);
 int ubi_ensure_anchor_pebs(struct ubi_device *ubi);
 int ubi_bitrot_check(struct ubi_device *ubi, int pnum, int force_scrub);
 void ubi_wl_update_rc(struct ubi_device *ubi, int pnum);
+int ubi_wl_report_stats(struct ubi_device *ubi, struct ubi_stats_req *req, struct ubi_stats_entry __user *se);
 
 /* io.c */
 int ubi_io_read(const struct ubi_device *ubi, void *buf, int pnum, int offset,
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 43466d4..90b715e 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -102,6 +102,7 @@
 #include <linux/crc32.h>
 #include <linux/freezer.h>
 #include <linux/kthread.h>
+#include <linux/uaccess.h>
 #include "ubi.h"
 #include "wl.h"
 
@@ -482,6 +483,66 @@ static void ubi_wl_get_rc(struct ubi_wl_entry *e, struct ubi_stats_entry *se)
 #endif
 }
 
+static int ubi_wl_fill_stats_entry(struct ubi_device *ubi,
+				   struct ubi_stats_entry *se, int pnum)
+{
+	struct ubi_wl_entry *e;
+
+	spin_lock(&ubi->wl_lock);
+	e = ubi->lookuptbl[pnum];
+	if (e) {
+		se->pnum = pnum;
+		se->ec = e->ec;
+		ubi_wl_get_rc(e, se);
+	}
+	spin_unlock(&ubi->wl_lock);
+
+	return e ? 0 : -1;
+}
+
+
+int ubi_wl_report_stats(struct ubi_device *ubi, struct ubi_stats_req *req,
+			struct ubi_stats_entry __user *se)
+{
+	int i, pnum, peb_end, peb_start;
+	struct ubi_stats_entry tmp_se;
+	size_t write_len;
+	int n = 0;
+
+	pnum = req->req_pnum;
+	if (pnum != -1) {
+		if (pnum < 0 || pnum >= ubi->peb_count)
+			return -EINVAL;
+
+		peb_start = pnum;
+		peb_end = pnum + 1;
+		write_len = sizeof(*se);
+	} else {
+		peb_start = 0;
+		peb_end = ubi->peb_count;
+		write_len = sizeof(*se) * ubi->good_peb_count;
+	}
+
+	if (write_len > req->req_len - sizeof(*req))
+		return -EFAULT;
+
+	if (!access_ok(VERIFY_WRITE, se, req->req_len - sizeof(*req) + write_len))
+		return -EFAULT;
+
+	for (i = peb_start; i < peb_end; i++) {
+		if (ubi_wl_fill_stats_entry(ubi, &tmp_se, i) == 0) {
+			if (__copy_to_user(se, &tmp_se, sizeof(tmp_se)))
+				return -EFAULT;
+
+			se++;
+			n++;
+		}
+	}
+
+	return n;
+}
+
+
 /**
  * sync_erase - synchronously erase a physical eraseblock.
  * @ubi: UBI device description object
diff --git a/include/uapi/mtd/ubi-user.h b/include/uapi/mtd/ubi-user.h
index 6e1ec98..8610440 100644
--- a/include/uapi/mtd/ubi-user.h
+++ b/include/uapi/mtd/ubi-user.h
@@ -173,6 +173,8 @@
 #define UBI_IOCRPEB _IOW(UBI_IOC_MAGIC, 4, __s32)
 #define UBI_IOCSPEB _IOW(UBI_IOC_MAGIC, 5, __s32)
 
+#define UBI_IOCSTATS _IOW(UBI_IOC_MAGIC, 6, struct ubi_stats_req)
+
 /* ioctl commands of the UBI control character device */
 
 #define UBI_CTRL_IOC_MAGIC 'o'
@@ -445,4 +447,18 @@ struct ubi_blkcreate_req {
 	__s8  padding[128];
 }  __packed;
 
+struct ubi_stats_entry {
+	__s32 pnum;
+	__s32 ec;
+	__s32 rc;
+	__s32 padding;
+} __packed;
+
+struct ubi_stats_req {
+	__s32 req_len;
+	__s32 req_pnum;
+	__s32 padding[2];
+	struct ubi_stats_entry stats[0];
+} __packed;
+
 #endif /* __UBI_USER_H__ */
-- 
1.8.4.5

Re: [PATCH 3/5] UBI: Expose the bitrot interface

[Artem Bityutskiy]

On Thu, 2015-11-05 at 23:56 +0100, Richard Weinberger wrote:
> +#define UBI_IOCRPEB _IOW(UBI_IOC_MAGIC, 4, __s32)
> +#define UBI_IOCSPEB _IOW(UBI_IOC_MAGIC, 5, __s32)

Could you please, add short comments telling what are these ioctls
about.

Re: [PATCH 3/5] UBI: Expose the bitrot interface

[Richard Weinberger]

Am 06.11.2015 um 12:59 schrieb Artem Bityutskiy:
> On Thu, 2015-11-05 at 23:56 +0100, Richard Weinberger wrote:
>> +#define UBI_IOCRPEB _IOW(UBI_IOC_MAGIC, 4, __s32)
>> +#define UBI_IOCSPEB _IOW(UBI_IOC_MAGIC, 5, __s32)
> 
> Could you please, add short comments telling what are these ioctls
> about.

Sure.

Something like:

+/* Trigger re-read of a given PEB */
+#define UBI_IOCRPEB _IOW(UBI_IOC_MAGIC, 4, __s32)
+/* Trigger scrubbing of a given PEB */
+#define UBI_IOCSPEB _IOW(UBI_IOC_MAGIC, 5, __s32)

?

While we're here, would it make sense to do a manpage for UBI's ioctl()s?

Thanks,
//richard

Re: [PATCH 3/5] UBI: Expose the bitrot interface

[Artem Bityutskiy]

All the comments in this e-mail are about improving commentaries and
naming conventions to make the code more consistent, may be a bit more
"formal", and more self-documenting.

On Thu, 2015-11-05 at 23:56 +0100, Richard Weinberger wrote:

> diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
> index d16fccf..f500451 100644
> --- a/drivers/mtd/ubi/cdev.c
> +++ b/drivers/mtd/ubi/cdev.c
> @@ -963,6 +963,36 @@ static long ubi_cdev_ioctl(struct file *file,
> unsigned int cmd,
>  		break;
>  	}
>  
> +	/* Read a PEB */

Naive reader's thoughts: Hmm, I thought tor read a PEB I can just read
from the character device. Hmm, there is an IOCTL for reading? And it
returns me the data?

Suggestion: please, improve the comment. Tell what it actually does and
why is it needed. Something like "Check whether the PEB has bit-flips
and hence, needs scrubbing" would do.

> +	case UBI_IOCRPEB:
> +	{
> +		int pnum;
> +
> +		err = get_user(pnum, (__user int32_t *)argp);
> +		if (err) {
> +			err = -EFAULT;
> +			break;
> +		}
> +
> +		err = ubi_bitrot_check(ubi, pnum, 0);

So I see 2 terms used: bitflips and bitrots. Would we please stick to
one of them, and I'd suggest the former, and leave the latter.

E.g.: ubi_check_bitflips() ?

> +	/* Scrub a PEB */

Naive reader's question: does it scrub unconditionally or only if there
are bit-flips. Is it really just scrub, or check-and-scrub?

Suggestion: improve the comment a bit.

> +	case UBI_IOCSPEB:
> +	{
> +		int pnum;
> +
> +		err = get_user(pnum, (__user int32_t *)argp);
> +		if (err) {
> +			err = -EFAULT;
> +			break;
> +		}
> +
> +		err = ubi_bitrot_check(ubi, pnum, 1);

Name is confusing, because it says "check", while the ioctl said
"scrub". But I guess we can leave with this, and I do not have better
ideas.

> +/**
> + * ubi_bitrot_check - Check an eraseblock for bitflips

But in the comment you do need to say something like 'check and scrub
an eraseblock' or something, so that it is clear this is not only about
'checking'.

> + * @pnum: the physical eraseblock to schedule
> + * @force_scrub: force scrubbing if non-zero
> + *

And here I'd put some additional comment about the function - what it
does, is scrubbing unconditional or not, etc.

Re: [PATCH 3/5] UBI: Expose the bitrot interface

[Artem Bityutskiy]

On Fri, 2015-11-06 at 13:14 +0100, Richard Weinberger wrote:
> Am 06.11.2015 um 12:59 schrieb Artem Bityutskiy:
> > On Thu, 2015-11-05 at 23:56 +0100, Richard Weinberger wrote:
> > > +#define UBI_IOCRPEB _IOW(UBI_IOC_MAGIC, 4, __s32)
> > > +#define UBI_IOCSPEB _IOW(UBI_IOC_MAGIC, 5, __s32)
> > 
> > Could you please, add short comments telling what are these ioctls
> > about.
> 
> Sure.
> 
> Something like:
> 
> +/* Trigger re-read of a given PEB */

Something more end-user oriented. Trigger re-read does not tell the end
user anything, but "check if a PEB has bitflips" does tell exactly why
the ioctl exists.

> +#define UBI_IOCRPEB _IOW(UBI_IOC_MAGIC, 4, __s32)
> +/* Trigger scrubbing of a given PEB */

Sounds good to me, although if you could also tell at the same time
whether this is unconditional or only if the PEB has bit-flips.

> +#define UBI_IOCSPEB _IOW(UBI_IOC_MAGIC, 5, __s32)

> While we're here, would it make sense to do a manpage for UBI's
> ioctl()s?

Well, more documentation is always a great thing, I invested a lot of
time into mtd web site at some point, and I think it payed off at the
end. Investing time into a man page would pay off too I believe.

Thanks!