ARM: OMAP: OneNAND support for 2430SDP

RFC/PATCH Kevin Hilman 4 participants 12 messages Mar 21, 2007 – May 2, 2007

Subsystems (6)

Arm (32-bit) Arm sub-architectures Memory technology devices (MTD) OMAP2+ support +2 more subsystems

lore

Share & tools

lore

Kevin Hilman

author

2007-03-21 00:40 UTC #0 lore raw

Adds platform device for use of existing OMAP2 driver. Also, the following changes are made to the existing driver:

- add option for not using DMA (by passing dma_channel = -1)
- removes obsoleted pt_regs arg from interrupt handler

RFC: Notice the change to onenand_base.c which comments out the last
OOB chunk. Without this, it would not work. Previous drivers for this board such as the 2.6.10-based MV kernel have this same change. Anyone know what is going on here?

Signed-off-by Kevin Hilman <[email protected]>

---
 arch/arm/mach-omap2/Makefile              |    3 
 arch/arm/mach-omap2/board-2430sdp-flash.c |  108 ++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-2430sdp.c       |    4 +
 arch/arm/mach-omap2/gpmc.c                |    1 
 drivers/mtd/onenand/omap2.c               |  103 +++++++++++++++++-----------
 drivers/mtd/onenand/onenand_base.c        |    2 
 include/asm-arm/arch-omap/gpmc.h          |    7 +
 include/asm-arm/arch-omap/onenand.h       |    1 
 8 files changed, 187 insertions(+), 42 deletions(-)

arch/arm/mach-omap2/Makefile +2 -1

Index: dev/arch/arm/mach-omap2/Makefile
===================================================================
@@ -20,7 +20,8 @@ mmu_mach-objs			:= mmu.o
 # Specific board support
 obj-$(CONFIG_MACH_OMAP_GENERIC)		+= board-generic.o
 obj-$(CONFIG_MACH_OMAP_H4)		+= board-h4.o
-obj-$(CONFIG_MACH_OMAP_2430SDP)		+= board-2430sdp.o
+obj-$(CONFIG_MACH_OMAP_2430SDP)		+= board-2430sdp.o \
+					   board-2430sdp-flash.o
 obj-$(CONFIG_MACH_OMAP_APOLLON)		+= board-apollon.o
 obj-$(CONFIG_MACH_NOKIA_N800)		+= board-n800.o board-n800-flash.o \
 					   board-n800-mmc.o board-n800-bt.o \

arch/arm/mach-omap2/board-2430sdp-flash.c new file +108 -0

108 lines not on this page · raw message

arch/arm/mach-omap2/board-2430sdp.c +4 -0

Index: dev/arch/arm/mach-omap2/board-2430sdp.c
===================================================================
@@ -192,12 +192,16 @@ static struct omap_board_config_kernel s
 	{OMAP_TAG_UART, &sdp2430_uart_config},
 };
 
+extern void __init sdp2430_flash_init(void);
+
 static void __init omap_2430sdp_init(void)
 {
 	platform_add_devices(sdp2430_devices, ARRAY_SIZE(sdp2430_devices));
 	omap_board_config = sdp2430_config;
 	omap_board_config_size = ARRAY_SIZE(sdp2430_config);
 	omap_serial_init();
+
+	sdp2430_flash_init();
 }
 
 static void __init omap_2430sdp_map_io(void)

arch/arm/mach-omap2/gpmc.c +0 -1

Index: dev/arch/arm/mach-omap2/gpmc.c
===================================================================
@@ -51,7 +51,6 @@
 #define GPMC_CS0		0x60
 #define GPMC_CS_SIZE		0x30
 
-#define GPMC_CS_NUM		8
 #define GPMC_MEM_START		0x00000000
 #define GPMC_MEM_END		0x3FFFFFFF
 #define BOOT_ROM_SPACE		0x100000	/* 1MB */

drivers/mtd/onenand/omap2.c +64 ~~-39~~

146 lines not on this page · raw message

drivers/mtd/onenand/onenand_base.c +1 -1

