[PATCH 1/6] mtd: nandsim: convert pages_written[] to bitmap

[PATCH 1/6] mtd: nandsim: convert pages_written[] to bitmap

[Akinobu Mita]

nandsim.pages_written[] is the array of unsigned char which is indexed
by the page number and used for identifying which pages have been written
when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
so it can be converted to bitmap.  This reduces the allocation size of
pages_written[] by 1/8.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/nand/nandsim.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index cb38f3d..a73dce0 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -363,7 +363,7 @@ struct nandsim {
 
 	/* Fields needed when using a cache file */
 	struct file *cfile; /* Open file */
-	unsigned char *pages_written; /* Which pages have been written */
+	unsigned long *pages_written; /* Which pages have been written */
 	void *file_buf;
 	struct page *held_pages[NS_MAX_HELD_PAGES];
 	int held_cnt;
@@ -586,7 +586,8 @@ static int alloc_device(struct nandsim *ns)
 			err = -EINVAL;
 			goto err_close;
 		}
-		ns->pages_written = vzalloc(ns->geom.pgnum);
+		ns->pages_written = vzalloc(BITS_TO_LONGS(ns->geom.pgnum) *
+					    sizeof(unsigned long));
 		if (!ns->pages_written) {
 			NS_ERR("alloc_device: unable to allocate pages written array\n");
 			err = -ENOMEM;
@@ -1479,7 +1480,7 @@ static void read_page(struct nandsim *ns, int num)
 	union ns_mem *mypage;
 
 	if (ns->cfile) {
-		if (!ns->pages_written[ns->regs.row]) {
+		if (!test_bit(ns->regs.row, ns->pages_written)) {
 			NS_DBG("read_page: page %d not written\n", ns->regs.row);
 			memset(ns->buf.byte, 0xFF, num);
 		} else {
@@ -1525,9 +1526,9 @@ static void erase_sector(struct nandsim *ns)
 
 	if (ns->cfile) {
 		for (i = 0; i < ns->geom.pgsec; i++)
-			if (ns->pages_written[ns->regs.row + i]) {
+			if (__test_and_clear_bit(ns->regs.row + i,
+						 ns->pages_written)) {
 				NS_DBG("erase_sector: freeing page %d\n", ns->regs.row + i);
-				ns->pages_written[ns->regs.row + i] = 0;
 			}
 		return;
 	}
@@ -1560,7 +1561,7 @@ static int prog_page(struct nandsim *ns, int num)
 		NS_DBG("prog_page: writing page %d\n", ns->regs.row);
 		pg_off = ns->file_buf + ns->regs.column + ns->regs.off;
 		off = (loff_t)ns->regs.row * ns->geom.pgszoob + ns->regs.column + ns->regs.off;
-		if (!ns->pages_written[ns->regs.row]) {
+		if (!test_bit(ns->regs.row, ns->pages_written)) {
 			all = 1;
 			memset(ns->file_buf, 0xff, ns->geom.pgszoob);
 		} else {
@@ -1580,7 +1581,7 @@ static int prog_page(struct nandsim *ns, int num)
 				NS_ERR("prog_page: write error for page %d ret %ld\n", ns->regs.row, (long)tx);
 				return -1;
 			}
-			ns->pages_written[ns->regs.row] = 1;
+			__set_bit(ns->regs.row, ns->pages_written);
 		} else {
 			tx = write_file(ns, ns->cfile, pg_off, num, off);
 			if (tx != num) {
-- 
1.8.3.1

[PATCH 2/6] mtd: nandsim: use kasprintf()

[Akinobu Mita]

Use kasprintf() which combines kmalloc and sprintf.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/nand/nandsim.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index a73dce0..ac1b46c 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -654,9 +654,7 @@ static void free_device(struct nandsim *ns)
 
 static char *get_partition_name(int i)
 {
-	char buf[64];
-	sprintf(buf, "NAND simulator partition %d", i);
-	return kstrdup(buf, GFP_KERNEL);
+	return kasprintf(GFP_KERNEL, "NAND simulator partition %d", i);
 }
 
 /*
-- 
1.8.3.1

[PATCH 3/6] mtd: nandsim: simplify NS_RAW_OFFSET()

[Akinobu Mita]

Simplify the definision of NS_RAW_OFFSET() by using (ns)->geom.pgszoob
which holds the sum of page size and OOB size.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/nand/nandsim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index ac1b46c..7aa7b19 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -205,7 +205,7 @@ MODULE_PARM_DESC(bch,		 "Enable BCH ecc and set how many bits should "
 
 /* Calculate the page offset in flash RAM image by (row, column) address */
 #define NS_RAW_OFFSET(ns) \
-	(((ns)->regs.row << (ns)->geom.pgshift) + ((ns)->regs.row * (ns)->geom.oobsz) + (ns)->regs.column)
+	(((ns)->regs.row * (ns)->geom.pgszoob) + (ns)->regs.column)
 
 /* Calculate the OOB offset in flash RAM image by (row, column) address */
 #define NS_RAW_OFFSET_OOB(ns) (NS_RAW_OFFSET(ns) + ns->geom.pgsz)
-- 
1.8.3.1

[PATCH 4/6] mtd: nandsim: use NS_RAW_OFFSET()

[Akinobu Mita]

Use NS_RAW_OFFSET() to calculate the page offset in flash RAM image by
(row, column) address.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/nand/nandsim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index 7aa7b19..0ff53ef 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -1489,7 +1489,7 @@ static void read_page(struct nandsim *ns, int num)
 				ns->regs.row, ns->regs.column + ns->regs.off);
 			if (do_read_error(ns, num))
 				return;
-			pos = (loff_t)ns->regs.row * ns->geom.pgszoob + ns->regs.column + ns->regs.off;
+			pos = (loff_t)NS_RAW_OFFSET(ns) + ns->regs.off;
 			tx = read_file(ns, ns->cfile, ns->buf.byte, num, pos);
 			if (tx != num) {
 				NS_ERR("read_page: read error for page %d ret %ld\n", ns->regs.row, (long)tx);
@@ -1558,7 +1558,7 @@ static int prog_page(struct nandsim *ns, int num)
 
 		NS_DBG("prog_page: writing page %d\n", ns->regs.row);
 		pg_off = ns->file_buf + ns->regs.column + ns->regs.off;
-		off = (loff_t)ns->regs.row * ns->geom.pgszoob + ns->regs.column + ns->regs.off;
+		off = (loff_t)NS_RAW_OFFSET(ns) + ns->regs.off;
 		if (!test_bit(ns->regs.row, ns->pages_written)) {
 			all = 1;
 			memset(ns->file_buf, 0xff, ns->geom.pgszoob);
-- 
1.8.3.1

[PATCH 5/6] mtd: nandsim: remove unused code

[Akinobu Mita]

Remove the leftover from commit 831d316b8b80b68dcdd2b3f5ede6d33c2bbf5835
("mtd: nandsim: remove autoincrement code").

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/nand/nandsim.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index 0ff53ef..ce90990 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -760,12 +760,6 @@ static int init_nandsim(struct mtd_info *mtd)
 		ns->nbparts += 1;
 	}
 
-	/* Detect how many ID bytes the NAND chip outputs */
-	for (i = 0; nand_flash_ids[i].name != NULL; i++) {
-		if (second_id_byte != nand_flash_ids[i].dev_id)
-			continue;
-	}
-
 	if (ns->busw == 16)
 		NS_WARN("16-bit flashes support wasn't tested\n");
 
-- 
1.8.3.1

[PATCH 6/6] mtd: nandsim: remove unused ns->geom.oobshift

[Akinobu Mita]

ns->geom.oobshift holds bits number in OOB size, but OOB size is not
always power of two.  So it is useless and it actually isn't used in
this driver except for just printing the value at module loading.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/nand/nandsim.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index ce90990..9e04de0 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -336,7 +336,6 @@ struct nandsim {
 		uint pgsec;         /* number of pages per sector */
 		uint secshift;      /* bits number in sector size */
 		uint pgshift;       /* bits number in page size */
-		uint oobshift;      /* bits number in OOB size */
 		uint pgaddrbytes;   /* bytes per page address */
 		uint secaddrbytes;  /* bytes per sector address */
 		uint idbytes;       /* the number ID bytes that this chip outputs */
@@ -689,7 +688,6 @@ static int init_nandsim(struct mtd_info *mtd)
 	ns->geom.totszoob = ns->geom.totsz + (uint64_t)ns->geom.pgnum * ns->geom.oobsz;
 	ns->geom.secshift = ffs(ns->geom.secsz) - 1;
 	ns->geom.pgshift  = chip->page_shift;
-	ns->geom.oobshift = ffs(ns->geom.oobsz) - 1;
 	ns->geom.pgsec    = ns->geom.secsz / ns->geom.pgsz;
 	ns->geom.secszoob = ns->geom.secsz + ns->geom.oobsz * ns->geom.pgsec;
 	ns->options = 0;
@@ -773,7 +771,7 @@ static int init_nandsim(struct mtd_info *mtd)
 	printk("bus width: %u\n",               ns->busw);
 	printk("bits in sector size: %u\n",     ns->geom.secshift);
 	printk("bits in page size: %u\n",       ns->geom.pgshift);
-	printk("bits in OOB size: %u\n",	ns->geom.oobshift);
+	printk("bits in OOB size: %u\n",	ffs(ns->geom.oobsz) - 1);
 	printk("flash size with OOB: %llu KiB\n",
 			(unsigned long long)ns->geom.totszoob >> 10);
 	printk("page address bytes: %u\n",      ns->geom.pgaddrbytes);
-- 
1.8.3.1

Re: [PATCH 1/6] mtd: nandsim: convert pages_written[] to bitmap

[Artem Bityutskiy]

On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
> nandsim.pages_written[] is the array of unsigned char which is indexed
> by the page number and used for identifying which pages have been written
> when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
> so it can be converted to bitmap.  This reduces the allocation size of
> pages_written[] by 1/8.

Did you test this patch-set, if yes, how?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/6] mtd: nandsim: convert pages_written[] to bitmap

[Artem Bityutskiy]

On Fri, 2013-08-02 at 17:38 +0300, Artem Bityutskiy wrote:
> On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
> > nandsim.pages_written[] is the array of unsigned char which is indexed
> > by the page number and used for identifying which pages have been written
> > when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
> > so it can be converted to bitmap.  This reduces the allocation size of
> > pages_written[] by 1/8.
> 
> Did you test this patch-set, if yes, how?

Note, I am asking because this is an ancient and probably poor piece of
code, but quite important, so I am just trying to understand whether I
should test your patches before applying, and if yes, how.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/6] mtd: nandsim: convert pages_written[] to bitmap

[Akinobu Mita]

2013/8/3 Artem Bityutskiy <dedekind1@gmail.com>:
> On Fri, 2013-08-02 at 17:38 +0300, Artem Bityutskiy wrote:
>> On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
>> > nandsim.pages_written[] is the array of unsigned char which is indexed
>> > by the page number and used for identifying which pages have been written
>> > when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
>> > so it can be converted to bitmap.  This reduces the allocation size of
>> > pages_written[] by 1/8.
>>
>> Did you test this patch-set, if yes, how?
>
> Note, I am asking because this is an ancient and probably poor piece of
> code, but quite important, so I am just trying to understand whether I
> should test your patches before applying, and if yes, how.

I tested this with mtd/tests modules and mounting a filesystem through
mtdblock.  I also tried cache_file module parameter to test patch 1/6
because nandsim.pages_written[] is only used when cache_file is used.
Also I tried second_id_byte module parameter to check both 8-bit and
16-bit bus width are working.

Re: [PATCH 1/6] mtd: nandsim: convert pages_written[] to bitmap

[Artem Bityutskiy]

On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
> nandsim.pages_written[] is the array of unsigned char which is indexed
> by the page number and used for identifying which pages have been written
> when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
> so it can be converted to bitmap.  This reduces the allocation size of
> pages_written[] by 1/8.

Pushed all to l2-mtd.git, thanks.

I wonder, what is your motivation for doing this? Did you just notice
the imperfection and decided to fix it, or you really suffer from too
much memory allocated?

Thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/6] mtd: nandsim: convert pages_written[] to bitmap

[Akinobu Mita]

2013/8/5 Artem Bityutskiy <dedekind1@gmail.com>:
> On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
>> nandsim.pages_written[] is the array of unsigned char which is indexed
>> by the page number and used for identifying which pages have been written
>> when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
>> so it can be converted to bitmap.  This reduces the allocation size of
>> pages_written[] by 1/8.
>
> Pushed all to l2-mtd.git, thanks.

Thanks.

> I wonder, what is your motivation for doing this? Did you just notice
> the imperfection and decided to fix it, or you really suffer from too
> much memory allocated?

I found all in this patch set when I was reading nandsim.c.  So I did
not suffer any real problems.