linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] UBI: add debugfs file for tracking PEB state
@ 2016-09-20 20:45 Zach Brown
  2016-09-21 11:13 ` Richard Weinberger
  0 siblings, 1 reply; 5+ messages in thread
From: Zach Brown @ 2016-09-20 20:45 UTC (permalink / raw)
  To: dwmw2; +Cc: computersforpeace, dedekind1, richard, linux-mtd, linux-kernel

From: Ben Shelton <ben.shelton@ni.com>

Add a file under debugfs to allow easy access to the erase count for
each physical erase block on an UBI device.  This is useful when
debugging data integrity issues with UBIFS on NAND flash devices.

Signed-off-by: Ben Shelton <ben.shelton@ni.com>
Signed-off-by: Zach Brown <zach.brown@ni.com>
---
v2
 * Cast pointer in unsigned long instead of int to avoid build warning
 * Use ubi->lookuptbl[] to get erase counter instead of reading from flash


 drivers/mtd/ubi/debug.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 144 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index f101a49..57d255c 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -22,6 +22,7 @@
 #include <linux/debugfs.h>
 #include <linux/uaccess.h>
 #include <linux/module.h>
+#include <linux/seq_file.h>
 
 
 /**
@@ -386,7 +387,9 @@ out:
 	return count;
 }
 
-/* File operations for all UBI debugfs files */
+/* File operations for all UBI debugfs files except
+ * detailed_erase_block_info
+ */
 static const struct file_operations dfs_fops = {
 	.read   = dfs_file_read,
 	.write  = dfs_file_write,
@@ -395,6 +398,140 @@ static const struct file_operations dfs_fops = {
 	.owner  = THIS_MODULE,
 };
 
+/* As long as the position is less then that total number of erase blocks,
+ * we still have more to print.
+ */
+static void *eraseblk_count_seq_start(struct seq_file *s, loff_t *pos)
+{
+	struct ubi_device *ubi = s->private;
+
+	if (*pos == 0)
+		return SEQ_START_TOKEN;
+
+	if (*pos < ubi->peb_count)
+		return pos;
+
+	return NULL;
+}
+
+/* Since we are using the position as the iterator, we just need to check if we
+ * are done and increment the position.
+ */
+static void *eraseblk_count_seq_next(struct seq_file *s, void *v, loff_t *pos)
+{
+	struct ubi_device *ubi = s->private;
+
+	if (v == SEQ_START_TOKEN)
+		return pos;
+	(*pos)++;
+
+	if (*pos < ubi->peb_count)
+		return pos;
+
+	return NULL;
+}
+
+static void eraseblk_count_seq_stop(struct seq_file *s, void *v)
+{
+}
+
+enum block_status {
+	BLOCK_STATUS_OK,
+	BLOCK_STATUS_BAD_BLOCK,
+	BLOCK_STATUS_ERASE_COUNT_BEYOND_MAX
+};
+
+static char const *block_status_names[] = {"OK", "marked_bad",
+					   "erase_count_beyond_max"};
+
+enum read_status {
+	READ_STATUS_OK,
+	READ_STATUS_ERR_READING_BLOCK,
+	READ_STATUS_ERR_READING_ERASE_COUNT
+};
+
+static char const *read_status_names[] = {"OK", "err_reading_block",
+					  "err_reading_erase_count"};
+
+static int eraseblk_count_seq_show(struct seq_file *s, void *iter)
+{
+	struct ubi_device *ubi = s->private;
+	struct ubi_wl_entry *wl;
+	int *block_number = iter;
+	int erase_count = -1;
+	enum block_status b_sts = BLOCK_STATUS_OK;
+	enum read_status r_sts = READ_STATUS_OK;
+	int err;
+
+	/* If this is the start, print a header */
+	if (iter == SEQ_START_TOKEN) {
+		seq_puts(s,
+			 "physical_block_number\terase_count\tblock_status\tread_status\n");
+		return 0;
+	}
+
+	err = ubi_io_is_bad(ubi, *block_number);
+	if (err) {
+		if (err < 0)
+			r_sts = READ_STATUS_ERR_READING_BLOCK;
+		else
+			b_sts = BLOCK_STATUS_BAD_BLOCK;
+	} else {
+		wl = ubi->lookuptbl[*block_number];
+		erase_count = wl->ec;
+		if (erase_count > UBI_MAX_ERASECOUNTER)
+			b_sts = BLOCK_STATUS_ERASE_COUNT_BEYOND_MAX;
+	}
+
+	seq_printf(s, "%-22d\t%-11d\t%-12s\t%-12s\n", *block_number,
+		   erase_count, block_status_names[b_sts],
+		   read_status_names[r_sts]);
+	return 0;
+}
+
+static const struct seq_operations eraseblk_count_seq_ops = {
+	.start = eraseblk_count_seq_start,
+	.next = eraseblk_count_seq_next,
+	.stop = eraseblk_count_seq_stop,
+	.show = eraseblk_count_seq_show
+};
+
+static int eraseblk_count_open(struct inode *inode, struct file *f)
+{
+	struct seq_file *s;
+	int err;
+
+	err = seq_open(f, &eraseblk_count_seq_ops);
+	if (err)
+		return err;
+
+	s = f->private_data;
+	s->private = ubi_get_device((unsigned long)inode->i_private);
+
+	if (!s->private)
+		return -ENODEV;
+	else
+		return 0;
+}
+
+static int eraseblk_count_release(struct inode *inode, struct file *f)
+{
+	struct seq_file *s = f->private_data;
+	struct ubi_device *ubi = s->private;
+
+	ubi_put_device(ubi);
+
+	return seq_release(inode, f);
+}
+
+static const struct file_operations eraseblk_count_fops = {
+	.owner = THIS_MODULE,
+	.open = eraseblk_count_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = eraseblk_count_release,
+};
+
 /**
  * ubi_debugfs_init_dev - initialize debugfs for an UBI device.
  * @ubi: UBI device description object
@@ -491,6 +628,12 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi)
 		goto out_remove;
 	d->dfs_power_cut_max = dent;
 
+	fname = "detailed_erase_block_info";
+	dent = debugfs_create_file(fname, S_IRUSR, d->dfs_dir, (void *)ubi_num,
+				   &eraseblk_count_fops);
+	if (IS_ERR_OR_NULL(dent))
+		goto out_remove;
+
 	return 0;
 
 out_remove:
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] UBI: add debugfs file for tracking PEB state
  2016-09-20 20:45 [PATCH v2] UBI: add debugfs file for tracking PEB state Zach Brown
@ 2016-09-21 11:13 ` Richard Weinberger
  2016-09-21 15:47   ` Zach Brown
  0 siblings, 1 reply; 5+ messages in thread
From: Richard Weinberger @ 2016-09-21 11:13 UTC (permalink / raw)
  To: Zach Brown, dwmw2; +Cc: computersforpeace, dedekind1, linux-mtd, linux-kernel

Zach,

On 20.09.2016 22:45, Zach Brown wrote:
> From: Ben Shelton <ben.shelton@ni.com>
> 
> Add a file under debugfs to allow easy access to the erase count for
> each physical erase block on an UBI device.  This is useful when
> debugging data integrity issues with UBIFS on NAND flash devices.
> 
> Signed-off-by: Ben Shelton <ben.shelton@ni.com>
> Signed-off-by: Zach Brown <zach.brown@ni.com>
> ---
> v2
>  * Cast pointer in unsigned long instead of int to avoid build warning
>  * Use ubi->lookuptbl[] to get erase counter instead of reading from flash
> 
> 

[...]

> +enum block_status {
> +	BLOCK_STATUS_OK,
> +	BLOCK_STATUS_BAD_BLOCK,
> +	BLOCK_STATUS_ERASE_COUNT_BEYOND_MAX
> +};

Do you plan to add more states?
In UBI a block can have much more states.
I'd like to see all states, free, in protection, used, bad, corrupted, scrub, etc...

AFAIK BLOCK_STATUS_ERASE_COUNT_BEYOND_MAX is also unreachable since UBI aborts before that.

> +static char const *block_status_names[] = {"OK", "marked_bad",
> +					   "erase_count_beyond_max"};
> +
> +enum read_status {
> +	READ_STATUS_OK,
> +	READ_STATUS_ERR_READING_BLOCK,
> +	READ_STATUS_ERR_READING_ERASE_COUNT
> +};
>

READ_STATUS_ERR_READING_ERASE_COUNT is now no longer needed, right?

> +static char const *read_status_names[] = {"OK", "err_reading_block",
> +					  "err_reading_erase_count"};
> +
> +static int eraseblk_count_seq_show(struct seq_file *s, void *iter)
> +{
> +	struct ubi_device *ubi = s->private;
> +	struct ubi_wl_entry *wl;
> +	int *block_number = iter;
> +	int erase_count = -1;
> +	enum block_status b_sts = BLOCK_STATUS_OK;
> +	enum read_status r_sts = READ_STATUS_OK;
> +	int err;
> +
> +	/* If this is the start, print a header */
> +	if (iter == SEQ_START_TOKEN) {
> +		seq_puts(s,
> +			 "physical_block_number\terase_count\tblock_status\tread_status\n");
> +		return 0;
> +	}
> +
> +	err = ubi_io_is_bad(ubi, *block_number);
> +	if (err) {
> +		if (err < 0)
> +			r_sts = READ_STATUS_ERR_READING_BLOCK;
> +		else
> +			b_sts = BLOCK_STATUS_BAD_BLOCK;
> +	} else {
> +		wl = ubi->lookuptbl[*block_number];
> +		erase_count = wl->ec;

What about locking? :-)
This is racy.
You need at least wl_lock. Otherwise wl might disappear under you.
And ->lookuptbl[] can return a NULL object too.

Thanks,
//richard

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] UBI: add debugfs file for tracking PEB state
  2016-09-21 11:13 ` Richard Weinberger
