mtd-utils: "flash_erase -j" failes with "unable to get NAND oobinfo" on large flash chips.

mtd-utils: "flash_erase -j" failes with "unable to get NAND oobinfo" on large flash chips.

[Daid]

I've made a patch to fix flash_erase with large flash chips.
flash_erase uses MEMGETOOBSEL which no longer works if the ECC area is
larger then 32 bytes. ECCGETLAYOUT is the replacement ioctl.

This patch is based on the work of Stanley Miao, he made a patch in
June 2010 for flash_eraseall.
http://lists.infradead.org/pipermail/linux-mtd/2010-June/031981.html
I've implemented the backwards compatibility differently, checking
kernel versions doesn't feel correct.


diff -ur mtd-utils-v1.4.4/flash_erase.c mtd-utils-patch/flash_erase.c
--- mtd-utils-v1.4.4/flash_erase.c	2011-04-01 18:31:43.000000000 +0200
+++ mtd-utils-patch/flash_erase.c	2011-04-12 11:41:43.000000000 +0200
@@ -193,37 +193,57 @@
 		if (!isNAND)
 			cleanmarker.totlen = cpu_to_je32(sizeof(cleanmarker));
 		else {
-			struct nand_oobinfo oobinfo;
+#if defined(ECCGETLAYOUT)
+			struct nand_ecclayout ecclayout;

-			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0)
-				return sys_errmsg("%s: unable to get NAND oobinfo", mtd_device);
-
-			/* Check for autoplacement */
-			if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
+			memset(&ecclayout, 0, sizeof(ecclayout));
+			if (ioctl(fd, ECCGETLAYOUT, &ecclayout) == 0) {
 				/* Get the position of the free bytes */
-				if (!oobinfo.oobfree[0][1])
-					return errmsg(" Eeep. Autoplacement selected and no empty space in oob");
-				clmpos = oobinfo.oobfree[0][0];
-				clmlen = oobinfo.oobfree[0][1];
-				if (clmlen > 8)
-					clmlen = 8;
+				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;
 			} else {
-				/* Legacy mode */
-				switch (mtd.oob_size) {
-					case 8:
-						clmpos = 6;
-						clmlen = 2;
-						break;
-					case 16:
-						clmpos = 8;
-						clmlen = 8;
-						break;
-					case 64:
-						clmpos = 16;
+#endif/*defined(ECCGETLAYOUT)*/
+				/* new ECC interface failed or not available, fall back to old
OOB interface, which does not support large flash */
+				struct nand_oobinfo oobinfo;
+				if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0)
+					return sys_errmsg("%s: unable to get NAND oobinfo", mtd_device);
+
+				/* Check for autoplacement */
+				if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
+					/* Get the position of the free bytes */
+					if (!oobinfo.oobfree[0][1])
+						return errmsg(" Eeep. Autoplacement selected and no empty space in oob");
+					clmpos = oobinfo.oobfree[0][0];
+					clmlen = oobinfo.oobfree[0][1];
+					if (clmlen > 8)
 						clmlen = 8;
-						break;
+				} else {
+					/* Legacy mode */
+					switch (mtd.oob_size) {
+						case 8:
+							clmpos = 6;
+							clmlen = 2;
+							break;
+						case 16:
+							clmpos = 8;
+							clmlen = 8;
+							break;
+						case 64:
+							clmpos = 16;
+							clmlen = 8;
+							break;
+					}
 				}
+#if defined(ECCGETLAYOUT)
 			}
+#endif/*defined(ECCGETLAYOUT)*/
+
+			if (clmlen > 8)
+				clmlen = 8;
 			cleanmarker.totlen = cpu_to_je32(8);
 		}
 		cleanmarker.hdr_crc = cpu_to_je32(mtd_crc32(0, &cleanmarker,
sizeof(cleanmarker) - 4));

Re: mtd-utils: "flash_erase -j" failes with "unable to get NAND oobinfo" on large flash chips.

[Artem Bityutskiy]

