| Message ID | BLU0-SMTP394611D4685CA5AE953006B8060@phx.gbl |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2011-09-16 at 16:46 -0400, Bill Pringlemeir wrote: > #ifdef CONFIG_ROMFS_ON_MTD > -#define ROMFS_MTD_READ(sb, ...) ((sb)->s_mtd->read((sb)->s_mtd, ##__VA_ARGS__)) > - > +#define ROMFS_MTD_READ(sb, ...) \ > + ({ int res; \ > + res = ((sb)->s_mtd->read((sb)->s_mtd, ##__VA_ARGS__)) == -EUCLEAN ? \ > + 0 : res; }) I do not think this is the most elegant way to handle this, but yes, EUCLEAN is used nowadays to report about bit-flips, which are actually not an error, more like an info that this eraseblock needs some attention. I am not sure MTD is the right subsystem for this patch, could you try to send it to fs-devel / Al Viro instead?
On 19 Sep 2011, dedekind1@gmail.com wrote: On Fri, 2011-09-16 at 16:46 -0400, Bill Pringlemeir wrote: >> #ifdef CONFIG_ROMFS_ON_MTD >> -#define ROMFS_MTD_READ(sb, ...) ((sb)->s_mtd->read((sb)->s_mtd, ##__VA_ARGS__)) >> - >> +#define ROMFS_MTD_READ(sb, ...) \ >> + ({ int res; \ >> + res = ((sb)->s_mtd->read((sb)->s_mtd, ##__VA_ARGS__)) == -EUCLEAN ? \ >> + 0 : res; }) > I do not think this is the most elegant way to handle this, but yes, > EUCLEAN is used nowadays to report about bit-flips, which are actually > not an error, more like an info that this eraseblock needs some > attention. Good. Neither do I. Sometimes code is better at describing a problem. > I am not sure MTD is the right subsystem for this patch, could you try > to send it to fs-devel / Al Viro instead? I kind of thought it was not a good patch. I will try to rework it. Now, I am sure of 'EUCLEAN's meaning. I also observe that my romfs works fine with a NAND flash if the BLOCK device is used. Currently the romfs code is making a decision based on... int romfs_dev_read(struct super_block *sb, unsigned long pos, ... if (sb->s_mtd) return romfs_mtd_read(sb, pos, buf, buflen); The NAND flash will not map directly to a CPU memory space like a NOR flash. I *think* the main benefit of this MTD is for the non-MMU in directly mapping files? What is the benefit of using the MTD versus the block device for NAND flash? [I think that is not a fs-devel question...]. Can we look at sb->s_mtd->type for 'MTD_NANDFLASH' (etc) and bind a vptr in romfs_get_sb(), so that 'romfs_dev_*()' would actually be function pointers. Then NAND and NOR mtd's could both have a RomFS on the same system. Err that only makes sense if I can get an answer to the last question. Also, it is not the place of RomFs to be writing flash. However, if there is an EUCLEAN return code, is it worth a printk? tia, Bill Pringlemeir.
On Mon, 2011-09-19 at 16:19 -0400, Bill Pringlemeir wrote: > On 19 Sep 2011, dedekind1@gmail.com wrote: > > On Fri, 2011-09-16 at 16:46 -0400, Bill Pringlemeir wrote: > >> #ifdef CONFIG_ROMFS_ON_MTD > >> -#define ROMFS_MTD_READ(sb, ...) ((sb)->s_mtd->read((sb)->s_mtd, ##__VA_ARGS__)) > >> - > >> +#define ROMFS_MTD_READ(sb, ...) \ > >> + ({ int res; \ > >> + res = ((sb)->s_mtd->read((sb)->s_mtd, ##__VA_ARGS__)) == -EUCLEAN ? \ > >> + 0 : res; }) > > > I do not think this is the most elegant way to handle this, but yes, > > EUCLEAN is used nowadays to report about bit-flips, which are actually > > not an error, more like an info that this eraseblock needs some > > attention. > > Good. Neither do I. Sometimes code is better at describing a problem. > > > I am not sure MTD is the right subsystem for this patch, could you try > > to send it to fs-devel / Al Viro instead? > > I kind of thought it was not a good patch. |I did not mean this patch is bad, on the opposite - it looks to be of "similar style" as ROMFS. I just meant that this patch is for a file-system, so most probably should go in via Al Viro. Feel free to put me to CC when re-sending. > I will try to rework it. > Now, I am sure of 'EUCLEAN's meaning. I also observe that my romfs > works fine with a NAND flash if the BLOCK device is used. > > Currently the romfs code is making a decision based on... > > int romfs_dev_read(struct super_block *sb, unsigned long pos, > ... > if (sb->s_mtd) > return romfs_mtd_read(sb, pos, buf, buflen); > > The NAND flash will not map directly to a CPU memory space like a NOR > flash. I *think* the main benefit of this MTD is for the non-MMU in > directly mapping files? What is the benefit of using the MTD versus the > block device for NAND flash? [I think that is not a fs-devel > question...]. For R/O FS there is probably not much benefits, except of less layers -> a bit faster. > Also, it is not the place of RomFs to be writing flash. However, if > there is an EUCLEAN return code, is it worth a printk? On the one hand, bit-flips happen very often in modern flashes, so if you print a message on every bit-flip, you may have a lot of messages. On the other hand, if you use RomFS on a modern flash, you are probably doing wrong thing because it is unable to handle bit-flips. You should rather use it on top of UBI. So printing messages is good, because it would make the user to start thinking about an issue. So I'd say, overall I think printing a warning is a good idea.
From 349cf90c20c7eabb6a9b215d68ea9c064e55460e Mon Sep 17 00:00:00 2001 From: Bill Pringlemeir <bpringle@sympatico.ca> Date: Fri, 16 Sep 2011 16:29:41 -0400 Subject: [PATCH] Fix romfs when using an NAND flash MTD. The code '-EUCLEAN' is returned when an NAND mtd has used ECC to correct data. This is fine for the reader of the RomFs and their data is correct. --- fs/romfs/storage.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/fs/romfs/storage.c b/fs/romfs/storage.c index 71e2b4d..4105d1c 100644 --- a/fs/romfs/storage.c +++ b/fs/romfs/storage.c @@ -19,8 +19,10 @@ #endif #ifdef CONFIG_ROMFS_ON_MTD -#define ROMFS_MTD_READ(sb, ...) ((sb)->s_mtd->read((sb)->s_mtd, ##__VA_ARGS__)) - +#define ROMFS_MTD_READ(sb, ...) \ + ({ int res; \ + res = ((sb)->s_mtd->read((sb)->s_mtd, ##__VA_ARGS__)) == -EUCLEAN ? \ + 0 : res; }) /* * read data from an romfs image on an MTD device */ -- 1.7.0.4
The macro ROMFS_MTD_READ seems to be passing on 'EUCLEAN' to the main RomFs code which doesn't deal with this error code. I think that the 'EUCLEAN' means that my 'mxc_nand' flash driver has performed error correction. I also see that most of the mtd tests change this error code to mean zero. So I think the RomFs MTD code needs something like attached. A large file allows md5sum, cp, etc with this patch. Without, strace shows 'cp' getting an 'EIO' error and giving up. Signed-off-by: Bill Pringlemeir <bpringle@sympatico.ca>