[PATCH 0/3] mtd-utils: bit errors in nandtest

[PATCH 0/3] mtd-utils: bit errors in nandtest

[Ben Gardiner]

The first patch is a simple increase in the amount of information output by 
mtddebug: adding the eccbytes and oobavail fields.

The next two are updates to nandtest to 1) print the number of bits corrected
during test and 2) report an estimate of the raw bit error rate (BER) of the
flash.


Ben Gardiner (3):
  mtd_debug: report ecc layout
  nandtest: print number of bits corrected during test
  nandtest: report estimated BER

 mtd_debug.c |   22 ++++++++++++++++++++++
 nandtest.c  |   29 +++++++++++++++++++++++++++--
 2 files changed, 49 insertions(+), 2 deletions(-)

-- 
1.7.3.5

[PATCH 1/3] mtd_debug: report ecc layout

[Ben Gardiner]

The mtd_debug 'info' command reports a great deal of useful information about
the mtd device is it given for a query.

Add the ECC size and OOB available to that list. The other entries in the
ecclayout struct are arrays and are not as meaningful as eccsize and oobavail.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>

---

I noticed that the struct nand_ecclayout_user and its corresponding ioctl
has been marked deprecated; however, since this is a 'debug' utility and at
least one person found it useful I thought it would be a good idea to propose
these changes anyways.
---
 mtd_debug.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/mtd_debug.c b/mtd_debug.c
index b82dabe..ea2a119 100644
--- a/mtd_debug.c
+++ b/mtd_debug.c
@@ -74,6 +74,14 @@ static int getregions (int fd,struct region_info_user *regions,int *n)
 	return (0);
 }
 
+/*
+ * ECCGETLAYOUT
+ */
+static int getecclayout (int fd, struct nand_ecclayout_user *ecclayout)
+{
+	return (ioctl (fd,ECCGETLAYOUT,ecclayout));
+}
+
 int erase_flash (int fd,u_int32_t offset,u_int32_t bytes)
 {
 	int err;
@@ -237,6 +245,7 @@ int showinfo (int fd)
 {
 	int i,err,n;
 	struct mtd_info_user mtd;
+	struct nand_ecclayout_user ecclayout;
 	static struct region_info_user region[1024];
 
 	err = getmeminfo (fd,&mtd);
@@ -246,6 +255,13 @@ int showinfo (int fd)
 		return (1);
 	}
 
+	err = getecclayout (fd,&ecclayout);
+	if (err < 0)
+	{
+		perror ("ECCGETLAYOUT");
+		return (1);
+	}
+
 	err = getregions (fd,region,&n);
 	if (err < 0)
 	{
@@ -330,6 +346,12 @@ int showinfo (int fd)
 	printf ("\nmtd.oobsize = ");
 	printsize (mtd.oobsize);
 
+	printf ("\necclayout.eccbytes = ");
+	printsize (ecclayout.eccbytes);
+
+	printf ("\necclayout.oobavail = ");
+	printsize (ecclayout.oobavail);
+
 	printf ("\n"
 			"regions = %d\n"
 			"\n",
-- 
1.7.3.5

Re: [PATCH 1/3] mtd_debug: report ecc layout

[Brian Norris]

On Tue, Aug 9, 2011 at 1:57 PM, Ben Gardiner <bengardiner@nanometrics.ca> wrote:
> Add the ECC size and OOB available to that list. The other entries in the
> ecclayout struct are arrays and are not as meaningful as eccsize and oobavail.
> ---
> I noticed that the struct nand_ecclayout_user and its corresponding ioctl
> has been marked deprecated; however, since this is a 'debug' utility and at
> least one person found it useful I thought it would be a good idea to propose
> these changes anyways.

While I agree with your rationale for ignoring the deprecation (it's
OK for a debug utility), users of this feature should be warned that
struct nand_ecclayout_user will not report all information if there
are more ECC entries than "MTD_MAX_ECCPOS_ENTRIES" (i.e., 64). It will
simply truncate ECC at 64. This may be a problem for large page flash
with large ECC regions. It's a similar story for "oobavail" as well.

And before the question is asked, I'm not sure how to implement my
suggestion that "users of this feature should be warned". Perhaps just
a comment in the code?

Brian

Re: [PATCH 1/3] mtd_debug: report ecc layout

[Ben Gardiner]

Hi Brian,

On Wed, Aug 10, 2011 at 1:07 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Tue, Aug 9, 2011 at 1:57 PM, Ben Gardiner <bengardiner@nanometrics.ca> wrote:
>> Add the ECC size and OOB available to that list. The other entries in the
>> ecclayout struct are arrays and are not as meaningful as eccsize and oobavail.
>> ---
>> I noticed that the struct nand_ecclayout_user and its corresponding ioctl
>> has been marked deprecated; however, since this is a 'debug' utility and at
>> least one person found it useful I thought it would be a good idea to propose
>> these changes anyways.
>
> While I agree with your rationale for ignoring the deprecation (it's
> OK for a debug utility),

Thanks for the review  -- and for agreeing with me :)

> users of this feature should be warned that
> struct nand_ecclayout_user will not report all information if there
> are more ECC entries than "MTD_MAX_ECCPOS_ENTRIES" (i.e., 64). It will
> simply truncate ECC at 64. This may be a problem for large page flash
> with large ECC regions. It's a similar story for "oobavail" as well.
>
> And before the question is asked, I'm not sure how to implement my
> suggestion that "users of this feature should be warned". Perhaps just
> a comment in the code?

Sounds reasonable to me -- I will insert a warning comment in v2.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

Re: [PATCH 1/3] mtd_debug: report ecc layout

[Artem Bityutskiy]

On Wed, 2011-08-10 at 10:07 -0700, Brian Norris wrote:
> On Tue, Aug 9, 2011 at 1:57 PM, Ben Gardiner <bengardiner@nanometrics.ca> wrote:
> > Add the ECC size and OOB available to that list. The other entries in the
> > ecclayout struct are arrays and are not as meaningful as eccsize and oobavail.
> > ---
> > I noticed that the struct nand_ecclayout_user and its corresponding ioctl
> > has been marked deprecated; however, since this is a 'debug' utility and at
> > least one person found it useful I thought it would be a good idea to propose
> > these changes anyways.
> 
> While I agree with your rationale for ignoring the deprecation (it's
> OK for a debug utility), users of this feature should be warned that
> struct nand_ecclayout_user will not report all information if there
> are more ECC entries than "MTD_MAX_ECCPOS_ENTRIES" (i.e., 64). It will
> simply truncate ECC at 64. This may be a problem for large page flash
> with large ECC regions. It's a similar story for "oobavail" as well.
> 
> And before the question is asked, I'm not sure how to implement my
> suggestion that "users of this feature should be warned". Perhaps just
> a comment in the code?

I guess we can do the following:

1. make sure mtd-utils does not use the depricated ioctl
2. release such mtd-utils
3. change the kernel and add a warning printk there
4. wait some time and kill the ioctl from the kernel.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/3] mtd_debug: report ecc layout

[Artem Bityutskiy]

On Tue, 2011-08-09 at 16:57 -0400, Ben Gardiner wrote:
> The mtd_debug 'info' command reports a great deal of useful information about
> the mtd device is it given for a query.
> 
> Add the ECC size and OOB available to that list. The other entries in the
> ecclayout struct are arrays and are not as meaningful as eccsize and oobavail.
> 
> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
> 
> ---
> 
> I noticed that the struct nand_ecclayout_user and its corresponding ioctl
> has been marked deprecated; however, since this is a 'debug' utility and at
> least one person found it useful I thought it would be a good idea to propose
> these changes anyways.

I think we could just kill depricated ioctls as soon as we make
mtd-utils stop using them. If you add this dependency - it will be more
difficult to kill them.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/3] mtd_debug: report ecc layout

