All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
@ 2011-08-03  5:50 ` b35362
  0 siblings, 0 replies; 13+ messages in thread
From: b35362 @ 2011-08-03  5:50 UTC (permalink / raw)
  To: dwmw2, linuxppc-dev; +Cc: Liu Shuo, linuxppc-dev, linux-mtd

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

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;
 }
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
@ 2011-08-03  5:50 ` b35362
  0 siblings, 0 replies; 13+ messages in thread
From: b35362 @ 2011-08-03  5:50 UTC (permalink / raw)
  To: dwmw2, linuxppc-dev; +Cc: Liu Shuo, linuxppc-dev, Li Yang, linux-mtd

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

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;
 }
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
  2011-08-03  5:50 ` b35362
@ 2011-08-16 15:06   ` Artem Bityutskiy
  -1 siblings, 0 replies; 13+ messages in thread
From: Artem Bityutskiy @ 2011-08-16 15:06 UTC (permalink / raw)
  To: b35362; +Cc: linuxppc-dev, dwmw2, linux-mtd, linuxppc-dev

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?

-- 
Best Regards,
Artem Bityutskiy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
@ 2011-08-16 15:06   ` Artem Bityutskiy
  0 siblings, 0 replies; 13+ messages in thread
From: Artem Bityutskiy @ 2011-08-16 15:06 UTC (permalink / raw)
  To: b35362; +Cc: linuxppc-dev, Li Yang, dwmw2, linux-mtd, linuxppc-dev

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?

-- 
Best Regards,
Artem Bityutskiy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
  2011-08-16 15:06   ` Artem Bityutskiy
@ 2011-08-16 19:42     ` Brian Norris
  -1 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2011-08-16 19:42 UTC (permalink / raw)
  To: dedekind1; +Cc: b35362, linuxppc-dev, linuxppc-dev, linux-mtd, dwmw2

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
@ 2011-08-16 19:42     ` Brian Norris
  0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2011-08-16 19:42 UTC (permalink / raw)
  To: dedekind1; +Cc: b35362, linuxppc-dev, linuxppc-dev, linux-mtd, Li Yang, dwmw2

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
  2011-08-16 15:06   ` Artem Bityutskiy
@ 2011-08-17  7:35     ` LiuShuo
  -1 siblings, 0 replies; 13+ messages in thread
From: LiuShuo @ 2011-08-17  7:35 UTC (permalink / raw)
  To: dedekind1; +Cc: linuxppc-dev, dwmw2, linux-mtd, linuxppc-dev

=E4=BA=8E 2011=E5=B9=B408=E6=9C=8816=E6=97=A5 23:06, Artem Bityutskiy =E5=
=86=99=E9=81=93:
> 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) !=3D 0)
>>   				return sys_errmsg("%s: unable to get NAND oobinfo", mtd_device);
>>
>> +			cleanmarker.totlen =3D cpu_to_je32(8);
>>   			/* Check for autoplacement */
>>   			if (oobinfo.useecc =3D=3D 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) !=3D 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=20
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 =3D MTD_OOB_PLACE;
     + ops.mode =3D mtd->oob_mode;



Could we do it like this ?

Thanks
-LiuShuo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
@ 2011-08-17  7:35     ` LiuShuo
  0 siblings, 0 replies; 13+ messages in thread
From: LiuShuo @ 2011-08-17  7:35 UTC (permalink / raw)
  To: dedekind1; +Cc: linuxppc-dev, Li Yang, dwmw2, linux-mtd, linuxppc-dev

于 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
  2011-08-17  7:35     ` LiuShuo
@ 2011-08-17  7:39       ` Artem Bityutskiy
  -1 siblings, 0 replies; 13+ messages in thread
From: Artem Bityutskiy @ 2011-08-17  7:39 UTC (permalink / raw)
  To: LiuShuo; +Cc: linuxppc-dev, dwmw2, linux-mtd, linuxppc-dev

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.

-- 
Best Regards,
Artem Bityutskiy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
@ 2011-08-17  7:39       ` Artem Bityutskiy
  0 siblings, 0 replies; 13+ messages in thread
