| Message ID | E14CAA9D00C5044EACE0C59FC2A1C7F5059AACE12D@FRMRSSXCHMBSC1.dc-m.alcatel-lucent.com |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2011-06-14 at 16:52 +0200, THOMSON, Adam (Adam) wrote: > In nand_do_write_ops() code it is possible for a caller to provide > ops.oobbuf populated and ops.mode == MTD_OOB_AUTO, which currently > means that the chip->oob_poi buffer isn't initialised to all 0xFF. > The nand_fill_oob() method then carries out the task of copying > the provided OOB data to oob_poi, but with MTD_OOB_AUTO it skips > areas marked as unavailable by the layout struct, including the > bad block marker bytes. > > An example of this causing issues is when the last OOB data read > was from the start of a bad block where the markers are not 0xFF, > and the caller wishes to write new OOB data at the beginning of > another block. In this scenario the caller would provide OOB data, > but nand_fill_oob() would skip the bad block marker bytes in > oob_poi before copying the OOB data provided by the caller. > This means that when the OOB data is written back to NAND, > the block is inadvertently marked as bad without the caller knowing. > This has been witnessed when using YAFFS2 where tags are stored > in the OOB. > > To avoid this oob_poi is always initialised to 0xFF to make sure > no left over data is inadvertently written back to the OOB area. > > Signed-off-by: Adam Thomson <adam.thomson@alcatel-lucent.com> Added the -stable CC here and pushed to l2-mtd-2.6.git with some modifications. > * nand_fill_oob - [Internal] Transfer client buffer to oob > * @chip: nand chip structure > + * @mtd: MTD device structure > * @oob: oob data buffer > * @len: oob data write length > * @ops: oob ops structure > */ > -static uint8_t *nand_fill_oob(struct nand_chip *chip, uint8_t *oob, size_t len, > - struct mtd_oob_ops *ops) > +static uint8_t *nand_fill_oob(struct nand_chip *chip, struct mtd_info *mtd, > + uint8_t *oob, size_t len, struct mtd_oob_ops *ops) > { Since we can get chip from mtd->prive, it is not necessary to pass both chip and mtd to this function, it is enough to only pass mtd. I've done this modification, the resulting patch is here: http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commit/6a146696fdcf383d90753145dfb367e499790940 Would you please take a look and even better - give it a try? Thanks!
Artem Wrote: > > Added the -stable CC here and pushed to l2-mtd-2.6.git with > some modifications. > This is probably my fault, but the mail previous to this had the version which contained the stable line. That patch version (v3)did not remove the final memset from nand_do_write_oob, as you requested. Named both patches the same as they were pretty much indentical, except for the memset. Didn't know the convention for patch naming where one was going to stable, and the other to the latest so marked the latest as upstream to differentiate. > > * nand_fill_oob - [Internal] Transfer client buffer to oob > > * @chip: nand chip structure > > + * @mtd: MTD device structure > > * @oob: oob data buffer > > * @len: oob data write length > > * @ops: oob ops structure > > */ > > -static uint8_t *nand_fill_oob(struct nand_chip *chip, > uint8_t *oob, size_t len, > > - struct mtd_oob_ops *ops) > > +static uint8_t *nand_fill_oob(struct nand_chip *chip, > struct mtd_info *mtd, > > + uint8_t *oob, size_t len, struct > mtd_oob_ops *ops) > > { > > Since we can get chip from mtd->prive, it is not necessary to > pass both chip and mtd to this function, it is enough to only > pass mtd. Fair enough. Didn't spot that. :) > > I've done this modification, the resulting patch is here: > http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commit/ 6a146696fdcf383d90753145dfb367e499790940 > > Would you please take a look and even better - give it a try? > Yep, no problem. Will run it up on target an check all still works. > Thanks! > > -- > Best Regards, > Artem Bityutskiy > > Thanks Adam
On Wed, 2011-06-22 at 12:41 +0200, THOMSON, Adam (Adam) wrote: > Artem Wrote: > > > > > Added the -stable CC here and pushed to l2-mtd-2.6.git with > > some modifications. > > > > This is probably my fault, but the mail previous to this had the > version which contained the stable line. I did not figure out why you sent 2 separate patches. Usually it is one patch which goes upstream, then GregKH and others pick it up and back ports to various stable trees. If they have issues, they may come and ask for help. So there should be 1 patch. > That patch version (v3)did > not remove the final memset from nand_do_write_oob, as you requested. Did I request so? I thought I just expressed a thought that the memset is redundant and should be killed, and I thought you'd take a deeper look and decide whether it is safe to do or not. > Named both patches the same as they were pretty much indentical, > except for the memset. Didn't know the convention for patch naming > where one was going to stable, and the other to the latest so > marked the latest as upstream to differentiate. So do we need that memset or not? :-)
Artem wrote: > I did not figure out why you sent 2 separate patches. Usually > it is one patch which goes upstream, then GregKH and others > pick it up and back ports to various stable trees. If they > have issues, they may come and ask for help. So there should > be 1 patch. > > > That patch version (v3)did > > not remove the final memset from nand_do_write_oob, as you > requested. > > Did I request so? I thought I just expressed a thought that > the memset is redundant and should be killed, and I thought > you'd take a deeper look and decide whether it is safe to do or not. > > > Named both patches the same as they were pretty much indentical, > > except for the memset. Didn't know the convention for patch naming > > where one was going to stable, and the other to the latest > so marked > > the latest as upstream to differentiate. > > So do we need that memset or not? :-) The original request was for a stable patch, and that would be accepted if I provided a proper tidy fix also. To be fair it was a few weeks ago now that that mail was sent. :-) Anyway, as it turns out the tidy fix was almost identical, and we don't need the memset as far as I can tell from reviewing code and from testing. Will run up your patch today to verify your additional change, but should be fine. > > -- > Best Regards, > Artem Bityutskiy > > Thanks Adam
Adam Thomson Wrote: > > The original request was for a stable patch, and that would > be accepted if I provided a proper tidy fix also. To be fair > it was a few weeks ago now that that mail was sent. :-) > Anyway, as it turns out the tidy fix was almost identical, > and we don't need the memset as far as I can tell from > reviewing code and from testing. Will run up your patch today > to verify your additional change, but should be fine. > Have tested the updated patch on hardware, and all seems good. No problems with OOB corruption, and mtd tests run without failure. Thanks Adam
On Wed, 2011-06-22 at 17:40 +0200, THOMSON, Adam (Adam) wrote: > Adam Thomson Wrote: > > > > The original request was for a stable patch, and that would > > be accepted if I provided a proper tidy fix also. To be fair > > it was a few weeks ago now that that mail was sent. :-) > > Anyway, as it turns out the tidy fix was almost identical, > > and we don't need the memset as far as I can tell from > > reviewing code and from testing. Will run up your patch today > > to verify your additional change, but should be fine. > > > > Have tested the updated patch on hardware, and all seems good. > No problems with OOB corruption, and mtd tests run without failure. Thank you!
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index a46e9bb..0815a3d 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2098,13 +2098,20 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, /** * nand_fill_oob - [Internal] Transfer client buffer to oob * @chip: nand chip structure + * @mtd: MTD device structure * @oob: oob data buffer * @len: oob data write length * @ops: oob ops structure */ -static uint8_t *nand_fill_oob(struct nand_chip *chip, uint8_t *oob, size_t len, - struct mtd_oob_ops *ops) +static uint8_t *nand_fill_oob(struct nand_chip *chip, struct mtd_info *mtd, + uint8_t *oob, size_t len, struct mtd_oob_ops *ops) { + /* + * Initialise to all 0xFF, to avoid the possibility of + * left over OOB data from a previous OOB read. + */ + memset(chip->oob_poi, 0xff, mtd->oobsize); + switch (ops->mode) { case MTD_OOB_PLACE: @@ -2201,10 +2208,6 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, (chip->pagebuf << chip->page_shift) < (to + ops->len)) chip->pagebuf = -1; - /* If we're not given explicit OOB data, let it be 0xFF */ - if (likely(!oob)) - memset(chip->oob_poi, 0xff, mtd->oobsize); - /* Don't allow multipage oob writes with offset */ if (oob && ops->ooboffs && (ops->ooboffs + ops->ooblen > oobmaxlen)) return -EINVAL; @@ -2226,7 +2229,7 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, if (unlikely(oob)) { size_t len = min(oobwritelen, oobmaxlen); - oob = nand_fill_oob(chip, oob, len, ops); + oob = nand_fill_oob(chip, mtd, oob, len, ops); oobwritelen -= len; } @@ -2401,10 +2404,8 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to, if (page == chip->pagebuf) chip->pagebuf = -1; - memset(chip->oob_poi, 0xff, mtd->oobsize); - nand_fill_oob(chip, ops->oobbuf, ops->ooblen, ops); + nand_fill_oob(chip, mtd, ops->oobbuf, ops->ooblen, ops); status = chip->ecc.write_oob(mtd, chip, page & chip->pagemask); - memset(chip->oob_poi, 0xff, mtd->oobsize); if (status) return status;
In nand_do_write_ops() code it is possible for a caller to provide ops.oobbuf populated and ops.mode == MTD_OOB_AUTO, which currently means that the chip->oob_poi buffer isn't initialised to all 0xFF. The nand_fill_oob() method then carries out the task of copying the provided OOB data to oob_poi, but with MTD_OOB_AUTO it skips areas marked as unavailable by the layout struct, including the bad block marker bytes. An example of this causing issues is when the last OOB data read was from the start of a bad block where the markers are not 0xFF, and the caller wishes to write new OOB data at the beginning of another block. In this scenario the caller would provide OOB data, but nand_fill_oob() would skip the bad block marker bytes in oob_poi before copying the OOB data provided by the caller. This means that when the OOB data is written back to NAND, the block is inadvertently marked as bad without the caller knowing. This has been witnessed when using YAFFS2 where tags are stored in the OOB. To avoid this oob_poi is always initialised to 0xFF to make sure no left over data is inadvertently written back to the OOB area. Signed-off-by: Adam Thomson <adam.thomson@alcatel-lucent.com> --- Applies to: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git Tested on: ARM based board with Hynix 256MB NAND chip drivers/mtd/nand/nand_base.c | 21 +++++++++++---------- 1 files changed, 11 insertions(+), 10 deletions(-)