[Ben Gardiner]

On Wed, Aug 17, 2011 at 4:03 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Tue, 2011-08-09 at 16:57 -0400, Ben Gardiner wrote:
>> The mtd_debug 'info' command reports a great deal of useful information about
>> the mtd device is it given for a query.
>>
>> Add the ECC size and OOB available to that list. The other entries in the
>> ecclayout struct are arrays and are not as meaningful as eccsize and oobavail.
>>
>> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
>>
>> ---
>>
>> I noticed that the struct nand_ecclayout_user and its corresponding ioctl
>> has been marked deprecated; however, since this is a 'debug' utility and at
>> least one person found it useful I thought it would be a good idea to propose
>> these changes anyways.
>
> I think we could just kill depricated ioctls as soon as we make
> mtd-utils stop using them. If you add this dependency - it will be more
> difficult to kill them.

Fair enough -- I'm happy to hear a definitive answer; thank you, Artem.

In PATCH 3/3 I use the same ioctl to get the ECC size for the purposes
of producing a BER estimate. What is the right way of getting the ECC
size now that ECCGETLAYOUT is deprecated? Is this even something that
should be from userspace at all?

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

Re: [PATCH 1/3] mtd_debug: report ecc layout

[Artem Bityutskiy]

On Thu, 2011-08-18 at 10:01 -0400, Ben Gardiner wrote:
> Fair enough -- I'm happy to hear a definitive answer; thank you, Artem.
> 
> In PATCH 3/3 I use the same ioctl to get the ECC size for the purposes
> of producing a BER estimate. What is the right way of getting the ECC
> size now that ECCGETLAYOUT is deprecated? Is this even something that
> should be from userspace at all?

I do not know. I think we need to expose it via sysfs. I think that in
general sysfs is better that "informational" ioctls.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/3] mtd_debug: report ecc layout

[Ben Gardiner]

On Mon, Aug 22, 2011 at 7:00 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Thu, 2011-08-18 at 10:01 -0400, Ben Gardiner wrote:
>> Fair enough -- I'm happy to hear a definitive answer; thank you, Artem.
>>
>> In PATCH 3/3 I use the same ioctl to get the ECC size for the purposes
>> of producing a BER estimate. What is the right way of getting the ECC
>> size now that ECCGETLAYOUT is deprecated? Is this even something that
>> should be from userspace at all?
>
> I do not know. I think we need to expose it via sysfs. I think that in
> general sysfs is better that "informational" ioctls.