@ 2016-09-21 15:47   ` Zach Brown
  0 siblings, 0 replies; 5+ messages in thread
From: Zach Brown @ 2016-09-21 15:47 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: dwmw2, computersforpeace, dedekind1, linux-mtd, linux-kernel

On Wed, Sep 21, 2016 at 01:13:29PM +0200, Richard Weinberger wrote:
> Zach,
>
> On 20.09.2016 22:45, Zach Brown wrote:
> > From: Ben Shelton <ben.shelton@ni.com>
> >
> > Add a file under debugfs to allow easy access to the erase count for
> > each physical erase block on an UBI device.  This is useful when
> > debugging data integrity issues with UBIFS on NAND flash devices.
> >
> > Signed-off-by: Ben Shelton <ben.shelton@ni.com>
> > Signed-off-by: Zach Brown <zach.brown@ni.com>
> > ---
> > v2
> >  * Cast pointer in unsigned long instead of int to avoid build warning
> >  * Use ubi->lookuptbl[] to get erase counter instead of reading from flash
> >
> >
>
> [...]
>
> > +enum block_status {
> > +	BLOCK_STATUS_OK,
> > +	BLOCK_STATUS_BAD_BLOCK,
> > +	BLOCK_STATUS_ERASE_COUNT_BEYOND_MAX
> > +};
>
> Do you plan to add more states?
> In UBI a block can have much more states.
> I'd like to see all states, free, in protection, used, bad, corrupted, scrub, etc...
>

Adding more states sounds like a good idea, but I'm not sure how to get that
information. If I made the in_wl_tree function accessible I could use that to
get the information, but I'd have to check each RB Tree. Do you have a
suggestion?

> AFAIK BLOCK_STATUS_ERASE_COUNT_BEYOND_MAX is also unreachable since UBI aborts before that.
>
Do you mean that the block would've have already been marked as bad? So there's
no point checking?


> What about locking? :-)
> This is racy.
> You need at least wl_lock. Otherwise wl might disappear under you.
> And ->lookuptbl[] can return a NULL object too.
>
Do you know what ->lookuptbl[] returning NULL would signify about the state of
the block? Currently I'm thinking of treating as a bad read status and letting
the show function move on.

> Thanks,
> //richard

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] UBI: add debugfs file for tracking PEB state
  2017-03-24 18:23 Zach Brown
@ 2017-03-25 15:23 ` kbuild test robot
  0 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2017-03-25 15:23 UTC (permalink / raw)
  To: Zach Brown
  Cc: kbuild-all, dwmw2, computersforpeace, boris.brezillon,
	marek.vasut, richard, cyrille.pitchen, dedekind1, linux-mtd,
	linux-kernel

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

