mtd-utils/flash_eraseall: tiny cleanup

mtd-utils/flash_eraseall: tiny cleanup

[Sebastian Andrzej Siewior]

tiny cleanup:
- #1 is almost a pure code move, except that now it is non fatal if a
  clean marker can't be prepared.
- #2 add support/ignore for flash types which don't support clean markers
- #3 is tricky. At this point we don't really which format is expected by
  the kernel. So we could either add another command line switch or get
  rid of this option :) Any comments on this?


Sebastian

[PATCH] flash_eraseall: create two functions from existing code

[Sebastian Andrzej Siewior]

create prepare_clean_marker() and write_clean_marker() from existing code.
The only difference to the earlier version is, that if the preparation of
a cleanmarker fails then the creation of clean markers is disabled instead
of an abort.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 flash_eraseall.c |  160 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 90 insertions(+), 70 deletions(-)

diff --git a/flash_eraseall.c b/flash_eraseall.c
index a22fc49..3da56ee 100644
--- a/flash_eraseall.c
+++ b/flash_eraseall.c
@@ -54,13 +54,97 @@ static void display_help (void);
 static void display_version (void);
 static struct jffs2_unknown_node cleanmarker;
 int target_endian = __BYTE_ORDER;
+static int fd;
+static int clmpos;
+static int clmlen = 8;
+
+static int prepare_clean_marker(mtd_info_t *meminfo)
+{
+	cleanmarker.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
+	cleanmarker.nodetype = cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER);
+	if (meminfo->type != MTD_NANDFLASH)
+		cleanmarker.totlen = cpu_to_je32(sizeof(struct jffs2_unknown_node));
+	else {
+		struct nand_oobinfo oobinfo;
+
+		if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0) {
+			fprintf(stderr, "%s: %s: unable to get NAND oobinfo\n",
+					exe_name, mtd_device);
+			return -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");
+				return -1;
+			}
+			clmpos = oobinfo.oobfree[0][0];
+			clmlen = oobinfo.oobfree[0][1];
+			if (clmlen > 8)
+				clmlen = 8;
+		} else {
+			/* Legacy mode */
+			switch (meminfo->oobsize) {
+			case 8:
+				clmpos = 6;
+				clmlen = 2;
+				break;
+			case 16:
+				clmpos = 8;
+				clmlen = 8;
+				break;
+			case 64:
+				clmpos = 16;
+				clmlen = 8;
+				break;
+			}
+		}
+		cleanmarker.totlen = cpu_to_je32(8);
+	}
+	cleanmarker.hdr_crc = cpu_to_je32(crc32(0, &cleanmarker,
+				sizeof(struct jffs2_unknown_node) - 4));
+	return 0;
+}
+
+static void write_clean_marker(int fd, int erase_start, int isNAND)
+{
+	struct mtd_oob_buf oob;
+
+	if (isNAND) {
+		oob.ptr = (unsigned char *) &cleanmarker;
+		oob.start = erase_start + clmpos;
+		oob.length = clmlen;
+		if (ioctl(fd, MEMWRITEOOB, &oob) != 0) {
+			fprintf(stderr, "\n%s: %s: MTD writeoob failure: %s\n",
+					exe_name, mtd_device, strerror(errno));
+			return;
+		}
+	} else {
+		if (lseek(fd, erase_start, SEEK_SET) < 0) {
+			fprintf(stderr, "\n%s: %s: MTD lseek failure: %s\n",
+					exe_name, mtd_device, strerror(errno));
+			return;
+		}
+		if (write(fd, &cleanmarker, sizeof(cleanmarker)) !=
+				sizeof(cleanmarker)) {
+			fprintf(stderr, "\n%s: %s: MTD write failure: %s\n",
+					exe_name, mtd_device, strerror(errno));
+			return;
+		}
+	}
+	if (!quiet)
+		printf(" Cleanmarker written at %x.", erase_start);
+}
 
 int main (int argc, char *argv[])
 {
 	mtd_info_t meminfo;
-	int fd, clmpos = 0, clmlen = 8;
 	erase_info_t erase;
 	int isNAND, bbtest = 1;
+	int ret;
 
 	process_options(argc, argv);
 
@@ -79,49 +163,9 @@ int main (int argc, char *argv[])
 	isNAND = meminfo.type == MTD_NANDFLASH ? 1 : 0;
 
 	if (jffs2) {
-		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;
-
-			if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0) {
-				fprintf(stderr, "%s: %s: unable to get NAND oobinfo\n", exe_name, mtd_device);
-				return 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");
-					return 1;
-				}
-				clmpos = oobinfo.oobfree[0][0];
-				clmlen = oobinfo.oobfree[0][1];
-				if (clmlen > 8)
-					clmlen = 8;
-			} else {
-				/* Legacy mode */
-				switch (meminfo.oobsize) {
-					case 8:
-						clmpos = 6;
-						clmlen = 2;
-						break;
-					case 16:
-						clmpos = 8;
-						clmlen = 8;
-						break;
-					case 64:
-						clmpos = 16;
-						clmlen = 8;
-						break;
-				}
-			}
-			cleanmarker.totlen = cpu_to_je32(8);
-		}
-		cleanmarker.hdr_crc =  cpu_to_je32 (crc32 (0, &cleanmarker,  sizeof (struct jffs2_unknown_node) - 4));
+		ret = prepare_clean_marker(&meminfo);
+		if (ret < 0)
+			jffs2 = 0;
 	}
 
 	for (erase.start = 0; erase.start < meminfo.size; erase.start += meminfo.erasesize) {
@@ -155,31 +199,8 @@ int main (int argc, char *argv[])
 		}
 
 		/* format for JFFS2 ? */