Ok -- I've heard that said before and for what it's worth I agree --
sysfs is better.

However, I'm seeing that there is no available sysfs mechanism at
present. I'm sorry but I can't take on the time to introduce one
although I agree it is best and I would like to be able to.

Please drop patches 1/3 and 3/3 from consideration. Patch 2/3 'print
number of bits corrected during test' is still useful and applicable
to mtd-utils -- IMHO.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

[PATCH 2/3] nandtest: print number of bits corrected during test

[Ben Gardiner]

The nandtest program monitors the corrected ecc stat to determine if an
ECC correction has taken place during the last write-read. If so, it
prints "ECC corrected".

The mtd subsytem will store the number of bits corrected in the corrected
ecc stat so update the nandtest output to print also the number of bits
corrected when performing the test.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
---
 nandtest.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/nandtest.c b/nandtest.c
index d03dc11..dc28d09 100644
--- a/nandtest.c
+++ b/nandtest.c
@@ -98,7 +98,9 @@ int erase_and_write(loff_t ofs, unsigned char *data, unsigned char *rbuf)
 	}
 
 	if (newstats.corrected > oldstats.corrected) {
-		printf("\nECC corrected at %08x\n", (unsigned) ofs);
+		printf("\n %d bit(s) ECC corrected at %08x\n",
+				newstats.corrected - oldstats.corrected,
+				(unsigned) ofs);
 		oldstats.corrected = newstats.corrected;
 	}
 	if (newstats.failed > oldstats.failed) {
-- 
1.7.3.5

Re: [PATCH 2/3] nandtest: print number of bits corrected during test

[Artem Bityutskiy]

On Tue, 2011-08-09 at 16:57 -0400, Ben Gardiner wrote:
> The nandtest program monitors the corrected ecc stat to determine if an
> ECC correction has taken place during the last write-read. If so, it
> prints "ECC corrected".
> 
> The mtd subsytem will store the number of bits corrected in the corrected
> ecc stat so update the nandtest output to print also the number of bits
> corrected when performing the test.
> 
> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>

Pushed this patch to mtd-utils, thanks!

-- 
Best Regards,
Artem Bityutskiy

[PATCH 3/3] nandtest: report estimated BER

[Ben Gardiner]

The nandtest program will excercise every block on a given mtd device by
writing to it and monitoring the ECC corrections and failures.

Augment this utility to report an estimate of the program-disturb raw BER
based on the writesize, eccsize, number of passes and ecc corrections stat.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>

---

This estimate relies on the eccbytes field of the now deprecated struct
nand_ecclayout_user. Since I found it useful I thought I should propose
the change anyways. I'm open to modifications to retrieve 'eccbytes' the
right way.
---
 nandtest.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/nandtest.c b/nandtest.c
index dc28d09..35e3e94 100644
--- a/nandtest.c
+++ b/nandtest.c
@@ -4,6 +4,7 @@
 #include <ctype.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -32,7 +33,8 @@ void usage(void)
 }
 
 struct mtd_info_user meminfo;
-struct mtd_ecc_stats oldstats, newstats;
+struct mtd_ecc_stats origstats, oldstats, newstats;
+struct nand_ecclayout_user ecclayout;
 int fd;
 int markbad=0;
 int seed;
@@ -126,6 +128,16 @@ int erase_and_write(loff_t ofs, unsigned char *data, unsigned char *rbuf)
 	return 0;
 }
 
+void report_raw_ber(unsigned long len)
+{
+	unsigned long corrections = newstats.corrected - origstats.corrected;
+	long double rate = (long double) corrections;
+	rate /= (long double) len;
+	rate /= CHAR_BIT;
+
+	printf("%lu bit corrections in %lu bytes. Estimated program-disturb raw BER: %Lg\n",
+			corrections, len, rate);
+}
 
 /*
  * Main program
@@ -204,6 +216,12 @@ int main(int argc, char **argv)
 		exit(1);
 	}
 
+	if (ioctl(fd, ECCGETLAYOUT, &ecclayout)) {
+		perror("ECCGETLAYOUT");
+		close(fd);
+		exit(1);
+	}
+
 	if (length == -1)
 		length = meminfo.size;
 
@@ -243,6 +261,8 @@ int main(int argc, char **argv)
 	printf("Bad blocks     : %d\n", oldstats.badblocks);
 	printf("BBT blocks     : %d\n", oldstats.bbtblocks);
 
+	origstats = oldstats;
+
 	for (pass = 0; pass < nr_passes; pass++) {
 		loff_t test_ofs;
 
@@ -281,6 +301,9 @@ int main(int argc, char **argv)
 		}
 		printf("\nFinished pass %d successfully\n", pass+1);
 	}
+
+	report_raw_ber(length * (meminfo.erasesize + meminfo.erasesize/meminfo.writesize * ecclayout.eccbytes) *(unsigned long)nr_passes);
+
 	/* Return happy */
 	return 0;
 }
-- 
1.7.3.5