Hi Ben,

[auto build test WARNING on v4.9-rc8]
[also build test WARNING on next-20170324]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Zach-Brown/UBI-add-debugfs-file-for-tracking-PEB-state/20170325-205127
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/mtd/ubi/debug.c: In function 'eraseblk_count_seq_show':
>> drivers/mtd/ubi/debug.c:465:19: warning: comparison of constant '0' with boolean expression is always false [-Wbool-compare]
     if (!erase_count < 0)
                      ^
>> drivers/mtd/ubi/debug.c:465:19: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]

vim +/0 +465 drivers/mtd/ubi/debug.c

   449				 "physical_block_number\terase_count\tblock_status\tread_status\n");
   450			return 0;
   451		}
   452	
   453		err = ubi_io_is_bad(ubi, *block_number);
   454		if (err)
   455			return err;
   456	
   457		spin_lock(&ubi->wl_lock);
   458	
   459		wl = ubi->lookuptbl[*block_number];
   460		if (wl)
   461			erase_count = wl->ec;
   462	
   463		spin_unlock(&ubi->wl_lock);
   464	
 > 465		if (!erase_count < 0)
   466			return 0;
   467	
   468		seq_printf(s, "%-22d\t%-11d\n", *block_number, erase_count);
   469	
   470		return 0;
   471	}
   472	
   473	static const struct seq_operations eraseblk_count_seq_ops = {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38054 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] UBI: add debugfs file for tracking PEB state
