Patchwork [2/3] Discard the legacy interface MEMGETOOBSEL in flash_eraseall

login
register
mail settings
Submitter Stanley.Miao
Date June 11, 2010, 9:36 a.m.
Message ID <1276248975-1822-3-git-send-email-stanley.miao@windriver.com>
Download mbox | patch
Permalink /patch/55303/
State New
Headers show

Comments

Stanley.Miao - June 11, 2010, 9:36 a.m.
The "struct nand_oobinfo" is able to record only 32 ECC code positions,which
is not enough for many big NAND chips. Therefore, this structure is replaced
by "struct nand_ecclayout" in linux kernel from the version 2.6.17.
Consequently, the ioctl command changed from MEMGETOOBSEL to ECCGETLAYOUT.

Now update flash_eraseall to use the new ioctl command ECCGETLAYOUT. In order
to keep compatible with the old linux kernel, a linux version detection
function is added.

Signed-off-by: Stanley.Miao <stanley.miao@windriver.com>
---
 flash_eraseall.c |   71 ++++++++++++++++++++++++++++++++----------------------
 1 files changed, 42 insertions(+), 29 deletions(-)
Joakim Tjernlund - June 11, 2010, 9:44 a.m.
>
> The "struct nand_oobinfo" is able to record only 32 ECC code positions,which
> is not enough for many big NAND chips. Therefore, this structure is replaced
> by "struct nand_ecclayout" in linux kernel from the version 2.6.17.
> Consequently, the ioctl command changed from MEMGETOOBSEL to ECCGETLAYOUT.
>
> Now update flash_eraseall to use the new ioctl command ECCGETLAYOUT. In order
> to keep compatible with the old linux kernel, a linux version detection
> function is added.
>
> Signed-off-by: Stanley.Miao <stanley.miao@windriver.com>
> ---
>  flash_eraseall.c |   71 ++++++++++++++++++++++++++++++++----------------------
>  1 files changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/flash_eraseall.c b/flash_eraseall.c
> index a22fc49..4e2108b 100644
> --- a/flash_eraseall.c
> +++ b/flash_eraseall.c
> @@ -43,6 +43,8 @@
>  #define PROGRAM "flash_eraseall"
>  #define VERSION "$Revision: 1.22 $"
>
> +#define LINUX_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
> +
>  static const char *exe_name;
>  static const char *mtd_device;
>  static int quiet;      /* true -- don't output progress */
> @@ -55,6 +57,21 @@ static void display_version (void);
>  static struct jffs2_unknown_node cleanmarker;
>  int target_endian = __BYTE_ORDER;
>
> +static int get_linux_version(void)
> +{
> +   FILE *fd;
> +   int a = 0, b = 0, c = 0;
> +
> +   fd = fopen("/proc/version", "r");
> +   if (fd) {
> +      fscanf(fd, "Linux version %d.%d.%d", &a, &b, &c);
> +      fclose(fd);
> +   }
> +
> +   return LINUX_VERSION(a, b, c);
> +}
> +
> +

The fopen could fail and if it does, I think you should default to "assume latest kernel"
Same goes for fscanf, you should at least test the return value.

   Jocke
