Patchwork [-next,1/7] mtd: tests: introduce mtd_test module

login
register
mail settings
Submitter Akinobu Mita
Date July 27, 2013, 2:14 a.m.
Message ID <1374891256-20248-2-git-send-email-akinobu.mita@gmail.com>
Download mbox | patch
Permalink /patch/262369/
State New
Headers show

Comments

Akinobu Mita - July 27, 2013, 2:14 a.m.
This introduces mtd_test module which contains the following functions
used by several mtd/tests modules.

- mtdtest_erase_eraseblock()
- mtdtest_scan_for_bad_eraseblocks()
- mtdtest_erase_whole_device()

This mtd_test module also provides the following wrapper functions for
mtd_read() and mtd_write() in order to simplify the return value check
in mtd/tests modules.

- mtdtest_read()
- mtdtest_write()

These functions will be used for reducing code duplication among
mtd/tests modules later.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/tests/Makefile   |   1 +
 drivers/mtd/tests/mtd_test.c | 117 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/tests/mtd_test.h |  11 ++++
 3 files changed, 129 insertions(+)
 create mode 100644 drivers/mtd/tests/mtd_test.c
 create mode 100644 drivers/mtd/tests/mtd_test.h
Brian Norris - July 27, 2013, 7:27 p.m.
On Fri, Jul 26, 2013 at 7:14 PM, Akinobu Mita <akinobu.mita@gmail.com> wrote:
> This introduces mtd_test module which contains the following functions
> used by several mtd/tests modules.
>
> - mtdtest_erase_eraseblock()
> - mtdtest_scan_for_bad_eraseblocks()
> - mtdtest_erase_whole_device()
>
> This mtd_test module also provides the following wrapper functions for
> mtd_read() and mtd_write() in order to simplify the return value check
> in mtd/tests modules.
>
> - mtdtest_read()
> - mtdtest_write()
>
> These functions will be used for reducing code duplication among
> mtd/tests modules later.

I like this idea. There is definitely too much code duplication.

However, there is an important tradeoff here: now to run these (very
simple) tests, we have a two-step process*:

insmod mtd_test.ko
insmod mtd_<actualtest>.ko dev=<MTD>

[* modprobe would solve this problem, but these tests are often
compiled and run by hand, sometimes on systems without the convenience
of modprobe ]

We could still accomplish the reduction in (source) code duplication
by simply including these simple routines in a header, then the code
would be compiled into each test module. I realize this isn't
typically the "best" way to share code, but since these are just test
modules and really don't need to be optimized for code size, I think
it is worth avoiding the extra step of inserting another module.

> ---
>  drivers/mtd/tests/Makefile   |   1 +
>  drivers/mtd/tests/mtd_test.c | 117 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/tests/mtd_test.h |  11 ++++
>  3 files changed, 129 insertions(+)
>  create mode 100644 drivers/mtd/tests/mtd_test.c
>  create mode 100644 drivers/mtd/tests/mtd_test.h

If we still keep the mtd_test module separate, I might rename it to
help distinguish it from any of the other modules, which actually run
tests. Perhaps include "lib" in the name? Like "libmtdtest.{c,h}"?

> diff --git a/drivers/mtd/tests/mtd_test.c b/drivers/mtd/tests/mtd_test.c
> new file mode 100644
> index 0000000..1fa1d63
> --- /dev/null
> +++ b/drivers/mtd/tests/mtd_test.c
> @@ -0,0 +1,117 @@
...
> +MODULE_LICENSE("GPL");

If we're keeping this as a module, we might want at least a
MODULE_DESCRIPTION (to help a user understand why they need this extra
module) and possibly a MODULE_AUTHOR?

Brian
Akinobu Mita - July 28, 2013, 2:21 a.m.
2013/7/28 Brian Norris <computersforpeace@gmail.com>:
> I like this idea. There is definitely too much code duplication.
>
> However, there is an important tradeoff here: now to run these (very
> simple) tests, we have a two-step process*:
>
> insmod mtd_test.ko
> insmod mtd_<actualtest>.ko dev=<MTD>
>
> [* modprobe would solve this problem, but these tests are often
> compiled and run by hand, sometimes on systems without the convenience
> of modprobe ]
>
> We could still accomplish the reduction in (source) code duplication
> by simply including these simple routines in a header, then the code
> would be compiled into each test module. I realize this isn't
> typically the "best" way to share code, but since these are just test
> modules and really don't need to be optimized for code size, I think
> it is worth avoiding the extra step of inserting another module.

I understand your concern and I'm going to change this series to move
all these functions into a header file.  But I'll wait other opinion
for a while.
Vikram Narayanan - July 28, 2013, 4:39 a.m.
On 28/Jul/2013 7:51 AM, Akinobu Mita wrote:
> 2013/7/28 Brian Norris <computersforpeace@gmail.com>:
>> I like this idea. There is definitely too much code duplication.
>>
>> However, there is an important tradeoff here: now to run these (very
>> simple) tests, we have a two-step process*:
>>
>> insmod mtd_test.ko
>> insmod mtd_<actualtest>.ko dev=<MTD>
>>
>> [* modprobe would solve this problem, but these tests are often
>> compiled and run by hand, sometimes on systems without the convenience
>> of modprobe ]
>>
>> We could still accomplish the reduction in (source) code duplication
>> by simply including these simple routines in a header, then the code
>> would be compiled into each test module. I realize this isn't
>> typically the "best" way to share code, but since these are just test
>> modules and really don't need to be optimized for code size, I think
>> it is worth avoiding the extra step of inserting another module.
>
> I understand your concern and I'm going to change this series to move
> all these functions into a header file.  But I'll wait other opinion
> for a while.

You can refer this.
<http://thread.gmane.org/gmane.linux.drivers.mtd/43932/focus=43941>

And also Artem's feedback here
<http://thread.gmane.org/gmane.linux.drivers.mtd/43932/focus=43941>

However, I didn't get much time to make this patch better as Artem 
suggested.

~Vikram
Vikram Narayanan - July 28, 2013, 4:41 a.m.
On 28/Jul/2013 10:09 AM, Vikram Narayanan wrote:
> On 28/Jul/2013 7:51 AM, Akinobu Mita wrote:
>> 2013/7/28 Brian Norris <computersforpeace@gmail.com>:
>>> I like this idea. There is definitely too much code duplication.
>>>
>>> However, there is an important tradeoff here: now to run these (very
>>> simple) tests, we have a two-step process*:
>>>
>>> insmod mtd_test.ko
>>> insmod mtd_<actualtest>.ko dev=<MTD>
>>>
>>> [* modprobe would solve this problem, but these tests are often
>>> compiled and run by hand, sometimes on systems without the convenience
>>> of modprobe ]
>>>
>>> We could still accomplish the reduction in (source) code duplication
>>> by simply including these simple routines in a header, then the code
>>> would be compiled into each test module. I realize this isn't
>>> typically the "best" way to share code, but since these are just test
>>> modules and really don't need to be optimized for code size, I think
>>> it is worth avoiding the extra step of inserting another module.
>>
>> I understand your concern and I'm going to change this series to move
>> all these functions into a header file.  But I'll wait other opinion
>> for a while.
>
> You can refer this.
> <http://thread.gmane.org/gmane.linux.drivers.mtd/43932/focus=43941>

http://article.gmane.org/gmane.linux.drivers.mtd/43933

> And also Artem's feedback here
> <http://thread.gmane.org/gmane.linux.drivers.mtd/43932/focus=43941>

http://article.gmane.org/gmane.linux.drivers.mtd/44007

>
> However, I didn't get much time to make this patch better as Artem
> suggested.
>
> ~Vikram

Sorry, Now updated the thread with correct links.

~Vikram
Akinobu Mita - July 29, 2013, 3:15 a.m.
2013/7/28 Vikram Narayanan <vikram186@gmail.com>:
> On 28/Jul/2013 10:09 AM, Vikram Narayanan wrote:
>>
>> On 28/Jul/2013 7:51 AM, Akinobu Mita wrote:
>>>
>>> 2013/7/28 Brian Norris <computersforpeace@gmail.com>:
>>>>
>>>> I like this idea. There is definitely too much code duplication.
>>>>
>>>> However, there is an important tradeoff here: now to run these (very
>>>> simple) tests, we have a two-step process*:
>>>>
>>>> insmod mtd_test.ko
>>>> insmod mtd_<actualtest>.ko dev=<MTD>
>>>>
>>>> [* modprobe would solve this problem, but these tests are often
>>>> compiled and run by hand, sometimes on systems without the convenience
>>>> of modprobe ]
>>>>
>>>> We could still accomplish the reduction in (source) code duplication
>>>> by simply including these simple routines in a header, then the code
>>>> would be compiled into each test module. I realize this isn't
>>>> typically the "best" way to share code, but since these are just test
>>>> modules and really don't need to be optimized for code size, I think
>>>> it is worth avoiding the extra step of inserting another module.
>>>
>>>
>>> I understand your concern and I'm going to change this series to move
>>> all these functions into a header file.  But I'll wait other opinion
>>> for a while.
>>
>>
>> You can refer this.
>> <http://thread.gmane.org/gmane.linux.drivers.mtd/43932/focus=43941>
>
>
> http://article.gmane.org/gmane.linux.drivers.mtd/43933

Thanks.  I noticed that there were a few more places where erase_eraseblock()
could be used in mtd/tests modules.  I'll include these in next version
of this patch set.

>> And also Artem's feedback here
>> <http://thread.gmane.org/gmane.linux.drivers.mtd/43932/focus=43941>
>
>
> http://article.gmane.org/gmane.linux.drivers.mtd/44007