-		if (!jffs2)
-			continue;
-
-		/* write cleanmarker */
-		if (isNAND) {
-			struct mtd_oob_buf oob;
-			oob.ptr = (unsigned char *) &cleanmarker;
-			oob.start = erase.start + clmpos;
-			oob.length = clmlen;
-			if (ioctl (fd, MEMWRITEOOB, &oob) != 0) {
-				fprintf(stderr, "\n%s: %s: MTD writeoob failure: %s\n", exe_name, mtd_device, strerror(errno));
-				continue;
-			}
-		} else {
-			if (lseek (fd, erase.start, SEEK_SET) < 0) {
-				fprintf(stderr, "\n%s: %s: MTD lseek failure: %s\n", exe_name, mtd_device, strerror(errno));
-				continue;
-			}
-			if (write (fd , &cleanmarker, sizeof (cleanmarker)) != sizeof (cleanmarker)) {
-				fprintf(stderr, "\n%s: %s: MTD write failure: %s\n", exe_name, mtd_device, strerror(errno));
-				continue;
-			}
-		}
-		if (!quiet)
-			printf (" Cleanmarker written at %x.", erase.start);
+		if (jffs2)
+			write_clean_marker(fd, erase.start, isNAND);
 	}
 	if (!quiet) {
 		show_progress(&meminfo, &erase);
@@ -189,7 +210,6 @@ int main (int argc, char *argv[])
 	return 0;
 }
 
-
 void process_options (int argc, char *argv[])
 {
 	int error = 0;
-- 
1.6.0.6

[PATCH] flash_eraseall: only add cleanmarkers on known flashtypes

[Sebastian Andrzej Siewior]

- data flash and an UBI flash do not use clean markers. Do not create
  them, even if the users says so.
- don't generate clean markers on unknown media/new media. Theoreticly
  nobody should use jffs2 on new media anyway :)

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 flash_eraseall.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/flash_eraseall.c b/flash_eraseall.c
index 3da56ee..0a4010e 100644
--- a/flash_eraseall.c
+++ b/flash_eraseall.c
@@ -60,12 +60,25 @@ static int clmlen = 8;
 
 static int prepare_clean_marker(mtd_info_t *meminfo)
 {
+	struct nand_oobinfo oobinfo;
+
 	cleanmarker.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
 	cleanmarker.nodetype = cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER);
-	if (meminfo->type != MTD_NANDFLASH)
+
+	switch (meminfo->type) {
+	case MTD_ROM:
+	case MTD_RAM:
+	case MTD_NORFLASH:
 		cleanmarker.totlen = cpu_to_je32(sizeof(struct jffs2_unknown_node));
-	else {
-		struct nand_oobinfo oobinfo;
+		break;
+
+	case MTD_DATAFLASH:
+	case MTD_UBIVOLUME:
+		fprintf(stderr, "JFFS2 layout not supported on this flash.\n");
+		return -1;
+		break;
+
+	case MTD_NANDFLASH:
 
 		if (ioctl(fd, MEMGETOOBSEL, &oobinfo) != 0) {
 			fprintf(stderr, "%s: %s: unable to get NAND oobinfo\n",
@@ -103,6 +116,12 @@ static int prepare_clean_marker(mtd_info_t *meminfo)
 			}
 		}
 		cleanmarker.totlen = cpu_to_je32(8);
+		break;
+	default:
+		fprintf(stderr, "Unknown flash type, just erasing\n");
+		return -1;
+		break;
+
 	}
 	cleanmarker.hdr_crc = cpu_to_je32(crc32(0, &cleanmarker,
 				sizeof(struct jffs2_unknown_node) - 4));
-- 
1.6.0.6

[RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Sebastian Andrzej Siewior]

this is difficult because it depends on CONFIG_JFFS2_FS_WRITEBUFFER

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 flash_eraseall.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/flash_eraseall.c b/flash_eraseall.c
index 0a4010e..3591db5 100644
--- a/flash_eraseall.c
+++ b/flash_eraseall.c
@@ -64,12 +64,24 @@ static int prepare_clean_marker(mtd_info_t *meminfo)
 
 	cleanmarker.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
 	cleanmarker.nodetype = cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER);