Joakim Tjernlund - June 11, 2010, 9:53 a.m.
Joakim Tjernlund/Transmode wrote on 2010/06/11 11:44:27:
>
> >
> > The "struct nand_oobinfo" is able to record only 32 ECC code positions,which
> > is not enough for many big NAND chips. Therefore, this structure is replaced
> > by "struct nand_ecclayout" in linux kernel from the version 2.6.17.
> > Consequently, the ioctl command changed from MEMGETOOBSEL to ECCGETLAYOUT.
> >
> > Now update flash_eraseall to use the new ioctl command ECCGETLAYOUT. In order
> > to keep compatible with the old linux kernel, a linux version detection
> > function is added.
> >
> > Signed-off-by: Stanley.Miao <stanley.miao@windriver.com>
> > ---
> >  flash_eraseall.c |   71 ++++++++++++++++++++++++++++++++----------------------
> >  1 files changed, 42 insertions(+), 29 deletions(-)
> >
> > diff --git a/flash_eraseall.c b/flash_eraseall.c
> > index a22fc49..4e2108b 100644
> > --- a/flash_eraseall.c
> > +++ b/flash_eraseall.c
> > @@ -43,6 +43,8 @@
> >  #define PROGRAM "flash_eraseall"
> >  #define VERSION "$Revision: 1.22 $"
> >
> > +#define LINUX_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
> > +
> >  static const char *exe_name;
> >  static const char *mtd_device;
> >  static int quiet;      /* true -- don't output progress */
> > @@ -55,6 +57,21 @@ static void display_version (void);
> >  static struct jffs2_unknown_node cleanmarker;
> >  int target_endian = __BYTE_ORDER;
> >
> > +static int get_linux_version(void)
> > +{
> > +   FILE *fd;
> > +   int a = 0, b = 0, c = 0;
> > +
> > +   fd = fopen("/proc/version", "r");
> > +   if (fd) {
> > +      fscanf(fd, "Linux version %d.%d.%d", &a, &b, &c);
> > +      fclose(fd);
> > +   }
> > +
> > +   return LINUX_VERSION(a, b, c);
> > +}
> > +
> > +
> The fopen could fail and if it does, I think you should default to "assume
> latest kernel"
> Same goes for fscanf, you should at least test the return value.

Forgot, use uname(2) instead of /proc/version

Does the above scanf work on "2.6.31-gentoo-r6"?

  Jocke
Stanley.Miao - June 11, 2010, 10:36 a.m.
Joakim Tjernlund wrote:
> Joakim Tjernlund/Transmode wrote on 2010/06/11 11:44:27:
>   
>>> The "struct nand_oobinfo" is able to record only 32 ECC code positions,which
>>> is not enough for many big NAND chips. Therefore, this structure is replaced
>>> by "struct nand_ecclayout" in linux kernel from the version 2.6.17.
>>> Consequently, the ioctl command changed from MEMGETOOBSEL to ECCGETLAYOUT.
>>>
>>> Now update flash_eraseall to use the new ioctl command ECCGETLAYOUT. In order
>>> to keep compatible with the old linux kernel, a linux version detection
>>> function is added.
>>>
>>> Signed-off-by: Stanley.Miao <stanley.miao@windriver.com>
>>> ---
>>>  flash_eraseall.c |   71 ++++++++++++++++++++++++++++++++----------------------
>>>  1 files changed, 42 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/flash_eraseall.c b/flash_eraseall.c
>>> index a22fc49..4e2108b 100644
>>> --- a/flash_eraseall.c
>>> +++ b/flash_eraseall.c
>>> @@ -43,6 +43,8 @@
>>>  #define PROGRAM "flash_eraseall"
>>>  #define VERSION "$Revision: 1.22 $"
>>>
>>> +#define LINUX_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
>>> +
>>>  static const char *exe_name;
>>>  static const char *mtd_device;
>>>  static int quiet;      /* true -- don't output progress */
>>> @@ -55,6 +57,21 @@ static void display_version (void);
>>>  static struct jffs2_unknown_node cleanmarker;
>>>  int target_endian = __BYTE_ORDER;
>>>
>>> +static int get_linux_version(void)
>>> +{
>>> +   FILE *fd;
>>> +   int a = 0, b = 0, c = 0;
>>> +
>>> +   fd = fopen("/proc/version", "r");
>>> +   if (fd) {
>>> +      fscanf(fd, "Linux version %d.%d.%d", &a, &b, &c);
>>> +      fclose(fd);
>>> +   }
>>> +
>>> +   return LINUX_VERSION(a, b, c);
>>> +}
>>> +
>>> +
>>>       
>> The fopen could fail and if it does, I think you should default to "assume
>> latest kernel"
>> Same goes for fscanf, you should at least test the return value.
>>     

I am not sure what kernel version the proc file "version" began to exist.
If the fopen failed, it must be a old kernel, so I set the default to 
the old kernel.
It is the same with the current mtd-utils.

>
> Forgot, use uname(2) instead of /proc/version
>   

Why ?

> Does the above scanf work on "2.6.31-gentoo-r6"?
>   

I didn't do the test on all the platforms. Is there any reason that 
cause it not to work ?


