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

login
register
mail settings
Submitter Sebastian Siewior
Date March 23, 2009, 10:42 p.m.
Message ID <1237848138-18157-4-git-send-email-sebastian@breakpoint.cc>
Download mbox | patch
Permalink /patch/24938/
State New
Headers show

Comments

Sebastian Siewior - March 23, 2009, 10:42 p.m.
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(-)
Artem Bityutskiy - March 24, 2009, 6:05 a.m.
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?
Sebastian Siewior - March 24, 2009, 8:55 a.m.
* 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
Artem Bityutskiy - March 25, 2009, 6:33 a.m.
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...
Jamie Lokier - March 26, 2009, 1:44 p.m.
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
Sebastian Siewior - March 26, 2009, 3:41 p.m.
* 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
Ricard Wanderlof - March 26, 2009, 3:54 p.m.
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
Artem Bityutskiy - March 27, 2009, 5:55 a.m.
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 :-)
Sebastian Siewior - April 5, 2009, 7:43 p.m.
* 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
Sebastian Siewior - April 5, 2009, 8:07 p.m.
* 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
Ricard Wanderlof - April 6, 2009, 8:09 a.m.
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
Artem Bityutskiy - April 6, 2009, 9:29 a.m.
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.
Artem Bityutskiy - April 6, 2009, 9:32 a.m.
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 :-)
Artem Bityutskiy - April 10, 2009, 12:30 p.m.
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.
Sebastian Siewior - Aug. 6, 2009, 2:58 p.m.
* 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
Artem Bityutskiy - Aug. 9, 2009, 5:20 a.m.
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.

Patch

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: