diff mbox

mtd-utils: fix corrupt cleanmarker with flash_erase -j command

Message ID 1312350638-25566-1-git-send-email-b35362@freescale.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

b35362@freescale.com Aug. 3, 2011, 5:50 a.m. UTC 
From: Liu Shuo <b35362@freescale.com>

Flash_erase -j should fill discrete freeoob areas with required bytes
of JFFS2 cleanmarker in jffs2_check_nand_cleanmarker(). Not just fill
the first freeoob area.

Signed-off-by: Liu Shuo <b35362@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
 flash_erase.c |   41 +++++++++++++++++++++++++++++++++++------
 1 files changed, 35 insertions(+), 6 deletions(-)

Comments

Artem Bityutskiy Aug. 16, 2011, 3:06 p.m. UTC | #1 
On Wed, 2011-08-03 at 13:50 +0800, b35362@freescale.com wrote:
> From: Liu Shuo <b35362@freescale.com>
> 
> Flash_erase -j should fill discrete freeoob areas with required bytes
> of JFFS2 cleanmarker in jffs2_check_nand_cleanmarker(). Not just fill
> the first freeoob area.
> 
> Signed-off-by: Liu Shuo <b35362@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>

...

>  	/*
>  	 * Process user arguments
> @@ -197,15 +198,40 @@ int main(int argc, char *argv[])
>  			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0)
>  				return sys_errmsg("%s: unable to get NAND oobinfo", mtd_device);
>  
> +			cleanmarker.totlen = cpu_to_je32(8);
>  			/* Check for autoplacement */
>  			if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
> +				struct nand_ecclayout_user ecclayout;
>  				/* Get the position of the free bytes */
> -				if (!oobinfo.oobfree[0][1])
> +				if (ioctl(fd, ECCGETLAYOUT, &ecclayout) != 0)
> +					return sys_errmsg("%s: unable to get NAND ecclayout", mtd_device);
> +

Hmm, shouldn't we instead make MTD_OOB_AUTO be available for userspace
via an ioctl instead and make flash_eraseall use it instead?
Brian Norris Aug. 16, 2011, 7:42 p.m. UTC | #2 
On Tue, Aug 16, 2011 at 8:06 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-08-03 at 13:50 +0800, b35362@freescale.com wrote:
>> From: Liu Shuo <b35362@freescale.com>
>>
>> Flash_erase -j should fill discrete freeoob areas with required bytes
>> of JFFS2 cleanmarker in jffs2_check_nand_cleanmarker(). Not just fill
>> the first freeoob area.
>
> Hmm, shouldn't we instead make MTD_OOB_AUTO be available for userspace
> via an ioctl instead and make flash_eraseall use it instead?

`nandwrite -o' does a similar thing, where it uses MEMGETOOBSEL to
find open spaces in OOB. Both MEMGETOOBSEL and ECCGETLAYOUT have been
declared obsolete. Plus, the code that uses any of these is somewhat
complicated and duplicated, so I agree that new code probably should
use some form of the internal MTD_OOB_AUTO.

Perhaps this can just be integrated into the new ioctl I'm writing as
a "mode" choice? See the thread:
http://lists.infradead.org/pipermail/linux-mtd/2011-August/037316.html

Brian
b35362@freescale.com Aug. 17, 2011, 7:35 a.m. UTC | #3 
于 2011年08月16日 23:06, Artem Bityutskiy 写道:
> On Wed, 2011-08-03 at 13:50 +0800, b35362@freescale.com wrote:
>> From: Liu Shuo<b35362@freescale.com>
>>
>> Flash_erase -j should fill discrete freeoob areas with required bytes
>> of JFFS2 cleanmarker in jffs2_check_nand_cleanmarker(). Not just fill
>> the first freeoob area.
>>
>> Signed-off-by: Liu Shuo<b35362@freescale.com>
>> Signed-off-by: Li Yang<leoli@freescale.com>
> ...
>
>>   	/*
>>   	 * Process user arguments
>> @@ -197,15 +198,40 @@ int main(int argc, char *argv[])
>>   			if (ioctl(fd, MEMGETOOBSEL,&oobinfo) != 0)
>>   				return sys_errmsg("%s: unable to get NAND oobinfo", mtd_device);
>>
>> +			cleanmarker.totlen = cpu_to_je32(8);
>>   			/* Check for autoplacement */
>>   			if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
>> +				struct nand_ecclayout_user ecclayout;
>>   				/* Get the position of the free bytes */
>> -				if (!oobinfo.oobfree[0][1])
>> +				if (ioctl(fd, ECCGETLAYOUT,&ecclayout) != 0)
>> +					return sys_errmsg("%s: unable to get NAND ecclayout", mtd_device);
>> +
> Hmm, shouldn't we instead make MTD_OOB_AUTO be available for userspace
> via an ioctl instead and make flash_eraseall use it instead?
>


I think We can add a new ioctl MEMSETOOBMODE for selecting a mode to 
access the OOB area.

Add new member into struct mtd_info:

struct mtd_info {
     ......

     enum {
          MTD_OOB_PLACE,
          MTD_OOB_AUTO,
          MTD_OOB_RAW,
     } oob_mode;
}

In function mtd_do_writeoob() (in drivers/mtd/mtdchar.c) :
     -  ops.mode = MTD_OOB_PLACE;
     + ops.mode = mtd->oob_mode;



Could we do it like this ?

Thanks
-LiuShuo
Artem Bityutskiy Aug. 17, 2011, 7:39 a.m. UTC | #4 
On Wed, 2011-08-17 at 15:35 +0800, LiuShuo wrote:
> I think We can add a new ioctl MEMSETOOBMODE for selecting a mode to 
> access the OOB area.
> 
> Add new member into struct mtd_info:
> 
> struct mtd_info {
>      ......
> 
>      enum {
>           MTD_OOB_PLACE,
>           MTD_OOB_AUTO,
>           MTD_OOB_RAW,
>      } oob_mode;
> }
> 
> In function mtd_do_writeoob() (in drivers/mtd/mtdchar.c) :
>      -  ops.mode = MTD_OOB_PLACE;
>      + ops.mode = mtd->oob_mode;
> 
> 
> 
> Could we do it like this ?

I think the OOB mode should not be a global MTD device state. Each
ioctl invocation should just specify the mode.

Brian's idea of having a completely new ioctl or a set of new ioctls for
OOB which would copletely deprecate the old ones is a good idea. Let's
wait for his patch.

May be we should even start physically removing depricated ioctls by
first adding a printk with a warning and then removing completely. But
first mtdutils should be updated to not use them.
Brian Norris Aug. 18, 2011, 1:20 a.m. UTC | #5 
On Wed, Aug 17, 2011 at 12:39 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> I think the OOB mode should not be a global MTD device state. Each
> ioctl invocation should just specify the mode.

I agree. This brings up some questions, though. See below.

> Brian's idea of having a completely new ioctl or a set of new ioctls for
> OOB which would copletely deprecate the old ones is a good idea. Let's
> wait for his patch.

I sent a patch series (RFC) to the MTD mailing list. You can see the
cover description, which describes my proposed changes and asks some
questions, at the following link, but for some reason the patches are
being held up by the mailing list software - for now, you'll only
receive them if you were CC'd directly:
http://lists.infradead.org/pipermail/linux-mtd/2011-August/037522.html
Follow that thread for more information.

> May be we should even start physically removing depricated ioctls by
> first adding a printk with a warning and then removing completely. But
> first mtdutils should be updated to not use them.

Yes, this sounds fine, although I'm not too familiar with the
MTD_OOB_AUTO stuff, so I'm not sure how well it will replace all the
uses of MEMGETOOBSEL. I don't actually see any users of ECCGETLAYOUT;
I think I've asked about this ioctl before, but I can't seem to find
any response...perhaps it can be killed with no work?

Brian
diff mbox

Patch

diff --git a/flash_erase.c b/flash_erase.c
index fe2eaca..e6747fc 100644
--- a/flash_erase.c
+++ b/flash_erase.c
@@ -98,6 +98,7 @@  int main(int argc, char *argv[])
 	int isNAND;
 	int error = 0;
 	uint64_t offset = 0;
+	void *oob_data = NULL;
 
 	/*
 	 * Process user arguments
@@ -197,15 +198,40 @@  int main(int argc, char *argv[])
 			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0)
 				return sys_errmsg("%s: unable to get NAND oobinfo", mtd_device);
 
+			cleanmarker.totlen = cpu_to_je32(8);
 			/* Check for autoplacement */
 			if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
+				struct nand_ecclayout_user ecclayout;
 				/* Get the position of the free bytes */
-				if (!oobinfo.oobfree[0][1])
+				if (ioctl(fd, ECCGETLAYOUT, &ecclayout) != 0)
+					return sys_errmsg("%s: unable to get NAND ecclayout", mtd_device);
+
+				if (!ecclayout.oobavail)
 					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;
+				clmlen = MIN(ecclayout.oobavail, 8);
+
+				if (oobinfo.oobfree[0][1] < 8 && ecclayout.oobavail >= 8) {
+					int i, left, n, last = 0;
+					void *cm;
+
+					oob_data = malloc(mtd.oob_size);
+					if (!oob_data)
+						return -ENOMEM;
+
+					memset(oob_data, 0xff, mtd.oob_size);
+					cm = &cleanmarker;
+					for (i = 0, left = clmlen; left ; i++) {
+						n = MIN(left, oobinfo.oobfree[i][1]);
+						memcpy(oob_data + oobinfo.oobfree[i][0],
+								cm, n);
+						left -= n;
+						cm   += n;
+						last = oobinfo.oobfree[i][0] + n;
+					}
+
+					clmlen = last - clmpos;
+				}
 			} else {
 				/* Legacy mode */
 				switch (mtd.oob_size) {
@@ -223,7 +249,6 @@  int main(int argc, char *argv[])
 						break;
 				}
 			}
-			cleanmarker.totlen = cpu_to_je32(8);
 		}
 		cleanmarker.hdr_crc = cpu_to_je32(mtd_crc32(0, &cleanmarker, sizeof(cleanmarker) - 4));
 	}
@@ -272,7 +297,8 @@  int main(int argc, char *argv[])
 
 		/* write cleanmarker */
 		if (isNAND) {
-			if (mtd_write_oob(mtd_desc, &mtd, fd, offset + clmpos, clmlen, &cleanmarker) != 0) {
+			void *data = oob_data ? oob_data + clmpos : &cleanmarker;
+			if (mtd_write_oob(mtd_desc, &mtd, fd, offset + clmpos, clmlen, data) != 0) {
 				sys_errmsg("%s: MTD writeoob failure", mtd_device);
 				continue;
 			}
@@ -291,5 +317,8 @@  int main(int argc, char *argv[])
 	show_progress(&mtd, offset, eb, eb_start, eb_cnt);
 	bareverbose(!quiet, "\n");
 
+	if (oob_data)
+		free(oob_data);
+
 	return 0;
 }