Index: dev/drivers/mtd/onenand/onenand_base.c
===================================================================
@@ -39,7 +39,7 @@ static struct nand_ecclayout onenand_oob
 		},
 	.oobfree	= {
 		{2, 3}, {14, 2}, {18, 3}, {30, 2},
-		{34, 3}, {46, 2}, {50, 3}, {62, 2}
+		{34, 3}, {46, 2}, {50, 3}, /* {62, 2} */
 	}
 };

include/asm-arm/arch-omap/gpmc.h +7 -0

Index: dev/include/asm-arm/arch-omap/gpmc.h
===================================================================
@@ -11,6 +11,9 @@
 #ifndef __OMAP2_GPMC_H
 #define __OMAP2_GPMC_H
 
+/* Maximum Number of Chip Selects */
+#define GPMC_CS_NUM		8
+
 #define GPMC_CS_CONFIG1		0x00
 #define GPMC_CS_CONFIG2		0x04
 #define GPMC_CS_CONFIG3		0x08

@@ -22,6 +25,10 @@
 #define GPMC_CS_NAND_ADDRESS	0x20
 #define GPMC_CS_NAND_DATA	0x24
 
+#define GPMC_CONFIG		0x50
+#define GPMC_STATUS		0x54
+#define GPMC_CONFIG1_0		0x60
+
 #define GPMC_CONFIG1_WRAPBURST_SUPP     (1 << 31)
 #define GPMC_CONFIG1_READMULTIPLE_SUPP  (1 << 30)
 #define GPMC_CONFIG1_READTYPE_ASYNC     (0 << 29)

include/asm-arm/arch-omap/onenand.h +1 -0

Index: dev/include/asm-arm/arch-omap/onenand.h
===================================================================
@@ -17,4 +17,5 @@ struct omap_onenand_platform_data {
 	struct mtd_partition	*parts;
 	int			nr_parts;
 	int                     (*onenand_setup)(void __iomem *);
+	int			dma_channel;
 };

Jalori, Mohit

2007-03-21 19:15 UTC #1 lore raw

Quoted 4 lines

-----Original Message-----
From: [email protected]

[mailto:linux-omap-

Quoted 8 lines

[email protected]] On Behalf Of Kevin Hilman
Sent: Tuesday, March 20, 2007 7:41 PM
To: [email protected]
Subject: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP
Adds platform device for use of existing OMAP2 driver. Also, the following changes are made to the existing driver:

Quoted 8 lines

- add option for not using DMA (by passing dma_channel = -1)
- removes obsoleted pt_regs arg from interrupt handler
RFC: Notice the change to onenand_base.c which comments out the last
OOB chunk. Without this, it would not work. Previous drivers for this board such as the 2.6.10-based MV kernel have this same change. Anyone know what is going on here?

We got this change from MV in the patch he sent to fix an issue we were seeing with mounting an empty jffs2 partition. Here are the comments that came in with it

" The oobfree list in the MTD OneNAND driver is of length 8. This exceeds the maximum length of 7, because the eighth entry must be used as a null terminator. The absence of a terminator causes a variety of problems in various places (especially JFFS2 and YAFFS2) where the oobfree list is scanned. The fix is to do without the eighth entry.

This fix is only of local interest because the handling of oobinfo has been completely redesigned as of 2.6.17-rc6. "

Quoted 1 line

From the last line we should not need this for later kernel versions.

Regards
Mohit

Kevin Hilman

author

2007-03-21 19:50 UTC #2 lore raw

Quoted 10 lines · Mohit

Quoted 5 lines
RFC: Notice the change to onenand_base.c which comments out the last
OOB chunk. Without this, it would not work. Previous drivers for this board such as the 2.6.10-based MV kernel have this same change. Anyone know what is going on here?
We got this change from MV in the patch he sent to fix an issue we were seeing with mounting an empty jffs2 partition. Here are the comments that came in with it

Quoted 8 lines

" The oobfree list in the MTD OneNAND driver is of length 8. This exceeds the maximum length of 7, because the eighth entry must be used as a null terminator. The absence of a terminator causes a variety of problems in various places (especially JFFS2 and YAFFS2) where the oobfree list is scanned. The fix is to do without the eighth entry.
This fix is only of local interest because the handling of oobinfo has been completely redesigned as of 2.6.17-rc6. "

