RomFS MTD and NAND Flash with ECC (EUCLEAN).

RomFS MTD and NAND Flash with ECC (EUCLEAN).

[Bill Pringlemeir]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]


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>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: romfs/storage.c patch --]
[-- Type: text/x-diff, Size: 1107 bytes --]

>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

Re: RomFS MTD and NAND Flash with ECC (EUCLEAN).

[Artem Bityutskiy]

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?

-- 
Best Regards,
Artem Bityutskiy

Re: RomFS MTD and NAND Flash with ECC (EUCLEAN).

[Bill Pringlemeir]

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.

-- 
It is reasoning and faith that bind truth .  - Rod Ryker...

-- 
A mind  that is stretched  to a new  idea never returns to  its original
dimension - Anonymous

Re: RomFS MTD and NAND Flash with ECC (EUCLEAN).

[Artem Bityutskiy]

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.


-- 
Best Regards,
Artem Bityutskiy