@ 2017-03-24 18:23 Zach Brown
  2017-03-25 15:23 ` kbuild test robot
  0 siblings, 1 reply; 5+ messages in thread
From: Zach Brown @ 2017-03-24 18:23 UTC (permalink / raw)
  To: dwmw2
  Cc: computersforpeace, boris.brezillon, marek.vasut, richard,
	cyrille.pitchen, dedekind1, linux-mtd, linux-kernel

From: Ben Shelton <ben.shelton@ni.com>

Add a file under debugfs to allow easy access to the erase count for
each physical erase block on an UBI device.  This is useful when
debugging data integrity issues with UBIFS on NAND flash devices.

Signed-off-by: Ben Shelton <ben.shelton@ni.com>
Signed-off-by: Zach Brown <zach.brown@ni.com>

v2:
* If ubi_io_is_bad eraseblk_count_seq_show just returns the err.
* if ubi->lookuptbl returns null, its no longer treated as an error
  instead info for that block is not printeded
* Removed check for UBI_MAX_ERASECOUNTER since it is impossible to hit
* Removed block state from print, if a block is printed then it is good and if
  it is not printed, then it is bad.

---
 drivers/mtd/ubi/debug.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 125 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index f101a49..5f2b65d 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -22,6 +22,7 @@
 #include <linux/debugfs.h>
 #include <linux/uaccess.h>
 #include <linux/module.h>
+#include <linux/seq_file.h>


 /**
@@ -386,7 +387,9 @@ static ssize_t dfs_file_write(struct file *file, const char __user *user_buf,
 	return count;
 }

-/* File operations for all UBI debugfs files */
+/* File operations for all UBI debugfs files except
+ * detailed_erase_block_info
+ */
 static const struct file_operations dfs_fops = {
 	.read   = dfs_file_read,
 	.write  = dfs_file_write,
@@ -395,6 +398,121 @@ static const struct file_operations dfs_fops = {
 	.owner  = THIS_MODULE,
 };

+/* As long as the position is less then that total number of erase blocks,
+ * we still have more to print.
+ */
+static void *eraseblk_count_seq_start(struct seq_file *s, loff_t *pos)
+{
+	struct ubi_device *ubi = s->private;
+
+	if (*pos == 0)
+		return SEQ_START_TOKEN;
+
+	if (*pos < ubi->peb_count)
+		return pos;
+
+	return NULL;
+}
+
+/* Since we are using the position as the iterator, we just need to check if we
+ * are done and increment the position.
+ */
+static void *eraseblk_count_seq_next(struct seq_file *s, void *v, loff_t *pos)
+{
+	struct ubi_device *ubi = s->private;
+
+	if (v == SEQ_START_TOKEN)
+		return pos;
+	(*pos)++;
+
+	if (*pos < ubi->peb_count)
+		return pos;
+
+	return NULL;
+}
+
+static void eraseblk_count_seq_stop(struct seq_file *s, void *v)
+{
+}
+
+static int eraseblk_count_seq_show(struct seq_file *s, void *iter)
+{
+	struct ubi_device *ubi = s->private;
+	struct ubi_wl_entry *wl;
+	int *block_number = iter;
+	int erase_count = -1;
+	int err;
+
+	/* If this is the start, print a header */
+	if (iter == SEQ_START_TOKEN) {
+		seq_puts(s,
+			 "physical_block_number\terase_count\tblock_status\tread_status\n");
+		return 0;
+	}
+
+	err = ubi_io_is_bad(ubi, *block_number);
+	if (err)
+		return err;
+
+	spin_lock(&ubi->wl_lock);
+
+	wl = ubi->lookuptbl[*block_number];
+	if (wl)
+		erase_count = wl->ec;
+
+	spin_unlock(&ubi->wl_lock);
+
+	if (!erase_count < 0)
+		return 0;
+
+	seq_printf(s, "%-22d\t%-11d\n", *block_number, erase_count);
+
+	return 0;
+}
+
+static const struct seq_operations eraseblk_count_seq_ops = {
+	.start = eraseblk_count_seq_start,
+	.next = eraseblk_count_seq_next,
+	.stop = eraseblk_count_seq_stop,
+	.show = eraseblk_count_seq_show
+};
+
+static int eraseblk_count_open(struct inode *inode, struct file *f)
+{
+	struct seq_file *s;
+	int err;
+
+	err = seq_open(f, &eraseblk_count_seq_ops);
+	if (err)
+		return err;
+
+	s = f->private_data;
+	s->private = ubi_get_device((unsigned long)inode->i_private);
+
+	if (!s->private)
+		return -ENODEV;
+	else
+		return 0;
+}
+
+static int eraseblk_count_release(struct inode *inode, struct file *f)
+{
+	struct seq_file *s = f->private_data;
+	struct ubi_device *ubi = s->private;
+
+	ubi_put_device(ubi);
+
+	return seq_release(inode, f);
+}
+
+static const struct file_operations eraseblk_count_fops = {
+	.owner = THIS_MODULE,
+	.open = eraseblk_count_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = eraseblk_count_release,
+};
+
 /**
  * ubi_debugfs_init_dev - initialize debugfs for an UBI device.
  * @ubi: UBI device description object
@@ -491,6 +609,12 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi)
 		goto out_remove;
 	d->dfs_power_cut_max = dent;

+	fname = "detailed_erase_block_info";
+	dent = debugfs_create_file(fname, S_IRUSR, d->dfs_dir, (void *)ubi_num,
+				   &eraseblk_count_fops);
+	if (IS_ERR_OR_NULL(dent))
+		goto out_remove;
+
 	return 0;

 out_remove:
--
2.7.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-03-25 15:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-20 20:45 [PATCH v2] UBI: add debugfs file for tracking PEB state Zach Brown
2016-09-21 11:13 ` Richard Weinberger
2016-09-21 15:47   ` Zach Brown
2017-03-24 18:23 Zach Brown
2017-03-25 15:23 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).