Cache mappings and invalidate

Cache mappings and invalidate

[Joakim Tjernlund]

Hi all

I am trying to make copy_from use cahced memory by assignig 
map_priv_2 = (unsigned long)__ioremap(flash_addr, flash_size, 0);
and then change copy_from routine to:
+#ifndef NO_CACHE
+       memcpy_fromio(to, (void *)(map->map_priv_2 + from), len);
+#else
        memcpy_fromio(to, (void *)(map->map_priv_1 + from), len);
+#endif

There are no cache invalidations in my map file, instead I have 
added invalidate_dcache_range() calls to drivers/mtd/chips/cfi_cmdset_0001.c(see below)
in do_write_oneword(), do_write_buffer() and do_erase_oneblock(). Note that
this is just a quick hack to try out my theory. Does this look sane or should I use
another invalidate_dcache_range() and/or place the invalidate calls somewhere else?

I am not using burst reads yet, that will come later once i have gotten the cached mapping
to work.
 
       Joakim



--- drivers/mtd/chips/cfi_cmdset_0001.c 2001/10/25 12:11:10     1.3
+++ drivers/mtd/chips/cfi_cmdset_0001.c 2001/11/12 12:02:33
@@ -502,6 +502,10 @@
        cfi_udelay(chip->word_write_time);
        spin_lock_bh(chip->mutex);