From: Artem Bityutskiy @ 2011-08-17  7:39 UTC (permalink / raw)
  To: LiuShuo; +Cc: linuxppc-dev, Li Yang, dwmw2, linux-mtd, linuxppc-dev

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.

-- 
Best Regards,
Artem Bityutskiy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
  2011-08-17  7:39       ` Artem Bityutskiy
@ 2011-08-18  1:20         ` Brian Norris
  -1 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2011-08-18  1:20 UTC (permalink / raw)
  To: dedekind1; +Cc: LiuShuo, linuxppc-dev, linuxppc-dev, linux-mtd, dwmw2

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
@ 2011-08-18  1:20         ` Brian Norris
  0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2011-08-18  1:20 UTC (permalink / raw)
  To: dedekind1; +Cc: LiuShuo, linuxppc-dev, linuxppc-dev, linux-mtd, Li Yang, dwmw2

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command
@ 2011-08-08  8:16 b35362
  0 siblings, 0 replies; 13+ messages in thread
From: b35362 @ 2011-08-08  8:16 UTC (permalink / raw)
  To: dwmw2; +Cc: Liu Shuo, Artem.Bityutskiy, Li Yang, linux-mtd

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.

The below is the result without this workaround:

JFFS2: Erase block at 0x00000000 is not formatted. It will be erased
JFFS2: Erase block at 0x00004000 is not formatted. It will be erased
JFFS2: Erase block at 0x00008000 is not formatted. It will be erased
JFFS2: Erase block at 0x0000c000 is not formatted. It will be erased
JFFS2: Erase block at 0x00010000 is not formatted. It will be erased
JFFS2: Erase block at 0x00014000 is not formatted. It will be erased
JFFS2: Erase block at 0x00018000 is not formatted. It will be erased
JFFS2: Erase block at 0x0001c000 is not formatted. It will be erased
JFFS2: Erase block at 0x00020000 is not formatted. It will be erased
JFFS2: Erase block at 0x00024000 is not formatted. It will be erased
JFFS2: Erase block at 0x00028000 is not formatted. It will be erased
...

Signed-off-by: Liu Shuo <b35362@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
 v2 : get length of availble freeoob bytes from oobinfo information,
      not use the ioctl ECCGETLAYOUT which is being deprecated.

 flash_erase.c |   46 +++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/flash_erase.c b/flash_erase.c
index fe2eaca..3e94495 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,43 @@ 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) {
-				/* Get the position of the free bytes */
-				if (!oobinfo.oobfree[0][1])
+				struct nand_ecclayout_user ecclayout;
+				int i, oobavail;
+
+				for (i = 0, oobavail = 0; oobinfo.oobfree[i][1]; i++)
+					oobavail += oobinfo.oobfree[i][1];
+
+				if (!oobavail)
 					return errmsg(" Eeep. Autoplacement selected and no empty space in oob");
+
+				/* Get the position of the free bytes */
 				clmpos = oobinfo.oobfree[0][0];
-				clmlen = oobinfo.oobfree[0][1];
-				if (clmlen > 8)
-					clmlen = 8;
+				clmlen = MIN(oobavail, 8);
+
+				if (oobinfo.oobfree[0][1] < 8 && oobavail >= 8) {
+					int 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 +252,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 +300,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 +320,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;
 }
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2011-08-18  1:21 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-03  5:50 [PATCH] mtd-utils: fix corrupt cleanmarker with flash_erase -j command b35362
2011-08-03  5:50 ` b35362
2011-08-16 15:06 ` Artem Bityutskiy
2011-08-16 15:06   ` Artem Bityutskiy
2011-08-16 19:42   ` Brian Norris
2011-08-16 19:42     ` Brian Norris
2011-08-17  7:35   ` LiuShuo
2011-08-17  7:35     ` LiuShuo
2011-08-17  7:39     ` Artem Bityutskiy
2011-08-17  7:39       ` Artem Bityutskiy
2011-08-18  1:20       ` Brian Norris
2011-08-18  1:20         ` Brian Norris
2011-08-08  8:16 b35362

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.