+	cleanmarker.totlen = cpu_to_je32(sizeof(struct jffs2_unknown_node));
 
 	switch (meminfo->type) {
 	case MTD_ROM:
 	case MTD_RAM:
+		break;
+
 	case MTD_NORFLASH:
-		cleanmarker.totlen = cpu_to_je32(sizeof(struct jffs2_unknown_node));
+		/* only if the kernel is compiled with
+		 * CONFIG_JFFS2_FS_WRITEBUFFER
+		 * Now, how to check that?
+		 */
+		if (!(meminfo->flags & MTD_BIT_WRITEABLE)) {
+			int l;
+
+			l = meminfo->writesize > 16 ? meminfo->writesize : 16;
+			cleanmarker.totlen = cpu_to_je32(l);
+		}
 		break;
 
 	case MTD_DATAFLASH:
-- 
1.6.0.6

Re: [PATCH] flash_eraseall: only add cleanmarkers on known flashtypes

[Artem Bityutskiy]

On Mon, 2009-03-23 at 23:42 +0100, Sebastian Andrzej Siewior wrote:
> +       switch (meminfo->type) {
> +       case MTD_ROM:
> +       case MTD_RAM:
> +       case MTD_NORFLASH:
>                 cleanmarker.totlen = cpu_to_je32(sizeof(struct
> jffs2_unknown_node));
> -       else {
> -               struct nand_oobinfo oobinfo;
> +               break;
> +
> +       case MTD_DATAFLASH:
> +       case MTD_UBIVOLUME:
> +               fprintf(stderr, "JFFS2 layout not supported on this
> flash.\n");
> +               return -1;
> +               break;

This should also include MLC NAND flash. To make you nice patches
even better you should call MEMGETOOBSEL ioctl in case of NAND and
check if there is really enough bytes for clean-marker.

But I guess you do not have to do this.

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

Re: [PATCH] flash_eraseall: only add cleanmarkers on known flashtypes

[Artem Bityutskiy]

On Tue, 2009-03-24 at 08:02 +0200, Artem Bityutskiy wrote:
> On Mon, 2009-03-23 at 23:42 +0100, Sebastian Andrzej Siewior wrote:
> > +       switch (meminfo->type) {
> > +       case MTD_ROM:
> > +       case MTD_RAM:
> > +       case MTD_NORFLASH:
> >                 cleanmarker.totlen = cpu_to_je32(sizeof(struct
> > jffs2_unknown_node));
> > -       else {
> > -               struct nand_oobinfo oobinfo;
> > +               break;
> > +
> > +       case MTD_DATAFLASH:
> > +       case MTD_UBIVOLUME:
> > +               fprintf(stderr, "JFFS2 layout not supported on this
> > flash.\n");
> > +               return -1;
> > +               break;
> 
> This should also include MLC NAND flash. To make you nice patches
> even better you should call MEMGETOOBSEL ioctl in case of NAND and
> check if there is really enough bytes for clean-marker.
> 
> But I guess you do not have to do this.

Err, sorry, this seems to be already there :-) Need some coffee.

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

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Artem Bityutskiy]

On Mon, 2009-03-23 at 23:42 +0100, Sebastian Andrzej Siewior wrote:
> this is difficult because it depends on CONFIG_JFFS2_FS_WRITEBUFFER
> 
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> ---
>  flash_eraseall.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/flash_eraseall.c b/flash_eraseall.c
> index 0a4010e..3591db5 100644
> --- a/flash_eraseall.c
> +++ b/flash_eraseall.c
> @@ -64,12 +64,24 @@ static int prepare_clean_marker(mtd_info_t *meminfo)
>  
>  	cleanmarker.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
>  	cleanmarker.nodetype = cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER);
> +	cleanmarker.totlen = cpu_to_je32(sizeof(struct jffs2_unknown_node));
>  
>  	switch (meminfo->type) {
>  	case MTD_ROM:
>  	case MTD_RAM:
> +		break;
> +
>  	case MTD_NORFLASH:
> -		cleanmarker.totlen = cpu_to_je32(sizeof(struct jffs2_unknown_node));
> +		/* only if the kernel is compiled with
> +		 * CONFIG_JFFS2_FS_WRITEBUFFER
> +		 * Now, how to check that?
> +		 */
> +		if (!(meminfo->flags & MTD_BIT_WRITEABLE)) {
> +			int l;
> +
> +			l = meminfo->writesize > 16 ? meminfo->writesize : 16;
> +			cleanmarker.totlen = cpu_to_je32(l);
> +		}

Sorry, not sure which problem this patch is trying to solve?

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

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Sebastian Andrzej Siewior]

* Artem Bityutskiy | 2009-03-24 08:05:27 [+0200]:

>On Mon, 2009-03-23 at 23:42 +0100, Sebastian Andrzej Siewior wrote:
>> this is difficult because it depends on CONFIG_JFFS2_FS_WRITEBUFFER
>> 
>> Signed-off-by: Sebastian Andrzej Siewior <sebastian at breakpoint.cc>
>> ---
>>  flash_eraseall.c |   14 +++++++++++++-
>>  1 files changed, 13 insertions(+), 1 deletions(-)
>> 
>> diff --git a/flash_eraseall.c b/flash_eraseall.c
>> index 0a4010e..3591db5 100644
>> --- a/flash_eraseall.c
>> +++ b/flash_eraseall.c
>> @@ -64,12 +64,24 @@ static int prepare_clean_marker(mtd_info_t *meminfo)
>>  
>>  	cleanmarker.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
>>  	cleanmarker.nodetype = cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER);
>> +	cleanmarker.totlen = cpu_to_je32(sizeof(struct jffs2_unknown_node));
>>  
>>  	switch (meminfo->type) {
>>  	case MTD_ROM:
>>  	case MTD_RAM:
>> +		break;
>> +
>>  	case MTD_NORFLASH:
>> -		cleanmarker.totlen = cpu_to_je32(sizeof(struct jffs2_unknown_node));
>> +		/* only if the kernel is compiled with
>> +		 * CONFIG_JFFS2_FS_WRITEBUFFER
>> +		 * Now, how to check that?
>> +		 */
>> +		if (!(meminfo->flags & MTD_BIT_WRITEABLE)) {
>> +			int l;
>> +
>> +			l = meminfo->writesize > 16 ? meminfo->writesize : 16;
>> +			cleanmarker.totlen = cpu_to_je32(l);
>> +		}
>
>Sorry, not sure which problem this patch is trying to solve?
CONFIG_JFFS2_FS_WRITEBUFFER enabled and MTD_BIT_WRITEABLE missing in
flags.
This is from jffs2_nor_wbuf_flash_setup() which is only executed if
jffs2_nor_wbuf_flash() evaluates 1 which is only the case if
CONFIG_JFFS2_FS_WRITEBUFFER is enabled and MTD_BIT_WRITEABLE is not
available in flags.
So to this write, we have to evaluate CONFIG_JFFS2_FS_WRITEBUFFER, don't
we?

>Best regards,
>Artem Bityutskiy (???????? ?????)

Sebastian

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Artem Bityutskiy]

On Tue, 2009-03-24 at 09:55 +0100, Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2009-03-24 08:05:27 [+0200]:
> 
> >On Mon, 2009-03-23 at 23:42 +0100, Sebastian Andrzej Siewior wrote:
> >> this is difficult because it depends on CONFIG_JFFS2_FS_WRITEBUFFER
> >> 
> >> Signed-off-by: Sebastian Andrzej Siewior <sebastian at breakpoint.cc>

It sounds wrong to me to make flash_eraseall depend on how JFFS2 is
compiled...

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

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Jamie Lokier]

Artem Bityutskiy wrote:
> It sounds wrong to me to make flash_eraseall depend on how JFFS2 is
> compiled...

It sounds wrong to me as well.

What if you're running flash_eraseall on one kernel - it might not
even have JFFS2 - and then use the flash later with a different kernel
with a different setting.

-- Jamie

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Sebastian Andrzej Siewior]

* Jamie Lokier | 2009-03-26 13:44:18 [+0000]:

>Artem Bityutskiy wrote:
>> It sounds wrong to me to make flash_eraseall depend on how JFFS2 is
>> compiled...
>
>It sounds wrong to me as well.
>
>What if you're running flash_eraseall on one kernel - it might not
>even have JFFS2 - and then use the flash later with a different kernel
>with a different setting.

I'm sorry. That is actually my point. The layout of the cleanmarker
(which are used only by JFFS2) depends on a specific kernel switch in
case of NOR flash which is not BIT_WRITEABLE and userland can't know
that.
I've sent a patch to remove the -j option because the generated
cleanmaker *may* be wrong. Artem replied that he would like to see this
fixed.
So here is an attempt to fix this: black listed ubi & data flash but I
dunno how fix the NOR case where does not have BIT_WRITEABLE bit. A
commandline switch for those who know what they do?
Why don't rip out -j and let the kernel create clean marker if it
needs it?

>-- Jamie

Sebastian

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Ricard Wanderlof]

