JFFS2 ignore ECC bytes in cleanmarker

JFFS2 ignore ECC bytes in cleanmarker

[Todd Poynor]

Floating for discussion a patch that avoids inspecting the ECC (and bad
block indicator) bytes in the OOB area when checking an autoplaced
chip's block for a valid JFFS2 cleanmarker.  It sounds like cleanmarkers
are on their way out, so this specific code might have a pretty short
lifespan.  But in general it may be best for the flash filesystems to
restrict themselves to looking at the free OOB bytes and ignoring those
that the hardware claims for its own purposes, whatever the FS-specific
data structure.

In my case I'm dealing with a NAND flash hardware controller that
automatically plunks down generated ECC bytes into the OOB (and so far I
haven't found a good way to avoid behaviors that modify those bytes), so
once the cleanmarker is written the ECC bytes will not be 0xFF and the
existing check will fail.

This falls under the general topic of recent discussion re: JFFS2 and
YAFFS* use of OOB bytes, and redefining what read_oob() returns based on
the chip-specific layout may well be a better answer (I think Vitaly
Wool's recent patches address this).  All in all, I'm not suggesting
commit the patch below at this time, but if it's useful to somebody with
a NAND controller that behaves similarly or it sparks any discussion
then have at. -- Todd

Index: fs/jffs2/wbuf.c
===================================================================
RCS file: /home/cvs/mtd/fs/jffs2/wbuf.c,v
retrieving revision 1.100
diff -u -r1.100 wbuf.c
--- fs/jffs2/wbuf.c	30 Sep 2005 13:59:13 -0000	1.100
+++ fs/jffs2/wbuf.c	18 Oct 2005 20:47:29 -0000
@@ -959,17 +959,40 @@
 	}
 	
 	/* Special check for first page */
-	for(i = 0; i < oob_size ; i++) {
-		/* Yeah, we know about the cleanmarker. */
-		if (mode && i >= c->fsdata_pos && 
-		    i < c->fsdata_pos + c->fsdata_len)
-			continue;
-
-		if (buf[i] != 0xFF) {
-			D2(printk(KERN_DEBUG "Found %02x at %x in OOB for %08x\n",
-				  buf[i], i, jeb->offset));
-			ret = 1; 
-			goto out;
+	if (c->mtd->oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
+		/* Walk through the autoplace chunks */
+		for (i = 0; c->mtd->oobinfo.oobfree[i][1]; i++) {
+			int j;
+
+			for (j = 0; j < c->mtd->oobinfo.oobfree[i][1]; j++) {
+				int pos = c->mtd->oobinfo.oobfree[i][0] + j;
+
+				if (mode && pos >= c->fsdata_pos &&
+				    pos < c->fsdata_pos + c->fsdata_len)
+					continue;
+
+				if (buf[pos] != 0xFF) {
+					D2(printk(KERN_DEBUG "Found %02x at 0x%x in OOB for %08x\n",
+						  buf[pos], pos,
+						  jeb->offset));
+					ret = 1;
+					goto out;
+				}
+			}
+		}
+	} else {
+		for(i = 0; i < oob_size ; i++) {
+			/* Yeah, we know about the cleanmarker. */
+			if (mode && i >= c->fsdata_pos && 
+			    i < c->fsdata_pos + c->fsdata_len)
+				continue;
+			
+			if (buf[i] != 0xFF) {
+				D2(printk(KERN_DEBUG "Found %02x at %x in OOB for %08x\n",
+					  buf[i], i, jeb->offset));
+				ret = 1; 
+				goto out;
+			}
 		}
 	}
 

Re: JFFS2 ignore ECC bytes in cleanmarker

[Charles Manning]

On Wednesday 19 October 2005 10:13, Todd Poynor wrote:
> Floating for discussion a patch that avoids inspecting the ECC (and bad
> block indicator) bytes in the OOB area when checking an autoplaced
> chip's block for a valid JFFS2 cleanmarker.  It sounds like cleanmarkers
> are on their way out, so this specific code might have a pretty short
> lifespan.  But in general it may be best for the flash filesystems to
> restrict themselves to looking at the free OOB bytes and ignoring those
> that the hardware claims for its own purposes, whatever the FS-specific
> data structure.

As to scrapping clean markers: One benefit I see in clean markers is that it 
protects against power loss/reset halfway through an erase. Even though an 
erase only takes ~2ms, the chip won't complete the erase if the write protect 
kicks in. For this reason, YAFFS2 *might* start using cleanmarkers at some 
stage.

This is definitely the way to go for the oob data. This is how YAFFS2 handles 
things. [YAFFS1 is Smartmedia-format centric, so that is another matter].

As recently discussed on YAFFS list this is the fundamental reason for having 
AUTOPLACE. Basically oob bytes go in and oob bytes come out and actual 
physical positioning (and other trickery) should not need to be visible 
through the fs<-->mtd interface.

>
> In my case I'm dealing with a NAND flash hardware controller that
> automatically plunks down generated ECC bytes into the OOB (and so far I
> haven't found a good way to avoid behaviors that modify those bytes), so
> once the cleanmarker is written the ECC bytes will not be 0xFF and the
> existing check will fail.

Yes, an excellent example of why the fs does not want to know, though I have 
seen even uglier ones!

>
> This falls under the general topic of recent discussion re: JFFS2 and
> YAFFS* use of OOB bytes, and redefining what read_oob() returns based on
> the chip-specific layout may well be a better answer (I think Vitaly
> Wool's recent patches address this).  All in all, I'm not suggesting
> commit the patch below at this time, but if it's useful to somebody with
> a NAND controller that behaves similarly or it sparks any discussion
> then have at. -- Todd

The current semantics of read_oob give raw info, not AUTOPLACEd info.  Whether 
the semantics should be changed, I don't know. 

An alternative way to get at the autoplaced oob is to change nand_read_ecc to 
accept a NULL parameter for the data. ie:
 mtd->read_ecc(mtd, addr,
                         0,  /* n-data-bytes == 0 : don't read any data */
                          &dummy,
                          NULL, /* data-ptr == 0: don't read any data */
                          oob_buffer,
                          NULL); /* AUTOPLACE */

This would be more consistent since it preserved read_oob() as "read raw". 
Some people might baulk at the use of NULL to signify this meaning, but this 
function already uses oob_buffer == NULL to signify that oob should not be 
transfered and oobsel==NULL that AUTOPLACE should be used.

- Charles

Re: JFFS2 ignore ECC bytes in cleanmarker

[Vitaly Wool]

Hi,

I do agree that the OOB data returned should be in the same format as 
OOB data provided. The layout-based model for handling NAND reads/writes 
is capable of fixing that, however, I didn't implement that in my 
layout-based patch due to the fact that I wanted to make it compatible 
with the current drivers.

So, I suggest the following way to go:

1. After linux-mtd is merged into the kernel, commit layout-based patch 
and fix the problems found (if any :)).
2. Agree on the format of OOB data.
3. Implement what was agreed upon.
4. Get rid of redundant structures (I hope that redundant will become 
eccbytes and maybe even oobfree).
5. Update the MTD userland utilities accordingly.

Does that make sense?

Best regards,
   Vitaly

Charles Manning wrote:

>On Wednesday 19 October 2005 10:13, Todd Poynor wrote:
>  
>
>>Floating for discussion a patch that avoids inspecting the ECC (and bad
>>block indicator) bytes in the OOB area when checking an autoplaced
>>chip's block for a valid JFFS2 cleanmarker.  It sounds like cleanmarkers
>>are on their way out, so this specific code might have a pretty short
>>lifespan.  But in general it may be best for the flash filesystems to
>>restrict themselves to looking at the free OOB bytes and ignoring those
>>that the hardware claims for its own purposes, whatever the FS-specific
>>data structure.
>>    
>>
>
>As to scrapping clean markers: One benefit I see in clean markers is that it 
>protects against power loss/reset halfway through an erase. Even though an 
>erase only takes ~2ms, the chip won't complete the erase if the write protect 
>kicks in. For this reason, YAFFS2 *might* start using cleanmarkers at some 
>stage.
>
>This is definitely the way to go for the oob data. This is how YAFFS2 handles 
>things. [YAFFS1 is Smartmedia-format centric, so that is another matter].
>
>As recently discussed on YAFFS list this is the fundamental reason for having 
>AUTOPLACE. Basically oob bytes go in and oob bytes come out and actual 
>physical positioning (and other trickery) should not need to be visible 
>through the fs<-->mtd interface.
>
>  
>
>>In my case I'm dealing with a NAND flash hardware controller that
>>automatically plunks down generated ECC bytes into the OOB (and so far I
>>haven't found a good way to avoid behaviors that modify those bytes), so
>>once the cleanmarker is written the ECC bytes will not be 0xFF and the
>>existing check will fail.
>>    
>>
>
>Yes, an excellent example of why the fs does not want to know, though I have 
>seen even uglier ones!
>
>  
>
>>This falls under the general topic of recent discussion re: JFFS2 and
>>YAFFS* use of OOB bytes, and redefining what read_oob() returns based on
>>the chip-specific layout may well be a better answer (I think Vitaly
>>Wool's recent patches address this).  All in all, I'm not suggesting
>>commit the patch below at this time, but if it's useful to somebody with
>>a NAND controller that behaves similarly or it sparks any discussion
>>then have at. -- Todd
>>    
>>
>
>The current semantics of read_oob give raw info, not AUTOPLACEd info.  Whether 
>the semantics should be changed, I don't know. 
>
>An alternative way to get at the autoplaced oob is to change nand_read_ecc to 
>accept a NULL parameter for the data. ie:
> mtd->read_ecc(mtd, addr,
>                         0,  /* n-data-bytes == 0 : don't read any data */
>                          &dummy,
>                          NULL, /* data-ptr == 0: don't read any data */
>                          oob_buffer,
>                          NULL); /* AUTOPLACE */
>
>This would be more consistent since it preserved read_oob() as "read raw". 
>Some people might baulk at the use of NULL to signify this meaning, but this 
>function already uses oob_buffer == NULL to signify that oob should not be 
>transfered and oobsel==NULL that AUTOPLACE should be used.
>
>- Charles
>
>
>______________________________________________________
>Linux MTD discussion mailing list
>http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
>
>  
>

Re: JFFS2 ignore ECC bytes in cleanmarker

[Charles Manning]

On Thursday 20 October 2005 01:00, Vitaly Wool wrote:
> Hi,
>
> I do agree that the OOB data returned should be in the same format as
> OOB data provided. The layout-based model for handling NAND reads/writes
> is capable of fixing that, however, I didn't implement that in my
> layout-based patch due to the fact that I wanted to make it compatible
> with the current drivers.

Sounds good. Fix one thing at a time.

In the mean time I (or maybe someone else) will see if that proposal patch can 
be made to work properly. If so, I think we can get useful semantics pretty 
quickly too (without impacting on your work).

I wonder whether fixing things the other way around makes your life easier for 
what you're doing now (ie. if you are not boxed in by the current oobsel 
stuff)?

>
> So, I suggest the following way to go:
>
> 1. After linux-mtd is merged into the kernel, commit layout-based patch
> and fix the problems found (if any :)).
> 2. Agree on the format of OOB data.
> 3. Implement what was agreed upon.
> 4. Get rid of redundant structures (I hope that redundant will become
> eccbytes and maybe even oobfree).
Most definitely progress. We've had about four or five iterations of these and 
keeping everyone on the same page all the time is tricky.
> 5. Update the MTD userland utilities accordingly.
>
> Does that make sense?

Makes good sense to me.

-- Charles