This simple MTD tests allows the user to see when read disturb happens.
By reading blocks over and over it reports flipped bits.
Currently it reports only flipped bits of the worst page of a block.
If within block X page P1 has 3 bit flips and P6 4, it will report 4.
By default every 50th block is read.
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/mtd/tests/Makefile | 2 +
drivers/mtd/tests/readdisturbtest.c | 246 ++++++++++++++++++++++++++++++++++++
2 files changed, 248 insertions(+)
create mode 100644 drivers/mtd/tests/readdisturbtest.c
diff --git a/drivers/mtd/tests/Makefile b/drivers/mtd/tests/Makefile
index 937a829..d9cb76a 100644
--- a/drivers/mtd/tests/Makefile
+++ b/drivers/mtd/tests/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_MTD_TESTS) += mtd_subpagetest.o
obj-$(CONFIG_MTD_TESTS) += mtd_torturetest.o
obj-$(CONFIG_MTD_TESTS) += mtd_nandecctest.o
obj-$(CONFIG_MTD_TESTS) += mtd_nandbiterrs.o
+obj-$(CONFIG_MTD_TESTS) += mtd_readdisturbtest.o
mtd_oobtest-objs := oobtest.o mtd_test.o
mtd_pagetest-objs := pagetest.o mtd_test.o
@@ -16,3 +17,4 @@ mtd_stresstest-objs := stresstest.o mtd_test.o
mtd_subpagetest-objs := subpagetest.o mtd_test.o
mtd_torturetest-objs := torturetest.o mtd_test.o
mtd_nandbiterrs-objs := nandbiterrs.o mtd_test.o
+mtd_readdisturbtest-objs := readdisturbtest.o mtd_test.o
diff --git a/drivers/mtd/tests/readdisturbtest.c b/drivers/mtd/tests/readdisturbtest.c
new file mode 100644
index 0000000..c22caf6
--- /dev/null
+++ b/drivers/mtd/tests/readdisturbtest.c
@@ -0,0 +1,246 @@
+/*
+ * Copyright (C) 2006-2008 Nokia Corporation
+ * Copyright (C) 2015 sigma star gmbh
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * Check MTD device for read disturb.
+ *
+ * Author: Richard Weinberger <[email protected]>
+ *
+ * Based on mtd_readtest.c and mtd_pagetest.c
+ * Author: Adrian Hunter <[email protected]>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/mtd/mtd.h>
+#include <linux/random.h>
+#include <linux/slab.h>
+
+#include "mtd_test.h"
+
+static int dev = -EINVAL;
+module_param(dev, int, S_IRUGO);
+MODULE_PARM_DESC(dev, "MTD device number to use");
+
+static int skip_blocks = 50;
+module_param(skip_blocks, int, S_IRUGO);
+MODULE_PARM_DESC(skip_blocks, "Test every n-th block, default is 50");
+
+static struct mtd_info *mtd;
+static unsigned char *iobuf;
+static unsigned char *iobuf_orig;
+static unsigned char *bbt;
+static unsigned long *bit_flips;
+
+static int pgsize;
+static int ebcnt;
+static int pgcnt;
+
+static struct rnd_state rnd_state;
+
+static int check_buf(unsigned char *buf, unsigned char *buf2, size_t len)
+{
+ int i;
+ int flips = 0;
+
+ for (i = 0; i < len; i++)
+ if (buf[i] != buf2[i])
+ flips += hweight8(buf[i] ^ buf2[i]);
+
+ return flips;
+}
+
+static int read_eraseblock_by_page(int ebnum, unsigned long iteration)
+{
+ size_t read;
+ int i, ret;
+ loff_t addr = ebnum * mtd->erasesize;
+ void *buf = iobuf;
+ struct mtd_oob_ops ops;
+ int flips;
+
+ for (i = 0; i < pgcnt; i++) {
+ ops.mode = MTD_OPS_RAW;
+ ops.len = pgsize;
+ ops.oobbuf = NULL;
+ ops.datbuf = buf;
+
+ ret = mtd_read_oob(mtd, addr, &ops);
+ read = ops.retlen;
+ if (ret || read != pgsize) {
+ pr_info("error: read failed at %#llx, block:%i, ret:%i, read:%zu\n",
+ (long long)addr, ebnum, ret, read);
+
+ return ret ?: -EIO;
+ }
+
+ flips = check_buf(buf, iobuf_orig + (pgsize * i), pgsize);
+
+ /*
+ * We count bit flips only per block. Worst page dominates.
+ */
+ if (flips > bit_flips[ebnum]) {
+ bit_flips[ebnum] = flips;
+ pr_info("Detected %i flipped bits at %#llx, block %i after %lu reads\n",
+ flips, addr, ebnum, iteration);
+ }
+
+ addr += pgsize;
+ buf += pgsize;
+ }
+
+ return 0;
+}
+
+static int __init mtd_readdisturbtest_init(void)
+{
+ uint64_t tmp;
+ unsigned long iteration = 0;
+ int err, i, ret = 0;
+
+ pr_info("\n");
+ pr_info("=================================================\n");
+
+ if (dev < 0) {
+ pr_info("Please specify a valid mtd-device via module paramter\n");
+ return -EINVAL;
+ }
+
+ pr_info("MTD device: %d\n", dev);
+
+ mtd = get_mtd_device(NULL, dev);
+ if (IS_ERR(mtd)) {
+ err = PTR_ERR(mtd);
+ pr_info("error: Cannot get MTD device\n");
+ return err;
+ }
+
+ if (mtd->writesize == 1) {
+ pr_info("not NAND flash, assume page size is 512 "
+ "bytes.\n");
+ pgsize = 512;
+ } else
+ pgsize = mtd->writesize;
+
+ tmp = mtd->size;
+ do_div(tmp, mtd->erasesize);
+ ebcnt = tmp;
+ pgcnt = mtd->erasesize / pgsize;
+
+ pr_info("MTD device size %llu, eraseblock size %u, "
+ "page size %u, count of eraseblocks %u, pages per "
+ "eraseblock %u, OOB size %u\n",
+ (unsigned long long)mtd->size, mtd->erasesize,
+ pgsize, ebcnt, pgcnt, mtd->oobsize);
+
+ err = -ENOMEM;
+ iobuf = kmalloc(mtd->erasesize, GFP_KERNEL);
+ if (!iobuf)
+ goto out;
+
+ iobuf_orig = kmalloc(mtd->erasesize, GFP_KERNEL);
+ if (!iobuf_orig)
+ goto out;
+
+ prandom_bytes_state(&rnd_state, iobuf_orig, mtd->erasesize);
+
+ bit_flips = kcalloc(ebcnt, sizeof(unsigned long), GFP_KERNEL);
+ if (!bit_flips)
+ goto out;
+
+ bbt = kzalloc(ebcnt, GFP_KERNEL);
+ if (!bbt)
+ goto out;
+
+ err = mtdtest_scan_for_bad_eraseblocks(mtd, bbt, 0, ebcnt);
+ if (err)
+ goto out;
+
+ pr_info("erasing and programming flash\n");
+ for (i = 0; i < ebcnt; ++i) {
+ if (skip_blocks && i % skip_blocks != 0)
+ continue;
+
+ if (bbt[i])
+ continue;
+
+ ret = mtdtest_erase_eraseblock(mtd, i);
+ if (ret) {
+ err = ret;
+ goto out;
+ }
+
+ ret = mtdtest_write(mtd, i * mtd->erasesize, mtd->erasesize,
+ iobuf_orig);
+ if (ret) {
+ err = ret;
+ goto out;
+ }
+
+ ret = mtdtest_relax();
+ if (ret)
+ goto out;
+ }
+
+ pr_info("starting read disturb test on every %ith block\n",
+ skip_blocks);
+ while (!ret) {
+ for (i = 0; i < ebcnt; ++i) {
+ if (skip_blocks && i % skip_blocks != 0)
+ continue;
+
+ if (bbt[i])
+ continue;
+
+ ret = read_eraseblock_by_page(i, iteration);
+
+ ret = mtdtest_relax();
+ if (ret)
+ goto out;
+ }
+
+ iteration++;
+ if (iteration % 1000 == 0)
+ pr_info("iteration %lu started\n", iteration);
+ }
+
+ if (err)
+ pr_info("finished with errors\n");
+ else
+ pr_info("finished\n");
+
+out:
+
+ kfree(bit_flips);
+ kfree(iobuf);
+ kfree(iobuf_orig);
+ kfree(bbt);
+ put_mtd_device(mtd);
+ if (err)
+ pr_info("error %i occurred\n", err);
+ pr_info("=================================================\n");
+ return err;
+}
+module_init(mtd_readdisturbtest_init);
+
+static void __exit mtd_readdisturbtest_exit(void)
+{
+}
+module_exit(mtd_readdisturbtest_exit);
+
+MODULE_DESCRIPTION("Read disturb test module");
+MODULE_AUTHOR("Richard Weinberger");
+MODULE_LICENSE("GPL");
--
1.8.4.5
On Thu, Apr 2, 2015 at 11:13 AM, Richard Weinberger <[email protected]> wrote:
> + err = -ENOMEM;
> + iobuf = kmalloc(mtd->erasesize, GFP_KERNEL);
> + if (!iobuf)
> + goto out;
The error handling here does not look right.
> +
> + iobuf_orig = kmalloc(mtd->erasesize, GFP_KERNEL);
> + if (!iobuf_orig)
> + goto out;
> +
> + prandom_bytes_state(&rnd_state, iobuf_orig, mtd->erasesize);
> +
> + bit_flips = kcalloc(ebcnt, sizeof(unsigned long), GFP_KERNEL);
> + if (!bit_flips)
> + goto out;
> +
> + bbt = kzalloc(ebcnt, GFP_KERNEL);
> + if (!bbt)
> + goto out;
> +
> + err = mtdtest_scan_for_bad_eraseblocks(mtd, bbt, 0, ebcnt);
> + if (err)
> + goto out;
> +
> + pr_info("erasing and programming flash\n");
> + for (i = 0; i < ebcnt; ++i) {
> + if (skip_blocks && i % skip_blocks != 0)
> + continue;
> +
> + if (bbt[i])
> + continue;
> +
> + ret = mtdtest_erase_eraseblock(mtd, i);
> + if (ret) {
> + err = ret;
> + goto out;
> + }
> +
> + ret = mtdtest_write(mtd, i * mtd->erasesize, mtd->erasesize,
> + iobuf_orig);
> + if (ret) {
> + err = ret;
> + goto out;
> + }
> +
> + ret = mtdtest_relax();
> + if (ret)
> + goto out;
> + }
> +
> + pr_info("starting read disturb test on every %ith block\n",
> + skip_blocks);
> + while (!ret) {
> + for (i = 0; i < ebcnt; ++i) {
> + if (skip_blocks && i % skip_blocks != 0)
> + continue;
> +
> + if (bbt[i])
> + continue;
> +
> + ret = read_eraseblock_by_page(i, iteration);
> +
> + ret = mtdtest_relax();
> + if (ret)
> + goto out;
> + }
> +
> + iteration++;
> + if (iteration % 1000 == 0)
> + pr_info("iteration %lu started\n", iteration);
> + }
> +
> + if (err)
> + pr_info("finished with errors\n");
> + else
> + pr_info("finished\n");
> +
> +out:
> +
> + kfree(bit_flips);
> + kfree(iobuf);
> + kfree(iobuf_orig);
> + kfree(bbt);
,as you have a single label to handle all the free's.
Am 02.04.2015 um 16:32 schrieb Fabio Estevam:
> On Thu, Apr 2, 2015 at 11:13 AM, Richard Weinberger <[email protected]> wrote:
>
>> + err = -ENOMEM;
>> + iobuf = kmalloc(mtd->erasesize, GFP_KERNEL);
>> + if (!iobuf)
>> + goto out;
>
> The error handling here does not look right.
>
>> +
>> + iobuf_orig = kmalloc(mtd->erasesize, GFP_KERNEL);
>> + if (!iobuf_orig)
>> + goto out;
>> +
>> + prandom_bytes_state(&rnd_state, iobuf_orig, mtd->erasesize);
>> +
>> + bit_flips = kcalloc(ebcnt, sizeof(unsigned long), GFP_KERNEL);
>> + if (!bit_flips)
>> + goto out;
>> +
>> + bbt = kzalloc(ebcnt, GFP_KERNEL);
>> + if (!bbt)
>> + goto out;
>> +
>> + err = mtdtest_scan_for_bad_eraseblocks(mtd, bbt, 0, ebcnt);
>> + if (err)
>> + goto out;
>> +
>> + pr_info("erasing and programming flash\n");
>> + for (i = 0; i < ebcnt; ++i) {
>> + if (skip_blocks && i % skip_blocks != 0)
>> + continue;
>> +
>> + if (bbt[i])
>> + continue;
>> +
>> + ret = mtdtest_erase_eraseblock(mtd, i);
>> + if (ret) {
>> + err = ret;
>> + goto out;
>> + }
>> +
>> + ret = mtdtest_write(mtd, i * mtd->erasesize, mtd->erasesize,
>> + iobuf_orig);
>> + if (ret) {
>> + err = ret;
>> + goto out;
>> + }
>> +
>> + ret = mtdtest_relax();
>> + if (ret)
>> + goto out;
>> + }
>> +
>> + pr_info("starting read disturb test on every %ith block\n",
>> + skip_blocks);
>> + while (!ret) {
>> + for (i = 0; i < ebcnt; ++i) {
>> + if (skip_blocks && i % skip_blocks != 0)
>> + continue;
>> +
>> + if (bbt[i])
>> + continue;
>> +
>> + ret = read_eraseblock_by_page(i, iteration);
>> +
>> + ret = mtdtest_relax();
>> + if (ret)
>> + goto out;
>> + }
>> +
>> + iteration++;
>> + if (iteration % 1000 == 0)
>> + pr_info("iteration %lu started\n", iteration);
>> + }
>> +
>> + if (err)
>> + pr_info("finished with errors\n");
>> + else
>> + pr_info("finished\n");
>> +
>> +out:
>> +
>> + kfree(bit_flips);
>> + kfree(iobuf);
>> + kfree(iobuf_orig);
>> + kfree(bbt);
>
> ,as you have a single label to handle all the free's.
Why? Free()ing a NULL pointer is perfectly fine.
What did I miss? :)
Thanks,
//richard
On Thu, Apr 2, 2015 at 11:33 AM, Richard Weinberger <[email protected]> wrote:
> Why? Free()ing a NULL pointer is perfectly fine.
> What did I miss? :)
If the first 'iobuf = kmalloc(mtd->erasesize, GFP_KERNEL);' fails then
you jump to the out label where you call 5 kfree() and then return the
error.
It would be much better just to return the error immediately in this
case and add one label for each allocation error, so that it only
kfree the previous successful allocations.
Am 02.04.2015 um 16:45 schrieb Fabio Estevam:
> On Thu, Apr 2, 2015 at 11:33 AM, Richard Weinberger <[email protected]> wrote:
>
>> Why? Free()ing a NULL pointer is perfectly fine.
>> What did I miss? :)
>
> If the first 'iobuf = kmalloc(mtd->erasesize, GFP_KERNEL);' fails then
> you jump to the out label where you call 5 kfree() and then return the
> error.
>
> It would be much better just to return the error immediately in this
> case and add one label for each allocation error, so that it only
> kfree the previous successful allocations.
It is not *much* better. It is just a matter of taste.
Thanks,
//richard
On Thu, Apr 2, 2015 at 11:13 AM, Richard Weinberger <[email protected]> wrote:
> + ret = mtdtest_erase_eraseblock(mtd, i);
> + if (ret) {
> + err = ret;
> + goto out;
> + }
Why not just do like this instead?
err = mtdtest_erase_eraseblock(mtd, i);
if (err)
goto out;
> +
> + ret = mtdtest_write(mtd, i * mtd->erasesize, mtd->erasesize,
> + iobuf_orig);
> + if (ret) {
> + err = ret;
> + goto out;
> + }
Same here.
> + ret = mtdtest_relax();
> + if (ret)
> + goto out;
Here you propagate the wrong error. You test for 'ret' and propagate 'err'.
On Thu, Apr 2, 2015 at 11:56 AM, Richard Weinberger <[email protected]> wrote:
> It is not *much* better. It is just a matter of taste.
... and instructions cycles as well ;-)
Am 02.04.2015 um 17:02 schrieb Fabio Estevam:
> On Thu, Apr 2, 2015 at 11:13 AM, Richard Weinberger <[email protected]> wrote:
>
>> + ret = mtdtest_erase_eraseblock(mtd, i);
>> + if (ret) {
>> + err = ret;
>> + goto out;
>> + }
>
> Why not just do like this instead?
>
> err = mtdtest_erase_eraseblock(mtd, i);
> if (err)
> goto out;
>
>> +
>> + ret = mtdtest_write(mtd, i * mtd->erasesize, mtd->erasesize,
>> + iobuf_orig);
>> + if (ret) {
>> + err = ret;
>> + goto out;
>> + }
>
> Same here.
The real question is why did I use ret and err at all? ;)
This test is based on existing tests, thus it got copy&pasted.
I'll think about merging these two variables.
Thank for pointing this out.
>> + ret = mtdtest_relax();
>> + if (ret)
>> + goto out;
>
> Here you propagate the wrong error. You test for 'ret' and propagate 'err'.
This is by design. I don't want to print an error message if the test is aborted.
mtdtest_relax() checks for that.
Thanks,
//richard
Am 02.04.2015 um 17:03 schrieb Fabio Estevam:
> On Thu, Apr 2, 2015 at 11:56 AM, Richard Weinberger <[email protected]> wrote:
>
>> It is not *much* better. It is just a matter of taste.
>
> ... and instructions cycles as well ;-)
You do understand that this is an error path?
Thanks,
//richard
Am 02.04.2015 um 16:32 schrieb Fabio Estevam:
>> +
>> + ret = read_eraseblock_by_page(i, iteration);
>> +
>> + ret = mtdtest_relax();
>> + if (ret)
>> + goto out;
BTW: While all the nitpicking you oversaw the real issue in my code.
I do not check ret after read_eraseblock_by_page(). ;-\
Thanks,
//richard
On Thu, Apr 02, 2015 at 04:13:46PM +0200, Richard Weinberger wrote:
> This simple MTD tests allows the user to see when read disturb happens.
> By reading blocks over and over it reports flipped bits.
> Currently it reports only flipped bits of the worst page of a block.
> If within block X page P1 has 3 bit flips and P6 4, it will report 4.
> By default every 50th block is read.
Didn't read through this much yet, but why do we need another in-kernel
test that coul (AFAICT) be easily replicated in userspace? The same goes
for several of the other tests, I think, actually. But at least with
those, we have a history of keeping them around, so it's not too much
burden [1].
Brian
[1] Although there are some latent issues in these tests that are still
getting get worked out (e.g., bad handling of 64-bit casting; too large
of stacks; uninterruptibility). The latter two would not even exist if
we were in user space.
Am 02.04.2015 um 18:04 schrieb Brian Norris:
> On Thu, Apr 02, 2015 at 04:13:46PM +0200, Richard Weinberger wrote:
>> This simple MTD tests allows the user to see when read disturb happens.
>> By reading blocks over and over it reports flipped bits.
>> Currently it reports only flipped bits of the worst page of a block.
>> If within block X page P1 has 3 bit flips and P6 4, it will report 4.
>> By default every 50th block is read.
>
> Didn't read through this much yet, but why do we need another in-kernel
> test that coul (AFAICT) be easily replicated in userspace? The same goes
> for several of the other tests, I think, actually. But at least with
> those, we have a history of keeping them around, so it's not too much
> burden [1].
I've added the test to drivers/mtd/tests/ because it fits into.
As simple as that.
> Brian
>
> [1] Although there are some latent issues in these tests that are still
> getting get worked out (e.g., bad handling of 64-bit casting; too large
> of stacks; uninterruptibility). The latter two would not even exist if
> we were in user space.
uninterruptibility got solved by my "[PATCH] mtd: Make MTD tests cancelable" patch.
But if we want to kill drivers/mtd/tests/ I'll happily help out.
Where shall we move these tests into? mtd-utils?
Thanks,
//richard
Hi all,
Il 02/04/2015 18:18, Richard Weinberger ha scritto:
> Am 02.04.2015 um 18:04 schrieb Brian Norris:
>> On Thu, Apr 02, 2015 at 04:13:46PM +0200, Richard Weinberger wrote:
>> [1] Although there are some latent issues in these tests that are still
>> getting get worked out (e.g., bad handling of 64-bit casting; too large
>> of stacks; uninterruptibility). The latter two would not even exist if
>> we were in user space.
>
> uninterruptibility got solved by my "[PATCH] mtd: Make MTD tests cancelable" patch.
And this is something I was looking for from years!
> But if we want to kill drivers/mtd/tests/ I'll happily help out.
> Where shall we move these tests into? mtd-utils?
I think so.
I'm writing a similar read disturb test on my own, mixing already
existing mtd-tools (flash_erase, nandwrite, nanddump) with some naive
bash scripting.
IMHO, we have a lot of pros running in userspace:
* dumping data
* better error/status log (which can be easily written on file, for
example, while mtdtests error log is mixed with other kernel messages)
* running test in parallel (if it make sense ;-)
For example on read disturb I already calculate RBER, which is, AFAIK, a
nice index on the quality of the NAND cell and of its data. I'm working
on writing down data on a separate CSV which can be easily processed
later (e.g. to make part to part comparison/statistics).
There's already a test directory inside mtd-utils, I think it's better
to start creating tests there, as userspace tools, whenever this is
possible.
Do we have any reason to have MTD tests as kernel module? (performance?)
Kind Regards,
--
Andrea SCIAN
DAVE Embedded Systems
Hi Richard,
On Thu, 02 Apr 2015 18:18:34 +0200
Richard Weinberger <[email protected]> wrote:
> Am 02.04.2015 um 18:04 schrieb Brian Norris:
> > On Thu, Apr 02, 2015 at 04:13:46PM +0200, Richard Weinberger wrote:
> >> This simple MTD tests allows the user to see when read disturb happens.
> >> By reading blocks over and over it reports flipped bits.
> >> Currently it reports only flipped bits of the worst page of a block.
> >> If within block X page P1 has 3 bit flips and P6 4, it will report 4.
> >> By default every 50th block is read.
> >
> > Didn't read through this much yet, but why do we need another in-kernel
> > test that coul (AFAICT) be easily replicated in userspace? The same goes
> > for several of the other tests, I think, actually. But at least with
> > those, we have a history of keeping them around, so it's not too much
> > burden [1].
>
> I've added the test to drivers/mtd/tests/ because it fits into.
> As simple as that.
>
> > Brian
> >
> > [1] Although there are some latent issues in these tests that are still
> > getting get worked out (e.g., bad handling of 64-bit casting; too large
> > of stacks; uninterruptibility). The latter two would not even exist if
> > we were in user space.
>
> uninterruptibility got solved by my "[PATCH] mtd: Make MTD tests cancelable" patch.
>
> But if we want to kill drivers/mtd/tests/ I'll happily help out.
I'd vote for that solution too.
I've looked at in-kernel mtd tests, and I'm pretty sure they can all be
done in userland.
This would prevent any kernel crash caused by buggy test modules.
> Where shall we move these tests into? mtd-utils?
I guess so, but I'll let Brian answer that one.
How about dispatching them in mtd-utils' tests/ directory (some of them
are NAND related tests, so creating a tests/nand would make sense,
and others are more generic).
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com