nand: nand_oobfree handling

nand: nand_oobfree handling

[chris]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

Hi

It seems that while OOB layout structures (nand_ecclayout) are defined
to handle 8 (MTD_MAX_OOBFREE_ENTRIES) free areas, the current nand_base
code expects at least one area with a zero length.

http://lxr.linux.no/linux+v2.6.26/include/mtd/mtd-abi.h#L124
http://lxr.linux.no/linux+v2.6.26/drivers/mtd/nand/nand_base.c#L2552

This could cause problems for drivers that want to use all 8 free areas.

Also, it seems that flash_eraseall -j expects the first free area to be
big enough for the cleanmarker, which might not always be true.

I have attached a couple of patches that fix the problems I have
described above. They are taken against 2.6.24 and a non-current version
of mtd-utils... would you like me to prepare patches for the latest trees?

Cheers,

Chris

[-- Attachment #2: flash_eraseall.patch --]
[-- Type: text/x-diff, Size: 3110 bytes --]

diff --git a/flash_eraseall.c b/flash_eraseall.c
index 3694e3d..1609859 100644
--- a/flash_eraseall.c
+++ b/flash_eraseall.c
@@ -56,12 +56,16 @@ static void display_version (void);
 static struct jffs2_unknown_node cleanmarker;
 int target_endian = __BYTE_ORDER;
 
+#define MTD_MAX_OOBFREE_ENTRIES	8
+
 int main (int argc, char *argv[])
 {
 	mtd_info_t meminfo;
 	int fd, clmpos = 0, clmlen = 8;
 	erase_info_t erase;
 	int isNAND, bbtest = 1;
+	struct nand_oobinfo oobinfo;
+	unsigned char oobdata[64];
 
 	process_options(argc, argv);
 
@@ -81,29 +85,46 @@ int main (int argc, char *argv[])
 	isNAND = meminfo.type == MTD_NANDFLASH ? 1 : 0;
 
 	if (jffs2) {
+
+		/* prepare cleanmarker */
 		cleanmarker.magic = cpu_to_je16 (JFFS2_MAGIC_BITMASK);
 		cleanmarker.nodetype = cpu_to_je16 (JFFS2_NODETYPE_CLEANMARKER);
 		if (!isNAND)
 			cleanmarker.totlen = cpu_to_je32 (sizeof (struct jffs2_unknown_node));
-		else {
-			struct nand_oobinfo oobinfo;
+		else
+			cleanmarker.totlen = cpu_to_je32(8);
+		cleanmarker.hdr_crc =  cpu_to_je32(crc32(0, &cleanmarker,
+			sizeof(struct jffs2_unknown_node) - 4));
 
+		if (isNAND) {
 			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0) {
 				fprintf(stderr, "%s: %s: unable to get NAND oobinfo\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");
+				int i, one_len, copied = 0;
+				memset(oobdata, 0xff, sizeof(oobdata));
+				for (i = 0; copied < 8 &&
+						i < MTD_MAX_OOBFREE_ENTRIES &&
+						oobinfo.oobfree[i][1]; i++) {
+					if (8 - copied <= oobinfo.oobfree[i][1])
+						one_len = 8 - copied;
+					else
+						one_len = oobinfo.oobfree[i][1];
+					memcpy(oobdata + oobinfo.oobfree[i][0],
+						(char *)&cleanmarker + copied,
+						one_len);
+					copied += one_len;
+				}
+				clmpos = 0;
+				clmlen = oobinfo.oobfree[i][0] + one_len;
+				if (clmlen < 2) {
+					fprintf (stderr, " Eeep. Autoplacement"
+						" selected and no empty space"
+						" in oob\n");
 					exit(1);
 				}
-				clmpos = oobinfo.oobfree[0][0];
-				clmlen = oobinfo.oobfree[0][1];
-				if (clmlen > 8)
-					clmlen = 8;
 			} else {
 				/* Legacy mode */
 				switch (meminfo.oobsize) {
@@ -121,9 +142,8 @@ int main (int argc, char *argv[])
 					break;
 				}
 			}
-			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) {
@@ -169,7 +189,10 @@ int main (int argc, char *argv[])
 		/* write cleanmarker */
 		if (isNAND) {
 			struct mtd_oob_buf oob;
-			oob.ptr = (unsigned char *) &cleanmarker;
+			if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE)
+				oob.ptr = oobdata;
+			else
+				oob.ptr = (unsigned char *) &cleanmarker;
 			oob.start = erase.start + clmpos;
 			oob.length = clmlen;
 			if (ioctl (fd, MEMWRITEOOB, &oob) != 0) {

[-- Attachment #3: nand-oobfree-loop-fix.patch --]
[-- Type: text/x-diff, Size: 2924 bytes --]

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index e29c1da..4ebfe54 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -905,6 +905,8 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
 static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob,
 				  struct mtd_oob_ops *ops, size_t len)
 {
+	int i;
+
 	switch(ops->mode) {
 
 	case MTD_OOB_PLACE:
@@ -917,20 +919,21 @@ static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob,
 		uint32_t boffs = 0, roffs = ops->ooboffs;
 		size_t bytes = 0;
 
-		for(; free->length && len; free++, len -= bytes) {
+		for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES &&
+				free[i].length && len; i++, len -= bytes) {
 			/* Read request not from offset 0 ? */
 			if (unlikely(roffs)) {
-				if (roffs >= free->length) {
-					roffs -= free->length;
+				if (roffs >= free[i].length) {
+					roffs -= free[i].length;
 					continue;
 				}
-				boffs = free->offset + roffs;
+				boffs = free[i].offset + roffs;
 				bytes = min_t(size_t, len,
-					      (free->length - roffs));
+					      (free[i].length - roffs));
 				roffs = 0;
 			} else {
-				bytes = min_t(size_t, len, free->length);
-				boffs = free->offset;
+				bytes = min_t(size_t, len, free[i].length);
+				boffs = free[i].offset;
 			}
 			memcpy(oob, chip->oob_poi + boffs, bytes);
 			oob += bytes;
@@ -1570,6 +1573,7 @@ static uint8_t *nand_fill_oob(struct nand_chip *chip, uint8_t *oob,
 				  struct mtd_oob_ops *ops)
 {
 	size_t len = ops->ooblen;
+	int i;
 
 	switch(ops->mode) {
 
@@ -1583,20 +1587,21 @@ static uint8_t *nand_fill_oob(struct nand_chip *chip, uint8_t *oob,
 		uint32_t boffs = 0, woffs = ops->ooboffs;
 		size_t bytes = 0;
 
-		for(; free->length && len; free++, len -= bytes) {
+		for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES &&
+				free[i].length && len; i++, len -= bytes) {
 			/* Write request not from offset 0 ? */
 			if (unlikely(woffs)) {
-				if (woffs >= free->length) {
-					woffs -= free->length;
+				if (woffs >= free[i].length) {
+					woffs -= free[i].length;
 					continue;
 				}
-				boffs = free->offset + woffs;
+				boffs = free[i].offset + woffs;
 				bytes = min_t(size_t, len,
-					      (free->length - woffs));
+					      (free[i].length - woffs));
 				woffs = 0;
 			} else {
-				bytes = min_t(size_t, len, free->length);
-				boffs = free->offset;
+				bytes = min_t(size_t, len, free[i].length);
+				boffs = free[i].offset;
 			}
 			memcpy(chip->oob_poi + boffs, oob, bytes);
 			oob += bytes;
@@ -2525,7 +2530,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	 * the out of band area
 	 */
 	chip->ecc.layout->oobavail = 0;
-	for (i = 0; chip->ecc.layout->oobfree[i].length; i++)
+	for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES; i++)
 		chip->ecc.layout->oobavail +=
 			chip->ecc.layout->oobfree[i].length;
 	mtd->oobavail = chip->ecc.layout->oobavail;