+#ifndef NO_CACHE
+       invalidate_dcache_range(map->map_priv_2 + adr, map->map_priv_2 + adr + 4); /* on a 32 bit bus */
+#endif
+
        timeo = jiffies + (HZ/2);
        z = 0;
        for (;;) {
@@ -691,7 +695,7 @@
        wbufsize = CFIDEV_INTERLEAVE << cfi->cfiq->MaxBufWriteSize;
        adr += chip->start;
        cmd_adr = adr & ~(wbufsize-1);
-
+
        /* Let's determine this according to the interleave only once */
        status_OK = CMD(0x80);
 
@@ -790,6 +794,10 @@
        cfi_udelay(chip->buffer_write_time);
        spin_lock_bh(chip->mutex);
 
+#ifndef NO_CACHE
+       invalidate_dcache_range(map->map_priv_2 + adr, map->map_priv_2 + adr + len);
+#endif
+
        timeo = jiffies + (HZ/2);
        z = 0;
        for (;;) {
@@ -994,6 +1002,10 @@
        spin_unlock_bh(chip->mutex);
        schedule_timeout(HZ);
        spin_lock_bh(chip->mutex);
+
+#ifndef NO_CACHE
+       invalidate_dcache_range(map->map_priv_2 + adr, map->map_priv_2 + adr + 0x40000);
/* 0x40000 is my erase size */
+#endif
 
        /* FIXME. Use a timer to check this, and return immediately. */
        /* Once the state machine's known to be working I'll do that */

RE: Cache mappings and invalidate

[Joakim Tjernlund]

Hi again

There is a bug somewhere in this, I get CRC error when I copy a file 
a few times, then rm all copies of the file and reboot. When the board
comes up again I see these CRC errors. Any ideas?
  
          Jocke
> 
> Hi all
> 
> I am trying to make copy_from use cached memory by assigning 
> map_priv_2 = (unsigned long)__ioremap(flash_addr, flash_size, 0);
> and then change copy_from routine to:
> +#ifndef NO_CACHE
> +       memcpy_fromio(to, (void *)(map->map_priv_2 + from), len);
> +#else
>         memcpy_fromio(to, (void *)(map->map_priv_1 + from), len);
> +#endif
> 
> There are no cache invalidations in my map file, instead I have 
> added invalidate_dcache_range() calls to drivers/mtd/chips/cfi_cmdset_0001.c(see below)
> in do_write_oneword(), do_write_buffer() and do_erase_oneblock(). Note that
> this is just a quick hack to try out my theory. Does this look sane or should I use
> another invalidate_dcache_range() and/or place the invalidate calls somewhere else?
> 
> I am not using burst reads yet, that will come later once i have gotten the cached mapping
> to work.
>  
>        Joakim
> 
> 
> 
> --- drivers/mtd/chips/cfi_cmdset_0001.c 2001/10/25 12:11:10     1.3
> +++ drivers/mtd/chips/cfi_cmdset_0001.c 2001/11/12 12:02:33
> @@ -502,6 +502,10 @@
>         cfi_udelay(chip->word_write_time);
>         spin_lock_bh(chip->mutex);
> 
> +#ifndef NO_CACHE
> +       invalidate_dcache_range(map->map_priv_2 + adr, map->map_priv_2 + adr + 4); /* on a 32 bit bus */
> +#endif
> +
>         timeo = jiffies + (HZ/2);
>         z = 0;
>         for (;;) {
> @@ -691,7 +695,7 @@
>         wbufsize = CFIDEV_INTERLEAVE << cfi->cfiq->MaxBufWriteSize;
>         adr += chip->start;
>         cmd_adr = adr & ~(wbufsize-1);
> -
> +
>         /* Let's determine this according to the interleave only once */
>         status_OK = CMD(0x80);
>  
> @@ -790,6 +794,10 @@
>         cfi_udelay(chip->buffer_write_time);
>         spin_lock_bh(chip->mutex);
>  
> +#ifndef NO_CACHE
> +       invalidate_dcache_range(map->map_priv_2 + adr, map->map_priv_2 + adr + len);
> +#endif
> +
>         timeo = jiffies + (HZ/2);
>         z = 0;
>         for (;;) {
> @@ -994,6 +1002,10 @@
>         spin_unlock_bh(chip->mutex);
>         schedule_timeout(HZ);
>         spin_lock_bh(chip->mutex);
> +
> +#ifndef NO_CACHE
> +       invalidate_dcache_range(map->map_priv_2 + adr, map->map_priv_2 + adr + 0x40000);
> /* 0x40000 is my erase size */
> +#endif
>  
>         /* FIXME. Use a timer to check this, and return immediately. */
>         /* Once the state machine's known to be working I'll do that */
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Burst read and other improvements

[Joakim Tjernlund]

Hi all

Previously I reported that enabling burst read on Flash did not improve 
performance. That's not true :-(

I have measured the mount time with  this command:
time mount /dev/mtdblock4 /jffs2/
(Currently I am mostly intressted in lowering my mount times)

Here is what I get:
No optimization:  10.51 s.
with burst read:  9.12 s.
as above + patch to scan.c: 8.29 s
as above + #define  ACCT_PARANOIA_CHECK(jeb)  to nothing: 7.95 s.

This is on a MPC860 cpu at 80MHz and 40 MHz bus speed. 
Questions:
   Is the ACCT_PARANOIA_CHECK still needed?

   Does anybody else get better mount times by patching scan.c?
   Would be nice if this patch could make it into CVS ...

     Jocke

patch to scan.c:
--- scan.c      2001/10/25 12:11:11     1.4
+++ scan.c      2001/12/17 12:43:36
@@ -334,7 +334,7 @@
        __u32 scanlen = (jeb->offset + c->sector_size) - *startofs;
        __u32 curofs = *startofs;
 
-       buf = kmalloc(min((__u32)PAGE_SIZE, scanlen), GFP_KERNEL);
+       buf = kmalloc(min((__u32)PAGE_SIZE/4, scanlen), GFP_KERNEL);
        if (!buf) {
                printk(KERN_WARNING "Scan buffer allocation failed\n");
                return -ENOMEM;
@@ -343,7 +343,7 @@
                ssize_t retlen;
                int ret, i;
 
-               ret = c->mtd->read(c->mtd, curofs, min((__u32)PAGE_SIZE, 
scanlen), &retlen, (char *)buf);
+               ret = c->mtd->read(c->mtd, curofs, min((__u32)PAGE_SIZE/4, 
scanlen), &retlen, (char *)buf);
                if(ret) {
                        D1(printk(KERN_WARNING "jffs2_scan_empty(): Read 0x%x 
bytes at 0x%08x returned %d\n", min((__u32)PAGE_SIZE, scanlen), curofs, ret));
                        kfree(buf);

    

CLEANMARKER question

[Joakim Tjernlund]

Hi all

I am trying to understand the scan procedure in JFFS2 since I want to improve 
the speed in mounting a JFFS2 FS.

I have problems to understand the CLEANMARKER concept. Can a flash erase 
sector which has a CLEANMARKER in the beginning contain any data or can one 
assume that the rest of the erase sector is erased?

  Jocke

Re: CLEANMARKER question

[David Woodhouse]

Joakim.Tjernlund@lumentis.se said:
> I have problems to understand the CLEANMARKER concept. Can a flash
> erase  sector which has a CLEANMARKER in the beginning contain any
> data or can one  assume that the rest of the erase sector is erased?

Yes, it can contain data. The CLEANMARKER is there just to show that the 
erase completed successfully. It's not erased before the block is actually 
filled with data.

What are your plans for improving the mount speed?

--
dwmw2

RE: CLEANMARKER question

[Joakim Tjernlund]

> From: David Woodhouse [mailto:dwmw2@redhat.com]On Behalf Of David
> Woodhouse
> Joakim.Tjernlund@lumentis.se said:
> > I have problems to understand the CLEANMARKER concept. Can a flash
> > erase  sector which has a CLEANMARKER in the beginning contain any
> > data or can one  assume that the rest of the erase sector is erased?
> 
> Yes, it can contain data. The CLEANMARKER is there just to show that the 
> erase completed successfully. It's not erased before the block is actually 
> filled with data.
OK, I suspected that much. So after a CLEANMARKER there can be a NODETYPE_INODE or
a NODETYPE_DIRENT?

Why do you do 2 scan_empty() calls in scan_eraseblock() ?

> 
> What are your plans for improving the mount speed?
Well, sofar I have only identified one improvement. One could add an isempty() function
in the mtd layer. That would improve scaning for empy flash so that you dont
have to mtd->read() into a buffer and then check the buffer for 0xffffffff. How
does that sound?

Re: CLEANMARKER question

[David Woodhouse]

joakim.tjernlund@lumentis.se said:
> OK, I suspected that much. So after a CLEANMARKER there can be a
> NODETYPE_INODE or a NODETYPE_DIRENT?

Or indeed any other type of node, when new ones get invented - yes.

> Why do you do 2 scan_empty() calls in scan_eraseblock() ?

Consider it loop unrolling. 

> Well, sofar I have only identified one improvement. One could add an 
> isempty() function in the mtd layer. That would improve scaning for 
> empy flash so that you dont have to mtd->read() into a buffer and then 
> check the buffer for 0xffffffff. How does that sound? 

Sounds ugly, but could be effective. Want to benchmark it to see if it's 
really worth it?

If there's a way to safely avoid having to check all the node CRCs on 
mount, that would also help.

The most useful thing to do, though, would probably be to implement 
checkpointing.

--
dwmw2

CLEANMARKER question

[Joakim Tjernlund]

>joakim.tjernlund@lumentis.se said:
>> OK, I suspected that much. So after a CLEANMARKER there can be a
>> NODETYPE_INODE or a NODETYPE_DIRENT?
>
>Or indeed any other type of node, when new ones get invented - yes.
>
>> Why do you do 2 scan_empty() calls in scan_eraseblock() ?
>
>Consider it loop unrolling.
>
>> Well, sofar I have only identified one improvement. One could add an
>> isempty() function in the mtd layer. That would improve scaning for
>> empy flash so that you dont have to mtd->read() into a buffer and then
>> check the buffer for 0xffffffff. How does that sound?
>
>Sounds ugly, but could be effective. Want to benchmark it to see if it's
>really worth it?

I did a very ugly hack to see if there was a big difference.
on my FS(¨~62 MB flash partition, 31% in use) it took
12.2 sec to mount with my ugly hack and it
tackes 13.3 sec without the ugly hack.

I guess it's not worth it.

>
>
>If there's a way to safely avoid having to check all the node CRCs on
>mount, that would also help.

Yes, I once commented out all crc checking in scan just to see what happened
and there was a difference, but I can not remeber how much.

>
>The most useful thing to do, though, would probably be to implement
>checkpointing.
I have seen alot of posts about it but I have no idea on how to proceed
with this.

Would checkpointing  still speed up mount when power is cut and then
restored?

Re: CLEANMARKER question

[David Woodhouse]

Joakim.Tjernlund@lumentis.se said:
> I have seen alot of posts about it but I have no idea on how to
> proceed with this.

The basic idea is to make it nice and quick for the FS code to reproduce 
its internal data (the jffs2_raw_node_ref lists and clean/dirty sizes) on 
mount without having to work it all out from scratch. Look at what the 
mount code does, and see how much could be avoided - then work out 
precisely what needs to go into the checkpoint - it'll probably end up 
being a tradeoff between remount speed and the size taken by the checkpoint 
nodes.

I wonder if we could get away with writing a node at the _end_ of each
eraseblock, just listing the start addresses of the nodes contained within
that block?

> Would checkpointing  still speed up mount when power is cut and then
> restored? 

If you write a checkpoint only on a clean unmount, then no.
If you do it periodically during normal operation. then yes.

--
dwmw2

MTD/CFI probe broken?

[Joakim Tjernlund]

Hi there

I am trying to upgrade our MTD/JFFS2 code(from October) to the latest  CVS in the jffs2-2_4-branch and I think
something is broken in the MTD sublayer. Now MTD does not find my
second chip. This is the output from MTD before and after upgrade.
Before upgrade:
Lumentis: Found 2 x16 devices at 0x2000000 in 32-bit mode
0: offset=0x0,size=0x40000,blocks=128
1: offset=0x2000000,size=0x40000,blocks=128
Creating 5 MTD partitions on "Lumentis":
0x00000000-0x00040000 : "PPCBoot"
0x00040000-0x00080000 : "Environment"
0x00080000-0x00100000 : "FPGA"
0x00100000-0x001c0000 : "Kernel"
0x001c0000-0x04000000 : "JFFS2"

After upgrade:
Lumentis flash device: 4000000 at f0000000
map_priv_1 ioremaped to : c9000000
map_priv_2 ioremaped to : cd001000
0: offset=0x0,size=0x40000,blocks=128
Creating 5 MTD partitions on "Lumentis":
0x00000000-0x00040000 : "PPCBoot"
0x00040000-0x00080000 : "Environment"
0x00080000-0x00100000 : "FPGA"
0x00100000-0x001c0000 : "Kernel"
0x001c0000-0x02000000 : "JFFS2"


    Jocke
PS.
   Had to apply this patch to compile util/mkfs.jffs2:

Index: util/Makefile
===================================================================
RCS file: /home/cvs/mtd/util/Makefile,v
retrieving revision 1.20
diff -u -r1.20 Makefile
--- util/Makefile       2001/09/05 00:18:11     1.20
+++ util/Makefile       2002/01/29 10:27:41
@@ -27,7 +27,7 @@
        $(CC) -o $@ $^ -lz
 
 compr.o: compr.c
-       $(CC) $(CFLAGS) -Dprintk=printf -DKERN_NOTICE= -c -o $@ $<
+       $(CC) $(CFLAGS) -Dprintk=printf -DKERN_NOTICE= -DKERN_WARNING= -c -o $@
$<
 
 jffs2reader: jffs2reader.c
        $(CC) $(CFLAGS) jffs2reader.c -o jffs2reader -lz

Re: MTD/CFI probe broken?

[Joakim Tjernlund]

Hi again

I found the problem in chips/gen_probe.c:
I changed 
1<<cfi.chipshift) < map->size 
part back to
1<<cfi.chipshift) <= map->size

+       for (base = (1<<cfi.chipshift); base + (1<<cfi.chipshift) <= map->size;
-       for (base = (1<<cfi.chipshift); base + (1<<cfi.chipshift) < map->size;
             base += (1<<cfi.chipshift))
                cp->probe_chip(map, base, &chip[0], &cfi);

What was the reason for this change? Have I broken something else now?

            Jocke

On Tuesday 29 January 2002 11:33, Joakim Tjernlund wrote:
> Hi there
>
> I am trying to upgrade our MTD/JFFS2 code(from October) to the latest  CVS
> in the jffs2-2_4-branch and I think something is broken in the MTD
> sublayer. Now MTD does not find my
> second chip. This is the output from MTD before and after upgrade.
> Before upgrade:
> Lumentis: Found 2 x16 devices at 0x2000000 in 32-bit mode
> 0: offset=0x0,size=0x40000,blocks=128
> 1: offset=0x2000000,size=0x40000,blocks=128
> Creating 5 MTD partitions on "Lumentis":
> 0x00000000-0x00040000 : "PPCBoot"
> 0x00040000-0x00080000 : "Environment"
> 0x00080000-0x00100000 : "FPGA"
> 0x00100000-0x001c0000 : "Kernel"
> 0x001c0000-0x04000000 : "JFFS2"
>
> After upgrade:
> Lumentis flash device: 4000000 at f0000000
> map_priv_1 ioremaped to : c9000000
> map_priv_2 ioremaped to : cd001000
> 0: offset=0x0,size=0x40000,blocks=128
> Creating 5 MTD partitions on "Lumentis":
> 0x00000000-0x00040000 : "PPCBoot"
> 0x00040000-0x00080000 : "Environment"
> 0x00080000-0x00100000 : "FPGA"
> 0x00100000-0x001c0000 : "Kernel"
> 0x001c0000-0x02000000 : "JFFS2"
>
>
>     Jocke
> PS.
>    Had to apply this patch to compile util/mkfs.jffs2:
>
> Index: util/Makefile
> ===================================================================
> RCS file: /home/cvs/mtd/util/Makefile,v
> retrieving revision 1.20
> diff -u -r1.20 Makefile
> --- util/Makefile       2001/09/05 00:18:11     1.20
> +++ util/Makefile       2002/01/29 10:27:41
> @@ -27,7 +27,7 @@
>         $(CC) -o $@ $^ -lz
>
>  compr.o: compr.c
> -       $(CC) $(CFLAGS) -Dprintk=printf -DKERN_NOTICE= -c -o $@ $<
> +       $(CC) $(CFLAGS) -Dprintk=printf -DKERN_NOTICE= -DKERN_WARNING= -c
> -o $@ $<
>
>  jffs2reader: jffs2reader.c
>         $(CC) $(CFLAGS) jffs2reader.c -o jffs2reader -lz

cfi_cmdset0001.c: bug fixes and new features.

[Joakim Tjernlund]

Hi again

I hve fixed a few minor bugs:
1)  
-               extp->FeatureSupport = cfi32_to_cpu(extp->FeatureSupport);
-               extp->BlkStatusRegMask = cfi32_to_cpu(extp->BlkStatusRegMask);
-               extp->ProtRegAddr = cfi32_to_cpu(extp->ProtRegAddr);
+               extp->FeatureSupport = le32_to_cpu(extp->FeatureSupport);
+               extp->BlkStatusRegMask = le16_to_cpu(extp->BlkStatusRegMask);
+               extp->ProtRegAddr = le16_to_cpu(extp->ProtRegAddr);

This flash always reports these in litte endian. This corrects things on a BE CPU. Not sure if this
is the correct fix though.

2)
-               if (!((struct cfi_pri_intelext *)cfi->cmdset_priv)->FeatureSupport & 2)
+               if (! (extp->FeatureSupport & 2))

was doing if ( (!*p) & 2), should be if (! (*p &2))

New features:
3)
+#ifdef CONFIG_BUGGY_CMDSET001_CHIPS
+               /* Disable Erase Suspend function in flash, needed by early revisions of Intel Strata chips */
+               extp->FeatureSupport &= ~2;
+#endif
No explanation needed :-)

4)
    Rest of the patch:
         improves suspending erase to do a read.
         adds suspending erase to do a write.

The path is against: $Id: cfi_cmdset_0001.c,v 1.91 2002/01/10 20:27:40 eric Exp $
since thats what I am using.

Comments?

             Jocke

Patch follows:
I--- /usr/local/src/MTD/mtd/drivers/mtd/chips/cfi_cmdset_0001.c	Mon Jan 21 14:06:02 2002
+++ drivers/mtd/chips/cfi_cmdset_0001.c	Thu Feb 14 15:02:15 2002
@@ -4,7 +4,7 @@
  *
  * (C) 2000 Red Hat. GPL'd
  *
- * $Id: cfi_cmdset_0001.c,v 1.91 2002/01/10 20:27:40 eric Exp $
+ * $Id: cfi_cmdset_0001.c,v 1.4 2002/01/31 13:56:10 jocke Exp $
  *
  * 
  * 10/10/2000	Nicolas Pitre <nico@cam.org>
@@ -61,6 +61,8 @@
 #ifdef DEBUG_CFI_FEATURES
 static void cfi_tell_features(struct cfi_pri_intelext *extp)
 {
+	int i;
+
 	printk("  Feature/Command Support: %4.4X\n", extp->FeatureSupport);
 	printk("     - Chip Erase:         %s\n", extp->FeatureSupport&1?"supported":"unsupported");
 	printk("     - Suspend Erase:      %s\n", extp->FeatureSupport&2?"supported":"unsupported");
@@ -151,15 +153,20 @@
 		}
 		
 		/* Do some byteswapping if necessary */
-		extp->FeatureSupport = cfi32_to_cpu(extp->FeatureSupport);
-		extp->BlkStatusRegMask = cfi32_to_cpu(extp->BlkStatusRegMask);
-		extp->ProtRegAddr = cfi32_to_cpu(extp->ProtRegAddr);
+		extp->FeatureSupport = le32_to_cpu(extp->FeatureSupport);
+		extp->BlkStatusRegMask = le16_to_cpu(extp->BlkStatusRegMask);
+		extp->ProtRegAddr = le16_to_cpu(extp->ProtRegAddr);
 			
 #ifdef DEBUG_CFI_FEATURES
 		/* Tell the user about it in lots of lovely detail */
 		cfi_tell_features(extp);
 #endif	
 
+#ifdef CONFIG_BUGGY_CMDSET001_CHIPS
+		/* Disable Erase Suspend function in flash, needed by early revisions of Intel Strata chips */
+		extp->FeatureSupport &= ~2;
+#endif	
+
 		/* Install our own private info structure */
 		cfi->cmdset_priv = extp;	
 	}
@@ -213,7 +220,7 @@
 		unsigned long ernum, ersize;
 		ersize = ((cfi->cfiq->EraseRegionInfo[i] >> 8) & ~0xff) * cfi->interleave;
 		ernum = (cfi->cfiq->EraseRegionInfo[i] & 0xffff) + 1;
-
+		
 		if (mtd->erasesize < ersize) {
 			mtd->erasesize = ersize;
 		}
@@ -223,25 +230,25 @@
 			mtd->eraseregions[(j*cfi->cfiq->NumEraseRegions)+i].numblocks = ernum;
 		}
 		offset += (ersize * ernum);
