| Message ID | 1237848138-18157-4-git-send-email-sebastian@breakpoint.cc |
|---|---|
| State | RFC |
| Headers | show |
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?
* 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
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...
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
* 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
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
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 :-)
* 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
* 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
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
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.
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 :-)
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.
* 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
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.
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:
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(-)