On Thu, 26 Mar 2009, Sebastian Andrzej Siewior wrote:

> I'm sorry. That is actually my point. The layout of the cleanmarker 
> (which are used only by JFFS2) depends on a specific kernel switch in 
> case of NOR flash which is not BIT_WRITEABLE and userland can't know 
> that. I've sent a patch to remove the -j option because the generated 
> cleanmaker *may* be wrong. Artem replied that he would like to see this 
> fixed. So here is an attempt to fix this: black listed ubi & data flash 
> but I dunno how fix the NOR case where does not have BIT_WRITEABLE bit. 
> A commandline switch for those who know what they do? Why don't rip out 
> -j and let the kernel create clean marker if it needs it?

There can be a performance benefit to having the image contain 
cleanmarkers rather than the kernel having to do it. Specifically, if a 
cleanmarker is missing, the kernel doesn't know if it's because it's a 
virgin image or because a previous erase failed (typically because of a 
power fail during erase). In the case of a failed erase, the sector might 
not be properly erased (i.e. even though the bytes might read 0xFF they 
might not be properly erase, resulting in spurious bitflips later). So, 
the kernel must erase the sector (which for a NOR flash is a slow 
operation) before writing the cleanmarker.

For a given application it may be possible to skip the 
erase-before-writing-cleanmarker, but in the general case it is necessary, 
which slows system startup (the flash cannot be used while it is being 
erased).

