[PATCH v2] nand: nand_base: Always initialise oob_poi before writing OOB data

[THOMSON, Adam (Adam)]

Artem Bityutskiy Wrote:

> 1. We need to send this to the -stable tree, so this needs a
> 
> Cc: stable at kernel.org
> 
> You say this is broken since 2.6.31, are you 100% sure? Can 
> you point the specific commit which broke this? Or you just 
> guess that this is 2.6.31?
>
> This is important because we'd need to add
> 
> Cc: stable at kernel.org [2.6.31+] in that case.
> 
> But AFAICS, the breakage happened after commit
> 
> commit 7dcdcbef5d2b60d1db68fd2c07351f7afd8ad376
> Author: David Woodhouse <dwmw2 at infradead.org>
> Date:   Sat Oct 21 17:09:53 2006 +0100
> 
>     [MTD] NAND: Combined oob buffer so it's contiguous with data
> 
> which was released in 2.6.20.
> 

The reason I mentioned 2.6.31 was because it was the earliest kernel I
had been using when I witnessed this issue. Having looked at the commit
you mentioned, I have to agree that's where the problem first appeared.
Do you want me to add the CC with [2.6.20+] (assume that should be part
of the patch text in the mail)?

> 
> 
> 2. I think the right place fir this memset is 
> 'nand_fill_oob()'. But in this case the first memset from 
> 'nand_do_write_oob()' has to be removed.
> 

Yes, that makes sense. Did consider that afterwards. Will update
Accordingly.

> 
> 
> 3. And finally, I think we should clean-up the following piece of code
>    in 'nand_do_write_oob()':
> 
>         memset(chip->oob_poi, 0xff, mtd->oobsize);
>         nand_fill_oob(chip, ops->oobbuf, ops->ooblen, ops);
>         status = chip->ecc.write_oob(mtd, chip, page & 
> chip->pagemask);
>         memset(chip->oob_poi, 0xff, mtd->oobsize);
> 
> This should not be part of your fix and should not go to 
> stable. But I think this should be a condition for your fix 
> to be accepted.
> Unfortunately, so few people love mtd and it is full of crap 
> which should be cleaned-up, and the only thing to do this is 
> to force people who want just a bug-fix to also do a bit of 
> cleaning up. Sorry, nothing personal.
>

No that's fine. Shouldn't be too much to do. Do you want both
the stable patch and the proper fix submitted around the same
time, or are you happy to get the initial fix in first, and follow up
with the more complete tidying of that code? Also am guessing
the complete patch should be based on the latest and greatest
Kernel (2.6.39)?

> 
> Would you please be kind enough and look into this (if what I 
> say makes technical sense, of course)? I think the second 
> memset should not exist at all. And instead, all read side 
> code has to memset the buffer for itself.
> 
> Most probably the memset can just be removed, but I'm not 
> 100% sure and this needs a more careful look.

Having looked briefly at the read side code in nand_base.c,
it does look like it should be enough to remove the second
memset and leave the read side code as is, but will examine
it more thoroughly before I post a patch.

Thanks

Adam