Skip to contentSkip to conversation

mtd/utils: sync with MTD ioctl interface rework to get rid of MEMGETOOBSEL/MEMSETOOBSEL

PATCH In review Vitaly Wool 3 participants 8 messages Dec 2, 2005 – Dec 13, 2005 Memory technology devices (MTD)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mtd/utils: sync with MTD ioctl interface rework to get rid of MEMGETOOBSEL/MEMSETOOBSEL

Hi,
this patch reorganizes the MTD utls to
- not use MEMGETOOBSEL/MEMSETOOBSEL ioctls as they're planned for removal and anyway not needed
- use MEMGETOOBAVAIL ioctl to get the number of available OOB bytes and read/write OOB accordingly

Any input is most welcome.

Vitaly

Index: util/flash_eraseall.c
===================================================================
RCS file: /home/cvs/mtd/util/flash_eraseall.c,v
retrieving revision 1.24

flash_eraseall.c +10 -45
diff -u -r1.24 flash_eraseall.c
@@ -59,10 +59,11 @@
 int main (int argc, char *argv[])
 {
 	mtd_info_t meminfo;
-	int fd, ebhpos = 0, ebhlen = 0;
+	int fd;
 	erase_info_t erase;
 	int isNAND, bbtest = 1;
 	uint32_t pages_per_eraseblock, available_oob_space;
+	uint32_t oobavail;
 
 	process_options(argc, argv);
 
@@ -95,41 +96,13 @@
 					 sizeof(struct jffs2_raw_ebh) - sizeof(struct jffs2_unknown_node) - 4));
 
 		if (isNAND) {
-			struct nand_oobinfo oobinfo;
-
-			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0) {
-				fprintf(stderr, "%s: %s: unable to get NAND oobinfo\n", exe_name, mtd_device);
+			if (ioctl(fd, MEMGETOOBAVAIL, &oobavail) != 0) {
+				fprintf(stderr, "%s: %s: unable to get NAND oobavail\n", exe_name, mtd_device);
 				exit(1);
 			}
 
-			/* 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");
-					exit(1);
-				}
-				ebhpos = oobinfo.oobfree[0][0];
-				ebhlen = oobinfo.oobfree[0][1];
-			} else {
-				/* Legacy mode */
-				switch (meminfo.oobsize) {
-				case 8:
-					ebhpos = 6;
-					ebhlen = 2;
-					break;
-				case 16:
-					ebhpos = 8;
-					ebhlen = 8;
-					break;
-				case 64:
-					ebhpos = 16;
-					ebhlen = 8;
-					break;
-				}
-			}
 			pages_per_eraseblock = meminfo.erasesize/meminfo.oobblock;
-			available_oob_space = ebhlen * pages_per_eraseblock;
+			available_oob_space = oobavail * pages_per_eraseblock;
 			if (available_oob_space < sizeof(struct jffs2_raw_ebh)) {
 				fprintf(stderr, "The OOB area(%d) is not big enough to hold eraseblock_header(%d)", available_oob_space, sizeof(struct jffs2_raw_ebh));
 				exit(1);
@@ -180,20 +153,12 @@
 		/* write cleanmarker */
 		if (isNAND) {
 			struct mtd_oob_buf oob;
-			uint32_t i = 0, written = 0;
 
-			while (written < sizeof(struct jffs2_raw_ebh)) {
-				oob.ptr = (unsigned char *) &ebh + written;
-				oob.start = erase.start + meminfo.oobblock*i + ebhpos;
-				oob.length = (sizeof(struct jffs2_raw_ebh) - written) < ebhlen ? (sizeof(struct jffs2_raw_ebh) - written) : ebhlen;
-				if (ioctl (fd, MEMWRITEOOB, &oob) != 0) {
-					fprintf(stderr, "\n%s: %s: MTD writeoob failure: %s\n", exe_name, mtd_device, strerror(errno));
-					break;
-				}
-				i++;
-				written += oob.length;
-			}
-			if (written < sizeof(struct jffs2_raw_ebh)) {
+			oob.ptr = (unsigned char *) &ebh;
+			oob.start = erase.start;
+			oob.length = sizeof(struct jffs2_raw_ebh) < oobavail ? sizeof(struct jffs2_raw_ebh) : oobavail;
+			if (ioctl (fd, MEMWRITEOOB, &oob) != 0) {
+				fprintf(stderr, "\n%s: %s: MTD writeoob failure: %s\n", exe_name, mtd_device, strerror(errno));
 				continue;
 			}
 		} else {

Index: util/nanddump.c
===================================================================
RCS file: /home/cvs/mtd/util/nanddump.c,v
retrieving revision 1.29

nanddump.c +21 -24

79 lines not on this page · raw message

Index: util/nandwrite.c
===================================================================
RCS file: /home/cvs/mtd/util/nandwrite.c,v
retrieving revision 1.32

nandwrite.c +13 -144

244 lines not on this page · raw message

cvs diff: Diffing util/checkfs
cvs diff: Diffing util/jittertest

#1 lore raw

Hi Vitaly, my comments...

Quoted 10 lines

+++ util/nanddump.c 2 Dec 2005 06:54:11 -0000

nanddump.c +1 -0
@@ -163,6 +163,7 @@
 	struct mtd_oob_buf oob = {0, 16, oobbuf};
 	mtd_info_t meminfo;
 	char pretty_buf[80];
+	uint32_t oobavail;
 
  	process_options(argc, argv);
 

nanddump.c line 187

Quoted 8 lines
nanddump.c +5 -0
@@ -179,6 +180,13 @@
 		exit (1);
 	}
 
+	if (ioctl(fd, MEMGETOOBAVAIL, &oobavail) != 0) {
+		perror("unable to get NAND oobavail");
+		close(fd);
+		exit(1);
+	}

How about fall back to all 16/64/etc. avail?

For debugging purposes it can be nice to see the raw OOB, in case that can be shoehorned in here somehow.

nandwrite.c line 81

Quoted 4 lines
nandwrite.c +1 -2
+++ util/nandwrite.c	2 Dec 2005 06:54:11 -0000
 
-	       "  -j, --jffs2  	 	force jffs2 oob layout (legacy support)\n"
-	       "  -y, --yaffs  	 	force yaffs oob layout (legacy support)\n"

Again, it may be good to keep a legacy mode around during the transition period.

nandwrite.c line 292

Quoted 4 lines
nandwrite.c +1 -1
 		if (writeoob) {
 			/* Read OOB data from input file, exit on failure */
-			if ((cnt = read(ifd, oobreadbuf, meminfo.oobsize)) != meminfo.oobsize) {
+			if ((cnt = read(ifd, oobreadbuf, oobavail)) != oobavail) {

This requires the input file to be tailored to the oobavail of a specific destination device, reducing the benefit of autoplacement. It may be best to continue to pad input files oob data to the full oobsize, much like the data portion is handled.

nandwrite.c line 297

Quoted 1 line
nandwrite.c +1 -0
+				memcpy(oobbuf, oobreadbuf, oobavail);

No need for oobreadbuf and copy if no OOB layout processing done anymore?

Signature · 1 line
Todd
#2 lore raw

Hi Todd,

Quoted 24 lines · Todd Poynor

Hi Vitaly, my comments...

Quoted 10 lines

+++ util/nanddump.c 2 Dec 2005 06:54:11 -0000

nanddump.c +1 -0
@@ -163,6 +163,7 @@
     struct mtd_oob_buf oob = {0, 16, oobbuf};
     mtd_info_t meminfo;
     char pretty_buf[80];
+    uint32_t oobavail;
 
      process_options(argc, argv);
 

nanddump.c line 187

Quoted 8 lines
nanddump.c +5 -0
@@ -179,6 +180,13 @@
         exit (1);
     }
 
+    if (ioctl(fd, MEMGETOOBAVAIL, &oobavail) != 0) {
+        perror("unable to get NAND oobavail");
+        close(fd);
+        exit(1);
+    }

How about fall back to all 16/64/etc. avail?

Quoted 1 line

For debugging purposes it can be nice to see the raw OOB, in case that can be shoehorned in here somehow.

Well, the main idea of what I'm implementing is to hide the internal structure of the OOB data from user. Therefore it's no guaranteed way in this implementation to read the raw OOB.
On the other hand, not being able to get oobavail means a problem so I wouldn't try to read OOB in this situation.
However I can a) add a flag to both read_oob/write_oob whether to read oobavail or raw OOB and implement corresponding functionality/flag in nanddump, or) fall back to considering oobavail being equal to oobsize if getting oobavail fails but neither variant seems good to me...

Quoted 11 lines
Quoted 8 lines
(quoted diff) +1 -1
+++ util/nandwrite.c    2 Dec 2005 06:54:11 -0000
 
 
-           "  -j, --jffs2           force jffs2 oob layout (legacy 

support)\n"
- " -y, --yaffs force yaffs oob layout (legacy
support)\n"

Again, it may be good to keep a legacy mode around during the transition period.

I'm afraid it's not possible. And this legacy support has been sheduled to go away long ago, I hope now is a good time :)