This problem still exists in the current OMAP tree (2.6.21-rc4), so the MTD driver still needs some work.

I will work to get a fix for this submitted upstream. In the mean-time, my patch is required for OneNAND to work in the current git tree.

Kevin

Kyungmin Park

2007-03-22 01:06 UTC #3 lore raw

Quoted 14 lines

Quoted 2 lines
Quoted 2 lines
RFC: Notice the change to onenand_base.c which comments
out the last
Quoted 1 line
Quoted 1 line
OOB chunk. Without this, it would not work.
Previous drivers
Quoted 5 lines
Quoted 3 lines
for this board such as the 2.6.10-based MV kernel have this same change. Anyone know what is going on here?
We got this change from MV in the patch he sent to fix an
issue we were
Quoted 3 lines
seeing with mounting an empty jffs2 partition. Here are the comments that came in with it

Quoted 8 lines

Quoted 2 lines
" The oobfree list in the MTD OneNAND driver is of length 8. This exceeds the maximum length of 7, because the eighth entry
must be used
Quoted 1 line
as a null terminator. The absence of a terminator causes a
variety of
Quoted 2 lines
problems in various places (especially JFFS2 and YAFFS2) where the oobfree list is scanned. The fix is to do without the
eighth entry.

Colud you point out the source code line where used the eightn entry used as a null terminator?

In linux-2.6.15-omap they also allocate 8 oobfree.

struct nand_oobinfo {
        uint32_t useecc;
        uint32_t eccbytes;
        uint32_t oobfree[8][2];
        uint32_t eccpos[32];
};

Quoted 8 lines

Quoted 2 lines
This fix is only of local interest because the handling of
oobinfo has
Quoted 1 line
been completely redesigned as of 2.6.17-rc6. "
This problem still exists in the current OMAP tree (2.6.21-rc4), so the
MTD driver still needs some work.

It's some strange. I'm always working with omap tree with OneNAND support. Of course it's working well with latest omap tree. The apollon board can boot and use the jffs2 on OneNAND.

There are two cases using OOB.
1. OOB_AUTO
2. OOB_PLACE
The recent change in JFFS2 which uses the OOB_AUTO.

In case 1, it's only used the first oobfree in JFFS2. So they don't use the last oobfree
In case 2, it uses the full oobfree. but actually it used 12 bytes in jffs2. it means it used 5 oobfree

Quoted 4 lines

I will work to get a fix for this submitted upstream. In the mean-time,
my patch is required for OneNAND to work in the current git tree.

Could you send the debug message or error mesage when it is used without oobfree patch?

Thank you,
Kyungmin Park

Kevin Hilman

author

2007-03-22 03:14 UTC #4 lore raw

Quoted 15 lines · Kyungmin Park

Quoted 15 lines
Quoted 1 line
Quoted 1 line
RFC: Notice the change to onenand_base.c which comments
out the last
Quoted 1 line
Quoted 1 line
OOB chunk. Without this, it would not work.
Previous drivers
Quoted 4 lines
Quoted 3 lines
for this board such as the 2.6.10-based MV kernel have this same change. Anyone know what is going on here?
We got this change from MV in the patch he sent to fix an
issue we were
Quoted 5 lines
seeing with mounting an empty jffs2 partition. Here are the comments that came in with it
" The oobfree list in the MTD OneNAND driver is of length 8. This exceeds the maximum length of 7, because the eighth entry
must be used

Quoted 9 lines

Quoted 5 lines
Quoted 1 line
as a null terminator. The absence of a terminator causes a
variety of
Quoted 2 lines
problems in various places (especially JFFS2 and YAFFS2) where the oobfree list is scanned. The fix is to do without the
eighth entry.
Colud you point out the source code line where used the eightn entry used as a null terminator?

There are several loops in both the NAND and the OneNAND code where the oobfree list is walked and the loop only terminates when it finds a NULL entry (specifically, one with a zero length.)

For example, look in onenand_fill_auto_oob() where you have a couple loops like this:

    for (free = this->ecclayout->oobfree; free->length; ++free) {
    }

In the case where there is 8 non-zero entries in the oobfree list, this loop will walk off the end of the list before it terminates, and having undefined results depending on what is after the oobfree list in memory.