Thanks
Stanley.
>   Jocke
>
>
>
Joakim Tjernlund - June 11, 2010, 10:44 a.m.
"stanley.miao" <stanley.miao@windriver.com> wrote on 2010/06/11 12:36:08:
>
> Joakim Tjernlund wrote:
> > Joakim Tjernlund/Transmode wrote on 2010/06/11 11:44:27:
> >
> >>> The "struct nand_oobinfo" is able to record only 32 ECC code positions,which
> >>> is not enough for many big NAND chips. Therefore, this structure is replaced
> >>> by "struct nand_ecclayout" in linux kernel from the version 2.6.17.
> >>> Consequently, the ioctl command changed from MEMGETOOBSEL to ECCGETLAYOUT.
> >>>
> >>> Now update flash_eraseall to use the new ioctl command ECCGETLAYOUT. In order
> >>> to keep compatible with the old linux kernel, a linux version detection
> >>> function is added.
> >>>
> >>> Signed-off-by: Stanley.Miao <stanley.miao@windriver.com>
> >>> ---
> >>>  flash_eraseall.c |   71 ++++++++++++++++++++++++++++++++----------------------
> >>>  1 files changed, 42 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/flash_eraseall.c b/flash_eraseall.c
> >>> index a22fc49..4e2108b 100644
> >>> --- a/flash_eraseall.c
> >>> +++ b/flash_eraseall.c
> >>> @@ -43,6 +43,8 @@
> >>>  #define PROGRAM "flash_eraseall"
> >>>  #define VERSION "$Revision: 1.22 $"
> >>>
> >>> +#define LINUX_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
> >>> +
> >>>  static const char *exe_name;
> >>>  static const char *mtd_device;
> >>>  static int quiet;      /* true -- don't output progress */
> >>> @@ -55,6 +57,21 @@ static void display_version (void);
> >>>  static struct jffs2_unknown_node cleanmarker;
> >>>  int target_endian = __BYTE_ORDER;
> >>>
> >>> +static int get_linux_version(void)
> >>> +{
> >>> +   FILE *fd;
> >>> +   int a = 0, b = 0, c = 0;
> >>> +
> >>> +   fd = fopen("/proc/version", "r");
> >>> +   if (fd) {
> >>> +      fscanf(fd, "Linux version %d.%d.%d", &a, &b, &c);
> >>> +      fclose(fd);
> >>> +   }
> >>> +
> >>> +   return LINUX_VERSION(a, b, c);
> >>> +}
> >>> +
> >>> +
> >>>
> >> The fopen could fail and if it does, I think you should default to "assume
> >> latest kernel"
> >> Same goes for fscanf, you should at least test the return value.
> >>
>

Forgot to say: Hi
:)

> I am not sure what kernel version the proc file "version" began to exist.

But the proc FS might not be mounted.

> If the fopen failed, it must be a old kernel, so I set the default to
> the old kernel.
> It is the same with the current mtd-utils.

I think that behaviour is wrong, one should assume current kernel should
one fail to retrive the exact version since it is much more likly that
the user is running someting new enough.

>
> >
> > Forgot, use uname(2) instead of /proc/version
> >
>
> Why ?

Cleaner, faster, shorter, more portable, less likely to fail.

>
> > Does the above scanf work on "2.6.31-gentoo-r6"?
> >
>
> I didn't do the test on all the platforms. Is there any reason that
> cause it not to work ?

I don't remeber how picky scanf is, will it parse the numbers correctly
when you have non WS trailing chars? In any case you need to check the
return value so you know if it failed.

  Jocke
Joakim Tjernlund - June 11, 2010, 12:11 p.m.
> "stanley.miao" <stanley.miao@windriver.com> wrote on 2010/06/11 12:36:08:
> >
> > Joakim Tjernlund wrote:
> > > Joakim Tjernlund/Transmode wrote on 2010/06/11 11:44:27:

> >
> > >
> > > Forgot, use uname(2) instead of /proc/version
> > >
> >
> > Why ?
>
> Cleaner, faster, shorter, more portable, less likely to fail.

Here is a quick example of uname(2):

#include <stdio.h>
#include <sys/utsname.h>

