Fix mtd-utils bugs

Fix mtd-utils bugs

[Stanley.Miao]

Re-send them because I didn't give a email subject in last version.

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 nandwrite to use the new ioctl command ECCGETLAYOUT. In order to
keep compatible with the old linux kernel, a linux version detection function
is added.

YAFFS and JFFS2 has updated and we don't need the arguments "forcelegacy",
"forcejffs2", "forceyaffs" anymore. Now clean them up. 


Stanley.

[PATCH 1/3] clean up the legacy interfaces in nandwrite.c

[Stanley.Miao]

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 nandwrite to use the new ioctl command ECCGETLAYOUT. In order to
keep compatible with the old linux kernel, a linux version detection function
is added.

YAFFS and JFFS2 has updated and we don't need the arguments "forcelegacy",
"forcejffs2", "forceyaffs" anymore. Now clean them up.

Signed-off-by: Stanley.Miao <stanley.miao@windriver.com>
---
 nandwrite.c |  215 +++++++++++++---------------------------------------------
 1 files changed, 48 insertions(+), 167 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index e691ebd..a0a843b 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -45,26 +45,21 @@
 #define MAX_PAGE_SIZE	4096
 #define MAX_OOB_SIZE	128
 
-// oob layouts to pass into the kernel as default
-static struct nand_oobinfo none_oobinfo = {
-	.useecc = MTD_NANDECC_OFF,
-};
-
-static struct nand_oobinfo jffs2_oobinfo = {
-	.useecc = MTD_NANDECC_PLACE,
-	.eccbytes = 6,
-	.eccpos = { 0, 1, 2, 3, 6, 7 }
-};
-
-static struct nand_oobinfo yaffs_oobinfo = {
-	.useecc = MTD_NANDECC_PLACE,
-	.eccbytes = 6,
-	.eccpos = { 8, 9, 10, 13, 14, 15}
-};
-
-static struct nand_oobinfo autoplace_oobinfo = {
-	.useecc = MTD_NANDECC_AUTOPLACE
-};
+#define LINUX_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
+
+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);
+}
 
 static void display_help (void)
 {
@@ -72,13 +67,7 @@ static void display_help (void)
 "Usage: nandwrite [OPTION] MTD_DEVICE [INPUTFILE|-]\n"
 "Writes to the specified MTD device.\n"
 "\n"
-"  -a, --autoplace         Use auto oob layout\n"
-"  -j, --jffs2             Force jffs2 oob layout (legacy support)\n"
-"  -y, --yaffs             Force yaffs oob layout (legacy support)\n"
-"  -f, --forcelegacy       Force legacy support on autoplacement-enabled mtd\n"
-"                          device\n"
 "  -m, --markbad           Mark blocks bad if write fails\n"
-"  -n, --noecc             Write without ecc\n"
 "  -o, --oob               Image contains oob data\n"
 "  -r, --raw               Image contains the raw oob data dumped by nanddump\n"
 "  -s addr, --start=addr   Set start address (default is 0)\n"
@@ -112,12 +101,7 @@ static int		mtdoffset = 0;
 static bool		quiet = false;
 static bool		writeoob = false;
 static bool		rawoob = false;
-static bool		autoplace = false;
 static bool		markbad = false;
-static bool		forcejffs2 = false;
-static bool		forceyaffs = false;
-static bool		forcelegacy = false;
-static bool		noecc = false;
 static bool		pad = false;
 static int		blockalign = 1; /*default to using 16K block size */
 
@@ -127,22 +111,17 @@ static void process_options (int argc, char * const argv[])
 
 	for (;;) {
 		int option_index = 0;
-		static const char *short_options = "ab:fjmnopqrs:y";
+		static const char *short_options = "b:mopqrs:";
 		static const struct option long_options[] = {
 			{"help", no_argument, 0, 0},
 			{"version", no_argument, 0, 0},
-			{"autoplace", no_argument, 0, 'a'},
 			{"blockalign", required_argument, 0, 'b'},
-			{"forcelegacy", no_argument, 0, 'f'},
-			{"jffs2", no_argument, 0, 'j'},
 			{"markbad", no_argument, 0, 'm'},
-			{"noecc", no_argument, 0, 'n'},
 			{"oob", no_argument, 0, 'o'},
 			{"pad", no_argument, 0, 'p'},
 			{"quiet", no_argument, 0, 'q'},
 			{"raw", no_argument, 0, 'r'},
 			{"start", required_argument, 0, 's'},
-			{"yaffs", no_argument, 0, 'y'},
 			{0, 0, 0, 0},
 		};
 
@@ -166,21 +145,6 @@ static void process_options (int argc, char * const argv[])
 			case 'q':
 				quiet = true;
 				break;
-			case 'a':
-				autoplace = true;
-				break;
-			case 'j':
-				forcejffs2 = true;
-				break;
-			case 'y':
-				forceyaffs = true;
-				break;
-			case 'f':
-				forcelegacy = true;
-				break;
-			case 'n':
-				noecc = true;
-				break;
 			case 'm':
 				markbad = true;
 				break;
@@ -258,8 +222,7 @@ int main(int argc, char * const argv[])
 	struct mtd_oob_buf oob;
 	loff_t offs;
 	int ret;
-	int oobinfochanged = 0;
-	struct nand_oobinfo old_oobinfo;
+	struct nand_ecclayout ecclayout;
 	bool failed = true;
 	// contains all the data read from the file so far for the current eraseblock
 	unsigned char *filebuf = NULL;
@@ -315,86 +278,30 @@ int main(int argc, char * const argv[])
 		exit(EXIT_FAILURE);
 	}
 
-	if (autoplace) {
-		/* Read the current oob info */
-		if (ioctl (fd, MEMGETOOBSEL, &old_oobinfo) != 0) {
-			perror ("MEMGETOOBSEL");
-			close (fd);
-			exit (EXIT_FAILURE);
-		}
-
-		// autoplace ECC ?
-		if (autoplace && (old_oobinfo.useecc != MTD_NANDECC_AUTOPLACE)) {
-
-			if (ioctl (fd, MEMSETOOBSEL, &autoplace_oobinfo) != 0) {
-				perror ("MEMSETOOBSEL");
-				close (fd);
-				exit (EXIT_FAILURE);
-			}
-			oobinfochanged = 1;
-		}
-	}
 
-	if (noecc)  {
-		ret = ioctl(fd, MTDFILEMODE, (void *) MTD_MODE_RAW);
-		if (ret == 0) {
-			oobinfochanged = 2;
-		} else {
-			switch (errno) {
-			case ENOTTY:
-				if (ioctl (fd, MEMGETOOBSEL, &old_oobinfo) != 0) {
-					perror ("MEMGETOOBSEL");
-					close (fd);
-					exit (EXIT_FAILURE);
-				}
-				if (ioctl (fd, MEMSETOOBSEL, &none_oobinfo) != 0) {
-					perror ("MEMSETOOBSEL");
-					close (fd);
-					exit (EXIT_FAILURE);
-				}
-				oobinfochanged = 1;
-				break;
-			default:
-				perror ("MTDFILEMODE");
-				close (fd);
-				exit (EXIT_FAILURE);
-			}
-		}
-	}
-
-	/*
-	 * force oob layout for jffs2 or yaffs ?
-	 * Legacy support
-	 */
-	if (forcejffs2 || forceyaffs) {
-		struct nand_oobinfo *oobsel = forcejffs2 ? &jffs2_oobinfo : &yaffs_oobinfo;
-
-		if (autoplace) {
-			fprintf(stderr, "Autoplacement is not possible for legacy -j/-y options\n");
-			goto restoreoob;
-		}
-		if ((old_oobinfo.useecc == MTD_NANDECC_AUTOPLACE) && !forcelegacy) {
-			fprintf(stderr, "Use -f option to enforce legacy placement on autoplacement enabled mtd device\n");
-			goto restoreoob;
-		}
-		if (meminfo.oobsize == 8) {
-			if (forceyaffs) {
-				fprintf (stderr, "YAFSS cannot operate on 256 Byte page size");
-				goto restoreoob;
-			}
-			/* Adjust number of ecc bytes */
-			jffs2_oobinfo.eccbytes = 3;
+	oob.length = meminfo.oobsize;
+	oob.ptr =  oobbuf;
+
+	memset(&ecclayout, 0, sizeof(ecclayout));
+	if (get_linux_version() > LINUX_VERSION(2, 6, 17)) {
+		if (ioctl(fd, ECCGETLAYOUT, &ecclayout) != 0) {
+			perror("ECCGETLAYOUT");
+			close(fd);
+			exit(EXIT_FAILURE);
 		}
+	} else {
+		struct nand_oobinfo oi;
 
-		if (ioctl (fd, MEMSETOOBSEL, oobsel) != 0) {
-			perror ("MEMSETOOBSEL");
-			goto restoreoob;
+		if (ioctl (fd, MEMGETOOBSEL, &oi) != 0) {
+			perror ("MEMGETOOBSEL");
+			close (fd);
+			exit (EXIT_FAILURE);
 		}
+		memcpy(&ecclayout.eccpos, &oi.eccpos, sizeof(oi.eccpos));
+		memcpy(&ecclayout.oobfree, &oi.oobfree, sizeof(oi.oobfree));
+		ecclayout.eccbytes = oi.eccbytes;
 	}
 
-	oob.length = meminfo.oobsize;
-	oob.ptr = noecc ? oobreadbuf : oobbuf;
-
 	/* Determine if we are reading from standard input or from a file. */
 	if (strcmp(img, standard_input) == 0) {
 		ifd = STDIN_FILENO;
@@ -558,6 +465,8 @@ int main(int argc, char * const argv[])
 		}
 
 		if (writeoob) {
+			int i, start, len;
+			int tags_pos = 0;
 			oobreadbuf = writebuf + meminfo.writesize;
 
 			// Read more data for the OOB from the input if there isn't enough in the buffer
@@ -594,40 +503,19 @@ int main(int argc, char * const argv[])
 				}
 			}
 
-			if (noecc) {
-				oob.ptr = oobreadbuf;
-			} else {
-				int i, start, len;
-				int tags_pos = 0;
-				/*
-				 *  We use autoplacement and have the oobinfo with the autoplacement
-				 * information from the kernel available
-				 *
-				 * Modified to support out of order oobfree segments,
-				 * such as the layout used by diskonchip.c
-				 */
-				if (!oobinfochanged && (old_oobinfo.useecc == MTD_NANDECC_AUTOPLACE)) {
-					for (i = 0;old_oobinfo.oobfree[i][1]; i++) {
-						/* Set the reserved bytes to 0xff */
-						start = old_oobinfo.oobfree[i][0];
-						len = old_oobinfo.oobfree[i][1];
-						if (rawoob)
-							memcpy(oobbuf + start,
-									oobreadbuf + start, len);
-						else
-							memcpy(oobbuf + start,
-									oobreadbuf + tags_pos, len);
-						tags_pos += len;
-					}
-				} else {
-					/* Set at least the ecc byte positions to 0xff */
-					start = old_oobinfo.eccbytes;
-					len = meminfo.oobsize - start;
+			for (i = 0; ecclayout.oobfree[i].length; i++) {
+				/* Set the reserved bytes to 0xff */
+				start = ecclayout.oobfree[i].offset;
+				len = ecclayout.oobfree[i].length;
+				if (rawoob)
 					memcpy(oobbuf + start,
-							oobreadbuf + start,
-							len);
-				}
+							oobreadbuf + start, len);
+				else
+					memcpy(oobbuf + start,
+							oobreadbuf + tags_pos, len);
+				tags_pos += len;
 			}
+
 			/* Write OOB data first, as ecc will be placed in there*/
 			oob.start = mtdoffset;
 			if (ioctl(fd, MEMWRITEOOB, &oob) != 0) {
@@ -686,13 +574,6 @@ closeall:
 	close(ifd);
 
 restoreoob:
-	if (oobinfochanged == 1) {
-		if (ioctl (fd, MEMSETOOBSEL, &old_oobinfo) != 0) {
-			perror ("MEMSETOOBSEL");
-			close (fd);
-			exit (EXIT_FAILURE);
-		}
-	}
 
 	close(fd);
 
-- 
1.5.4.3

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

[Stanley.Miao]

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); 
+}
+
+
 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));
