RE: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

Kevin Hilman
2007-03-22T16:53:10Z

On Thu, 2007-03-22 at 14:02 +0900, Kyungmin Park wrote:
> > >
> > > Kyungmin Park wrote:
> > > >>>> 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.
> > > >
> > > > 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.
> >
> > 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

Index: dev/drivers/mtd/onenand/onenand_base.c
===================================================================
--- dev.orig/drivers/mtd/onenand/onenand_base.c
+++ 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;