bug in drivers/mtd/onenand/onenand_base.c?

bug in drivers/mtd/onenand/onenand_base.c?

[Vitaly Wool]

Hi,

the piece of code below, though looking harmless, might cause side effects:

/**
 * onenand_oob_64 - oob info for large (2KB) page
 */
static struct nand_ecclayout onenand_oob_64 = {
        .eccbytes       = 20,
        .eccpos         = {
                8, 9, 10, 11, 12,
                24, 25, 26, 27, 28,
                40, 41, 42, 43, 44,
                56, 57, 58, 59, 60,
                },
        .oobfree        = {
                {2, 3}, {14, 2}, {18, 3}, {30, 2},
                {34, 3}, {46, 2}, {50, 3}, {62, 2}
        }
};

The problem is, the entry of oobfree past the last valid 
one should have length 0.

Here it's not the case: all the entries are occupied by 
OOB chunks. Therefore, once we get into a loop like

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

we might end up scanning past the real oobfree array.

Probably the best way out, as the same thing might happen for common NAND 
as well, is to check index against MTD_MAX_OOBFREE_ENTRIES.

Comments?

Vitaly

Re: bug in drivers/mtd/onenand/onenand_base.c?

[Artem Bityutskiy]

On Thu, 2007-03-22 at 12:36 +0300, Vitaly Wool wrote:
> The problem is, the entry of oobfree past the last valid 
> one should have length 0.
> 
> Here it's not the case: all the entries are occupied by 
> OOB chunks. Therefore, once we get into a loop like
> 
>         for (free = this->ecclayout->oobfree; free->length; ++free) {
> 
> we might end up scanning past the real oobfree array.

We have this fixed in OneNAND.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: bug in drivers/mtd/onenand/onenand_base.c?

[Adrian Hunter]

ext Artem Bityutskiy wrote:
> On Thu, 2007-03-22 at 12:36 +0300, Vitaly Wool wrote:
>> The problem is, the entry of oobfree past the last valid 
>> one should have length 0.
>>
>> Here it's not the case: all the entries are occupied by 
>> OOB chunks. Therefore, once we get into a loop like
>>
>>         for (free = this->ecclayout->oobfree; free->length; ++free) {
>>
>> we might end up scanning past the real oobfree array.
> 
> We have this fixed in OneNAND.
> 

No we don't

I thought Kyungmin Park was going to fix it 
(see http://lists.infradead.org/pipermail/linux-mtd/2007-February/017479.html)
, then I forgot about it, sorry :-(

Re: bug in drivers/mtd/onenand/onenand_base.c?

[Vitaly Wool]

Adrian Hunter wrote:

>Yes, I thought Kyungmin Park was going to fix it.  See:
>
>http://lists.infradead.org/pipermail/linux-mtd/2007-February/017479.html
>
>  
>
Looks like it hasn't gotten in mtd main tree still.

Vitaly

bug in drivers/mtd/onenand/onenand_base.c?

[Kyungmin Park]

Hi Vitaly Wool,

Sorry, I'm not office so I can't access my computer.
To help your work I send the URL related this problem.
This problem is reported previous time but not commit it.

Please check this mail. It would help your work.
http://linux.omap.com/pipermail/linux-omap-open-source/2007-March/009395.htm
l

I saw your mtd git tree related with yaffs. I tested the yaffs on onenand
before. But as you know it failed.
Now with your patch how many bytes are needed in yaffs if ecc provided?
Maybe we need to modify onenand read & write function to support read/write
with oob simultaneously.
Currently it only supports separated read or oob function only, instead of
both.

If the yaffs requires less or equal with 20 bytes, it will work with
OneNAND. Of course, if the page is 1KB, it can't. Since it has only 10 bytes
OOB.

I hope yaffs can run with OneNAND.

Thank you,
Kyungmin Park

RE: Onenand and yaffs [was: Re: bug in drivers/mtd/onenand/onenand_base.c?]

[Kyungmin Park]

> 
> Kyungmin Park wrote:
> 
> >I saw your mtd git tree related with yaffs. I tested the 
> yaffs on onenand
> >before. But as you know it failed.
> >Now with your patch how many bytes are needed in yaffs if 
> ecc provided?
> >  
> >
> Not sure if I understood you right here. YAFFS2 generally needs 28 OOB
> bytes, and 16 without ECCOther.

Oh, Good!
16 bytes enough for 2KB page OneNAND. and Maybe we can run 1KB page OneNAND
with minor patch.

Note: It's out of Spec. So it's used for development only.

/**
 * onenand_oob_32 - oob info for middle (1KB) page
 */
static struct nand_ecclayout onenand_oob_32 = {
        .eccbytes       = 10,
        .eccpos         = {
                8, 9, 10, 11, 12,
                24, 25, 26, 27, 28,
                },
#ifdef ONENAND_FOR_YAFFS
        .oobfree        = { {2, 6}, {14, 2}, {18, 6}, {30, 2} }	
#else
        .oobfree        = { {2, 3}, {14, 2}, {18, 3}, {30, 2} }
#endif
};

Thank you,
Kyungmin Park