-		}
-
-		if (offset != devsize) {
-			/* Argh */
-			printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize);
-			kfree(mtd->eraseregions);
-			kfree(cfi->cmdset_priv);
-			return NULL;
-		}
-
-		for (i=0; i<mtd->numeraseregions;i++){
-			printk(KERN_DEBUG "%d: offset=0x%x,size=0x%x,blocks=%d\n",
-			       i,mtd->eraseregions[i].offset,
-			       mtd->eraseregions[i].erasesize,
-			       mtd->eraseregions[i].numblocks);
-		}
+	}
+	
+	if (offset != devsize) {
+		/* Argh */
+		printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize);
+		kfree(mtd->eraseregions);
+		kfree(cfi->cmdset_priv);
+		return NULL;
+	}
+	
+	for (i=0; i<mtd->numeraseregions;i++){
+		printk(KERN_DEBUG "%d: offset=0x%x,size=0x%x,blocks=%d\n",
+		       i,mtd->eraseregions[i].offset,
+		       mtd->eraseregions[i].erasesize,
+		       mtd->eraseregions[i].numblocks);
+	}
 
 	/* Also select the correct geometry setup too */ 
-		mtd->erase = cfi_intelext_erase_varsize;
+	mtd->erase = cfi_intelext_erase_varsize;
 	mtd->read = cfi_intelext_read;
 	if ( cfi->cfiq->BufWriteTimeoutTyp ) {
 		//printk(KERN_INFO "Using buffer write method\n" );
@@ -273,6 +280,7 @@
 	int suspended = 0;
 	unsigned long cmd_addr;
 	struct cfi_private *cfi = map->fldrv_priv;
+	struct cfi_pri_intelext *extp = cfi->cmdset_priv;
 
 	adr += chip->start;
 
@@ -291,7 +299,7 @@
 	 */
 	switch (chip->state) {
 	case FL_ERASING:
-		if (!((struct cfi_pri_intelext *)cfi->cmdset_priv)->FeatureSupport & 2)
+		if (! (extp->FeatureSupport & 2))
 			goto sleep; /* We don't support erase suspend */
 		
 		cfi_write (map, CMD(0xb0), cmd_addr);
@@ -325,8 +333,10 @@
 			cfi_udelay(1);
 			spin_lock_bh(chip->mutex);
 		}
+		/* see if chip was suspended or if it completed the erase cycle */
+		if (status & CMD(0x40))
+			suspended = 1; 	/* at least one chip has been suspended */
 		
-		suspended = 1;
 		cfi_write(map, CMD(0xff), cmd_addr);
 		chip->state = FL_READY;
 		break;
@@ -551,10 +561,11 @@
 static int do_write_oneword(struct map_info *map, struct flchip *chip, unsigned long adr, __u32 datum)
 {
 	struct cfi_private *cfi = map->fldrv_priv;
+	struct cfi_pri_intelext *extp = cfi->cmdset_priv;
 	__u32 status, status_OK;
 	unsigned long timeo;
 	DECLARE_WAITQUEUE(wait, current);
-	int z;
+	int z, suspended=0;
 
 	adr += chip->start;
 
@@ -565,11 +576,6 @@
  retry:
 	spin_lock_bh(chip->mutex);
 
-	/* Check that the chip's ready to talk to us.
-	 * Later, we can actually think about interrupting it
-	 * if it's in FL_ERASING state.
-	 * Not just yet, though.
-	 */
 	switch (chip->state) {
 	case FL_READY:
 		break;
@@ -596,7 +602,52 @@
 		cfi_udelay(1);
 		goto retry;
 
+	case FL_ERASING:
+		if (! ((extp->FeatureSupport & 2) && (extp->SuspendCmdSupport & 1)))
+			goto sleep; /* We don't support program in erase suspend */
+		
+		cfi_write (map, CMD(0xb0), adr);
+
+		/* If the flash has finished erasing, then 'erase suspend'
+		 * appears to make some (28F320) flash devices switch to
+		 * 'read' mode.  Make sure that we switch to 'read status'
+		 * mode so we get the right data. --rmk
+		 */
+		cfi_write(map, CMD(0x70), adr);
+		chip->oldstate = FL_ERASING;
+		chip->state = FL_ERASE_SUSPENDING;
+		//printk("Erase suspending at 0x%lx\n", adr);
+		for (;;) {
+			status = cfi_read(map, adr);
+			if ((status & status_OK) == status_OK)
+				break;
+			
+			if (time_after(jiffies, timeo)) {
+				/* Urgh */
+				cfi_write(map, CMD(0xd0), adr);
+				/* make sure we're in 'read status' mode */
+				cfi_write(map, CMD(0x70), adr);
+				chip->state = FL_ERASING;
+				spin_unlock_bh(chip->mutex);
+				printk(KERN_ERR "Chip not ready after erase "
+				       "suspended: status = 0x%x\n", status);
+				return -EIO;
+			}
+			
+			spin_unlock_bh(chip->mutex);
+			cfi_udelay(1);
+			spin_lock_bh(chip->mutex);
+		}
+		/* see if chip(s) was suspended or if it completed the erase cycle */
+		if (status & CMD(0x40)){
+			suspended = 1; 	/* at least one chip has been suspended */
+			//printk("Erase was suspended at 0x%lx\n", adr);
+		}
+		chip->state = FL_STATUS;
+		break;
+
 	default:
+	sleep:
 		/* Stick ourselves on a wait queue to be woken when
 		   someone changes the status */
 		set_current_state(TASK_UNINTERRUPTIBLE);
@@ -661,20 +712,33 @@
 
 	/* Done and happy. */
 	DISABLE_VPP(map);
-	chip->state = FL_STATUS;
+
 	/* check for lock bit */
 	if (status & CMD(0x02)) {
-		/* clear status */
-		cfi_write(map, CMD(0x50), adr);
-		/* put back into read status register mode */
+		cfi_write(map, CMD(0x50), adr); /* clear status */
+	}
+
+	if (suspended) {
+		chip->state = chip->oldstate;
+		/* What if one interleaved chip has finished and the 
+		   other hasn't? The old code would leave the finished
+		   one in READY mode. That's bad, and caused -EROFS 
+		   errors to be returned from do_erase_oneblock because
+		   that's the only bit it checked for at the time.
+		   As the state machine appears to explicitly allow 
+		   sending the 0x70 (Read Status) command to an erasing
+		   chip and expecting it to be ignored, that's what we 
+		   do. */
+		cfi_write(map, CMD(0xd0), adr);
 		cfi_write(map, CMD(0x70), adr);
-		wake_up(&chip->wq);
-		spin_unlock_bh(chip->mutex);
-		return -EROFS;
+	} else {
+                /* Must be leave with FL_READY since we may have suspended an erase which compleated during suspend */
+		cfi_write(map, CMD(0xff), adr);
+		chip->state = FL_READY;
 	}
 	wake_up(&chip->wq);
 	spin_unlock_bh(chip->mutex);
-	return 0;
+	return (status & CMD(0x02) ? -EROFS : 0);
 }
 
 
@@ -798,10 +862,11 @@
 				  unsigned long adr, const u_char *buf, int len)
 {
 	struct cfi_private *cfi = map->fldrv_priv;
+	struct cfi_pri_intelext *extp = cfi->cmdset_priv;
 	__u32 status, status_OK;
 	unsigned long cmd_adr, timeo;
 	DECLARE_WAITQUEUE(wait, current);
-	int wbufsize, z;
+	int wbufsize, z, suspended=0;
 
 	wbufsize = CFIDEV_INTERLEAVE << cfi->cfiq->MaxBufWriteSize;
 	adr += chip->start;
@@ -814,11 +879,6 @@
  retry:
 	spin_lock_bh(chip->mutex);
 
-	/* Check that the chip's ready to talk to us.
-	 * Later, we can actually think about interrupting it
-	 * if it's in FL_ERASING state.
-	 * Not just yet, though.
-	 */
 	switch (chip->state) {
 	case FL_READY:
 	case FL_CFI_QUERY:
@@ -842,7 +902,53 @@
 		cfi_udelay(1);
 		goto retry;
 
+	case FL_ERASING:
+		if (! ((extp->FeatureSupport & 2) && (extp->SuspendCmdSupport & 1)))
+			goto sleep; /* We don't support erase suspend */
+		
+		cfi_write (map, CMD(0xb0), adr);
+
+		/* If the flash has finished erasing, then 'erase suspend'
+		 * appears to make some (28F320) flash devices switch to
+		 * 'read' mode.  Make sure that we switch to 'read status'
+		 * mode so we get the right data. --rmk
+		 */
+		cfi_write(map, CMD(0x70), adr);
+		chip->oldstate = FL_ERASING;
+		chip->state = FL_ERASE_SUSPENDING;
+		//printk("Erase suspending at 0x%lx\n", adr);
+		for (;;) {
+			status = cfi_read(map, adr);
+			if ((status & status_OK) == status_OK)
+				break;
+			
+			if (time_after(jiffies, timeo)) {
+				/* Urgh */
+				cfi_write(map, CMD(0xd0), adr);
+				/* make sure we're in 'read status' mode */
+				cfi_write(map, CMD(0x70), adr);
+				chip->state = FL_ERASING;
+				spin_unlock_bh(chip->mutex);
+				printk(KERN_ERR "Chip not ready after erase "
+				       "suspended: status = 0x%x\n", status);
+				return -EIO;
+			}
+			
+			spin_unlock_bh(chip->mutex);
+			cfi_udelay(1);
+			spin_lock_bh(chip->mutex);
+		}
+		/* see if chip was suspended or if it completed the erase cycle */
+		if (status & CMD(0x40)){
+			suspended = 1; 	/* at least one chip has been suspended */
+			//printk("Erase was suspended at 0x%lx\n", adr);
+		}
+		
+		chip->state = FL_STATUS;
+		break;
+
 	default:
+	sleep:
 		/* Stick ourselves on a wait queue to be woken when
 		   someone changes the status */
 		set_current_state(TASK_UNINTERRUPTIBLE);
@@ -959,20 +1065,33 @@
 
 	/* Done and happy. */
 	DISABLE_VPP(map);
-	chip->state = FL_STATUS;
-	/* check for lock bit */
+	
+        /* check for lock bit */
 	if (status & CMD(0x02)) {
-		/* clear status */
-		cfi_write(map, CMD(0x50), cmd_adr);
-		/* put back into read status register mode */
-		cfi_write(map, CMD(0x70), adr);
-		wake_up(&chip->wq);
-		spin_unlock_bh(chip->mutex);
-		return -EROFS;
+		cfi_write(map, CMD(0x50), adr); /* clear status */
+	}
+	if (suspended) {
+		//printk("Erase resumed at 0x%lx\n", adr);
+		chip->state = chip->oldstate;
+		/* What if one interleaved chip has finished and the 
+		   other hasn't? The old code would leave the finished
+		   one in READY mode. That's bad, and caused -EROFS 
+		   errors to be returned from do_erase_oneblock because
+		   that's the only bit it checked for at the time.
+		   As the state machine appears to explicitly allow 
+		   sending the 0x70 (Read Status) command to an erasing
+		   chip and expecting it to be ignored, that's what we 
+		   do. */
+		cfi_write(map, CMD(0xd0), adr);
+		cfi_write(map, CMD(0x70), adr);		
+	} else {
+		/* Must be leave with FL_READY since we may have suspended an erase which compleated during suspend */
+		cfi_write(map, CMD(0xff), adr);
+		chip->state = FL_READY;
 	}
 	wake_up(&chip->wq);
 	spin_unlock_bh(chip->mutex);
-	return 0;
+	return (status & CMD(0x02) ? -EROFS : 0);
 }
 
 static int cfi_intelext_write_buffers (struct mtd_info *mtd, loff_t to, 
@@ -1126,6 +1245,8 @@
 
 	timeo = jiffies + (HZ*20);
 	for (;;) {
+		if(chip->state == FL_READY)
+			break; /* erase has compleated and someone put the chip into ready state */ 
 		if (chip->state != FL_ERASING) {
 			/* Someone's suspended the erase. Sleep */
 			set_current_state(TASK_UNINTERRUPTIBLE);
@@ -1151,7 +1272,7 @@
 		if (time_after(jiffies, timeo)) {
 			cfi_write(map, CMD(0x70), adr);
 			chip->state = FL_STATUS;
-			printk(KERN_ERR "waiting for erase at %08x to complete timed out. Xstatus = %x, status = %x.\n", adr, status, cfi_read(map, adr));
+			printk(KERN_ERR "waiting for erase at %08lx to complete timed out. Xstatus = %x, status = %x.\n", adr, status, cfi_read(map, adr));
 			/* Clear status bits */
 			cfi_write(map, CMD(0x50), adr);
 			cfi_write(map, CMD(0x70), adr);

scan.c & ACCURATE

[Joakim Tjernlund]

Hi 

Patch 1 skips scanning of inodes and dirents which are obsoleted.  Improves
scan speed. 

Patch 2 increases INOCACHE_HASHSIZE to 14 which also make scan alot
faster.

Tested in the stable branch, but these patches
is agains latest CVS. Please add to stable branch when you think it's stable.

       jocke 

Index: fs/jffs2/scan.c
===================================================================
RCS file: /home/cvs/mtd/fs/jffs2/scan.c,v
retrieving revision 1.61
diff -u -u -r1.61 scan.c
--- fs/jffs2/scan.c     2002/02/21 23:50:46     1.61
+++ fs/jffs2/scan.c     2002/02/26 13:37:27
@@ -509,7 +509,12 @@
                       retlen, *ofs, sizeof(ri));
                return -EIO;
        }
-
+       if (!(ri.nodetype & JFFS2_NODE_ACCURATE)) {
+               /* Wheee. This is an obsoleted node */
+               DIRTY_SPACE(PAD(ri.totlen));
+               *ofs += PAD(ri.totlen);
+               return 0;
+       }
        /* We sort of assume that the node was accurate when it was
           first written to the medium :) */
        oldnodetype = ri.nodetype;
@@ -685,7 +690,12 @@
                       retlen, *ofs, sizeof(rd));
                return -EIO;
        }
-
+       if (!(rd.nodetype & JFFS2_NODE_ACCURATE)) {
+               /* Wheee. This is an obsoleted node */
+               DIRTY_SPACE(PAD(rd.totlen));
+               *ofs += PAD(rd.totlen);
+               return 0;
+       }
        /* We sort of assume that the node was accurate when it was
           first written to the medium :) */
        oldnodetype = rd.nodetype;




Index: include/linux/jffs2_fs_sb.h
===================================================================
RCS file: /home/cvs/mtd/include/linux/jffs2_fs_sb.h,v
retrieving revision 1.21
diff -u -u -r1.21 jffs2_fs_sb.h
--- include/linux/jffs2_fs_sb.h 2002/02/24 18:11:35     1.21
+++ include/linux/jffs2_fs_sb.h 2002/02/26 13:39:30
@@ -9,7 +9,7 @@
 #include <asm/semaphore.h>
 #include <linux/list.h>
 
-#define INOCACHE_HASHSIZE 1
+#define INOCACHE_HASHSIZE 14
 
 #define JFFS2_SB_FLAG_RO 1
 #define JFFS2_SB_FLAG_MOUNTING 2

Re: scan.c & ACCURATE

[David Woodhouse]

Joakim.Tjernlund@lumentis.se said:
>  Patch 1 skips scanning of inodes and dirents which are obsoleted.
> Improves scan speed.  

You're trusting ri.totlen before verifying ri.node_crc. Naughty.

--
dwmw2

Re: scan.c & ACCURATE

[David Woodhouse]

Joakim.Tjernlund@lumentis.se said:
>  Patch 2 increases INOCACHE_HASHSIZE to 14 which also make scan alot
> faster. 

How does this compare with 8, which is easier to divide by?

--
dwmw2

Re: scan.c & ACCURATE

[Joakim Tjernlund]

>
> Joakim.Tjernlund@lumentis.se said:
> >  Patch 1 skips scanning of inodes and dirents which are obsoleted.
> > Improves scan speed.
>
> You're trusting ri.totlen before verifying ri.node_crc. Naughty.

Totlen has been checked by jffs2_scan_eraseblock() before calling
jffs2_scan_inode_node(), but that is not obvious. Move it to after the CRC
check
if you like.

         Jocke

Re: scan.c & ACCURATE

[Joakim Tjernlund]

> Joakim.Tjernlund@lumentis.se said:
> >  Patch 2 increases INOCACHE_HASHSIZE to 14 which also make scan alot
> > faster.
>
> How does this compare with 8, which is easier to divide by?

Well, I don't remember any figures and I don't think I tested 8, but I do
remember getting better mount times each time I increased INOCACHE_HASHSIZE.
I don't think dividing is a problem, keeping the lists short in the hash
table is more
important.

         Jocke

Re: scan.c & ACCURATE

[David Woodhouse]

Joakim.Tjernlund@lumentis.se said:
> > You're trusting ri.totlen before verifying ri.node_crc. Naughty.
> Totlen has been checked by jffs2_scan_eraseblock() before calling
> jffs2_scan_inode_node(), but that is not obvious. Move it to after the
> CRC check if you like.

Sorry, was confusing node_crc with hdr_crc. You're absolutely right. What 
I've actually done is move the logic to jffs2_erase_eraseblock(), and made 
it trigger for all types of node.

--
dwmw2

RE: scan.c & ACCURATE

[Joakim Tjernlund]

> 
> Sorry, was confusing node_crc with hdr_crc. You're absolutely right. What 
> I've actually done is move the logic to jffs2_erase_eraseblock(), and made 
> it trigger for all types of node.

yes, that's a better place.

 Jocke 

compr_zlib.c

[Joakim Tjernlund]

Hi all

I noticed that zlib.c by default adds a small header and a adler32 checksum to the compressed
data. This is not needed since JFFS2 adds it's own CRC32. There is an 'undocumented' way to 
avoid the header and adler32 checksum, specify a negative windowBits to deflateInit2()/inflateInit2().
Also it is possible to trim the memory usage by adjusting  windowBits and memLevel accordinly. I have
left these at their default values since I don't know zlib very well. Perhaps someone else can comment?

Also I wonder about STREAM_END_SPACE, which is defined to 12. Is it useful to try and compress
data as small as 13 bytes with zlib? Maybe the simpler rtime should be used for small amounts of
data?

Below is a patch against the stable 2.4 branch, which is backwards compatible.

        Jocke

PS.
     There has been a few improvements to JFFS2 that I would like to see in the stable 2.4 branch, we
     have been using these for about 2 weeks now and I think they are stable. 


Index: fs/jffs2/compr_zlib.c
===================================================================
RCS file: /home/cvs/mtd/fs/jffs2/compr_zlib.c,v
retrieving revision 1.8
diff -u -r1.8 compr_zlib.c
--- fs/jffs2/compr_zlib.c       2001/09/20 15:28:31     1.8
+++ fs/jffs2/compr_zlib.c       2002/03/11 08:31:35
@@ -31,7 +31,7 @@
  * provisions above, a recipient may use your version of this file
  * under either the RHEPL or the GPL.
  *
- * $Id: compr_zlib.c,v 1.8 2001/09/20 15:28:31 dwmw2 Exp $
+ * $Id: compr_zlib.c,v 1.8 2002/01/31 13:56:11 jocke Exp $
  *
  */

@@ -92,9 +92,21 @@
        strm.zalloc = (void *)0;
        strm.zfree = (void *)0;
 #endif
-
-       if (Z_OK != deflateInit(&strm, 3)) {
-               printk(KERN_WARNING "deflateInit failed\n");
+       /* The memory requirements for deflate are (in bytes):
+          1 << (windowBits+2)   +  1 << (memLevel+9)
+          that is: 128K for windowBits=15  +  128K for memLevel = 8  (default values)
+          plus a few kilobytes for small objects. For example, if you want to reduce
+          the default memory requirements from 256K to 128K, compile with
+              make CFLAGS="-O -DMAX_WBITS=14 -DMAX_MEM_LEVEL=7"
+          Of course this will generally degrade compression (there's no free lunch).
+
+          The memory requirements for inflate are (in bytes) 1 << windowBits
+          that is, 32K for windowBits=15 (default value) plus a few kilobytes
+          for small objects.
+       */
+       /* -MAX_WBITS impiles -> suppress zlib header and adler32 */
+       if (Z_OK != deflateInit2(&strm, 3, Z_DEFLATED, -MAX_WBITS, 8, Z_DEFAULT_STRATEGY)) {
+               printk(KERN_WARNING "deflateInit2 failed\n");
                return -1;
        }
        strm.next_in = data_in;
@@ -142,7 +154,7 @@
                      __u32 srclen, __u32 destlen)
 {
        z_stream strm;
-       int ret;
+       int ret, wbits, i;

 #ifdef __KERNEL__
        strm.zalloc = zalloc;
@@ -151,23 +163,34 @@
        strm.zalloc = (void *)0;
        strm.zfree = (void *)0;
 #endif
-
-       if (Z_OK != inflateInit(&strm)) {
-               printk(KERN_WARNING "inflateInit failed\n");
-               return;
-       }
-       strm.next_in = data_in;
-       strm.avail_in = srclen;
-       strm.total_in = 0;
-
-       strm.next_out = cpage_out;
-       strm.avail_out = destlen;
-       strm.total_out = 0;
+       /* -MAX_WBITS impiles -> suppress zlib header and adler32.
+          try first with -MAX_WBITS, if that fails, try MAX_WBITS to be
+          backwards compatible */
+       wbits = -MAX_WBITS;
+       for(i = 0; i < 2; i++){
+               if (Z_OK != inflateInit2(&strm, wbits)) {
+                       printk(KERN_WARNING "inflateInit2 failed\n");
+                       return;
+               }
+               strm.next_in = data_in;
+               strm.avail_in = srclen;
+               strm.total_in = 0;
+
+               strm.next_out = cpage_out;
+               strm.avail_out = destlen;
+               strm.total_out = 0;
+
+               while((ret = inflate(&strm, Z_FINISH)) == Z_OK)
+                       ;
+               inflateEnd(&strm);
+               if (ret == Z_DATA_ERROR){
+                       wbits = -wbits;
+                       continue; /* try again with next wbits */
+               }

-       while((ret = inflate(&strm, Z_FINISH)) == Z_OK)
-               ;
-       if (ret != Z_STREAM_END) {
-               printk(KERN_NOTICE "inflate returned %d\n", ret);
+               if (ret != Z_STREAM_END) {
+                       printk(KERN_NOTICE "inflate returned %d, wbits: %d\n", ret, wbits);
+               }
+               break; /* all went well */
        }
-       inflateEnd(&strm);
 }

Re: compr_zlib.c

[Joakim Tjernlund]

No comments so far.
Anyone?

      Jocke

On Monday 11 March 2002 09.56, Joakim Tjernlund wrote:
> Hi all
>
> I noticed that zlib.c by default adds a small header and a adler32 checksum
> to the compressed data. This is not needed since JFFS2 adds it's own CRC32.
> There is an 'undocumented' way to avoid the header and adler32 checksum,
> specify a negative windowBits to deflateInit2()/inflateInit2(). Also it is
> possible to trim the memory usage by adjusting  windowBits and memLevel
> accordinly. I have left these at their default values since I don't know
> zlib very well. Perhaps someone else can comment?
>
> Also I wonder about STREAM_END_SPACE, which is defined to 12. Is it useful
> to try and compress data as small as 13 bytes with zlib? Maybe the simpler
> rtime should be used for small amounts of data?
>
> Below is a patch against the stable 2.4 branch, which is backwards
> compatible.
>
>         Jocke
>
> PS.
>      There has been a few improvements to JFFS2 that I would like to see in
> the stable 2.4 branch, we have been using these for about 2 weeks now and I
> think they are stable.
>
>
> Index: fs/jffs2/compr_zlib.c
> ===================================================================
> RCS file: /home/cvs/mtd/fs/jffs2/compr_zlib.c,v
> retrieving revision 1.8
> diff -u -r1.8 compr_zlib.c
> --- fs/jffs2/compr_zlib.c       2001/09/20 15:28:31     1.8
> +++ fs/jffs2/compr_zlib.c       2002/03/11 08:31:35
> @@ -31,7 +31,7 @@
>   * provisions above, a recipient may use your version of this file
>   * under either the RHEPL or the GPL.
>   *
> - * $Id: compr_zlib.c,v 1.8 2001/09/20 15:28:31 dwmw2 Exp $
> + * $Id: compr_zlib.c,v 1.8 2002/01/31 13:56:11 jocke Exp $
>   *
>   */
>
> @@ -92,9 +92,21 @@
>         strm.zalloc = (void *)0;
>         strm.zfree = (void *)0;
>  #endif
> -
> -       if (Z_OK != deflateInit(&strm, 3)) {
> -               printk(KERN_WARNING "deflateInit failed\n");
> +       /* The memory requirements for deflate are (in bytes):
> +          1 << (windowBits+2)   +  1 << (memLevel+9)
> +          that is: 128K for windowBits=15  +  128K for memLevel = 8 
> (default values) +          plus a few kilobytes for small objects. For
> example, if you want to reduce +          the default memory requirements
> from 256K to 128K, compile with +              make CFLAGS="-O
> -DMAX_WBITS=14 -DMAX_MEM_LEVEL=7"
> +          Of course this will generally degrade compression (there's no
> free lunch). +
> +          The memory requirements for inflate are (in bytes) 1 <<
> windowBits +          that is, 32K for windowBits=15 (default value) plus a
> few kilobytes +          for small objects.
> +       */
> +       /* -MAX_WBITS impiles -> suppress zlib header and adler32 */
> +       if (Z_OK != deflateInit2(&strm, 3, Z_DEFLATED, -MAX_WBITS, 8,
> Z_DEFAULT_STRATEGY)) { +               printk(KERN_WARNING "deflateInit2
> failed\n");
>                 return -1;
>         }
>         strm.next_in = data_in;
> @@ -142,7 +154,7 @@
>                       __u32 srclen, __u32 destlen)
>  {
>         z_stream strm;
> -       int ret;
> +       int ret, wbits, i;
>
>  #ifdef __KERNEL__
>         strm.zalloc = zalloc;
> @@ -151,23 +163,34 @@
>         strm.zalloc = (void *)0;
>         strm.zfree = (void *)0;
>  #endif
> -
> -       if (Z_OK != inflateInit(&strm)) {
> -               printk(KERN_WARNING "inflateInit failed\n");
> -               return;
> -       }
> -       strm.next_in = data_in;
> -       strm.avail_in = srclen;
> -       strm.total_in = 0;
> -
> -       strm.next_out = cpage_out;
> -       strm.avail_out = destlen;
> -       strm.total_out = 0;
> +       /* -MAX_WBITS impiles -> suppress zlib header and adler32.
> +          try first with -MAX_WBITS, if that fails, try MAX_WBITS to be
> +          backwards compatible */
> +       wbits = -MAX_WBITS;
> +       for(i = 0; i < 2; i++){
> +               if (Z_OK != inflateInit2(&strm, wbits)) {
> +                       printk(KERN_WARNING "inflateInit2 failed\n");
> +                       return;
> +               }
> +               strm.next_in = data_in;
> +               strm.avail_in = srclen;
> +               strm.total_in = 0;
> +
> +               strm.next_out = cpage_out;
> +               strm.avail_out = destlen;
> +               strm.total_out = 0;
> +
> +               while((ret = inflate(&strm, Z_FINISH)) == Z_OK)
> +                       ;
> +               inflateEnd(&strm);
> +               if (ret == Z_DATA_ERROR){
> +                       wbits = -wbits;
> +                       continue; /* try again with next wbits */
> +               }
>
> -       while((ret = inflate(&strm, Z_FINISH)) == Z_OK)
> -               ;
> -       if (ret != Z_STREAM_END) {
> -               printk(KERN_NOTICE "inflate returned %d\n", ret);
> +               if (ret != Z_STREAM_END) {
> +                       printk(KERN_NOTICE "inflate returned %d, wbits:
> %d\n", ret, wbits); +               }
> +               break; /* all went well */
>         }
> -       inflateEnd(&strm);
>  }

Re: compr_zlib.c

[David Woodhouse]

On Tue, 19 Mar 2002, Joakim Tjernlund wrote:

> No comments so far.
> Anyone?

Looks good. Suspect we should make it a new compression type instead, and 
fix the mount routine to check for compression types we don't support, 
like we check for node types we don't support.

-- 
dwmw2

point()/unpoint() questions + small cfi_cmdset_0001.c patch

[Joakim Tjernlund]

Hi all

I am looking into using point()/unpoint() in scan.c instead of mtd->read(). A few questions:

1)  There is no point()/unpoint() function pointers in struct map_info, so I wounder where one should
     add the point()/unpoint() functions?

2)  It is not clear to me how the semantics for point()/unpoint() works, is the following sequence legal:
     point()
     ...
     read()
     ...
     point()
     ....
     point()
     ....
     unpoint()
     ...
     write()



Also here is a small patch which fixes 2 bugs:
 1) Flash always reports these in litte endian. This corrects things on a BE CPU.
 2) Was doing if ( (!*p) & 2), should be if (! (*p &2))

Index: cfi_cmdset_0001.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/chips/cfi_cmdset_0001.c,v
retrieving revision 1.98
diff -u -r1.98 cfi_cmdset_0001.c
--- cfi_cmdset_0001.c   16 Jun 2002 17:14:06 -0000      1.98
+++ cfi_cmdset_0001.c   17 Jun 2002 09:17:05 -0000
@@ -156,9 +156,9 @@
                }

                /* Do some byteswapping if necessary */
-               extp->FeatureSupport = cfi32_to_cpu(extp->FeatureSupport);
-               extp->BlkStatusRegMask = cfi32_to_cpu(extp->BlkStatusRegMask);
-               extp->ProtRegAddr = cfi32_to_cpu(extp->ProtRegAddr);
+               extp->FeatureSupport = le32_to_cpu(extp->FeatureSupport);
+               extp->BlkStatusRegMask = le16_to_cpu(extp->BlkStatusRegMask);
+               extp->ProtRegAddr = le16_to_cpu(extp->ProtRegAddr);

 #ifdef DEBUG_CFI_FEATURES
                /* Tell the user about it in lots of lovely detail */
@@ -300,7 +300,7 @@
         */
        switch (chip->state) {
        case FL_ERASING:
-               if (!((struct cfi_pri_intelext *)cfi->cmdset_priv)->FeatureSupport & 2)
+               if (!(((struct cfi_pri_intelext *)cfi->cmdset_priv)->FeatureSupport & 2))
                        goto sleep; /* We don't support erase suspend */

                cfi_write (map, CMD(0xb0), cmd_addr);

[PATCH] scan.c

[Joakim Tjernlund]

Hi again 

I was sure that I sent this one long time ago, but my mind must play tricks on me.

Do not scan blocks which just contain a CLEANMARKER, it's a waste of CPU cycles.

        Jocke

Index: scan.c
===================================================================
RCS file: /home/cvs/mtd/fs/jffs2/scan.c,v
retrieving revision 1.76
diff -u -r1.76 scan.c
--- scan.c      20 May 2002 14:56:38 -0000      1.76
+++ scan.c      17 Jun 2002 09:42:13 -0000
@@ -364,6 +364,18 @@
                                jeb->first_node = jeb->last_node = marker_ref;

                                USED_SPACE(PAD(sizeof(struct jffs2_unknown_node)));
+
+                               err = c->mtd->read(c->mtd, ofs+sizeof(struct jffs2_unknown_node),
+                                                  sizeof(node), &retlen, (char*)&node);
+                               if (node_p->magic == JFFS2_EMPTY_BITMASK &&
+                                   node_p->nodetype == JFFS2_EMPTY_BITMASK) {
+                                       /* We need to check if the first bits
+                                           after the CLEANMARKER is empty since
+                                           older FSes does not obsolete dirty EB's
+                                       */
+                                       ofs += c->sector_size;
+                                       break;
+                               }
                        }
                        ofs += PAD(sizeof(struct jffs2_unknown_node));
                        break;

Re: [PATCH] scan.c

[Joakim Tjernlund]

Oops, some of my point()/unpoint() got in by mistake. Replace the "node_p->" with "node." in my 
previous patch.

      Jocke

On Monday 17 June 2002 11:48, Joakim Tjernlund wrote:
> Hi again
>
> I was sure that I sent this one long time ago, but my mind must play tricks
> on me.
>
> Do not scan blocks which just contain a CLEANMARKER, it's a waste of CPU
> cycles.
>
>         Jocke
>
> Index: scan.c
> ===================================================================
> RCS file: /home/cvs/mtd/fs/jffs2/scan.c,v
> retrieving revision 1.76
> diff -u -r1.76 scan.c
> --- scan.c      20 May 2002 14:56:38 -0000      1.76
> +++ scan.c      17 Jun 2002 09:42:13 -0000
> @@ -364,6 +364,18 @@
>                                 jeb->first_node = jeb->last_node =
> marker_ref;
>
>                                 USED_SPACE(PAD(sizeof(struct
> jffs2_unknown_node))); +
> +                               err = c->mtd->read(c->mtd,
> ofs+sizeof(struct jffs2_unknown_node), +                                   
>               sizeof(node), &retlen, (char*)&node); +                      
>         if (node_p->magic == JFFS2_EMPTY_BITMASK && +                      
>             node_p->nodetype == JFFS2_EMPTY_BITMASK) { +                   
>                    /* We need to check if the first bits +                 
>                          after the CLEANMARKER is empty since +            
>                               older FSes does not obsolete dirty EB's +    
>                                   */
> +                                       ofs += c->sector_size;
> +                                       break;
> +                               }
>                         }
>                         ofs += PAD(sizeof(struct jffs2_unknown_node));
>                         break;

Re: point()/unpoint() questions + small cfi_cmdset_0001.c patch

[David Woodhouse]

Joakim.Tjernlund@lumentis.se said:
> 1)  There is no point()/unpoint() function pointers in struct
> map_info, so I wounder where one should
>      add the point()/unpoint() functions?