int main()
{
	int a, b, c , ret;
	struct utsname buf;

	ret = uname (&buf);
	if (ret < 0)
		return -1;
	ret = sscanf(buf.release, "%d.%d.%d", &a, &b, &c);
	if (ret != 3)
		return -1;
	printf("a:%d, b:%d, c:%d, ret:%d\n", a, b, c, ret);
	return 0;
}

Patch

diff --git a/flash_eraseall.c b/flash_eraseall.c
index a22fc49..4e2108b 100644
--- a/flash_eraseall.c
+++ b/flash_eraseall.c
@@ -43,6 +43,8 @@ 
 #define PROGRAM "flash_eraseall"
 #define VERSION "$Revision: 1.22 $"
 
+#define LINUX_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
+
 static const char *exe_name;
 static const char *mtd_device;
 static int quiet;		/* true -- don't output progress */
@@ -55,6 +57,21 @@  static void display_version (void);
 static struct jffs2_unknown_node cleanmarker;
 int target_endian = __BYTE_ORDER;
 
+static int get_linux_version(void)
+{
+	FILE *fd;
+	int a = 0, b = 0, c = 0;
+	
+	fd = fopen("/proc/version", "r");
+	if (fd) {
+		fscanf(fd, "Linux version %d.%d.%d", &a, &b, &c);
+		fclose(fd);
+	}
+
+	return LINUX_VERSION(a, b, c); 
+}
+
+
 int main (int argc, char *argv[])
 {
 	mtd_info_t meminfo;
@@ -84,41 +101,37 @@  int main (int argc, char *argv[])
 		if (!isNAND)
 			cleanmarker.totlen = cpu_to_je32 (sizeof (struct jffs2_unknown_node));
 		else {
-			struct nand_oobinfo oobinfo;
-
-			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0) {
-				fprintf(stderr, "%s: %s: unable to get NAND oobinfo\n", exe_name, mtd_device);
-				return 1;
-			}
+			struct nand_ecclayout ecclayout;
 
-			/* Check for autoplacement */
-			if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
-				/* Get the position of the free bytes */
-				if (!oobinfo.oobfree[0][1]) {
-					fprintf (stderr, " Eeep. Autoplacement selected and no empty space in oob\n");
+			memset(&ecclayout, 0, sizeof(ecclayout));
+			if (get_linux_version() > LINUX_VERSION(2, 6, 17)) {
+				if (ioctl(fd, ECCGETLAYOUT, &ecclayout) != 0) {
+					fprintf(stderr,	"%s: %s: unable to get NAND oob layout\n", \
+							exe_name, mtd_device);
 					return 1;
 				}
-				clmpos = oobinfo.oobfree[0][0];
-				clmlen = oobinfo.oobfree[0][1];
-				if (clmlen > 8)
-					clmlen = 8;
 			} else {
-				/* Legacy mode */
-				switch (meminfo.oobsize) {
-					case 8:
-						clmpos = 6;
-						clmlen = 2;
-						break;
-					case 16:
-						clmpos = 8;
-						clmlen = 8;
-						break;
-					case 64:
-						clmpos = 16;
-						clmlen = 8;
-						break;
+				struct nand_oobinfo oi;
+
+				if (ioctl(fd, MEMGETOOBSEL, &oi) != 0) {
+					fprintf(stderr, "%s: %s: unable to get NAND oobinfo\n", \
+							exe_name, mtd_device);
+					return 1;
 				}
+				memcpy(&ecclayout.eccpos, &oi.eccpos, sizeof(oi.eccpos));
+				memcpy(&ecclayout.oobfree, &oi.oobfree, sizeof(oi.oobfree));
+				ecclayout.eccbytes = oi.eccbytes;
+			}
+
+			/* Get the position of the free bytes */
+			if (!ecclayout.oobfree[0].length) {
+				fprintf(stderr, " Eeep. Autoplacement selected and no empty space in oob\n");
+				return 1;
 			}
+			clmpos = ecclayout.oobfree[0].offset;
+			clmlen = ecclayout.oobfree[0].length;
+			if (clmlen > 8)
+				clmlen = 8;
 			cleanmarker.totlen = cpu_to_je32(8);
 		}
 		cleanmarker.hdr_crc =  cpu_to_je32 (crc32 (0, &cleanmarker,  sizeof (struct jffs2_unknown_node) - 4));