Instead, these loops should probably loop over MTD_MAX_OOBFREE_ENTRIES, and break out early if they find a zero-length entry.

I'm not sure why this works for you. For me, here's a simple way to trigger the bug using the OneNAND device on the OMAP2430SDP:

# flash_eraseall /dev/mtd8
Erasing 128 Kibyte @ 7e80000 -- 99 % complete.
# mount -t jffs2 /dev/mtdblock8 /media/onenand
Cowardly refusing to erase blocks on filesystem with no valid JFFS2 nodes
empty_blocks 0, bad_blocks 0, c->nr_blocks 1013
mount: /dev/mtdblock8: can't read superblock

Kevin

Kyungmin Park

2007-03-22 04:50 UTC #5 lore raw

Quoted 56 lines

-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Thursday, March 22, 2007 12:14 PM
To: [email protected]
Cc: 'Jalori, Mohit'; [email protected]
Subject: Re: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP
Quoted 10 lines · Kyungmin Park
Quoted 10 lines
Quoted 1 line
Quoted 1 line
RFC: Notice the change to onenand_base.c which comments
out the last
Quoted 1 line
Quoted 1 line
OOB chunk. Without this, it would not work.
Previous drivers
Quoted 4 lines
Quoted 3 lines
for this board such as the 2.6.10-based MV kernel have this same change. Anyone know what is going on here?
We got this change from MV in the patch he sent to fix an
issue we were
Quoted 1 line
seeing with mounting an empty jffs2 partition. Here are
the comments
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
that came in with it
" The oobfree list in the MTD OneNAND driver is of length 8. This exceeds the maximum length of 7, because the eighth entry
Quoted 8 lines
Quoted 6 lines
must be used
Quoted 1 line
as a null terminator. The absence of a terminator causes a
variety of
Quoted 2 lines
problems in various places (especially JFFS2 and YAFFS2) where the oobfree list is scanned. The fix is to do without the
eighth entry.
Colud you point out the source code line where used the
eightn entry used as a
Quoted 2 lines
null terminator?
There are several loops in both the NAND and the OneNAND code where the
oobfree list is walked and the loop only terminates when it finds a NULL
entry (specifically, one with a zero length.)
For example, look in onenand_fill_auto_oob() where you have a couple loops like this:
    for (free = this->ecclayout->oobfree; free->length; ++free) {
    }
In the case where there is 8 non-zero entries in the oobfree list, this
loop will walk off the end of the list before it terminates, and having
undefined results depending on what is after the oobfree list in memory.

Quoted 3 lines

Instead, these loops should probably loop over MTD_MAX_OOBFREE_ENTRIES,
and break out early if they find a zero-length entry.

Yes, you're right. It met the same problem in the previous mail [1]. Could you apply patch [2] and test again. it will be work.

Anyway I will fix it to check array length.

But I wonder it is only affected with OOB_AUTO. however the linux 2.6.10 kernel don't use OOB_AUTO. but you said it has same problem.
Is there another point?

Quoted 6 lines

I'm not sure why this works for you. For me, here's a simple way to trigger the bug using the OneNAND device on the OMAP2430SDP:

Quoted 8 lines

# flash_eraseall /dev/mtd8
Erasing 128 Kibyte @ 7e80000 -- 99 % complete.
# mount -t jffs2 /dev/mtdblock8 /media/onenand
Cowardly refusing to erase blocks on filesystem with no valid 
JFFS2 nodes
empty_blocks 0, bad_blocks 0, c->nr_blocks 1013
mount: /dev/mtdblock8: can't read superblock

Thank you,
Kyungmin Park

1. http://lists.infradead.org/pipermail/linux-mtd/2007-February/017418.html
2. http://git.infradead.org/?p=users/kmpark/onenand-mtd-
2.6.git;a=commitdiff;h=5bc399e9ef430efd5725b66aa2ad7ad2d81e372b;hp=81280d5879761 f90b3a341d52371d03998730d8e

Kyungmin Park

2007-03-22 05:02 UTC #6 lore raw

Quoted 51 lines