> 2)  It is not clear to me how the semantics for point()/unpoint()
> works, is the following sequence legal: 

The point()/unpoint() methods date back to about 1997, and were never used
or properly defined.

The idea was to return a pointer directly into the flash chip, and to lock 
the chip state machine so that it can't be removed from read mode until the 
corresponding unpoint(). Flash drivers would either have to keep track of 
multiple point() requests or disallow subsequent point() calls when one was 
outstanding. 

Any code with a point() call outstanding would need to take care not to do 
anything which would deadlock, of course.

These methods would be implemented in the flash chip driver -- the map 
driver would only need to provide the physical (or virtual) address to look 
at, in the common case where the flash isn't paged. I'd argue that we don't 
want to allow point() to happen in the case where the flash _is_ paged -- 
and hence even read requests outside the region of the point() request 
could deadlock.

The thing is; I'm not convinced I like the potential for deadlock. I've 
been tempted to use this kind of hack for mounting JFFS2 without multiple 
out of line read calls, but haven't actually got round to even hacking up 
the code to test how fast it would be, by replacing the jffs2_flash_read() 
in scan.c with a memcpy from a hard-coded address.

For execute in place, we need something similar, but more sophisticated. The
plan for that, which Linus approved at the time, was to provide a new mmzone
-- and array of 'struct page's, which can be added into the page cache on
demand by the XIP-aware file system. When a given (logical) page from a file
moves from one location to another, or when the chip is temporarily in a
mode other than read mode, we have to mark those pages invalid, and
subsequent faults on those pages must sleep until the chip is back in read
mode.