/Ricard
--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Artem Bityutskiy]

On Thu, 2009-03-26 at 16:41 +0100, Sebastian Andrzej Siewior wrote:
> * Jamie Lokier | 2009-03-26 13:44:18 [+0000]:
> 
> >Artem Bityutskiy wrote:
> >> It sounds wrong to me to make flash_eraseall depend on how JFFS2 is
> >> compiled...
> >
> >It sounds wrong to me as well.
> >
> >What if you're running flash_eraseall on one kernel - it might not
> >even have JFFS2 - and then use the flash later with a different kernel
> >with a different setting.
> 
> I'm sorry. That is actually my point. The layout of the cleanmarker
> (which are used only by JFFS2) depends on a specific kernel switch in
> case of NOR flash which is not BIT_WRITEABLE and userland can't know
> that.
> I've sent a patch to remove the -j option because the generated
> cleanmaker *may* be wrong. Artem replied that he would like to see this
> fixed.
> So here is an attempt to fix this: black listed ubi & data flash but I
> dunno how fix the NOR case where does not have BIT_WRITEABLE bit. A
> commandline switch for those who know what they do?
> Why don't rip out -j and let the kernel create clean marker if it
> needs it?

I looked into your patch closer, and there is no such dependency
actually. Now it looks fine for me.

I can apply them if there are not complaints. But how you have
tested them? I would not want to break 'flash_eraseall' once again :-)

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

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Sebastian Andrzej Siewior]

* Ricard Wanderlof | 2009-03-26 16:54:21 [+0100]:

> On Thu, 26 Mar 2009, Sebastian Andrzej Siewior wrote:
>
>> I'm sorry. That is actually my point. The layout of the cleanmarker (which 
>> are used only by JFFS2) depends on a specific kernel switch in case of NOR 
>> flash which is not BIT_WRITEABLE and userland can't know that. I've sent a 
>> patch to remove the -j option because the generated cleanmaker *may* be 
>> wrong. Artem replied that he would like to see this fixed. So here is an 
>> attempt to fix this: black listed ubi & data flash but I dunno how fix the 
>> NOR case where does not have BIT_WRITEABLE bit. A commandline switch for 
>> those who know what they do? Why don't rip out -j and let the kernel 
>> create clean marker if it needs it?
>
> There can be a performance benefit to having the image contain cleanmarkers 
> rather than the kernel having to do it. Specifically, if a cleanmarker is 
> missing, the kernel doesn't know if it's because it's a virgin image or 
> because a previous erase failed (typically because of a power fail during 

okay. So providing clean-markers is here a one-time optimization.


> erase). In the case of a failed erase, the sector might not be properly 
> erased (i.e. even though the bytes might read 0xFF they might not be 
> properly erase, resulting in spurious bitflips later). So, the kernel must 
> erase the sector (which for a NOR flash is a slow operation) before writing 
> the cleanmarker.

and this can happen anyway so your clean-marker support in
flash_eraseall does not help here.

>
> For a given application it may be possible to skip the 
> erase-before-writing-cleanmarker, but in the general case it is necessary, 
> which slows system startup (the flash cannot be used while it is being 
> erased).

Please don't get me wrong. I don't want to rip clean-markers out from
jffs2. I just thought, that it is enough to have it in kernel since it is
the only place where the final layout is known. 


> /Ricard
Sebastian

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Sebastian Andrzej Siewior]

* Artem Bityutskiy | 2009-03-27 07:55:32 [+0200]:

