| Message ID | 1290156045-11719-1-git-send-email-r64343@freescale.com |
|---|---|
| State | Accepted |
| Commit | e14feafbe0d5c6d64bb6fe4eba928cb57ac9a4c8 |
| Headers | show |
On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote: > When run mtd_oobtest case, there will be one error for step(4), > which turned out it need add one check for out of page read in > nand_do_read_oob just like mtd_do_write_oob did it already. > This commit also fix one typo error for comments in mtd_do_write_oob > > Signed-off-by: Jason Liu <r64343@freescale.com> > --- > drivers/mtd/nand/nand_base.c | 9 ++++++++- > 1 files changed, 8 insertions(+), 1 deletions(-) Pushed to l2-mtd-2.6.git, thanks!
On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote: > When run mtd_oobtest case, there will be one error for step(4), > which turned out it need add one check for out of page read in > nand_do_read_oob just like mtd_do_write_oob did it already. > This commit also fix one typo error for comments in mtd_do_write_oob > > Signed-off-by: Jason Liu <r64343@freescale.com> > --- > drivers/mtd/nand/nand_base.c | 9 ++++++++- > 1 files changed, 8 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 1f75a1b..75d199e 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -1782,6 +1782,13 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, > else > len = mtd->oobsize; > > + /* Do not allow read past end of page */ > + if ((ops->ooboffs + readlen) > len) { > + DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to read " > + "past end of page\n", __func__); > + return -EINVAL; > + } As you reported to me in a private e-mail (although I prefer to always have a public ML in CC when dealing with open source stuff) this patch is wrong. Indeed, it limits the maximum amount of bytes which can be read at one go to the OOB size, which is incorrect, because mtd->read_oob() allows reading multiple pages at a time, see comment near "struct mtd_oob_ops" at include/linux/mtd/mtd.h. So this patch breaks ABI and hence, has to be reverted. > if (unlikely(ops->ooboffs >= len)) { > DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start read " > "outside oob\n", __func__); Side note: nand_base.c has a bunch of senseless unlikely() hints, would be nice to clean that up some day. > - /* Do not allow reads past end of device */ > + /* Do not allow write past end of device */ Care to make this a separate clean-up patch meanwhile? Thank!
On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote: > When run mtd_oobtest case, there will be one error for step(4), > which turned out it need add one check for out of page read in > nand_do_read_oob just like mtd_do_write_oob did it already. > This commit also fix one typo error for comments in mtd_do_write_oob > > Signed-off-by: Jason Liu <r64343@freescale.com> I cannot reproduce this with nandsim, can you? I tried: 1. sudo modprobe nandsim sudo modprobe mtd_oobtest dev=0 2. sudo ./load_nandsim.sh 128 128 2048 sudo modprobe mtd_oobtest dev=0 In both cases the test passes: "mtd_oobtest: finished with 0 errors" And in 'nand_do_read_oob()' I see the following piece of code: /* Do not allow reads past end of device */ if (unlikely(from >= mtd->size || ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) - (from >> chip->page_shift)) * len)) { DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end " "of device\n", __func__); return -EINVAL; } which should handle the case AFAICS. So, although I did think hard about this, it looks like there is no problem. What is the kernel version you are looking at? I checked it with my l2-mtd-2.6.git, which is the latest mtd-2.6.git plus a revert commit of your patch: http://git.infradead.org/users/dedekind/l2-mtd-2.6.git
Hi, Artem, 2010/12/14 Artem Bityutskiy <dedekind1@gmail.com>: > On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote: >> When run mtd_oobtest case, there will be one error for step(4), >> which turned out it need add one check for out of page read in >> nand_do_read_oob just like mtd_do_write_oob did it already. >> This commit also fix one typo error for comments in mtd_do_write_oob >> >> Signed-off-by: Jason Liu <r64343@freescale.com> > > I cannot reproduce this with nandsim, can you? > > I tried: > > 1. sudo modprobe nandsim > sudo modprobe mtd_oobtest dev=0 > > 2. sudo ./load_nandsim.sh 128 128 2048 > sudo modprobe mtd_oobtest dev=0 > > In both cases the test passes: "mtd_oobtest: finished with 0 errors" > > And in 'nand_do_read_oob()' I see the following piece of code: > > /* Do not allow reads past end of device */ > if (unlikely(from >= mtd->size || > ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) - > (from >> chip->page_shift)) * len)) { > DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end " > "of device\n", __func__); > return -EINVAL; > } Here the mtd->size in nand_base.c should be the NAND flash chip size, while in nand_oobtest, /* Attempt to read off end of device */ ops.mode = MTD_OOB_AUTO; ops.len = 0; ops.retlen = 0; ops.ooblen = mtd->ecclayout->oobavail; ops.oobretlen = 0; ops.ooboffs = 1; ops.datbuf = NULL; ops.oobbuf = readbuf; printk(PRINT_PREF "attempting to read past end of device\n"); printk(PRINT_PREF "an error is expected...\n"); err = mtd->read_oob(mtd, mtd->size - mtd->writesize, &ops); if (err) { printk(PRINT_PREF "error occurred as expected\n"); err = 0; } else { printk(PRINT_PREF "error: read past end of device\n"); errcnt += 1; } } here, mtd->size is mtd partition size right? when the mtd partition is not the last MTD partition, the error will happen. I have tested on real NAND, while not on nandsim. > > which should handle the case AFAICS. > > So, although I did think hard about this, it looks like there is no > problem. What is the kernel version you are looking at? I checked it > with my l2-mtd-2.6.git, which is the latest mtd-2.6.git plus a revert > commit of your patch: > > http://git.infradead.org/users/dedekind/l2-mtd-2.6.git Yes, as I have written the email to you before in a private email to tell that this patch has issue and please drop it. Thanks, > > -- > Best Regards, > Artem Bityutskiy (Артём Битюцкий) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
On Wed, 2010-12-15 at 09:55 +0800, Jason Liu wrote: > > /* Do not allow reads past end of device */ > > if (unlikely(from >= mtd->size || > > ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) - > > (from >> chip->page_shift)) * len)) { > > DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end " > > "of device\n", __func__); > > return -EINVAL; > > } > > Here the mtd->size in nand_base.c should be the NAND flash chip size, I think this is partition size as well. > while in nand_oobtest, > > /* Attempt to read off end of device */ > ops.mode = MTD_OOB_AUTO; > ops.len = 0; > ops.retlen = 0; > ops.ooblen = mtd->ecclayout->oobavail; > ops.oobretlen = 0; > ops.ooboffs = 1; > ops.datbuf = NULL; > ops.oobbuf = readbuf; > printk(PRINT_PREF "attempting to read past end of device\n"); > printk(PRINT_PREF "an error is expected...\n"); > err = mtd->read_oob(mtd, mtd->size - mtd->writesize, &ops); > if (err) { > printk(PRINT_PREF "error occurred as expected\n"); > err = 0; > } else { > printk(PRINT_PREF "error: read past end of device\n"); > errcnt += 1; > } > } > > here, mtd->size is mtd partition size right? when the mtd partition is > not the last MTD partition, the error will happen. I think this is the same in both. Try to debug this with printks(). > > I have tested on real NAND, while not on nandsim. Please, try to reproduce this with nandsim as well, and if it works, try to find out why (using printk debugging)
Hi, Artem, 2010/12/20 Artem Bityutskiy <dedekind1@gmail.com>: > On Wed, 2010-12-15 at 09:55 +0800, Jason Liu wrote: >> > /* Do not allow reads past end of device */ >> > if (unlikely(from >= mtd->size || >> > ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) - >> > (from >> chip->page_shift)) * len)) { >> > DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end " >> > "of device\n", __func__); >> > return -EINVAL; >> > } >> >> Here the mtd->size in nand_base.c should be the NAND flash chip size, > > I think this is partition size as well. I think you are wrong. This should be the NAND flash chip size not the partition size. > >> while in nand_oobtest, >> >> /* Attempt to read off end of device */ >> ops.mode = MTD_OOB_AUTO; >> ops.len = 0; >> ops.retlen = 0; >> ops.ooblen = mtd->ecclayout->oobavail; >> ops.oobretlen = 0; >> ops.ooboffs = 1; >> ops.datbuf = NULL; >> ops.oobbuf = readbuf; >> printk(PRINT_PREF "attempting to read past end of device\n"); >> printk(PRINT_PREF "an error is expected...\n"); >> err = mtd->read_oob(mtd, mtd->size - mtd->writesize, &ops); >> if (err) { >> printk(PRINT_PREF "error occurred as expected\n"); >> err = 0; >> } else { >> printk(PRINT_PREF "error: read past end of device\n"); >> errcnt += 1; >> } >> } >> >> here, mtd->size is mtd partition size right? when the mtd partition is >> not the last MTD partition, the error will happen. > > I think this is the same in both. Try to debug this with printks(). > >> >> I have tested on real NAND, while not on nandsim. > > Please, try to reproduce this with nandsim as well, and if it works, try > to find out why (using printk debugging) Yes, I have reproduce this issue with nandsim, please check the following log, root@freescale ~$ insmod nandsim.ko first_id_byte=0x20 second_id_byte=0xaa third_id_byte=0x00 fourth_id_byte=0x15 parts=0x400,0x400 [nandsim] warning: read_byte: unexpected data output cycle, state is STATE_READY return 0x0 [nandsim] warning: read_byte: unexpected data output cycle, state is STATE_READY return 0x0 [nandsim] warning: read_byte: unexpected data output cycle, state is STATE_READY return 0x0 [nandsim] warning: read_byte: unexpected data output cycle, state is STATE_READY return 0x0 NAND device: Manufacturer ID: 0x20, Chip ID: 0xaa (ST Micro NAND 256MiB 1,8V 8-bit) flash size: 256 MiB page size: 2048 bytes OOB area size: 64 bytes sector size: 128 KiB pages number: 131072 pages per sector: 64 bus width: 8 bits in sector size: 17 bits in page size: 11 bits in OOB size: 6 flash size with OOB: 270336 KiB page address bytes: 5 sector address bytes: 3 options: 0x8 Scanning device for bad blocks Creating 2 MTD partitions on "NAND 256MiB 1,8V 8-bit": 0x000000000000-0x000008000000 : "NAND simulator partition 0" 0x000008000000-0x000010000000 : "NAND simulator partition 1" root@freescale ~$ cat /proc/mtd dev: size erasesize name mtd0: 00300000 00080000 "bootloader" mtd1: 00500000 00080000 "nand.kernel" mtd2: 10000000 00080000 "nand.rootfs" mtd3: 10000000 00080000 "nand.userfs1" mtd4: df800000 00080000 "nand.userfs2" mtd5: 08000000 00020000 "NAND simulator partition 0" mtd6: 08000000 00020000 "NAND simulator partition 1" root@freescale ~$ insmod mtd_oobtest.ko dev=5 ================================================= mtd_oobtest: MTD device: 5 mtd_oobtest: MTD device size 134217728, eraseblock size 131072, page size 2048, count of eraseblocks 1024, pages per eraseblock 64, OOB size 64 mtd_oobtest: scanning for bad eraseblocks mtd_oobtest: scanned 1024 eraseblocks, 0 are bad mtd_oobtest: test 1 of 5 mtd_oobtest: erasing whole device mtd_oobtest: erased 1024 eraseblocks mtd_oobtest: writing OOBs of whole device mtd_oobtest: written up to eraseblock 0 mtd_oobtest: written up to eraseblock 256 mtd_oobtest: written up to eraseblock 512 mtd_oobtest: written up to eraseblock 768 mtd_oobtest: written 1024 eraseblocks mtd_oobtest: verifying all eraseblocks mtd_oobtest: verified up to eraseblock 0 mtd_oobtest: verified up to eraseblock 256 mtd_oobtest: verified up to eraseblock 512 mtd_oobtest: verified up to eraseblock 768 mtd_oobtest: verified 1024 eraseblocks mtd_oobtest: test 2 of 5 mtd_oobtest: erasing whole device mtd_oobtest: erased 1024 eraseblocks mtd_oobtest: writing OOBs of whole device mtd_oobtest: written up to eraseblock 0 mtd_oobtest: written up to eraseblock 256 mtd_oobtest: written up to eraseblock 512 mtd_oobtest: written up to eraseblock 768 mtd_oobtest: written 1024 eraseblocks mtd_oobtest: verifying all eraseblocks mtd_oobtest: verified up to eraseblock 0 mtd_oobtest: verified up to eraseblock 256 mtd_oobtest: verified up to eraseblock 512 mtd_oobtest: verified up to eraseblock 768 mtd_oobtest: verified 1024 eraseblocks mtd_oobtest: test 3 of 5 mtd_oobtest: erasing whole device mtd_oobtest: erased 1024 eraseblocks mtd_oobtest: writing OOBs of whole device mtd_oobtest: written up to eraseblock 0 mtd_oobtest: written up to eraseblock 256 mtd_oobtest: written up to eraseblock 512 mtd_oobtest: written up to eraseblock 768 mtd_oobtest: written 1024 eraseblocks mtd_oobtest: verifying all eraseblocks mtd_oobtest: verified up to eraseblock 0 mtd_oobtest: verified up to eraseblock 256 mtd_oobtest: verified up to eraseblock 512 mtd_oobtest: verified up to eraseblock 768 mtd_oobtest: verified 1024 eraseblocks mtd_oobtest: test 4 of 5 mtd_oobtest: erasing whole device mtd_oobtest: erased 1024 eraseblocks mtd_oobtest: attempting to start write past end of OOB mtd_oobtest: an error is expected... mtd_oobtest: error occurred as expected mtd_oobtest: attempting to start read past end of OOB mtd_oobtest: an error is expected... mtd_oobtest: error occurred as expected mtd_oobtest: attempting to write past end of device mtd_oobtest: an error is expected... mtd_oobtest: error occurred as expected mtd_oobtest: attempting to read past end of device mtd_oobtest: an error is expected... mtd_oobtest: error: read past end of device mtd_oobtest: attempting to write past end of device mtd_oobtest: an error is expected... mtd_oobtest: error occurred as expected mtd_oobtest: attempting to read past end of device mtd_oobtest: an error is expected... mtd_oobtest: error: read past end of device mtd_oobtest: test 5 of 5 mtd_oobtest: erasing whole device mtd_oobtest: erased 1024 eraseblocks mtd_oobtest: writing OOBs of whole device mtd_oobtest: written up to eraseblock 0 mtd_oobtest: written up to eraseblock 0 mtd_oobtest: written up to eraseblock 256 mtd_oobtest: written up to eraseblock 256 mtd_oobtest: written up to eraseblock 512 mtd_oobtest: written up to eraseblock 512 mtd_oobtest: written up to eraseblock 768 mtd_oobtest: written up to eraseblock 768 mtd_oobtest: written 1023 eraseblocks mtd_oobtest: verifying all eraseblocks mtd_oobtest: verified up to eraseblock 0 mtd_oobtest: verified up to eraseblock 256 mtd_oobtest: verified up to eraseblock 512 mtd_oobtest: verified up to eraseblock 768 mtd_oobtest: verified 1023 eraseblocks mtd_oobtest: finished with 2 errors ================================================= > > -- > Best Regards, > Artem Bityutskiy (Битюцкий Артём) > >
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 1f75a1b..75d199e 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1782,6 +1782,13 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, else len = mtd->oobsize; + /* Do not allow read past end of page */ + if ((ops->ooboffs + readlen) > len) { + DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to read " + "past end of page\n", __func__); + return -EINVAL; + } + if (unlikely(ops->ooboffs >= len)) { DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start read " "outside oob\n", __func__); @@ -2377,7 +2384,7 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to, return -EINVAL; } - /* Do not allow reads past end of device */ + /* Do not allow write past end of device */ if (unlikely(to >= mtd->size || ops->ooboffs + ops->ooblen > ((mtd->size >> chip->page_shift) -
When run mtd_oobtest case, there will be one error for step(4), which turned out it need add one check for out of page read in nand_do_read_oob just like mtd_do_write_oob did it already. This commit also fix one typo error for comments in mtd_do_write_oob Signed-off-by: Jason Liu <r64343@freescale.com> --- drivers/mtd/nand/nand_base.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)