On Tue, 2011-04-12 at 11:58 +0200, Daid wrote:
> I've made a patch to fix flash_erase with large flash chips.
> flash_erase uses MEMGETOOBSEL which no longer works if the ECC area is
> larger then 32 bytes. ECCGETLAYOUT is the replacement ioctl.
> 
> This patch is based on the work of Stanley Miao, he made a patch in
> June 2010 for flash_eraseall.
> http://lists.infradead.org/pipermail/linux-mtd/2010-June/031981.html
> I've implemented the backwards compatibility differently, checking
> kernel versions doesn't feel correct.

Hi Daid, would you please use libmtd instead? If libmtd does not have
some functionality you need - just add it there.

The thing is that we are trying to unify this stuff a little.

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

Re: mtd-utils: "flash_erase -j" failes with "unable to get NAND oobinfo" on large flash chips.

[Daid]

On Thu, Apr 14, 2011 at 8:09 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Tue, 2011-04-12 at 11:58 +0200, Daid wrote:
>> I've made a patch to fix flash_erase with large flash chips.
>> flash_erase uses MEMGETOOBSEL which no longer works if the ECC area is
>> larger then 32 bytes. ECCGETLAYOUT is the replacement ioctl.
>>
>> This patch is based on the work of Stanley Miao, he made a patch in
>> June 2010 for flash_eraseall.
>> http://lists.infradead.org/pipermail/linux-mtd/2010-June/031981.html
>> I've implemented the backwards compatibility differently, checking
>> kernel versions doesn't feel correct.
>
> Hi Daid, would you please use libmtd instead? If libmtd does not have
> some functionality you need - just add it there.
>
> The thing is that we are trying to unify this stuff a little.
>
> --
> Best Regards,
> Artem Bityutskiy (Артём Битюцкий)
>
>

Hi Artem, libmtd didn't have the functionality I needed. I've made a new patch
with the functionality build into libmtd.

I've added the extra information to struct mtd_dev_info, however, the
info needed is
only available with ioctls, so even with the sysfs interface it needs
to open /dev/mtdXX.
When you use mtd_get_dev_info, it reads all the information, which simple for
the user of libmtd, but sort of mixes legacy and the sysfs interface.

Note that I can only verify if it works with the legacy interface,
because we are still a few
kernel versions behind on the hardware I run on. (2.6.27)

Best Regards,
David Braam

diff -ur mtd-utils-ori/flash_erase.c mtd-utils-v1.4.4/flash_erase.c
--- mtd-utils-ori/flash_erase.c	2011-04-01 18:31:43.000000000 +0200
+++ mtd-utils-v1.4.4/flash_erase.c	2011-04-14 13:06:16.000000000 +0200
@@ -190,40 +190,18 @@
 	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(cleanmarker));
-		else {
-			struct nand_oobinfo oobinfo;
-
-			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0)
-				return sys_errmsg("%s: unable to get NAND oobinfo", mtd_device);
-
-			/* Check for autoplacement */
-			if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
-				/* Get the position of the free bytes */
-				if (!oobinfo.oobfree[0][1])
-					return errmsg(" Eeep. Autoplacement selected and no empty space in oob");
-				clmpos = oobinfo.oobfree[0][0];
-				clmlen = oobinfo.oobfree[0][1];
-				if (clmlen > 8)
-					clmlen = 8;
-			} else {
-				/* Legacy mode */
-				switch (mtd.oob_size) {
-					case 8:
-						clmpos = 6;
-						clmlen = 2;
-						break;
-					case 16:
-						clmpos = 8;
-						clmlen = 8;
-						break;
-					case 64:
-						clmpos = 16;
-						clmlen = 8;
-						break;
-				}
-			}
+		} else {
+			if (!mtd.oob_free[0].length) {
+				fprintf(stderr, " Eeep. No empty space in oob for JFFS2 cleanmarkers\n");
+				return 1;
+ 			}
+			clmpos = mtd.oob_free[0].offset;
+			clmlen = mtd.oob_free[0].length;
+			if (clmlen > 8)
+				clmlen = 8;
+			
 			cleanmarker.totlen = cpu_to_je32(8);
 		}
 		cleanmarker.hdr_crc = cpu_to_je32(mtd_crc32(0, &cleanmarker,
sizeof(cleanmarker) - 4));
diff -ur mtd-utils-ori/include/libmtd.h mtd-utils-v1.4.4/include/libmtd.h
--- mtd-utils-ori/include/libmtd.h	2011-04-01 18:31:43.000000000 +0200
+++ mtd-utils-v1.4.4/include/libmtd.h	2011-04-14 11:40:48.000000000 +0200
@@ -31,6 +31,8 @@
 #define MTD_NAME_MAX 127
 /* Maximum MTD device type string length */
 #define MTD_TYPE_MAX 64
