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

login
register
mail settings
Submitter Daid
Date April 14, 2011, 11:43 a.m.
Message ID <BANLkTikgzbGGNg7AXYt89MvM4whvKxXHiQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/91222/
State New
Headers show

Comments

Daid - April 14, 2011, 11:43 a.m.
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

 	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;
Artem Bityutskiy - April 14, 2011, 2:12 p.m.
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.
Artem Bityutskiy - April 21, 2011, 12:55 p.m.
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.
Artem Bityutskiy - April 21, 2011, 12:58 p.m.
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.

Patch

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