Re: [PATCH] jffs2 summary allocation

Re: [PATCH] jffs2 summary allocation

[matthieu castet]

Hi,

I think the main problem is that mtd buffers are not dma compatible [1].
They can be allocated by vmalloc, don't be aligned on a page, ...

The problem was catch on jffs2 summary, but the same things could happen 
in other places.

So for me the correct fix, is either to fix mtd to have all buffer dma 
compatible or either fix mtd_dataflash to don't pass mtd buffer to spi 
stack, but to allocate dma buffer and do some copy.

BTW aren't there sparse annotation to track dma buffer ?

Matthieu

[1] http://article.gmane.org/gmane.linux.drivers.mtd/20663
As for the original question... I'm not sure. At the moment I don't
believe it's true that all such buffers are suitable for DMA. Perhaps it
would be sensible for us to redefine the MTD API so that it is required
(and fix the users).

[PATCH] jffs2 summary allocation

[Michael Trimarchi]

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

Hi,

I apply this patch to fix this oops.


Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = c0004000
[00000000] *pgd=00000000
stopped custom tracer.
Internal error: Oops: 817 [#1] PREEMPT
Modules linked in:
CPU: 0    Not tainted  (2.6.24-rc5-rt1 #37)
PC is at dma_cache_maint+0x40/0x80
LR is at atmel_spi_transfer+0x94/0x178
pc : [<c002488c>]    lr : [<c013eedc>]    psr: 20000013
sp : c044db84  ip : c044db94  fp : c044db90
r10: ffffffff  r9 : 00000000  r8 : c04e4c00
r7 : c03ee310  r6 : c044dcfc  r5 : c109d3bc  r4 : c044dcd8
r3 : 00000000  r2 : 00000001  r1 : c109d7dc  r0 : c109d3bc
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 0005317f  Table: 20588000  DAC: 00000017
Process jffs2_gcd_mtd1 (pid: 313, stack limit = 0xc044c258)
Stack: (0xc044db84 to 0xc044e000)
db80:          c044dbb8 c044db94 c013eedc c002485c c04e4c00 c044dbbc c044dcfc
dba0: c044dc20 00000000 c044dcfc c044dca4 c044dbbc c013e124 c013ee58 00000000
dbc0: 00000001 dead4ead ffffffff ffffffff c02e7844 00000000 c01933d4 c044dc40
dbe0: c044dc40 c044dc48 c044dc48 c044dc24 00000000 005593e0 c018f8b4 c044607c
dc00: 00000420 c044dc88 c044dc78 c01defa8 00000000 c018f8b4 c044dc7c c044dc7c
dc20: 00000000 00000001 dead4ead ffffffff ffffffff c02e7844 00000000 c01933d4
dc40: c044dc40 c044dc40 c044dc48 c044dc48 c044dc24 00000000 005593e0 c018f8b4
dc60: c044607c 00000420 c044dc88 c044dc78 c01defa8 00000000 c018f8b4 c044dc7c
dc80: c044dc7c 00a5f800 00000420 c0446000 00000420 000014bf c044dd50 c044dca8
dca0: c013dcb0 c013e0c0 c0446024 c04e4c00 c109d3bc c0446000 00000000 00000004
dcc0: 20446000 ffffffff 00000000 00000000 c044dcf4 c044dcfc c109d3bc 00000000
dce0: 00000420 ffffffff ffffffff 00000000 00000000 c044dcfc c044dcd0 c044dcd0
dd00: c044dcf4 c04e4c00 00000000 c013e148 c044dc20 00000000 00000000 00000000
dd20: 00000000 00000000 00000000 00427fe0 00000000 c109d3bc 00427fe0 000003dc
dd40: 00000000 c044dd74 c044dd54 c013a00c c013db10 c044dd9c c109d3bc 00000420
dd60: 00000420 c04d9000 c044ddc8 c044dd78 c00e0724 c0139f74 c044dd9c c109d3bc
dd80: 00000002 c044de08 c04d93a0 00427c24 00000001 c044de10 00427c04 00000000
dda0: c044ed70 00427c04 c109d048 c04d9000 00000000 0000078c c056689c c044de44
ddc0: c044ddd0 c00e1f40 c00e04c0 00427c04 00000000 c044de18 00000000 c04d91cc
dde0: 000007fc 000007dc 20061985 000007fc 37fa2120 00000004 00000000 00000364
de00: 36d8bf4d b504e572 c044dde8 00000020 c109d000 000007dc 00000000 c044ed70
de20: c056689c 00000938 c04d9000 000008c8 000000c4 c04d91cc c044de84 c044de48
de40: c00d5ca0 c00e1c1c c044de68 c044de58 c04d91b4 000000c4 c044dec4 c044ed70
de60: c04d9000 000000c4 c04d91cc 000008c8 c044ed70 c044dec4 c044deac c044de88
de80: c00d6014 c00d5c1c c044ed70 000008c8 c0568950 c04d9000 c04d9000 c037bde4
dea0: c044def4 c044deb0 c00da938 c00d5fe4 c04d92e8 c044c000 00000001 c044dec8
dec0: c014e3c0 000000c4 c04d92e8 c044ed70 c04e1ecc c0568950 c037bde4 c04d9000
dee0: c04d90dc c04d92e8 c044df48 c044def8 c00dc32c c00da8ec c044df04 c04d9194
df00: c04d9174 c04d91c4 000015e8 c044c000 c044df30 c044df20 c014ecfc c014e37c
df20: 00000001 c039c3e0 c044c000 c04d9000 00000008 00000002 c044df4c c044dff4
df40: c044df4c c00dd730 c00dbdac 00000001 00000000 00000080 00000000 00000000
df60: 00000000 00000000 00000000 c044df98 c044df7c c0060ea0 c005bd74 c0324020
df80: 00000000 c0324020 00000000 c044dfac c044df9c c00326f4 c0060f78 00000000
dfa0: 00000000 c044dfb0 c001ff44 c00326c4 00000000 c04d9000 c00dd630 c0039818
dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
dfe0: 00000000 00000000 00000000 c044dff8 c0039818 c00dd640 00000000 00000000
Backtrace:
[<c002484c>] (dma_cache_maint+0x0/0x80) from [<c013eedc>] (atmel_spi_transfer+0x94/0x178)
[<c013ee48>] (atmel_spi_transfer+0x0/0x178) from [<c013e124>] (spi_sync+0x74/0x98)
[<c013e0b0>] (spi_sync+0x0/0x98) from [<c013dcb0>] (dataflash_write+0x1b0/0x270)
 r8:000014bf r7:00000420 r6:c0446000 r5:00000420 r4:00a5f800
[<c013db00>] (dataflash_write+0x0/0x270) from [<c013a00c>] (part_write+0xa8/0xb0)
[<c0139f64>] (part_write+0x0/0xb0) from [<c00e0724>] (jffs2_flash_writev+0x278/0x434)
 r6:c04d9000 r5:00000420 r4:00000420
[<c00e04b0>] (jffs2_flash_writev+0x4/0x434) from [<c00e1f40>] (jffs2_sum_write_sumnode+0x334/0x420)
[<c00e1c0c>] (jffs2_sum_write_sumnode+0x0/0x420) from [<c00d5ca0>] (jffs2_do_reserve_space+0x94/0x3c8)
[<c00d5c0c>] (jffs2_do_reserve_space+0x0/0x3c8) from [<c00d6014>] (jffs2_reserve_space_gc+0x40/0x78)
[<c00d5fd4>] (jffs2_reserve_space_gc+0x0/0x78) from [<c00da938>] (jffs2_garbage_collect_pristine+0x5c/0x3a8)
[<c00da8dc>] (jffs2_garbage_collect_pristine+0x0/0x3a8) from [<c00dc32c>] (jffs2_garbage_collect_pass+0x590/0x714)
[<c00dbd9c>] (jffs2_garbage_collect_pass+0x0/0x714) from [<c00dd730>] (jffs2_garbage_collect_thread+0x100/0x18c)
[<c00dd630>] (jffs2_garbage_collect_thread+0x0/0x18c) from [<c0039818>] (do_exit+0x0/0x73c)
Code: 9a000001 e15c0003 3a000001 e3a03000 (e5833000)



      Inviato da Yahoo! Mail. 
La casella di posta intelligente.
http://it.docs.yahoo.com/mail/overview/index.html

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: summary-buffer-allocation.patch --]
[-- Type: text/x-patch; name="summary-buffer-allocation.patch", Size: 1107 bytes --]

Change the summary buffer allocation for resolving an oops during
dma transfer using spi bus.

Signed-off-by: michael <trimarchi@gandalf.sssup.it>

---
 fs/jffs2/summary.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/jffs2/summary.c b/fs/jffs2/summary.c
index 629af01..5962b3b 100644
--- a/fs/jffs2/summary.c
+++ b/fs/jffs2/summary.c
@@ -17,7 +17,6 @@
 #include <linux/pagemap.h>
 #include <linux/crc32.h>
 #include <linux/compiler.h>
-#include <linux/vmalloc.h>
 #include "nodelist.h"
 #include "debug.h"
 
@@ -30,7 +29,7 @@ int jffs2_sum_init(struct jffs2_sb_info *c)
 		return -ENOMEM;
 	}
 
-	c->summary->sum_buf = vmalloc(c->sector_size);
+	c->summary->sum_buf = kmalloc(c->sector_size, GFP_KERNEL);
 
 	if (!c->summary->sum_buf) {
 		JFFS2_WARNING("Can't allocate buffer for writing out summary information!\n");
@@ -49,7 +48,7 @@ void jffs2_sum_exit(struct jffs2_sb_info *c)
 
 	jffs2_sum_disable_collecting(c->summary);
 
-	vfree(c->summary->sum_buf);
+	kfree(c->summary->sum_buf);
 	c->summary->sum_buf = NULL;
 
 	kfree(c->summary);
-- 
1.5.2.1.174.gcd03-dirty

Re: [PATCH] jffs2 summary allocation

[Andrew Morton]

On Fri, 4 Apr 2008 10:23:55 +0000 (GMT)
Michael Trimarchi <trimarchimichael@yahoo.it> wrote:

> Hi,
> 
> I apply this patch to fix this oops.
> 
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> pgd = c0004000
> [00000000] *pgd=00000000
> stopped custom tracer.
> Internal error: Oops: 817 [#1] PREEMPT
> Modules linked in:
> CPU: 0    Not tainted  (2.6.24-rc5-rt1 #37)
> PC is at dma_cache_maint+0x40/0x80
> LR is at atmel_spi_transfer+0x94/0x178
> pc : [<c002488c>]    lr : [<c013eedc>]    psr: 20000013
> sp : c044db84  ip : c044db94  fp : c044db90
> r10: ffffffff  r9 : 00000000  r8 : c04e4c00
> r7 : c03ee310  r6 : c044dcfc  r5 : c109d3bc  r4 : c044dcd8
> r3 : 00000000  r2 : 00000001  r1 : c109d7dc  r0 : c109d3bc
> Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> Control: 0005317f  Table: 20588000  DAC: 00000017
> Process jffs2_gcd_mtd1 (pid: 313, stack limit = 0xc044c258)
> Stack: (0xc044db84 to 0xc044e000)
> ...
> Backtrace:
> [<c002484c>] (dma_cache_maint+0x0/0x80) from [<c013eedc>] (atmel_spi_transfer+0x94/0x178)
> [<c013ee48>] (atmel_spi_transfer+0x0/0x178) from [<c013e124>] (spi_sync+0x74/0x98)
> [<c013e0b0>] (spi_sync+0x0/0x98) from [<c013dcb0>] (dataflash_write+0x1b0/0x270)
>  r8:000014bf r7:00000420 r6:c0446000 r5:00000420 r4:00a5f800
> [<c013db00>] (dataflash_write+0x0/0x270) from [<c013a00c>] (part_write+0xa8/0xb0)
> [<c0139f64>] (part_write+0x0/0xb0) from [<c00e0724>] (jffs2_flash_writev+0x278/0x434)
>  r6:c04d9000 r5:00000420 r4:00000420
> [<c00e04b0>] (jffs2_flash_writev+0x4/0x434) from [<c00e1f40>] (jffs2_sum_write_sumnode+0x334/0x420)
> [<c00e1c0c>] (jffs2_sum_write_sumnode+0x0/0x420) from [<c00d5ca0>] (jffs2_do_reserve_space+0x94/0x3c8)
> [<c00d5c0c>] (jffs2_do_reserve_space+0x0/0x3c8) from [<c00d6014>] (jffs2_reserve_space_gc+0x40/0x78)
> [<c00d5fd4>] (jffs2_reserve_space_gc+0x0/0x78) from [<c00da938>] (jffs2_garbage_collect_pristine+0x5c/0x3a8)
> [<c00da8dc>] (jffs2_garbage_collect_pristine+0x0/0x3a8) from [<c00dc32c>] (jffs2_garbage_collect_pass+0x590/0x714)
> [<c00dbd9c>] (jffs2_garbage_collect_pass+0x0/0x714) from [<c00dd730>] (jffs2_garbage_collect_thread+0x100/0x18c)
> [<c00dd630>] (jffs2_garbage_collect_thread+0x0/0x18c) from [<c0039818>] (do_exit+0x0/0x73c)
> Code: 9a000001 e15c0003 3a000001 e3a03000 (e5833000)
> 

--- a/fs/jffs2/summary.c~jffs2-summary-allocation
+++ a/fs/jffs2/summary.c
@@ -17,7 +17,6 @@
 #include <linux/pagemap.h>
 #include <linux/crc32.h>
 #include <linux/compiler.h>
-#include <linux/vmalloc.h>
 #include "nodelist.h"
 #include "debug.h"
 
@@ -30,7 +29,7 @@ int jffs2_sum_init(struct jffs2_sb_info 
 		return -ENOMEM;
 	}
 
-	c->summary->sum_buf = vmalloc(c->sector_size);
+	c->summary->sum_buf = kmalloc(c->sector_size, GFP_KERNEL);
 
 	if (!c->summary->sum_buf) {
 		JFFS2_WARNING("Can't allocate buffer for writing out summary information!\n");
@@ -49,7 +48,7 @@ void jffs2_sum_exit(struct jffs2_sb_info
 
 	jffs2_sum_disable_collecting(c->summary);
 
-	vfree(c->summary->sum_buf);
+	kfree(c->summary->sum_buf);
 	c->summary->sum_buf = NULL;
 
 	kfree(c->summary);
_

All this does is switch sum_buf from vmalloced-memory over to
kmalloced-memory.

I'm assuming from the trace that the arm code tried to put that memory
under DMA (or at least, passed it into part of the DMA management code to
get the various caches sorted out) and that the arm DMA support code
doesn't like being given vmalloced memory.

So the question is: who is wrong here?  Is jffs wrong to use vmalloced
memory in this application, or is arm wrong to not handle it?

Re: [PATCH] jffs2 summary allocation

[Russell King - ARM Linux]

On Fri, Apr 04, 2008 at 12:48:12PM -0700, Andrew Morton wrote:
> On Fri, 4 Apr 2008 10:23:55 +0000 (GMT)
> Michael Trimarchi <trimarchimichael@yahoo.it> wrote:
> 
> > Hi,
> > 
> > I apply this patch to fix this oops.
> > 
> > 
> > Unable to handle kernel NULL pointer dereference at virtual address 00000000
> > pgd = c0004000
> > [00000000] *pgd=00000000
> > stopped custom tracer.
> > Internal error: Oops: 817 [#1] PREEMPT
> > Modules linked in:
> > CPU: 0    Not tainted  (2.6.24-rc5-rt1 #37)
> > PC is at dma_cache_maint+0x40/0x80
> > LR is at atmel_spi_transfer+0x94/0x178
> > pc : [<c002488c>]    lr : [<c013eedc>]    psr: 20000013
> > sp : c044db84  ip : c044db94  fp : c044db90
> > r10: ffffffff  r9 : 00000000  r8 : c04e4c00
> > r7 : c03ee310  r6 : c044dcfc  r5 : c109d3bc  r4 : c044dcd8
> > r3 : 00000000  r2 : 00000001  r1 : c109d7dc  r0 : c109d3bc
> > Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> > Control: 0005317f  Table: 20588000  DAC: 00000017
> > Process jffs2_gcd_mtd1 (pid: 313, stack limit = 0xc044c258)
> > Stack: (0xc044db84 to 0xc044e000)
> > ...
> > Backtrace:
> > [<c002484c>] (dma_cache_maint+0x0/0x80) from [<c013eedc>] (atmel_spi_transfer+0x94/0x178)
> > [<c013ee48>] (atmel_spi_transfer+0x0/0x178) from [<c013e124>] (spi_sync+0x74/0x98)
> > [<c013e0b0>] (spi_sync+0x0/0x98) from [<c013dcb0>] (dataflash_write+0x1b0/0x270)
> >  r8:000014bf r7:00000420 r6:c0446000 r5:00000420 r4:00a5f800
> > [<c013db00>] (dataflash_write+0x0/0x270) from [<c013a00c>] (part_write+0xa8/0xb0)
> > [<c0139f64>] (part_write+0x0/0xb0) from [<c00e0724>] (jffs2_flash_writev+0x278/0x434)
> >  r6:c04d9000 r5:00000420 r4:00000420
> > [<c00e04b0>] (jffs2_flash_writev+0x4/0x434) from [<c00e1f40>] (jffs2_sum_write_sumnode+0x334/0x420)
> > [<c00e1c0c>] (jffs2_sum_write_sumnode+0x0/0x420) from [<c00d5ca0>] (jffs2_do_reserve_space+0x94/0x3c8)
> > [<c00d5c0c>] (jffs2_do_reserve_space+0x0/0x3c8) from [<c00d6014>] (jffs2_reserve_space_gc+0x40/0x78)
> > [<c00d5fd4>] (jffs2_reserve_space_gc+0x0/0x78) from [<c00da938>] (jffs2_garbage_collect_pristine+0x5c/0x3a8)
> > [<c00da8dc>] (jffs2_garbage_collect_pristine+0x0/0x3a8) from [<c00dc32c>] (jffs2_garbage_collect_pass+0x590/0x714)
> > [<c00dbd9c>] (jffs2_garbage_collect_pass+0x0/0x714) from [<c00dd730>] (jffs2_garbage_collect_thread+0x100/0x18c)
> > [<c00dd630>] (jffs2_garbage_collect_thread+0x0/0x18c) from [<c0039818>] (do_exit+0x0/0x73c)
> > Code: 9a000001 e15c0003 3a000001 e3a03000 (e5833000)
> > 
> 
> --- a/fs/jffs2/summary.c~jffs2-summary-allocation
> +++ a/fs/jffs2/summary.c
> @@ -17,7 +17,6 @@
>  #include <linux/pagemap.h>
>  #include <linux/crc32.h>
>  #include <linux/compiler.h>
> -#include <linux/vmalloc.h>
>  #include "nodelist.h"
>  #include "debug.h"
>  
> @@ -30,7 +29,7 @@ int jffs2_sum_init(struct jffs2_sb_info 
>  		return -ENOMEM;
>  	}
>  
> -	c->summary->sum_buf = vmalloc(c->sector_size);
> +	c->summary->sum_buf = kmalloc(c->sector_size, GFP_KERNEL);
>  
>  	if (!c->summary->sum_buf) {
>  		JFFS2_WARNING("Can't allocate buffer for writing out summary information!\n");
> @@ -49,7 +48,7 @@ void jffs2_sum_exit(struct jffs2_sb_info
>  
>  	jffs2_sum_disable_collecting(c->summary);
>  
> -	vfree(c->summary->sum_buf);
> +	kfree(c->summary->sum_buf);
>  	c->summary->sum_buf = NULL;
>  
>  	kfree(c->summary);
> _
> 
> All this does is switch sum_buf from vmalloced-memory over to
> kmalloced-memory.
> 
> I'm assuming from the trace that the arm code tried to put that memory
> under DMA (or at least, passed it into part of the DMA management code to
> get the various caches sorted out) and that the arm DMA support code
> doesn't like being given vmalloced memory.
> 
> So the question is: who is wrong here?  Is jffs wrong to use vmalloced
> memory in this application, or is arm wrong to not handle it?

If ARM is wrong, we're going to have to walk page tables and all that
mess so that we can perform L2 cache maintainence for DMA... which
becomes quite expensive and I believe starts to make DMA pointless on
ARM CPUs.

The DMA docs say this of dma_map_coherent():

  Notes:  Not all memory regions in a machine can be mapped by this
  API.  Further, regions that appear to be physically contiguous in
  kernel virtual space may not be contiguous as physical memory.  Since
  this API does not provide any scatter/gather capability, it will fail
  if the user tries to map a non-physically contiguous piece of memory.
  For this reason, it is recommended that memory mapped by this API be
  obtained only from sources which guarantee it to be physically contiguous
  (like kmalloc).

vmalloc memory is not physically contiguous, so the attempt to create
the mapping fails.

Re: [PATCH] jffs2 summary allocation

[David Brownell]

On Friday 04 April 2008, Andrew Morton wrote:

> I'm assuming from the trace that the arm code tried to put that memory
> under DMA (or at least, passed it into part of the DMA management code to
> get the various caches sorted out) and that the arm DMA support code
> doesn't like being given vmalloced memory.

Actually, Documentation/DMA-Mapping.txt has a section right up
front called "What memory is DMA'able?" ... which despite its
ungrammatical title, says clearly:

  ... This means specifically that you may _not_ use the
  memory/addresses returned from vmalloc() for DMA.  ...

So I'm rather surprised to see *ANY* kernel code trying to do
that.  That rule has been in effect for many, many years now.

- Dave

Re: [PATCH] jffs2 summary allocation

[Josh Boyer]

On Fri, 2008-04-04 at 16:09 -0700, David Brownell wrote:
> On Friday 04 April 2008, Andrew Morton wrote:
> 
> > I'm assuming from the trace that the arm code tried to put that memory
> > under DMA (or at least, passed it into part of the DMA management code to
> > get the various caches sorted out) and that the arm DMA support code
> > doesn't like being given vmalloced memory.
> 
> Actually, Documentation/DMA-Mapping.txt has a section right up
> front called "What memory is DMA'able?" ... which despite its
> ungrammatical title, says clearly:
> 
>   ... This means specifically that you may _not_ use the
>   memory/addresses returned from vmalloc() for DMA.  ...
> 
> So I'm rather surprised to see *ANY* kernel code trying to do
> that.  That rule has been in effect for many, many years now.

I don't think it was intentional.  You're going through several layers
here:

JFFS2 -> mtd parts -> mtd dataflash -> atmel_spi.

Typically MTD drivers aren't doing DMAs to flash and JFFS2 has no idea
which particular chip driver is being used because it's abstracted by
MTD.

josh

Re: [PATCH] jffs2 summary allocation

[David Brownell]

On Friday 04 April 2008, Josh Boyer wrote:
> 
> >   ... This means specifically that you may _not_ use the
> >   memory/addresses returned from vmalloc() for DMA.  ...
> > 
> > So I'm rather surprised to see *ANY* kernel code trying to do
> > that.  That rule has been in effect for many, many years now.
> 
> I don't think it was intentional.  You're going through several layers
> here:
> 
> JFFS2 -> mtd parts -> mtd dataflash -> atmel_spi.
> 
> Typically MTD drivers aren't doing DMAs to flash and JFFS2 has no idea
> which particular chip driver is being used because it's abstracted by
> MTD.

That's true ... although I can imagine using DMA to
avoid dcache trashing if its setup cost is low enough,
with either NAND or NOR chips.

Still:  in this context vmalloc() is wrong.

- Dave

Re: [PATCH] jffs2 summary allocation

[Josh Boyer]

On Fri, 2008-04-04 at 16:58 -0700, David Brownell wrote:
> On Friday 04 April 2008, Josh Boyer wrote:
> > 
> > >   ... This means specifically that you may _not_ use the
> > >   memory/addresses returned from vmalloc() for DMA.  ...
> > > 
> > > So I'm rather surprised to see *ANY* kernel code trying to do
> > > that.  That rule has been in effect for many, many years now.
> > 
> > I don't think it was intentional.  You're going through several layers
> > here:
> > 
> > JFFS2 -> mtd parts -> mtd dataflash -> atmel_spi.
> > 
> > Typically MTD drivers aren't doing DMAs to flash and JFFS2 has no idea
> > which particular chip driver is being used because it's abstracted by
> > MTD.
> 
> That's true ... although I can imagine using DMA to
> avoid dcache trashing if its setup cost is low enough,
> with either NAND or NOR chips.
> 
> Still:  in this context vmalloc() is wrong.

Agreed.  One issue is that the summary code allocates a buffer that
equals the eraseblock size of the underlying MTD device.  For larger
NAND chips, that may be up to 256KiB.  I believe this is within the
allowable kmalloc size for most architectures these days, but the
summary code is 3 years old and was likely expecting a smaller limit.
And there is always the question on whether finding that much contiguous
memory will be an issue.

I don't see much harm with the actual patch itself, assuming larger
kmallocs work as I think they should.  It does make me wonder if we have
other cases of vmalloc'd buffers being passed to lower drivers using DMA
though.

josh

Re: [PATCH] jffs2 summary allocation

[Kyungmin Park]

On Sat, Apr 5, 2008 at 10:11 AM, Josh Boyer <jwboyer@gmail.com> wrote:
> On Fri, 2008-04-04 at 16:58 -0700, David Brownell wrote:
>  > On Friday 04 April 2008, Josh Boyer wrote:
>  > >
>  > > >   ... This means specifically that you may _not_ use the
>  > > >   memory/addresses returned from vmalloc() for DMA.  ...
>  > > >
>  > > > So I'm rather surprised to see *ANY* kernel code trying to do
>  > > > that.  That rule has been in effect for many, many years now.
>  > >
>  > > I don't think it was intentional.  You're going through several layers
>  > > here:
>  > >
>  > > JFFS2 -> mtd parts -> mtd dataflash -> atmel_spi.
>  > >
>  > > Typically MTD drivers aren't doing DMAs to flash and JFFS2 has no idea
>  > > which particular chip driver is being used because it's abstracted by
>  > > MTD.
>  >
>  > That's true ... although I can imagine using DMA to
>  > avoid dcache trashing if its setup cost is low enough,
>  > with either NAND or NOR chips.
>  >
>  > Still:  in this context vmalloc() is wrong.
>
>  Agreed.  One issue is that the summary code allocates a buffer that
>  equals the eraseblock size of the underlying MTD device.  For larger
>  NAND chips, that may be up to 256KiB.  I believe this is within the
>  allowable kmalloc size for most architectures these days, but the
>  summary code is 3 years old and was likely expecting a smaller limit.
>  And there is always the question on whether finding that much contiguous
>  memory will be an issue.

In MLC chips it goes up to 512KiB. It means it can't allocate the
eraseblock size memory with kmalloc().
In ARM environment I can't see the 256KiB or more memory allocation
with kmalloc().
So I now changed the kmalloc eraseblock to vmalloc at both jffs2 and mtd-utils.

Thank you,
Kyungmin Park

Re: [PATCH] jffs2 summary allocation

[Andrew Morton]

On Sat, 5 Apr 2008 10:29:25 +0900 "Kyungmin Park" <kmpark@infradead.org> wrote:

> On Sat, Apr 5, 2008 at 10:11 AM, Josh Boyer <jwboyer@gmail.com> wrote:
> > On Fri, 2008-04-04 at 16:58 -0700, David Brownell wrote:
> >  > On Friday 04 April 2008, Josh Boyer wrote:
> >  > >
> >  > > >   ... This means specifically that you may _not_ use the
> >  > > >   memory/addresses returned from vmalloc() for DMA.  ...
> >  > > >
> >  > > > So I'm rather surprised to see *ANY* kernel code trying to do
> >  > > > that.  That rule has been in effect for many, many years now.
> >  > >
> >  > > I don't think it was intentional.  You're going through several layers
> >  > > here:
> >  > >
> >  > > JFFS2 -> mtd parts -> mtd dataflash -> atmel_spi.
> >  > >
> >  > > Typically MTD drivers aren't doing DMAs to flash and JFFS2 has no idea
> >  > > which particular chip driver is being used because it's abstracted by
> >  > > MTD.
> >  >
> >  > That's true ... although I can imagine using DMA to
> >  > avoid dcache trashing if its setup cost is low enough,
> >  > with either NAND or NOR chips.
> >  >
> >  > Still:  in this context vmalloc() is wrong.
> >
> >  Agreed.  One issue is that the summary code allocates a buffer that
> >  equals the eraseblock size of the underlying MTD device.  For larger
> >  NAND chips, that may be up to 256KiB.  I believe this is within the
> >  allowable kmalloc size for most architectures these days, but the
> >  summary code is 3 years old and was likely expecting a smaller limit.
> >  And there is always the question on whether finding that much contiguous
> >  memory will be an issue.

Yes.  This is why I'm reluctant to whizz this patch into 2.6.25.  It'll
break more than it fixes.

> In MLC chips it goes up to 512KiB. It means it can't allocate the
> eraseblock size memory with kmalloc().
> In ARM environment I can't see the 256KiB or more memory allocation
> with kmalloc().
> So I now changed the kmalloc eraseblock to vmalloc at both jffs2 and mtd-utils.

Does this eraseblock really really really need to be a single
virtually-contiguous hunk of kernel memory?  Or was that just easy to do at
the time?



This problem comes up pretty often.  Rather than open-coding it yet again
it'd be nice to have a little bit of library code which manages an array of
pages and which has accessors for common operations like
read/write-u8/u16/u32/u64, memset, memcpy, etc.

Then again, given that this memory is often fed into IO subsystems, perhaps
we should do this by adding more accessors and helpers to
scatterlists/sg_table.  Unfortunately they're not presently well set up for
random access.

Re: [PATCH] jffs2 summary allocation

[David Brownell]

On Friday 04 April 2008, Andrew Morton wrote:
> This problem comes up pretty often. 

Which problem -- kmalloc(BIG)?  Or dma(dma-unsafe-mem)?
Or something else?

The specific issue here might best be described as JFFS2
making a bad assumption:  that MTD drivers never use DMA.
The I2C stack does that in some cases (for example, I/O
buffers in i2c_smbus_xfer_emulated are on-stack), but I'd
not call that an especially common assumption.


> Rather than open-coding it yet again 
> it'd be nice to have a little bit of library code which manages an array of
> pages and which has accessors for common operations like
> read/write-u8/u16/u32/u64, memset, memcpy, etc.

If array-of-pages is to be more widely adopted, that'd
make sense.  The MTD framework is a bit odd in that
respect ... it has block devices but doesn't use the
scatterlist primitives.

- Dave

Re: [PATCH] jffs2 summary allocation

[Andrew Morton]

On Fri, 4 Apr 2008 19:41:34 -0700 David Brownell <david-b@pacbell.net> wrote:

> On Friday 04 April 2008, Andrew Morton wrote:
> > This problem comes up pretty often. 
> 
> Which problem -- kmalloc(BIG)?  Or dma(dma-unsafe-mem)?
> Or something else?

Code needing a large amount of memory, trying to kmalloc it, then getting
into trouble, them using vmalloc, then getting into more trouble.

This is kernel - order-0 allocations will always be the most robust.

Re: [PATCH] jffs2 summary allocation

[David Brownell]

On Friday 04 April 2008, Kyungmin Park wrote:
> So I now changed the kmalloc eraseblock to vmalloc at both jffs2 and mtd-utils.

I hope you meant: vmalloc changed to kmalloc?

And mtd-utils is userspace, not kernel, so it can't care...