Quoted 7 lines
Quoted 5 lines · Kyungmin Park
Quoted 5 lines
Quoted 1 line
Quoted 1 line
RFC: Notice the change to onenand_base.c which comments
out the last
Quoted 1 line
Quoted 1 line
OOB chunk. Without this, it would not work.
Previous drivers
Quoted 1 line
Quoted 1 line
for this board such as the 2.6.10-based MV kernel
have this
Quoted 9 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
same change. Anyone know what is going on here?
We got this change from MV in the patch he sent to fix an
issue we were
Quoted 1 line
seeing with mounting an empty jffs2 partition. Here are
the comments
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that came in with it
" The oobfree list in the MTD OneNAND driver is of
length 8. This
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
exceeds the maximum length of 7, because the eighth entry
must be used
Quoted 1 line
as a null terminator. The absence of a terminator causes a
variety of
Quoted 1 line
problems in various places (especially JFFS2 and
YAFFS2) where the
Quoted 27 lines
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
oobfree list is scanned. The fix is to do without the
eighth entry.
Colud you point out the source code line where used the
eightn entry used as a
Quoted 2 lines
null terminator?
There are several loops in both the NAND and the OneNAND code where the
oobfree list is walked and the loop only terminates when it finds a NULL
entry (specifically, one with a zero length.)
For example, look in onenand_fill_auto_oob() where you have a couple loops like this:
    for (free = this->ecclayout->oobfree; free->length; ++free) {
    }
In the case where there is 8 non-zero entries in the oobfree list, this
loop will walk off the end of the list before it terminates, and having
undefined results depending on what is after the oobfree list in memory.

Quoted 8 lines

Quoted 3 lines
Instead, these loops should probably loop over MTD_MAX_OOBFREE_ENTRIES,
and break out early if they find a zero-length entry.
Yes, you're right. It met the same problem in the previous mail [1]. Could you apply patch [2] and test again. it will be work.
Anyway I will fix it to check array length.

Here's patch. Yes I know it's maybe wordwrapped. Sorry :)

(quoted diff) +0 -0

b/drivers/mtd/onenand/onenand_base.c
index 9e14a26..2a88a95 100644

drivers/mtd/onenand/onenand_base.c +0 -0

@@ -836,9 +836,11 @@ static int onenand_transfer_auto_oob(struct mtd_info *mtd,

uint8_t *buf, int col

        int readcol = column;
        int readend = column + thislen;
        int lastgap = 0;
+       unsigned int i;
        uint8_t *oob_buf = this->oob_buf;

-       for (free = this->ecclayout->oobfree; free->length; ++free) {
+       free = this->ecclayout->oobfree;
+       for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++) {

if (readcol >= lastgap)
readcol += free->offset - lastgap;
if (readend >= lastgap)
@@ -846,7 +848,8 @@ static int onenand_transfer_auto_oob(struct mtd_info *mtd, uint8_t *buf, int col

                lastgap = free->offset + free->length;
        }
        this->read_bufferram(mtd, ONENAND_SPARERAM, oob_buf, 0, mtd->oobsize);
-       for (free = this->ecclayout->oobfree; free->length; ++free) {
+       free = this->ecclayout->oobfree;
+       for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++) {
                int free_end = free->offset + free->length;
                if (free->offset < readend && free_end > readcol) {
                        int st = max_t(int,free->offset,readcol);
@@ -1280,15 +1283,18 @@ static int onenand_fill_auto_oob(struct mtd_info *mtd,
u_char *oob_buf,
        int writecol = column;
        int writeend = column + thislen;
        int lastgap = 0;

+ unsigned int i;

-       for (free = this->ecclayout->oobfree; free->length; ++free) {
+       free = this->ecclayout->oobfree;
+       for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++) {
                if (writecol >= lastgap)
                        writecol += free->offset - lastgap;
                if (writeend >= lastgap)
                        writeend += free->offset - lastgap;
                lastgap = free->offset + free->length;
        }
-       for (free = this->ecclayout->oobfree; free->length; ++free) {
+       free = this->ecclayout->oobfree;
+       for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++) {
                int free_end = free->offset + free->length;
                if (free->offset < writeend && free_end > writecol) {
                        int st = max_t(int,free->offset,writecol);

Kevin Hilman

author

2007-03-22 16:53 UTC #7 lore raw

Quoted 55 lines · Kyungmin Park

Quoted 55 lines
Quoted 7 lines
Quoted 5 lines · Kyungmin Park
Quoted 5 lines
Quoted 1 line
Quoted 1 line
RFC: Notice the change to onenand_base.c which comments
out the last
Quoted 1 line
Quoted 1 line
OOB chunk. Without this, it would not work.
Previous drivers
Quoted 1 line
Quoted 1 line
for this board such as the 2.6.10-based MV kernel
have this
Quoted 9 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
same change. Anyone know what is going on here?
We got this change from MV in the patch he sent to fix an
issue we were
Quoted 1 line
seeing with mounting an empty jffs2 partition. Here are
the comments
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that came in with it
" The oobfree list in the MTD OneNAND driver is of
length 8. This
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
exceeds the maximum length of 7, because the eighth entry
must be used
Quoted 1 line
as a null terminator. The absence of a terminator causes a
variety of
Quoted 1 line
problems in various places (especially JFFS2 and
YAFFS2) where the
Quoted 30 lines
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
oobfree list is scanned. The fix is to do without the
eighth entry.
Colud you point out the source code line where used the
eightn entry used as a
Quoted 2 lines
null terminator?
There are several loops in both the NAND and the OneNAND code where the
oobfree list is walked and the loop only terminates when it finds a NULL
entry (specifically, one with a zero length.)
For example, look in onenand_fill_auto_oob() where you have a couple loops like this:
    for (free = this->ecclayout->oobfree; free->length; ++free) {
    }
In the case where there is 8 non-zero entries in the oobfree list, this
loop will walk off the end of the list before it terminates, and having
undefined results depending on what is after the oobfree list in memory.
Instead, these loops should probably loop over MTD_MAX_OOBFREE_ENTRIES,
and break out early if they find a zero-length entry.

Quoted 7 lines

Quoted 4 lines
Yes, you're right. It met the same problem in the previous mail [1]. Could you apply patch [2] and test again. it will be work.
Anyway I will fix it to check array length.
Here's patch. Yes I know it's maybe wordwrapped. Sorry :)

You missed the loop in onenand_scan() which walks the freelist. Patch below covers all the cases, and was tested to work on 2430.

Note that you should raise this on linux-mtd as there are similar loops in the drivers/mtd/nand* code. The problem has never come up because all the drivers have less than 8 oobfree chunks defined.

Kevin

drivers/mtd/onenand/onenand_base.c +3 -1

Index: dev/drivers/mtd/onenand/onenand_base.c
===================================================================
@@ -837,8 +837,10 @@ static int onenand_transfer_auto_oob(str
 	int readend = column + thislen;
 	int lastgap = 0;
 	uint8_t *oob_buf = this->oob_buf;
+	unsigned int i;
 
-	for (free = this->ecclayout->oobfree; free->length; ++free) {
+	free = this->ecclayout->oobfree;
+	for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++)

{
if (readcol >= lastgap)
readcol += free->offset - lastgap;
if (readend >= lastgap)
@@ -846,7 +848,8 @@ static int onenand_transfer_auto_oob(str
lastgap = free->offset + free->length;
}
this->read_bufferram(mtd, ONENAND_SPARERAM, oob_buf, 0, mtd->oobsize);
- for (free = this->ecclayout->oobfree; free->length; ++free) {
+ free = this->ecclayout->oobfree;
+ for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++)
{
int free_end = free->offset + free->length;
if (free->offset < readend && free_end > readcol) {
int st = max_t(int,free->offset,readcol);
@@ -1280,15 +1283,18 @@ static int onenand_fill_auto_oob(struct
int writecol = column;
int writeend = column + thislen;
int lastgap = 0;
+ unsigned int i;

- for (free = this->ecclayout->oobfree; free->length; ++free) {
+ free = this->ecclayout->oobfree;
+ for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++)
{
if (writecol >= lastgap)
writecol += free->offset - lastgap;
if (writeend >= lastgap)
writeend += free->offset - lastgap;
lastgap = free->offset + free->length;
}
- for (free = this->ecclayout->oobfree; free->length; ++free) {
+ free = this->ecclayout->oobfree;
+ for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++)
{
int free_end = free->offset + free->length; if (free->offset < writeend && free_end > writecol) {
int st = max_t(int,free->offset,writecol);
@@ -2386,7 +2392,9 @@ int onenand_scan(struct mtd_info *mtd, i
* the out of band area
*/
this->ecclayout->oobavail = 0;
- for (i = 0; this->ecclayout->oobfree[i].length; i++)
+ for (i = 0;
+ i < MTD_MAX_OOBFREE_ENTRIES &&
this->ecclayout->oobfree[i].length;
+ i++)
this->ecclayout->oobavail +=
this->ecclayout->oobfree[i].length;
mtd->oobavail = this->ecclayout->oobavail;

Kyungmin Park

2007-03-23 04:40 UTC #8 lore raw

Quoted 62 lines

Quoted 33 lines
Quoted 33 lines
Quoted 8 lines
Quoted 5 lines · Kyungmin Park
Quoted 5 lines
Quoted 1 line
Quoted 1 line
RFC: Notice the change to onenand_base.c which comments
out the last
Quoted 1 line
Quoted 1 line
OOB chunk. Without this, it would not work.
Previous drivers
Quoted 1 line
Quoted 1 line
for this board such as the 2.6.10-based MV kernel
have this
Quoted 9 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
same change. Anyone know what is going on here?
We got this change from MV in the patch he sent to fix an
issue we were
Quoted 1 line
seeing with mounting an empty jffs2 partition. Here are
the comments
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that came in with it
" The oobfree list in the MTD OneNAND driver is of
length 8. This
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
exceeds the maximum length of 7, because the eighth entry
must be used
Quoted 1 line
as a null terminator. The absence of a terminator causes a
variety of
Quoted 1 line
problems in various places (especially JFFS2 and
YAFFS2) where the
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
oobfree list is scanned. The fix is to do without the
eighth entry.
Colud you point out the source code line where used the
eightn entry used as a
Quoted 2 lines
null terminator?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
There are several loops in both the NAND and the OneNAND code where the
oobfree list is walked and the loop only terminates when it finds a NULL
entry (specifically, one with a zero length.)
For example, look in onenand_fill_auto_oob() where you
have a couple
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
loops like this:
    for (free = this->ecclayout->oobfree; free->length; 
++free) {
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
    }
In the case where there is 8 non-zero entries in the oobfree list, this
loop will walk off the end of the list before it terminates, and having
undefined results depending on what is after the oobfree list in memory.
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Instead, these loops should probably loop over MTD_MAX_OOBFREE_ENTRIES,
and break out early if they find a zero-length entry.
Yes, you're right. It met the same problem in the
previous mail [1].
Quoted 2 lines
Quoted 2 lines
Could you apply patch [2] and test again. it will be work.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Anyway I will fix it to check array length.
Here's patch. Yes I know it's maybe wordwrapped. Sorry :)
You missed the loop in onenand_scan() which walks the freelist. Patch below covers all the cases, and was tested to work on 2430.

Ah yes, thank you for point out.
Then is it OK to work on 2430?

Quoted 6 lines

Note that you should raise this on linux-mtd as there are similar loops
in the drivers/mtd/nand* code. The problem has never come up because all the drivers have less than 8 oobfree chunks defined.

I commit it to OneNAND MTD tree.

http://git.infradead.org/?p=users/kmpark/onenand-mtd-
2.6.git;a=commitdiff;h=f8405f395dc8a53426ba4ed629a90955b9a46ade

Frankly why do we calculate the oobavail variable? It's fixed. is it?

I prepared to patch moving from data section to rodata section by make static const.

Thank you,
Kyungmin Park

Tony Lindgren

maintainer

2007-04-26 17:35 UTC #9 lore raw

In reply to Kevin Hilman

Quoted 10 lines · Kevin Hilman

Adds platform device for use of existing OMAP2 driver. Also, the following changes are made to the existing driver:
- add option for not using DMA (by passing dma_channel = -1)
- removes obsoleted pt_regs arg from interrupt handler
RFC: Notice the change to onenand_base.c which comments out the last
OOB chunk. Without this, it would not work. Previous drivers for this board such as the 2.6.10-based MV kernel have this same change. Anyone know what is going on here?

Can you please refresh this one and split it into two patches:

Quoted 6 lines

 arch/arm/mach-omap2/Makefile              |    3 
 arch/arm/mach-omap2/board-2430sdp-flash.c |  108 ++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-2430sdp.c       |    4 +
 arch/arm/mach-omap2/gpmc.c                |    1 
 include/asm-arm/arch-omap/gpmc.h          |    7 +
 include/asm-arm/arch-omap/onenand.h       |    1

The above could be one patch ready for omap-upstream queue.

Quoted 2 lines

 drivers/mtd/onenand/omap2.c               |  103 +++++++++++++++++-----------
 drivers/mtd/onenand/onenand_base.c        |    2

This should be separate and sent to MTD list.

Regards,

Tony

Kevin Hilman

author

2007-04-30 22:34 UTC #10 lore raw

Quoted 21 lines · Tony Lindgren

Quoted 10 lines · Kevin Hilman
Adds platform device for use of existing OMAP2 driver. Also, the following changes are made to the existing driver:
- add option for not using DMA (by passing dma_channel = -1)
- removes obsoleted pt_regs arg from interrupt handler
RFC: Notice the change to onenand_base.c which comments out the last
OOB chunk. Without this, it would not work. Previous drivers for this board such as the 2.6.10-based MV kernel have this same change. Anyone know what is going on here?
Can you please refresh this one and split it into two patches:
Quoted 6 lines
 arch/arm/mach-omap2/Makefile              |    3 
 arch/arm/mach-omap2/board-2430sdp-flash.c |  108 ++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-2430sdp.c       |    4 +
 arch/arm/mach-omap2/gpmc.c                |    1 
 include/asm-arm/arch-omap/gpmc.h          |    7 +
 include/asm-arm/arch-omap/onenand.h       |    1 

Quoted 6 lines

The above could be one patch ready for omap-upstream queue.
Quoted 2 lines
 drivers/mtd/onenand/omap2.c               |  103 +++++++++++++++++-----------
 drivers/mtd/onenand/onenand_base.c        |    2 
This should be separate and sent to MTD list.

Kyungmin,

What's the status of the patch you submitted to the MTD list for this fix?

FWIW, attached is a patch which fixes the walking of the ooblist, as well as an update to the omap2 driver for the non-DMA case.

Kevin

Tony Lindgren

maintainer

2007-05-02 17:45 UTC #11 lore raw

Quoted 29 lines · Kevin Hilman

Quoted 21 lines · Tony Lindgren
Quoted 10 lines · Kevin Hilman
Adds platform device for use of existing OMAP2 driver. Also, the following changes are made to the existing driver:
- add option for not using DMA (by passing dma_channel = -1)
- removes obsoleted pt_regs arg from interrupt handler
RFC: Notice the change to onenand_base.c which comments out the last
OOB chunk. Without this, it would not work. Previous drivers for this board such as the 2.6.10-based MV kernel have this same change. Anyone know what is going on here?
Can you please refresh this one and split it into two patches:
Quoted 6 lines
 arch/arm/mach-omap2/Makefile              |    3 
 arch/arm/mach-omap2/board-2430sdp-flash.c |  108 ++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-2430sdp.c       |    4 +
 arch/arm/mach-omap2/gpmc.c                |    1 
 include/asm-arm/arch-omap/gpmc.h          |    7 +
 include/asm-arm/arch-omap/onenand.h       |    1 
Quoted 6 lines
The above could be one patch ready for omap-upstream queue.
Quoted 2 lines
 drivers/mtd/onenand/omap2.c               |  103 +++++++++++++++++-----------
 drivers/mtd/onenand/onenand_base.c        |    2 
This should be separate and sent to MTD list.

Quoted 7 lines

Kyungmin,
What's the status of the patch you submitted to the MTD list for this fix?
FWIW, attached is a patch which fixes the walking of the ooblist, as well as an update to the omap2 driver for the non-DMA case.

The attached patch did not seem to make it to the mailing list.

Tony