The RMAP VM helps us a lot here -- until we could find all the virtual 
mappings of a given physical page, it was just about impossible to do what 
we need. 

Note that using AMD chips instead of Intel chips seems to be particularly 
sensible if you want XIP -- with Intel chips, the whole chip goes into 
status mode when you're doing things with it. Yet I believe that with AMD 
chips, only the block to which you're actually writing returns status bits, 
and all other erase blocks remain readable during the operation.

Anyway, to get back to the point... given that I don't like the possibility 
of deadlock with the naïve point/unpoint routines, and that I think we will 
need to do the page-based thing for XIP anyway, I suspect we should just 
use the suggested XIP setup for doing what you want in kernel space.

So if you're looking at letting JFFS2 read directly from the flash, it 
would call a chip driver method akin to ioremap(), which would return a 
kernel virtual address from which all the flash can be read. The chip 
driver then marks pages absent as an when necessary -- for kernel mappings 
we can do that even without the rmap VM. We can probably also mark these 
pages as cacheable, to let burst reads work.

Comments?

--
dwmw2

Re: [PATCH] scan.c

[David Woodhouse]

Joakim.Tjernlund@lumentis.se said:
> I was sure that I sent this one long time ago, but my mind must play
> tricks on me.

> Do not scan blocks which just contain a CLEANMARKER, it's a waste of
> CPU cycles.

I'd rather we still scanned them, but deferred it till later -- as long as
it gets done before we actually try to write to them, that's fine.

We can go one better than that -- we can avoid doing the crc32 check on 
data nodes during mount. As as we do it in read_inode(), and we do _all_ 
inodes before actually starting to garbage-collect, we don't have to make 
the user wait for it during mount.

I tried both of these last week (well, actually I just disabled the crc32 
without doing it later), and the latter gave more of an improvement. If we 
skip the building up of fragment lists during mount too, it'll probably get 
even faster.

Of course, the two things that gave me the best improvement in mount time 
were fixing the inocache_last optimisation, and then just increasing 
INOCACHE_HASHSIZE to 128. The latter rendered the whole inocache_last 
optimisation pointless, btw.

--
dwmw2

RE: [PATCH] scan.c

[Joakim Tjernlund]

> 
> Joakim.Tjernlund@lumentis.se said:
> > I was sure that I sent this one long time ago, but my mind must play
> > tricks on me.
> 
> > Do not scan blocks which just contain a CLEANMARKER, it's a waste of
> > CPU cycles.
> 
> I'd rather we still scanned them, but deferred it till later -- as long as
> it gets done before we actually try to write to them, that's fine.

Why do you still want to scan them? It's safe not to by design. Maybe
as a debug opion?

