* Re: [PATCH]Erase block header(revision 1) [not found] <BAY17-F141EA92F45DB70328CE1C5E8960@phx.gbl> @ 2005-09-23 9:29 ` Ferenc Havasi 2005-09-23 9:33 ` Artem B. Bityutskiy 2005-09-23 10:24 ` zhao forrest 2005-09-25 11:04 ` Jörn Engel 1 sibling, 2 replies; 7+ messages in thread From: Ferenc Havasi @ 2005-09-23 9:29 UTC (permalink / raw) To: zhao forrest; +Cc: dedekind, linux-mtd Hi Zhao, First of all: your implementation of EBH summary support a good job, congratulation! I noticed only two things: + case JFFS2_NODETYPE_ERASEBLOCK_HEADER: + s->sum_size += JFFS2_SUMMARY_EBH_SIZE(je16_to_cpu(item->eh.dsize)*4); Here I think you should use sizeof(uint32_t) instead of simple 4, as you used it otherwhere. I's a very small thing, I know. The other is related to it: if I were you I would store eh.dsize in bytes, not in uint32. In every other places of JFFS2 the size is stored byte based (if I am right), and there is no need to always multible by sizeof(uint32_t) if the real size is needed. I have only one big problem with this patch. I'm strongly against to introduce INCOMPAT nodes JFFS2_NODETYPE_DIRENT_EBH and JFFS2_NODETYPE_INODE_EBH just to make unusable new JFFS2 images for old JFFS2 code on NAND devices, too. Do I understand well: is this the only reason? Ferenc ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH]Erase block header(revision 1) 2005-09-23 9:29 ` [PATCH]Erase block header(revision 1) Ferenc Havasi @ 2005-09-23 9:33 ` Artem B. Bityutskiy 2005-09-23 10:24 ` zhao forrest 1 sibling, 0 replies; 7+ messages in thread From: Artem B. Bityutskiy @ 2005-09-23 9:33 UTC (permalink / raw) To: Ferenc Havasi; +Cc: zhao forrest, linux-mtd Ferenc Havasi wrote: > I have only one big problem with this patch. I'm strongly against to > introduce INCOMPAT nodes JFFS2_NODETYPE_DIRENT_EBH and > JFFS2_NODETYPE_INODE_EBH just to make unusable new JFFS2 images for old > JFFS2 code on NAND devices, too. Do I understand well: is this the only > reason? Haven't we already noted/agreed that JFFS2_NODETYPE_DIRENT_EBH is ugly and should go away??? -- Best Regards, Artem B. Bityuckiy, St.-Petersburg, Russia. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH]Erase block header(revision 1) 2005-09-23 9:29 ` [PATCH]Erase block header(revision 1) Ferenc Havasi 2005-09-23 9:33 ` Artem B. Bityutskiy @ 2005-09-23 10:24 ` zhao forrest 1 sibling, 0 replies; 7+ messages in thread From: zhao forrest @ 2005-09-23 10:24 UTC (permalink / raw) To: havasi; +Cc: dedekind, linux-mtd > >I have only one big problem with this patch. I'm strongly against to >introduce INCOMPAT nodes JFFS2_NODETYPE_DIRENT_EBH and >JFFS2_NODETYPE_INODE_EBH just to make unusable new JFFS2 images for old >JFFS2 code on NAND devices, too. Do I understand well: is this the only >reason? Yes. We have decided to remove JFFS2_NODETYPE_DIRENT_EBH and JFFS2_NODETYPE_INODE_EBH. I'll send out a patch to fix all these problems next week. Have a happy weekend! Forrest ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH]Erase block header(revision 1) [not found] <BAY17-F141EA92F45DB70328CE1C5E8960@phx.gbl> 2005-09-23 9:29 ` [PATCH]Erase block header(revision 1) Ferenc Havasi @ 2005-09-25 11:04 ` Jörn Engel 2005-09-26 8:02 ` zhao forrest 1 sibling, 1 reply; 7+ messages in thread From: Jörn Engel @ 2005-09-25 11:04 UTC (permalink / raw) To: zhao forrest; +Cc: dedekind, linux-mtd On Fri, 23 September 2005 14:53:09 +0800, zhao forrest wrote: > > This is the revised version of eraseblock header patch. > Compared with previous version, the following changes are > made: > 1 eliminate the too long lines, and make them shorter > 2 change eraseblock_heaer_size to ebh_size; has_eraseblock > _header to has_ebh to avoid long variable name > 3 change jffs2_write_nand_cleanmarker() to jffs2_write_nand_ebh() > to avoid confusion > 4 change jffs2_check_nand_cleanmarker() to > jffs2_check_nand_cleanmarker_ebh() to avoid confusion I ignored all the code and took a look at the new node type only. > +struct jffs2_eraseblock_header > +{ > + jint16_t magic; > + jint16_t nodetype; /* == JFFS2_NODETYPE_ERASEBLOCK_HEADER */ > + jint32_t totlen; > + jint32_t hdr_crc; > + uint8_t fs_version; /* the version of this JFFS2 fs image */ Version is completely useless. > + uint8_t compat_fset; > + uint8_t incompat_fset; > + uint8_t rocompat_fset; > + jint32_t erase_count; /* the erase count of this erase block */ > + jint16_t dsize; /* the size of additional data behind node_crc */ Breaks alignment and pretty useless. Information can be deduced from totlen. Remove. > + jint32_t node_crc; > + jint32_t data_crc; Combine and move up, just below hdr_crc. > + jint32_t data[0]; > +} __attribute__((packed)); > + > struct jffs2_raw_dirent > { > jint16_t magic; Jörn -- Good warriors cause others to come to them and do not go to others. -- Sun Tzu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH]Erase block header(revision 1) 2005-09-25 11:04 ` Jörn Engel @ 2005-09-26 8:02 ` zhao forrest 2005-09-26 8:19 ` Jörn Engel 2005-09-26 9:08 ` Artem B. Bityutskiy 0 siblings, 2 replies; 7+ messages in thread From: zhao forrest @ 2005-09-26 8:02 UTC (permalink / raw) To: joern, dedekind; +Cc: linux-mtd > > > +struct jffs2_eraseblock_header > > +{ > > + jint16_t magic; > > + jint16_t nodetype; /* == JFFS2_NODETYPE_ERASEBLOCK_HEADER */ > > + jint32_t totlen; > > + jint32_t hdr_crc; > > + uint8_t fs_version; /* the version of this JFFS2 fs image */ > >Version is completely useless. I agree that version field is not used in my patch after compat_fset, incompat_fset and rocompat_fset are introduced in my patch. But I'm not sure if we should keep this field. Artem, What's your opinion about this? > > + uint8_t compat_fset; > > + uint8_t incompat_fset; > > + uint8_t rocompat_fset; > > + jint32_t erase_count; /* the erase count of this erase block */ > > + jint16_t dsize; /* the size of additional data behind node_crc */ > >Breaks alignment and pretty useless. Information can be deduced from >totlen. Remove. > Sometimes we can't deduce the exact additional data size from totlen. In particular this is true for NOR ECC. For example, sizeof(struct jffs2_eraseblock_header) is 34 bytes; totlen is 40 bytes, dsize is 0 byte. Thanks, Forrest ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH]Erase block header(revision 1) 2005-09-26 8:02 ` zhao forrest @ 2005-09-26 8:19 ` Jörn Engel 2005-09-26 9:08 ` Artem B. Bityutskiy 1 sibling, 0 replies; 7+ messages in thread From: Jörn Engel @ 2005-09-26 8:19 UTC (permalink / raw) To: zhao forrest; +Cc: dedekind, linux-mtd On Mon, 26 September 2005 16:02:50 +0800, zhao forrest wrote: > > >> +struct jffs2_eraseblock_header > >> +{ > >> + jint16_t magic; > >> + jint16_t nodetype; /* == JFFS2_NODETYPE_ERASEBLOCK_HEADER */ > >> + jint32_t totlen; > >> + jint32_t hdr_crc; > >> + uint8_t fs_version; /* the version of this JFFS2 fs image */ > > > >Version is completely useless. > > I agree that version field is not used in my patch after compat_fset, > incompat_fset and rocompat_fset are introduced in my patch. > But I'm not sure if we should keep this field. > Artem, > What's your opinion about this? Just give it a useful name like "reserved" or "unused". We still need the alignment. > >> + uint8_t compat_fset; > >> + uint8_t incompat_fset; > >> + uint8_t rocompat_fset; What does "fset" mean? Why not just "flags_compat" etc.? > >> + jint32_t erase_count; /* the erase count of this erase block */ > >> + jint16_t dsize; /* the size of additional data behind node_crc */ > > > >Breaks alignment and pretty useless. Information can be deduced from > >totlen. Remove. > > > Sometimes we can't deduce the exact additional data size from totlen. > In particular this is true for NOR ECC. For example, sizeof(struct > jffs2_eraseblock_header) is 34 bytes; totlen is 40 bytes, dsize is > 0 byte. Even then it doesn't matter. We just copy all fields and ignore the ones we don't know about. What we know about is defined in the three flags fields. Jörn -- Simplicity is prerequisite for reliability. -- Edsger W. Dijkstra ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH]Erase block header(revision 1) 2005-09-26 8:02 ` zhao forrest 2005-09-26 8:19 ` Jörn Engel @ 2005-09-26 9:08 ` Artem B. Bityutskiy 1 sibling, 0 replies; 7+ messages in thread From: Artem B. Bityutskiy @ 2005-09-26 9:08 UTC (permalink / raw) To: zhao forrest; +Cc: linux-mtd zhao forrest wrote: > I agree that version field is not used in my patch after compat_fset, > incompat_fset and rocompat_fset are introduced in my patch. > But I'm not sure if we should keep this field. Artem, > What's your opinion about this? Well, expect that EBH format may be changed in future, for example if some Mr.Smith will and one more per-eraseblock field. The same may in principle happen with any other node type. In this case, we may use the 'version' filed as the FS format identifier. So, IMO, the field is not useless. >> > + uint8_t compat_fset; >> > + uint8_t incompat_fset; >> > + uint8_t rocompat_fset; >> > + jint32_t erase_count; /* the erase count of this erase block */ >> > + jint16_t dsize; /* the size of additional data behind node_crc */ I still not really like the data[] field. At least the name... Is it just for future EBH extentions? -- Best Regards, Artem B. Bityuckiy, St.-Petersburg, Russia. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2005-09-26 9:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <BAY17-F141EA92F45DB70328CE1C5E8960@phx.gbl>
2005-09-23 9:29 ` [PATCH]Erase block header(revision 1) Ferenc Havasi
2005-09-23 9:33 ` Artem B. Bityutskiy
2005-09-23 10:24 ` zhao forrest
2005-09-25 11:04 ` Jörn Engel
2005-09-26 8:02 ` zhao forrest
2005-09-26 8:19 ` Jörn Engel
2005-09-26 9:08 ` Artem B. Bityutskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox