| Message ID | 1374891256-20248-2-git-send-email-akinobu.mita@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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.
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);
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