I checked all callsites of mtd_erase() in the tree, but there is no places where
we can simply use erase_eraseblock().  If erase_eraseblock() can take
additional arguments (callback and priv), there are only a few places
(in mtdswap.c and ubi/io.c) where we can use it.  So I think we don't
need to extend mtdcore.c for now.

Patch

diff --git a/drivers/mtd/tests/Makefile b/drivers/mtd/tests/Makefile
index bd0065c..3d642f4 100644
--- a/drivers/mtd/tests/Makefile
+++ b/drivers/mtd/tests/Makefile
@@ -1,3 +1,4 @@ 
+obj-$(CONFIG_MTD_TESTS) += mtd_test.o
 obj-$(CONFIG_MTD_TESTS) += mtd_oobtest.o
 obj-$(CONFIG_MTD_TESTS) += mtd_pagetest.o
 obj-$(CONFIG_MTD_TESTS) += mtd_readtest.o
diff --git a/drivers/mtd/tests/mtd_test.c b/drivers/mtd/tests/mtd_test.c
new file mode 100644
index 0000000..1fa1d63
--- /dev/null
+++ b/drivers/mtd/tests/mtd_test.c
@@ -0,0 +1,117 @@ 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/printk.h>
+
+#include "mtd_test.h"
+
+int mtdtest_erase_eraseblock(struct mtd_info *mtd, int ebnum)
+{
+	int err;
+	struct erase_info ei;
+	loff_t addr = ebnum * mtd->erasesize;
+
+	memset(&ei, 0, sizeof(struct erase_info));
+	ei.mtd  = mtd;
+	ei.addr = addr;
+	ei.len  = mtd->erasesize;
+
+	err = mtd_erase(mtd, &ei);
+	if (err) {
+		pr_info("error %d while erasing EB %d\n", err, ebnum);
+		return err;
+	}
+
+	if (ei.state == MTD_ERASE_FAILED) {
+		pr_info("some erase error occurred at EB %d\n", ebnum);
+		return -EIO;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mtdtest_erase_eraseblock);
+
+static int is_block_bad(struct mtd_info *mtd, int ebnum)
+{
+	int ret;
+	loff_t addr = ebnum * mtd->erasesize;
+
+	ret = mtd_block_isbad(mtd, addr);
+	if (ret)
+		pr_info("block %d is bad\n", ebnum);
+
+	return ret;
+}
+
+int mtdtest_scan_for_bad_eraseblocks(struct mtd_info *mtd, unsigned char *bbt,
+					int ebcnt)
+{
+	int i, bad = 0;
+
+	if (!mtd_can_have_bb(mtd))
+		return 0;
+
+	pr_info("scanning for bad eraseblocks\n");
+	for (i = 0; i < ebcnt; ++i) {
+		bbt[i] = is_block_bad(mtd, i) ? 1 : 0;
+		if (bbt[i])
+			bad += 1;
+		cond_resched();
+	}
+	pr_info("scanned %d eraseblocks, %d are bad\n", i, bad);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mtdtest_scan_for_bad_eraseblocks);
+
+int mtdtest_erase_whole_device(struct mtd_info *mtd, unsigned char *bbt,
+				int ebcnt)
+{
+	int err;
+	unsigned int i;
+
+	for (i = 0; i < ebcnt; ++i) {
+		if (bbt[i])
+			continue;
+		err = mtdtest_erase_eraseblock(mtd, i);
+		if (err)
+			return err;
+		cond_resched();
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mtdtest_erase_whole_device);
+
+int mtdtest_read(struct mtd_info *mtd, loff_t addr, size_t size, void *buf)
+{
+	size_t read;
+	int err;
+
+	err = mtd_read(mtd, addr, size, &read, buf);
+	/* Ignore corrected ECC errors */
+	if (mtd_is_bitflip(err))
+		err = 0;
+	if (!err && read != size)
+		err = -EINVAL;
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(mtdtest_read);
+
+int mtdtest_write(struct mtd_info *mtd, loff_t addr, size_t size,
+		const void *buf)
+{
+	size_t written;
+	int err;
+
+	err = mtd_write(mtd, addr, size, &written, buf);
+	if (!err && written != size)
+		err = -EIO;
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(mtdtest_write);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/mtd/tests/mtd_test.h b/drivers/mtd/tests/mtd_test.h
new file mode 100644
index 0000000..e61fb67
--- /dev/null
+++ b/drivers/mtd/tests/mtd_test.h
@@ -0,0 +1,11 @@ 
+#include <linux/mtd/mtd.h>
+
+int mtdtest_erase_eraseblock(struct mtd_info *mtd, int ebnum);
+int mtdtest_scan_for_bad_eraseblocks(struct mtd_info *mtd, unsigned char *bbt,
+					int ebcnt);
+int mtdtest_erase_whole_device(struct mtd_info *mtd, unsigned char *bbt,
+				int ebcnt);
+
+int mtdtest_read(struct mtd_info *mtd, loff_t addr, size_t size, void *buf);
+int mtdtest_write(struct mtd_info *mtd, loff_t addr, size_t size,
+		const void *buf);