> 
> We can go one better than that -- we can avoid doing the crc32 check on 
> data nodes during mount. As as we do it in read_inode(), and we do _all_ 
> inodes before actually starting to garbage-collect, we don't have to make 
> the user wait for it during mount.

yes, this is what I tested(skipping the crc32 check) some time ago and I 
noticed a big improvement. I also noticed that adler32 was much faster
than crc32(btw you should not use 0 as start seed to the crc32 since that
will yield a correct csum(0) on a zeroed buffer).
Is it safe to do skip the CRC check in the 2.4 branch as well? 

I have an addon idea: Add a new flag to node.nodetype.
This flag is set when a node is first written to the flash. The first time
the node is read the CRC is calculated and if the CRC:s match, clear
the CRC flag(on the flash as well). The next time the same node is 
read you don't have to check the CRC again since it has already
been verified. That will save a lot of CPU cycles during file I/O as well. 

> 
> I tried both of these last week (well, actually I just disabled the crc32 
> without doing it later), and the latter gave more of an improvement. If we 
> skip the building up of fragment lists during mount too, it'll probably get 
> even faster.
> 
> Of course, the two things that gave me the best improvement in mount time 
> were fixing the inocache_last optimisation, and then just increasing 
> INOCACHE_HASHSIZE to 128. The latter rendered the whole inocache_last 
> optimisation pointless, btw.

128! In my 2.4.2 kernel(from Montavista) I can not go higher than
13, then it Oops on me.

> 
> --
> dwmw2

 Jocke

Re: [PATCH] scan.c

[David Woodhouse]

joakim.tjernlund@lumentis.se said:
>  Why do you still want to scan them? It's safe not to by design. Maybe
> as a debug opion?

It's safe in theory. I want it robust in practice. Shit happens.

>  yes, this is what I tested(skipping the crc32 check) some time ago
> and I  noticed a big improvement. I also noticed that adler32 was much
> faster than crc32(btw you should not use 0 as start seed to the crc32
> since that will yield a correct csum(0) on a zeroed buffer). 

It's not safe to just skip it -- I just did that to see how it affected the 
profiling results. We can only skip it if we make sure we do it later.

The reason we build up all the fragment lists for each inode at boot, and
hence the reason we had to check all the crcs, is because we need to know
which nodes are obsolete, so we can build up accurate information about free/
dirty space per block. But we don't really _need_ that information until we 
start to garbage-collect, so there's no real reason for us to do it at boot 
time.

What I did was just disable the CRC32 check and still build up the fragment 
lists as if the CRC32 was OK -- so nodes with bad CRC could still obsolete 
good nodes, causing corruption. It was only done as a test. The real way 
forward would be to skip the building up of the per-inode fragment list 
_too_, and just make sure it's done for all inodes by the time we start to 
do GC, later.

> Is it safe to do skip the CRC check in the 2.4 branch as well?  

The above will be quite intrusive ,and I would rather not change the 2.4
branch. If you want optimisations, you should really be using the 
development branch -- it shouldn't really any less stable in general than 
the 2.4 branch was until we branched it. 

Don't think of them as stable and development, in the same way as the 2.4 
and 2.5 Linux kernel trees -- think of them more as paranoia and stable, 
respectively; more like 2.2 and 2.4.


> I have an addon idea: Add a new flag to node.nodetype. This flag is
> set when a node is first written to the flash. The first time the node
> is read the CRC is calculated and if the CRC:s match, clear the CRC
> flag(on the flash as well). The next time the same node is  read you
> don't have to check the CRC again since it has already been verified.
> That will save a lot of CPU cycles during file I/O as well.  

But what if the act of clearing the CRC flag is just enough to trigger a 
nearby bit to flip, in an ageing flash chip? Also, how does this increase 
the probability of random data being interpreted as a real node? The CRC 
gives us a fairly small probability of that. 

I'd be more willing to entertain the possibility of having such a flag in 
_memory_, so we don't check the CRC32 more than once per mount cycle.

>  128! In my 2.4.2 kernel(from Montavista) I can not go higher than 13,
> then it Oops on me.

Sounds like you don't have the jffs2_sb in the superblock union, so when 
the JFFS2 superblock info gets larger than the existing size of the union, 
the union doesn't grow to accommodate it.

For 2.5 kernels this isn't a problem because we allocate our own fs-private 
superblock info anyway, but for 2.4 we should probably allocate the hash 
table separately if we want it that large -- otherwise _all_ superblocks 
will have enough space for it. 

In fact, we should probably allocate the hash table dynamically anyway, and 
vary its size according to the size of the flash device.

I'd be willing to let that into the 2.4 branch, as it's so obviously 
correct and makes such a difference.

--
dwmw2

Re: point()/unpoint() questions + small cfi_cmdset_0001.c patch

[David Woodhouse]

dwmw2@infradead.org said:
> I've  been tempted to use this kind of hack for mounting JFFS2 without
> multiple  out of line read calls, but haven't actually got round to
> even hacking up  the code to test how fast it would be, by replacing
> the jffs2_flash_read()  in scan.c with a memcpy from a hard-coded
> address.

Er, this is of course complete crap -- half the point is to remove the need
for copying at all. So the reason I've not got round to doing the
replacement with memcpy() is mostly because in the moments when I'm actually
capable of writing code which compiles, I'm not stupid enough to contemplate
it.

Note to self: Do not reply to mailing list posts before getting to the 
office and drinking coffee; especially on Mondays.

--
dwmw2

RE: [PATCH] scan.c

[Joakim Tjernlund]

> 
> 
> joakim.tjernlund@lumentis.se said:
> >  Why do you still want to scan them? It's safe not to by design. Maybe
> > as a debug opion?
> 
> It's safe in theory. I want it robust in practice. Shit happens.

Yes shit happens, but removing this scan here lowers my mount time alot(don't have
the figures anymore) and shit has yet to happen :-) 
> 
> >  yes, this is what I tested(skipping the crc32 check) some time ago
> > and I  noticed a big improvement. I also noticed that adler32 was much
> > faster than crc32(btw you should not use 0 as start seed to the crc32
> > since that will yield a correct csum(0) on a zeroed buffer). 
> 
> It's not safe to just skip it -- I just did that to see how it affected the 
> profiling results. We can only skip it if we make sure we do it later.

And right now we don't make sure that we do it later, I see.

> 
> The reason we build up all the fragment lists for each inode at boot, and
> hence the reason we had to check all the crcs, is because we need to know
> which nodes are obsolete, so we can build up accurate information about free/
> dirty space per block. But we don't really _need_ that information until we 
> start to garbage-collect, so there's no real reason for us to do it at boot 
> time.
OK.
> 
> What I did was just disable the CRC32 check and still build up the fragment 
> lists as if the CRC32 was OK -- so nodes with bad CRC could still obsolete 
> good nodes, causing corruption. It was only done as a test. The real way 
> forward would be to skip the building up of the per-inode fragment list 
> _too_, and just make sure it's done for all inodes by the time we start to 
> do GC, later.
Yes, this is what I did too(removing the check of the data CRC) and that reduces my mount
time alot. It's the biggest part of my mount time.

> 
> > Is it safe to do skip the CRC check in the 2.4 branch as well?  
> 
> The above will be quite intrusive ,and I would rather not change the 2.4
> branch. If you want optimisations, you should really be using the 
> development branch -- it shouldn't really any less stable in general than 
> the 2.4 branch was until we branched it. 
> 
> Don't think of them as stable and development, in the same way as the 2.4 
> and 2.5 Linux kernel trees -- think of them more as paranoia and stable, 
> respectively; more like 2.2 and 2.4.

Well, I will look into switching over to the devel branch when I get around 
to upgrade my kernel.

> 
> 
> > I have an addon idea: Add a new flag to node.nodetype. This flag is
> > set when a node is first written to the flash. The first time the node
> > is read the CRC is calculated and if the CRC:s match, clear the CRC
> > flag(on the flash as well). The next time the same node is  read you
> > don't have to check the CRC again since it has already been verified.
> > That will save a lot of CPU cycles during file I/O as well.  
> 
> But what if the act of clearing the CRC flag is just enough to trigger a 
> nearby bit to flip, in an ageing flash chip? Also, how does this increase 
> the probability of random data being interpreted as a real node? The CRC 
> gives us a fairly small probability of that.

I am not that familiar with flash HW, but I thought that once data was correctly written
to flash, it's more or less "safe". A nearby flipping bit can by checked for once the flag 
has been cleared. I don't see how this would increase the probability for random data being
interpreted as a real node, it's only the data CRC check that is omitted, not the whole node CRC.  
 
> 
> I'd be more willing to entertain the possibility of having such a flag in 
> _memory_, so we don't check the CRC32 more than once per mount cycle.

Yes, this is a positive side effect, but it would be nice to have it in the flash as well.

> 
> >  128! In my 2.4.2 kernel(from Montavista) I can not go higher than 13,
> > then it Oops on me.
> 
> Sounds like you don't have the jffs2_sb in the superblock union, so when 
> the JFFS2 superblock info gets larger than the existing size of the union, 
> the union doesn't grow to accommodate it.

probably, I will check. 
> 
> For 2.5 kernels this isn't a problem because we allocate our own fs-private 
> superblock info anyway, but for 2.4 we should probably allocate the hash 
> table separately if we want it that large -- otherwise _all_ superblocks 
> will have enough space for it. 