-- 
1.5.4.3

[PATCH 3/3] Place the cleanmarker in OOB area according to the mode MTD_OOB_AUTO

[Stanley.Miao]

In the current linux kernel, Jffs2 writes the cleanmarker according to the
mode MTD_OOB_AUTO, so modify the layout of the cleanmarker from
MTD_OOB_PLACE to MTD_OOB_AUTO in flash_eraseall.

Signed-off-by: Stanley.Miao <stanley.miao@windriver.com>
---
 flash_eraseall.c |   40 ++++++++++++++++++++++------------------
 1 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/flash_eraseall.c b/flash_eraseall.c
index 4e2108b..f0f0371 100644
--- a/flash_eraseall.c
+++ b/flash_eraseall.c
@@ -34,7 +34,7 @@
 #include <time.h>
 #include <getopt.h>
 #include <sys/ioctl.h>
-#include <sys/mount.h>
+#include <sys/param.h>
 #include "crc32.h"
 
 #include <mtd/mtd-user.h>
@@ -75,9 +75,11 @@ static int get_linux_version(void)
 int main (int argc, char *argv[])
 {
 	mtd_info_t meminfo;
-	int fd, clmpos = 0, clmlen = 8;
+	int fd;
 	erase_info_t erase;
 	int isNAND, bbtest = 1;
+	unsigned char oobbuf[128];
+	memset(oobbuf, 0xFF, 128);
 
 	process_options(argc, argv);
 
@@ -98,11 +100,18 @@ int main (int argc, char *argv[])
 	if (jffs2) {
 		cleanmarker.magic = cpu_to_je16 (JFFS2_MAGIC_BITMASK);
 		cleanmarker.nodetype = cpu_to_je16 (JFFS2_NODETYPE_CLEANMARKER);
-		if (!isNAND)
+		if (!isNAND) {
 			cleanmarker.totlen = cpu_to_je32 (sizeof (struct jffs2_unknown_node));
-		else {
+			cleanmarker.hdr_crc = cpu_to_je32(crc32(0, &cleanmarker, \
+						sizeof(struct jffs2_unknown_node) - 4));
+		} else {
+			int already_read, num, i, start;
 			struct nand_ecclayout ecclayout;
 
+			cleanmarker.totlen = cpu_to_je32(8);
+			cleanmarker.hdr_crc = cpu_to_je32(crc32(0, &cleanmarker, \
+						sizeof(struct jffs2_unknown_node) - 4));
+
 			memset(&ecclayout, 0, sizeof(ecclayout));
 			if (get_linux_version() > LINUX_VERSION(2, 6, 17)) {
 				if (ioctl(fd, ECCGETLAYOUT, &ecclayout) != 0) {
@@ -122,19 +131,14 @@ int main (int argc, char *argv[])
 				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;
+			already_read = 0;
+			for (i = 0; (already_read < 8) && ecclayout.oobfree[i].length; i++) {
+				num = MIN(8 - already_read, ecclayout.oobfree[i].length);
+				start = ecclayout.oobfree[i].offset;
+				memcpy(oobbuf + start, (unsigned char *)&cleanmarker + already_read, num);
+				already_read += num;
 			}
-			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));
 	}
 
 	for (erase.start = 0; erase.start < meminfo.size; erase.start += meminfo.erasesize) {
@@ -174,9 +178,9 @@ int main (int argc, char *argv[])
 		/* write cleanmarker */
 		if (isNAND) {
 			struct mtd_oob_buf oob;
-			oob.ptr = (unsigned char *) &cleanmarker;
-			oob.start = erase.start + clmpos;
-			oob.length = clmlen;
+			oob.ptr = oobbuf;
+			oob.start = erase.start;
+			oob.length = meminfo.oobsize;
 			if (ioctl (fd, MEMWRITEOOB, &oob) != 0) {
 				fprintf(stderr, "\n%s: %s: MTD writeoob failure: %s\n", exe_name, mtd_device, strerror(errno));
 				continue;
-- 
1.5.4.3

Re: [PATCH 2/3] Discard the legacy interface MEMGETOOBSEL in flash_eraseall

[Joakim Tjernlund]

>
> 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

Re: [PATCH 2/3] Discard the legacy interface MEMGETOOBSEL in flash_eraseall

[Joakim Tjernlund]

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

Re: [PATCH 2/3] Discard the legacy interface MEMGETOOBSEL in flash_eraseall

[stanley.miao]

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
>
>
>   

Re: [PATCH 2/3] Discard the legacy interface MEMGETOOBSEL in flash_eraseall

[Joakim Tjernlund]

"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

Re: [PATCH 2/3] Discard the legacy interface MEMGETOOBSEL in flash_eraseall

[Joakim Tjernlund]

> "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;
}

Re: Fix mtd-utils bugs

[Artem Bityutskiy]

On Fri, 2010-06-11 at 17:36 +0800, Stanley.Miao wrote:
> Re-send them because I didn't give a email subject in last version.
> 
> 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 nandwrite to use the new ioctl command ECCGETLAYOUT. In order to
> keep compatible with the old linux kernel, a linux version detection function
> is added.
> 
> YAFFS and JFFS2 has updated and we don't need the arguments "forcelegacy",
> "forcejffs2", "forceyaffs" anymore. Now clean them up. 

Please, make sure your submissions have proper version. I have limited
amount of time and do not want to spend it figuring out which patch is
the latest. Use the best kernel practices and apply prefix your stuff
with [PATCHv3] or something like that.

Could you please re-submit your latest version?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: Fix mtd-utils bugs

[Artem Bityutskiy]

On Sun, 2010-06-13 at 12:51 +0300, Artem Bityutskiy wrote:
> On Fri, 2010-06-11 at 17:36 +0800, Stanley.Miao wrote:
> > Re-send them because I didn't give a email subject in last version.
> > 
> > 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 nandwrite to use the new ioctl command ECCGETLAYOUT. In order to
> > keep compatible with the old linux kernel, a linux version detection function
> > is added.
> > 
> > YAFFS and JFFS2 has updated and we don't need the arguments "forcelegacy",
> > "forcejffs2", "forceyaffs" anymore. Now clean them up. 
> 
> Please, make sure your submissions have proper version. I have limited
> amount of time and do not want to spend it figuring out which patch is
> the latest. Use the best kernel practices and apply prefix your stuff
> with [PATCHv3] or something like that.
> 
> Could you please re-submit your latest version?

Also, it is nice to put a short changelog, but I can live without it.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: Fix mtd-utils bugs

[stanley.miao]

Artem Bityutskiy wrote:
> On Sun, 2010-06-13 at 12:51 +0300, Artem Bityutskiy wrote:
>   
>> On Fri, 2010-06-11 at 17:36 +0800, Stanley.Miao wrote:
>>     
>>> Re-send them because I didn't give a email subject in last version.
>>>
>>> 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 nandwrite to use the new ioctl command ECCGETLAYOUT. In order to
>>> keep compatible with the old linux kernel, a linux version detection function
>>> is added.
>>>
>>> YAFFS and JFFS2 has updated and we don't need the arguments "forcelegacy",
>>> "forcejffs2", "forceyaffs" anymore. Now clean them up. 
>>>       
>> Please, make sure your submissions have proper version. I have limited
>> amount of time and do not want to spend it figuring out which patch is
>> the latest. Use the best kernel practices and apply prefix your stuff
>> with [PATCHv3] or something like that.
>>
>> Could you please re-submit your latest version?
>>     
>
> Also, it is nice to put a short changelog, but I can live without it.
>   

Sorry for the delayed reply. I was on vacation in the past 6 days.

I will re-send them soon.

Stanley.

Re: Fix mtd-utils bugs

[stanley.miao]

Artem Bityutskiy wrote:
> On Sun, 2010-06-13 at 12:51 +0300, Artem Bityutskiy wrote:
>   
>> On Fri, 2010-06-11 at 17:36 +0800, Stanley.Miao wrote:
>>     
>>> <snip>
>>>       
>> Please, make sure your submissions have proper version. I have limited
>> amount of time and do not want to spend it figuring out which patch is
>> the latest. Use the best kernel practices and apply prefix your stuff
>> with [PATCHv3] or something like that.
>>
>> Could you please re-submit your latest version?
>>     
>
> Also, it is nice to put a short changelog, but I can live without it.
>   

Is the subject of every emails is the short changelog you wanted ?

Stanley.

Re: Fix mtd-utils bugs

[Artem Bityutskiy]

On Fri, 2010-06-18 at 16:16 +0800, stanley.miao wrote:
> Artem Bityutskiy wrote:
> > On Sun, 2010-06-13 at 12:51 +0300, Artem Bityutskiy wrote:
> >   
> >> On Fri, 2010-06-11 at 17:36 +0800, Stanley.Miao wrote:
> >>     
> >>> <snip>
> >>>       
> >> Please, make sure your submissions have proper version. I have limited
> >> amount of time and do not want to spend it figuring out which patch is
> >> the latest. Use the best kernel practices and apply prefix your stuff
> >> with [PATCHv3] or something like that.
> >>
> >> Could you please re-submit your latest version?
> >>     
> >
> > Also, it is nice to put a short changelog, but I can live without it.
> >   
> 
> Is the subject of every emails is the short changelog you wanted ?

Hi,

Sorry, I do not understand the question. I do not need changelogs. I
just noted that there are best practices, and people use changelogs. You
can take a look at lkml archives at patch series from good kernel
hackers for example.

If you asked whether the changelog should be in each patch - no, just in
the "0" patch, and just short.

Anyway, I just want you to re-send your up-to-date patches as a separate
series with a v2 prefix.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: Fix mtd-utils bugs

[stanley.miao]

Artem Bityutskiy wrote:
> On Fri, 2010-06-18 at 16:16 +0800, stanley.miao wrote:
>   
>> Artem Bityutskiy wrote:
>>     
>>> On Sun, 2010-06-13 at 12:51 +0300, Artem Bityutskiy wrote:
>>>   
>>>       
>>>> On Fri, 2010-06-11 at 17:36 +0800, Stanley.Miao wrote:
>>>>     
>>>>         
>>>>> <snip>
>>>>>       
>>>>>           
>>>> Please, make sure your submissions have proper version. I have limited
>>>> amount of time and do not want to spend it figuring out which patch is
>>>> the latest. Use the best kernel practices and apply prefix your stuff
>>>> with [PATCHv3] or something like that.
>>>>
>>>> Could you please re-submit your latest version?
>>>>     
>>>>         
>>> Also, it is nice to put a short changelog, but I can live without it.
>>>   
>>>       
>> Is the subject of every emails is the short changelog you wanted ?
>>     
>
> Hi,
>
> Sorry, I do not understand the question. I do not need changelogs. I
> just noted that there are best practices, and people use changelogs. You
> can take a look at lkml archives at patch series from good kernel
> hackers for example.
>
> If you asked whether the changelog should be in each patch - no, just in
> the "0" patch, and just short.
>   

Oh, Got it. I misunderstood the "short changelog" previously.
I have sent the V3.

Thanks
Stanley.
> Anyway, I just want you to re-send your up-to-date patches as a separate
> series with a v2 prefix.
>
>   

Fix mtd-utils bugs

[Stanley.Miao]

(Re-send it because the previous one don't have a subject.)

During some BSP development process, I found some tools in mtd-utils
contain legacy interfaces and it is not suitable with the current linux
kernel. I post my patches here in case somebody else need them.

Re: Fix mtd-utils bugs

[stanley.miao]

BTW, because ECCGETLAYOUT was added in 2.6.17, so these patches will 
break on
linux kernel prior to 2.6.17.

Stanley.

Stanley.Miao wrote:
> (Re-send it because the previous one don't have a subject.)
>
> During some BSP development process, I found some tools in mtd-utils
> contain legacy interfaces and it is not suitable with the current linux
> kernel. I post my patches here in case somebody else need them.
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
>   

Re: Fix mtd-utils bugs

[Artem Bityutskiy]

On Wed, 2010-02-03 at 12:45 +0800, Stanley.Miao wrote:
> (Re-send it because the previous one don't have a subject.)
> 
> During some BSP development process, I found some tools in mtd-utils
> contain legacy interfaces and it is not suitable with the current linux
> kernel. I post my patches here in case somebody else need them.

In general, I really support your "let's kick ancient crap out"
approach.

But could you please be a bit more verbose, and sell your patchset a bit
better. E.g., write about who will suffer - which kernel versions? Why
you think we should not care about them suffering?

Please, write a good explanation. Then I'll try to bug dwmw2 to ack or
nack this. And then I can push it.

Although I have push rights to the mtd-utils, but I am a bit usure about
pushing your stuff.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)