Quoted 9 lines
Quoted 6 lines
         if (writeoob) {
             /* Read OOB data from input file, exit on failure */

- if ((cnt = read(ifd, oobreadbuf, meminfo.oobsize)) !=
meminfo.oobsize) {
+ if ((cnt = read(ifd, oobreadbuf, oobavail)) != oobavail) {

This requires the input file to be tailored to the oobavail of a specific destination device, reducing the benefit of autoplacement. It may be best to continue to pad input files oob data to the full oobsize, much like the data portion is handled.

Well, padding doesn't work in nandwrite if the image to be written contains OOB data. The idea as I see it is if you are trying to write the image with OOB data, you should know what you're doing, and that implies knowledge of the oobavail size. On the other hand, it might be useful to implement an option for nandwrite which specifies what OOB data length user supposes (default will be the oobavail).

Quoted 6 lines
Quoted 2 lines

+ memcpy(oobbuf, oobreadbuf, oobavail);

No need for oobreadbuf and copy if no OOB layout processing done anymore?

Yeah, thanks.
And 'noecc' parameter should also go away. An update of the patch will follow...

Vitaly

#3 lore raw
Quoted 1 line · Vitaly Wool

Todd Poynor wrote:

...

Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
         if (writeoob) {
             /* Read OOB data from input file, exit on failure */

- if ((cnt = read(ifd, oobreadbuf, meminfo.oobsize)) !=
meminfo.oobsize) {
+ if ((cnt = read(ifd, oobreadbuf, oobavail)) != oobavail) {

This requires the input file to be tailored to the oobavail of a specific destination device, reducing the benefit of autoplacement. It may be best to continue to pad input files oob data to the full oobsize, much like the data portion is handled.

Well, padding doesn't work in nandwrite if the image to be written contains OOB data. The idea as I see it is if you are trying to write the image with OOB data, you should know what you're doing, and that implies knowledge of the oobavail size. On the other hand, it might be useful to implement an option for nandwrite which specifies what OOB data length user supposes (default will be the oobavail).

In order to make it work as above, we'd need a way for users to find out what oobavail their mtd driver uses (in dmesg and/or a util), and an option on all the fs utils to write that number of bytes. A file format that specifies what data and OOB size have been generated would do the trick, if we want to get that fancy about it. Just filling out all 16 or 64 bytes in the input file and truncating to oobavail at nandwrite time seems a lower-cost workaround.

Signature · 1 line
Todd
#4 lore raw
Quoted 11 lines · Todd Poynor
Quoted 8 lines

Well, padding doesn't work in nandwrite if the image to be written contains OOB data. The idea as I see it is if you are trying to write the image with OOB data, you should know what you're doing, and that implies knowledge of the oobavail size.
On the other hand, it might be useful to implement an option for nandwrite which specifies what OOB data length user supposes (default will be the oobavail).

In order to make it work as above, we'd need a way for users to find out what oobavail their mtd driver uses (in dmesg and/or a util)

Definitely not by consulting dmesg. There is no guarantee that dmesg contains the information you need at the time you are asking for it.

Quoted 6 lines

, and an
option on all the fs utils to write that number of bytes. A file format that specifies what data and OOB size have been generated would do the trick, if we want to get that fancy about it. Just filling out all 16 or 64 bytes in the input file and truncating to oobavail at nandwrite time seems a lower-cost workaround.

Find a way to give access to raw data and to the "translated" data is the goal. Everything else is just an ivory tower experiment.

tglx

#5 lore raw
In reply to Vitaly Wool
Quoted 8 lines · Vitaly Wool

Hi,
this patch reorganizes the MTD utls to
- not use MEMGETOOBSEL/MEMSETOOBSEL ioctls as they're planned for removal and anyway not needed
- use MEMGETOOBAVAIL ioctl to get the number of available OOB bytes and read/write OOB accordingly

Any input is most welcome.

1. Rendering the utils unusable for older kernels is wrong.

Its simple to handle this in the utils depending on the functionality which is exposed by the kernel.

2. Your current approach prevents me to
- dump the raw flash content including OOB data
- write a nand image with given oob data

This is essential for troubleshooting and must still be possible somehow.

tglx

#6 lore raw
Quoted 10 lines · Thomas Gleixner
Quoted 7 lines · Vitaly Wool

Hi,
this patch reorganizes the MTD utls to
- not use MEMGETOOBSEL/MEMSETOOBSEL ioctls as they're planned for removal and anyway not needed
- use MEMGETOOBAVAIL ioctl to get the number of available OOB bytes and read/write OOB accordingly

Quoted 10 lines
Quoted 3 lines

Any input is most welcome.

1. Rendering the utils unusable for older kernels is wrong.

Its simple to handle this in the utils depending on the functionality which is exposed by the kernel.

Makes sense to me.

Quoted 10 lines

2. Your current approach prevents me to
- dump the raw flash content including OOB data
- write a nand image with given oob data

This is essential for troubleshooting and must still be possible somehow.

Lemme disagree with you here. Raw OOB content-related stuff is not IMHO the thing to debug from userland. Suggest prinitng out the raw OOB contents for MTD_DEBUG_LEVEL3 instead.

Vitaly

#7 lore raw
Quoted 10 lines · Vitaly Wool
Quoted 7 lines

2. Your current approach prevents me to
- dump the raw flash content including OOB data
- write a nand image with given oob data

This is essential for troubleshooting and must still be possible somehow.

Lemme disagree with you here. Raw OOB content-related stuff is not IMHO the thing to debug from userland.
Suggest prinitng out the raw OOB contents for MTD_DEBUG_LEVEL3 instead.

Uuurg. Do you really want do analyse 20MB crappy formatted printk output?

We want output from live systems without requesting the user to recompile the kernel and setup capturing scenarios.

We tracked down problems in the past by asking for a simple

nanddump /dev/mtdX >file.dmp

and I really want to keep it that way. Its simple, proven and people can handle it.

tglx