+/* Maximum MTD nand OOB free entries */
+#define MTD_NAND_MAX_OOBFREE_ENTRIES 8

 /* MTD library descriptor */
 typedef void * libmtd_t;
@@ -50,6 +52,17 @@
 };

 /**
+ * struct mtd_nand_oobfree - information about free OOB areas
+ * @offset: offset of free OOB area from start of OOB area
+ * @length: length of free OOB area
+ */
+struct mtd_nand_oobfree
+{
+	int offset;
+	int length;
+};
+
+/**
  * struct mtd_dev_info - information about an MTD device.
  * @mtd_num: MTD device number
  * @major: major number of corresponding character device
@@ -66,6 +79,10 @@
  * @region_cnt: count of additional erase regions
  * @writable: zero if the device is read-only
  * @bb_allowed: non-zero if the MTD device may have bad eraseblocks
+ * @eccbytes: number of ecc bytes in OOB area
+ * @eccpos: position of ecc bytes in OOB area
+ * @oob_available: number of bytes available in OOB area free other
use then ECC
+ * @oob_free: location and sizes of free OOB areas
  */
 struct mtd_dev_info
 {
@@ -84,6 +101,10 @@
 	int region_cnt;
 	unsigned int writable:1;
 	unsigned int bb_allowed:1;
+	int eccbytes;
+	int eccpos[64];
+	int oob_available;
+	struct mtd_nand_oobfree oob_free[MTD_NAND_MAX_OOBFREE_ENTRIES];
 };

 /**
diff -ur mtd-utils-ori/lib/libmtd.c mtd-utils-v1.4.4/lib/libmtd.c
--- mtd-utils-ori/lib/libmtd.c	2011-04-01 18:31:43.000000000 +0200
+++ mtd-utils-v1.4.4/lib/libmtd.c	2011-04-14 13:30:20.000000000 +0200
@@ -37,6 +37,8 @@
 #include "libmtd_int.h"
 #include "common.h"

+#define MTD_DEV_PATT  "/dev/mtd%d"
+
 /**
  * mkpath - compose full path from 2 given components.
  * @path: the first component
@@ -548,6 +550,80 @@
 	return 1;
 }

+/**
+ * mtd_get_oob_info - fill the mtd_dev_info structure with information
+ *  about the OOB data.
+ */
+static int mtd_get_oob_info(struct mtd_dev_info *mtd)
+{
+	int i, fd;
+	char node[sizeof(MTD_DEV_PATT) + 20];
+
+	sprintf(node, MTD_DEV_PATT, mtd->mtd_num);
+	fd = open(node, O_RDWR);
+	if (fd == -1)
+		return sys_errmsg("cannot open \"%s\"", node);
+		
+	if (mtd->type == MTD_NANDFLASH) {
+		/* get ECC/OOB information for NAND flash */
+#if defined(ECCGETLAYOUT)
+		struct nand_ecclayout ecclayout;
+		memset(&ecclayout, 0, sizeof(ecclayout));
+		if (ioctl(fd, ECCGETLAYOUT, &ecclayout) == 0) {
+			mtd->eccbytes = ecclayout.eccbytes;
+			for(i=0; i<64; i++) {
+				mtd->eccpos[i] = ecclayout.eccpos[i];
+			}
+			mtd->oob_available = ecclayout.oobavail;
+			for (i=0; i<MTD_NAND_MAX_OOBFREE_ENTRIES; i++) {
+				mtd->oob_free[i].offset = ecclayout.oobfree[i].offset;
+				mtd->oob_free[i].length = ecclayout.oobfree[i].length;
+			}
+		} else {
+#endif/*defined(ECCGETLAYOUT)*/
+			/* new ECC interface failed or not available, fall back to old OOB
interface, which does not support large flash */
+			struct nand_oobinfo oobinfo;
+			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) == 0)
+			{
+				/* Check for autoplacement */
+				mtd->eccbytes = oobinfo.eccbytes;
+				for(i=0;i<32;i++) {
+					mtd->eccpos[i] = oobinfo.eccpos[i];
+				}
+				if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
+					for(i=0;i<8;i++) {
+						mtd->oob_free[i].offset = oobinfo.oobfree[i][0];
+						mtd->oob_free[i].length = oobinfo.oobfree[i][1];
+						mtd->oob_available += mtd->oob_free[i].length;
+					}
+				} else{
+					/* Legacy mode */
+					switch (mtd->oob_size) {
+						case 8:
+							mtd->oob_free[0].offset = 6;
+							mtd->oob_free[0].length = 2;
+							break;
+						case 16:
+							mtd->oob_free[0].offset = 8;
+							mtd->oob_free[0].length = 8;
+							break;
+						case 64:
+							mtd->oob_free[0].offset = 16;
+							mtd->oob_free[0].length = 8;
+							break;
+					}
+					mtd->oob_available = mtd->oob_free[0].length;
+				}
+			}
+#if defined(ECCGETLAYOUT)
+		}
+#endif/*defined(ECCGETLAYOUT)*/
+	}
+	close(fd);
+	
+	return 0;
+}
+
 libmtd_t libmtd_open(void)
 {
 	struct libmtd *lib;
@@ -720,9 +796,12 @@
 	memset(mtd, 0, sizeof(struct mtd_dev_info));
 	mtd->mtd_num = mtd_num;

-	if (!lib->sysfs_supported)
-		return legacy_get_dev_info1(mtd_num, mtd);
-	else {
+	if (!lib->sysfs_supported) {
+		ret = legacy_get_dev_info1(mtd_num, mtd);
+		if (ret < 0)
+			return ret;
+		return mtd_get_oob_info(mtd);
+	} else {
 		char file[strlen(lib->mtd) + 10];

 		sprintf(file, lib->mtd, mtd_num);
@@ -767,6 +846,9 @@
 	mtd->eb_cnt = mtd->size / mtd->eb_size;
 	mtd->type = type_str2int(mtd->type_str);
 	mtd->bb_allowed = !!(mtd->type == MTD_NANDFLASH);
+	
+	if (mtd_get_oob_info(mtd))
+		return -1;

 	return 0;
 }
@@ -777,7 +859,12 @@
 	struct libmtd *lib = (struct libmtd *)desc;

 	if (!lib->sysfs_supported)
-		return legacy_get_dev_info(node, mtd);
+	{
+		int ret = legacy_get_dev_info(node, mtd);
+		if (ret < 0)
+			return ret;
+		return mtd_get_oob_info(mtd);
+	}

 	if (dev_node2num(lib, node, &mtd_num))
 		return -1;

Re: mtd-utils: "flash_erase -j" failes with "unable to get NAND oobinfo" on large flash chips.

[Artem Bityutskiy]

On Thu, 2011-04-14 at 13:43 +0200, Daid wrote:
> On Thu, Apr 14, 2011 at 8:09 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Tue, 2011-04-12 at 11:58 +0200, Daid wrote:
> >> I've made a patch to fix flash_erase with large flash chips.
> >> flash_erase uses MEMGETOOBSEL which no longer works if the ECC area is
> >> larger then 32 bytes. ECCGETLAYOUT is the replacement ioctl.
> >>
> >> This patch is based on the work of Stanley Miao, he made a patch in
> >> June 2010 for flash_eraseall.
> >> http://lists.infradead.org/pipermail/linux-mtd/2010-June/031981.html
> >> I've implemented the backwards compatibility differently, checking
> >> kernel versions doesn't feel correct.
> >
> > Hi Daid, would you please use libmtd instead? If libmtd does not have
> > some functionality you need - just add it there.
> >
> > The thing is that we are trying to unify this stuff a little.
> >
> > --
> > Best Regards,
> > Artem Bityutskiy (Артём Битюцкий)
> >
> >
> 
> Hi Artem, libmtd didn't have the functionality I needed. I've made a new patch
> with the functionality build into libmtd.
> 
> I've added the extra information to struct mtd_dev_info, however, the
> info needed is
> only available with ioctls, so even with the sysfs interface it needs
> to open /dev/mtdXX.
> When you use mtd_get_dev_info, it reads all the information, which simple for
> the user of libmtd, but sort of mixes legacy and the sysfs interface.

Could you please split this on 2 patches:
1. Add new functionality to libmtd
2. Start using it in mkfs.jffs2.

This way it is easier to review, and if you screw up mkfs.jffs2 we can
always revert patch #2 without reverting libmtd.

> +/**
> + * mtd_get_oob_info - fill the mtd_dev_info structure with information
> + *  about the OOB data.
> + */
> +static int mtd_get_oob_info(struct mtd_dev_info *mtd)
> +{
> +	int i, fd;
> +	char node[sizeof(MTD_DEV_PATT) + 20];
> +
> +	sprintf(node, MTD_DEV_PATT, mtd->mtd_num);
> +	fd = open(node, O_RDWR);
> +	if (fd == -1)
> +		return sys_errmsg("cannot open \"%s\"", node);
> +		
> +	if (mtd->type == MTD_NANDFLASH) {
> +		/* get ECC/OOB information for NAND flash */
> +#if defined(ECCGETLAYOUT)

Where does this define come from?

> +		struct nand_ecclayout ecclayout;
> +		memset(&ecclayout, 0, sizeof(ecclayout));
> +		if (ioctl(fd, ECCGETLAYOUT, &ecclayout) == 0) {
> +			mtd->eccbytes = ecclayout.eccbytes;
> +			for(i=0; i<64; i++) {
> +				mtd->eccpos[i] = ecclayout.eccpos[i];
> +			}
> +			mtd->oob_available = ecclayout.oobavail;
> +			for (i=0; i<MTD_NAND_MAX_OOBFREE_ENTRIES; i++) {
> +				mtd->oob_free[i].offset = ecclayout.oobfree[i].offset;
> +				mtd->oob_free[i].length = ecclayout.oobfree[i].length;
> +			}
> +		} else {
> +#endif/*defined(ECCGETLAYOUT)*/


> +			/* new ECC interface failed or not available, fall back to old OOB
> interface, which does not support large flash */
> +			struct nand_oobinfo oobinfo;
> +			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) == 0)

The legacy piece of code should be handled in libmtd_legacy.c There are
already some things you can re-use.

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

Re: Fwd: mtd-utils: "flash_erase -j" failes with "unable to get NAND oobinfo" on large flash chips.

[Artem Bityutskiy]

Hi,

On Mon, 2011-04-18 at 11:42 +0200, Daid wrote:
> The define comes from mtd/mtd-abi.h, if you are building against older
> kernel versions then this define does not exists. If compatibility
> with older kernel versions is not an issue then this #ifdef could be
> removed.

mtd utils has its own copy of mtd/mtd-abi.h and it does not depend on
the kernel.

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

Re: Fwd: mtd-utils: "flash_erase -j" failes with "unable to get NAND oobinfo" on large flash chips.

[Artem Bityutskiy]

Hi,

your patch is line-wrapped and it is impossible to apply it,
unfortunately.

On Mon, 2011-04-18 at 11:42 +0200, Daid wrote:
> I've split up the patches, and moved the code the libmtd_legacy.c:

Could you pleas send separate patches as separate e-mails? Could you
please also add a nice git commit message and your Signed-off-by? Just
look how other people do this. To make a patches I usually first commit
my changes, then use git format-patch, and then use git send-email to
send them.

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