>> I'm sorry. That is actually my point. The layout of the cleanmarker
>> (which are used only by JFFS2) depends on a specific kernel switch in
>> case of NOR flash which is not BIT_WRITEABLE and userland can't know
>> that.
>> I've sent a patch to remove the -j option because the generated
>> cleanmaker *may* be wrong. Artem replied that he would like to see this
>> fixed.
>> So here is an attempt to fix this: black listed ubi & data flash but I
>> dunno how fix the NOR case where does not have BIT_WRITEABLE bit. A
>> commandline switch for those who know what they do?
>> Why don't rip out -j and let the kernel create clean marker if it
>> needs it?
>
>I looked into your patch closer, and there is no such dependency
>actually. Now it looks fine for me.

okay. what about this:
in jffs2_do_fill_super() we have:


| c->cleanmarker_size = sizeof(struct jffs2_unknown_node);
12 bytes.

| 
| /* NAND (or other bizarre) flash... do setup accordingly */
| ret = jffs2_flash_setup(c);
| if (ret)
|         return ret;

and jfffs_flash_setup() is:
|static int jffs2_flash_setup(struct jffs2_sb_info *c) {
|        int ret = 0;
|        
|        if (jffs2_cleanmarker_oob(c)) {
|                /* NAND flash... do setup accordingly */
|                ret = jffs2_nand_flash_setup(c);
|                if (ret)
|                        return ret;
|        }       
|
|        /* and Dataflash */
|        if (jffs2_dataflash(c)) {
|                ret = jffs2_dataflash_setup(c);
|                if (ret)
|                        return ret;
|        }
|
|        /* and Intel "Sibley" flash */
|        if (jffs2_nor_wbuf_flash(c)) {
and jffs2_nor_wbuf_flash() depends on CONFIG_JFFS2_FS_WRITEBUFFER.
Without it is 0 and with it is defined to
(c->mtd->type == MTD_NORFLASH && ! (c->mtd->flags & MTD_BIT_WRITEABLE))

|                ret = jffs2_nor_wbuf_flash_setup(c);
and jffs2_nor_wbuf_flash_setup() changes the cleanmarker size via

         c->cleanmarker_size = max(16u, c->mtd->writesize);

and this is != 12 in every case.

|                if (ret)
|                        return ret;
|        }
|        
|        /* and an UBI volume */
|        if (jffs2_ubivol(c)) {
|                ret = jffs2_ubivol_setup(c);
|                if (ret)
|                        return ret;
|        }
|   
|        return ret;
|}

Do I overlook something or why is there no dependency on
CONFIG_JFFS2_FS_WRITEBUFFER on NOR flash which does not have the
MTD_BIT_WRITEABLE flag?

>
>I can apply them if there are not complaints. But how you have
>tested them? I would not want to break 'flash_eraseall' once again :-)
understand. Nope, I do not have such flash. I got a bug report against
clean marker on a data flash and that is why I started looking into
this.

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

Sebastian

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Ricard Wanderlof]

On Sun, 5 Apr 2009, Sebastian Andrzej Siewior wrote:

>> erase). In the case of a failed erase, the sector might not be properly
>> erased (i.e. even though the bytes might read 0xFF they might not be
>> properly erase, resulting in spurious bitflips later). So, the kernel must
>> erase the sector (which for a NOR flash is a slow operation) before writing
>> the cleanmarker.
>
> and this can happen anyway so your clean-marker support in
> flash_eraseall does not help here.

Yes, it can happen anyway, but if there are no cleanmarkers in the image, 
you get a performance penalty on every boot from a virgin image. While 
not often in the course of a product's lifetime, imagine someone 
upgrading a bunch of identical products, the rewriting of cleanmarkers can 
make the boot after each upgrade much slower, thus significantly 
increasing the upgrade time. Similarly in production, where time is a 
premium, any lengthening of the product boot time (e.g. before final 
testing) is bad.

Of course, it must still be present in the kernel to handle the case of 
failed erase, but that doesn't it stop it from being very handy in 
flash_eraseall as well.

One thing that would be handy would be if one could give a 
"jffs2-cleanmarker" flag to the erase ioctl which would automatically 
write a cleanmarker after erasing the block, in accordance with the custom 
of the day for the particular kernel version in question. However, apart 
from the fact that it may be difficult to extend the ioctl functionality, 
mixing driver levels (i.e. mtd and jffs2) in this way is probably not 
considered too clean.