Yes, that would be bad.
> 
> In fact, we should probably allocate the hash table dynamically anyway, and 
> vary its size according to the size of the flash device.
> 
> I'd be willing to let that into the 2.4 branch, as it's so obviously 
> correct and makes such a difference.
> 
> --
> dwmw2

RE: point()/unpoint() questions + small cfi_cmdset_0001.c patch

[Joakim Tjernlund]

Hi again

This will take some time to digest. Here is my initial comments  ... :-)

I am mainly interested in using point() in scan.c to reduce mount time. A quick and dirty hack
to impl. point() gives me better a scan time, from 4.43 sec to 3.53 sec, but this will probably go up
a bit if I do a proper impl. of point().


>
> Note that using AMD chips instead of Intel chips seems to be particularly
> sensible if you want XIP -- with Intel chips, the whole chip goes into
> status mode when you're doing things with it. Yet I believe that with AMD
> chips, only the block to which you're actually writing returns status bits,
> and all other erase blocks remain readable during the operation.

They do? I have not seen any evidence of this, do you happen to known
any perticular AMD device that does this?

>
> Anyway, to get back to the point... given that I don't like the possibility
> of deadlock with the naïve point/unpoint routines, and that I think we will
> need to do the page-based thing for XIP anyway, I suspect we should just
> use the suggested XIP setup for doing what you want in kernel space.
>
> So if you're looking at letting JFFS2 read directly from the flash, it
> would call a chip driver method akin to ioremap(), which would return a
> kernel virtual address from which all the flash can be read. The chip
> driver then marks pages absent as an when necessary -- for kernel mappings
> we can do that even without the rmap VM. We can probably also mark these
> pages as cacheable, to let burst reads work.
>
> Comments?
>
> --
> dwmw2
>
>
>

Re: point()/unpoint() questions + small cfi_cmdset_0001.c patch

[David Woodhouse]

joakim.tjernlund@lumentis.se said:
>  I am mainly interested in using point() in scan.c to reduce mount
> time. A quick and dirty hack to impl. point() gives me better a scan
> time, from 4.43 sec to 3.53 sec, but this will probably go up a bit if
> I do a proper impl. of point(). 

Is this on your board with burst reads working? Are you using a cached 
mapping for point()? 

>  They do? I have not seen any evidence of this, do you happen to known
> any perticular AMD device that does this? 

Looking at the AM29LV320D datasheet, page 26, under 'Sector Erase Command 
Sequence'...

"Note that while the Embedded Erase operation is in progress, the system can
read data from the non-erasing sector. The system can determine the status
of the erase operation by reading DQ7, DQ6, DQ2 or RY/BY# in the erasing
sector."

(http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/23579a3.pdf)

OTOH, the next page talks about the Erase Suspend command, which "allows 
the system to interrupt a sector erase operation and then read data from, 
or program data to, any sector not selected for erasure." So perhaps I was 
imagining it.

--
dwmw2

Re: [PATCH] scan.c

[David Woodhouse]

joakim.tjernlund@lumentis.se said:
>  Yes shit happens, but removing this scan here lowers my mount time
> alot(don't have the figures anymore) and shit has yet to happen :-) 

Yeah - I've been doing profiling too and although it's not that close to 
the top of the list, I suspect that's because my flash is mostly full, so 
there aren't many empty blocks to be scanned.

I'm happy to remove it from the mount path -- but I'd like to do it later,
before actually trying to use those blocks, rather than simply refraining
from doing it at all.

> Yes, this is what I did too(removing the check of the data CRC) and
> that reduces my mount time alot. It's the biggest part of my mount
> time.

Yep, mine too -- after jffs2_get_ino_cache() was fixed, anyway. 

c0083ff0 jffs2_scan_empty                             10   0.0225
c0081d30 jffs2_add_full_dnode_to_fraglist             12   0.0181
c0140e90 huft_build                                   13   0.0093
c00bb6c8 cfi_intelext_read                            17   0.0127
c0140014 zlib_inflate_fast                            31   0.0356
c00838ec jffs2_scan_eraseblock                        79   0.0440
c0084258 jffs2_scan_inode_node                        99   0.0709
c013ad60 memmove                                     214   0.1911

But we do need to check data CRC at some point before starting to each node,
or deciding that other nodes are obsolete due to the existence of the node.

>  I am not that familiar with flash HW, but I thought that once data
> was correctly written to flash, it's more or less "safe". A nearby
> flipping bit can by checked for once the flag  has been cleared. 

Nah, it's never safe -- you can always manage to corrupt it somehow :)

> I don't see how this would increase the probability for random data
> being interpreted as a real node, it's only the data CRC check that is
> omitted, not the whole node CRC.  

OK, that wouldn't increase the chances of error by too much, and could 
perhaps be acceptable. I'd be interested in seeing how much that gives us 
over the once-per-mount approach. P'raps we could make it an option, 
although I'd prefer it to be disabled by default. 

--
dwmw2

RE: point()/unpoint() questions + small cfi_cmdset_0001.c patch

[Joakim Tjernlund]

> 
> joakim.tjernlund@lumentis.se said:
> >  I am mainly interested in using point() in scan.c to reduce mount
> > time. A quick and dirty hack to impl. point() gives me better a scan
> > time, from 4.43 sec to 3.53 sec, but this will probably go up a bit if
> > I do a proper impl. of point(). 
> 
> Is this on your board with burst reads working? Are you using a cached 
> mapping for point()? 

Yes x 2

> 
> >  They do? I have not seen any evidence of this, do you happen to known
> > any perticular AMD device that does this? 
> 
> Looking at the AM29LV320D datasheet, page 26, under 'Sector Erase Command 
> Sequence'...
> 
> "Note that while the Embedded Erase operation is in progress, the system can
> read data from the non-erasing sector. The system can determine the status
> of the erase operation by reading DQ7, DQ6, DQ2 or RY/BY# in the erasing
> sector."
> 
> (http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/23579a3.pdf)
> 
> OTOH, the next page talks about the Erase Suspend command, which "allows 
> the system to interrupt a sector erase operation and then read data from, 
> or program data to, any sector not selected for erasure." So perhaps I was 
> imagining it.
> 

After a quick read I think it's NOT possible to read data without suspending an Erase
first.

     Jocke

RE: [PATCH] scan.c

[Joakim Tjernlund]

> 
> joakim.tjernlund@lumentis.se said:
> >  Yes shit happens, but removing this scan here lowers my mount time
> > alot(don't have the figures anymore) and shit has yet to happen :-) 
> 
> Yeah - I've been doing profiling too and although it's not that close to 
> the top of the list, I suspect that's because my flash is mostly full, so 
> there aren't many empty blocks to be scanned.
> 
> I'm happy to remove it from the mount path -- but I'd like to do it later,
> before actually trying to use those blocks, rather than simply refraining
> from doing it at all.
> 
> > Yes, this is what I did too(removing the check of the data CRC) and
> > that reduces my mount time alot. It's the biggest part of my mount
> > time.
> 
> Yep, mine too -- after jffs2_get_ino_cache() was fixed, anyway. 
> 
> c0083ff0 jffs2_scan_empty                             10   0.0225
> c0081d30 jffs2_add_full_dnode_to_fraglist             12   0.0181
> c0140e90 huft_build                                   13   0.0093
> c00bb6c8 cfi_intelext_read                            17   0.0127
> c0140014 zlib_inflate_fast                            31   0.0356
> c00838ec jffs2_scan_eraseblock                        79   0.0440
> c0084258 jffs2_scan_inode_node                        99   0.0709
> c013ad60 memmove                                     214   0.1911

hmm, I just save the jiffies from the beginning of jffs2_scan_medium()
and printk the difference just before I leave that function.
 
> 
> But we do need to check data CRC at some point before starting to each node,
> or deciding that other nodes are obsolete due to the existence of the node.
> 
> >  I am not that familiar with flash HW, but I thought that once data
> > was correctly written to flash, it's more or less "safe". A nearby
> > flipping bit can by checked for once the flag  has been cleared. 
> 
> Nah, it's never safe -- you can always manage to corrupt it somehow :)

:-), but seriously once it's on the flash, what's the probability for a random
     flipping bit? 

> 
> > I don't see how this would increase the probability for random data
> > being interpreted as a real node, it's only the data CRC check that is
> > omitted, not the whole node CRC.  
> 
> OK, that wouldn't increase the chances of error by too much, and could 
> perhaps be acceptable. I'd be interested in seeing how much that gives us 
> over the once-per-mount approach. P'raps we could make it an option, 
> although I'd prefer it to be disabled by default. 

At mount time it will save us the trouble to CRC all the data in the inode's, won't it?

    Jocke 

Re: [PATCH] scan.c

[David Woodhouse]

joakim.tjernlund@lumentis.se said:
>  hmm, I just save the jiffies from the beginning of
> jffs2_scan_medium() and printk the difference just before I leave that
> function.

The real kernel profiling shows a little more detail, which can sometimes 
be useful. Moving the crc32 routine out of line makes it show up nicely too.

>  :-), but seriously once it's on the flash, what's the probability for
> a random flipping bit?  

There are other ways to screw up your flash contents. It doesn't have to be 
a hardware error. I'd be a _lot_ happier if we could continue to expect 
such things and deal with them.

>  At mount time it will save us the trouble to CRC all the data in the
> inode's, won't it? 

Yeah, but we can avoid the need to do that at mount time _anyway_. We can 
probably also ditch most of build.c -- although that doesn't really seem to 
be showing up very high on the profiles anyway.

--
dwmw2

Re: point()/unpoint() questions + small cfi_cmdset_0001.c patch

[David Woodhouse]

joakim.tjernlund@lumentis.se said:
> Is this on your board with burst reads working? Are you using a cached
> mapping for point()? 

> Yes x 2 

As discussed on IRC -- perhaps we want to investigate using prefetch() then.

>  After a quick read I think it's NOT possible to read data without
> suspending an Erase first.

Yeah - it looks that way to me too. XIP on a writable file system really 
does seem like a silly plan, given current flash technology.

--
dwmw2