> Please don't get me wrong. I don't want to rip clean-markers out from
> jffs2. I just thought, that it is enough to have it in kernel since it is
> the only place where the final layout is known.

I didn't misunderstand you. I just want to argue that putting down 
cleanmarkers when the flash is erased rather than later is an optimization 
that can be useful in practice.

/Ricard
--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Artem Bityutskiy]

On Sun, 2009-04-05 at 22:07 +0200, Sebastian Andrzej Siewior wrote:
> >I can apply them if there are not complaints. But how you have
> >tested them? I would not want to break 'flash_eraseall' once again :-)
> understand. Nope, I do not have such flash. I got a bug report against
> clean marker on a data flash and that is why I started looking into
> this.

Well, I meant I did not find dependencies in flash_eraseall.

I think JFFS2 should be just fixed and should always use
12 bytes.

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

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Artem Bityutskiy]

On Mon, 2009-04-06 at 12:29 +0300, Artem Bityutskiy wrote:
> On Sun, 2009-04-05 at 22:07 +0200, Sebastian Andrzej Siewior wrote:
> > >I can apply them if there are not complaints. But how you have
> > >tested them? I would not want to break 'flash_eraseall' once again :-)
> > understand. Nope, I do not have such flash. I got a bug report against
> > clean marker on a data flash and that is why I started looking into
> > this.
> 
> Well, I meant I did not find dependencies in flash_eraseall.
> 
> I think JFFS2 should be just fixed and should always use
> 12 bytes.

But well, I do not use JFFS2 and do not have time to delve into
that :-(

The only thing I can help you with is to try to review your userspace
changes and push them if they are fine :-)

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

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Artem Bityutskiy]

On Mon, 2009-03-23 at 23:42 +0100, Sebastian Andrzej Siewior wrote:
> this is difficult because it depends on CONFIG_JFFS2_FS_WRITEBUFFER
> 

So do you still support your patches? I think that if your patches

a. do not break anything
b. have at least some improvements, even if they do not work for all
cases

then we may just take them.

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

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Sebastian Andrzej Siewior]

* Artem Bityutskiy | 2009-04-10 15:30:55 [+0300]:

Sorry for late reply. You killed from To/Cc in the thread and I received
this email just via the mailing list which I read irregulary.

>On Mon, 2009-03-23 at 23:42 +0100, Sebastian Andrzej Siewior wrote:
>> this is difficult because it depends on CONFIG_JFFS2_FS_WRITEBUFFER
>> 
>
>So do you still support your patches?
Well, at that time I received a bug report where someone was using
dataflash with jffs2 so I tried to clean it up.

>I think that if your patches
>
>a. do not break anything
The breakage was that one person showed up and comlained about longer
first time mount time which was critical for him because it was
production/test or something like that and the timeframe was fixed,
short or something like that.

>b. have at least some improvements, even if they do not work for all
>cases
The improvement was that flash_eraseall did not attempt to create erase
markers on data flash. And something dependent on a kernel option which
userland could not know. Therefore the clean thing was to remove the
clean marker all together but this breaks your point a
So adding an option which sets the sate of CONFIG_JFFS2_FS_WRITEBUFFER
in kernel might be a solution.

>then we may just take them.
I don't use JFFS2 anymore so I would leave as it. If someone needs/wants
to fix it, he can take the patches probably as it since the jffs2
doesn't change much these days. Unless you really want me get it fixed
some way but I doubt it :)

>Artem Bityutskiy (???????????????? ??????????)

Sebastian

Re: [RFC/PATCH] flash_eraseall: extra care if NOR flash is not BIT_WRITEABLE

[Artem Bityutskiy]

On Thu, 2009-08-06 at 16:58 +0200, Sebastian Andrzej Siewior wrote:
> >then we may just take them.
> I don't use JFFS2 anymore so I would leave as it. If someone needs/wants
> to fix it, he can take the patches probably as it since the jffs2
> doesn't change much these days. Unless you really want me get it fixed
> some way but I doubt it :)

Right, let's not touch this then.

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