Message ID | 20081101031505.GA5177@almesberger.net |
---|---|
State | New, archived |
Headers | show |
On Sat, Nov 01, 2008 at 01:15:05AM -0200, Werner Almesberger wrote: > In recent kernels, the optimized OOB reads in nand_read_subpage often > make requests which aren't an exact multiple of a word, which caused > nand/s3c2410.c to do a partial read, yielding chaos and mayhem. > > I'm not entirely sure whether nand_read_subpage is to blame for making > non-word requests, or whether nand/s3c2410.c is to blame for not > supporting them, but here's a patch for the latter. As noted on the openmoko list, I think we can do 256byte subpage reads as long as they are aligned to 256bytes. We could make the ECC code deal with non-256 byte power-of-two aligned blocks without huge changes but my belief is that we cannot support anything that isn't a power of two. I think the best thing to do is to either force the caller to read a power of two (pref. >4 bytes), so either we need some form of flag to say this, or change the behaviour of the callers to never try this. > Note that s3c2410_nand_write_buf has a similar issue. RNDIN is used in > nand_write_oob_syndrome, so this may also be a real problems. > > - Werner > > ---------------------------------- cut here ----------------------------------- > > fix-s3c-nand-read-bytes.patch > > With the introduction of optimized OOB reads in nand_read_subpage, > the length of the data requested may not be a multiple of four bytes. > > This caused a partial read on the 2440, leading to false ECC errors > and, worse, attempts to "correct" them. > > Note that there is a similar issue in s3c2440_nand_write_buf, which > doesn't seem to cause trouble yet. > > Signed-off-by: Werner Almesberger <werner@openmoko.org> > > --- > > Index: ktrack/drivers/mtd/nand/s3c2410.c > =================================================================== > --- ktrack.orig/drivers/mtd/nand/s3c2410.c 2008-11-01 00:29:45.000000000 -0200 > +++ ktrack/drivers/mtd/nand/s3c2410.c 2008-11-01 00:38:10.000000000 -0200 > @@ -530,7 +530,14 @@ > static void s3c2440_nand_read_buf(struct mtd_info *mtd, u_char *buf, int len) > { > struct s3c2410_nand_info *info = s3c2410_nand_mtd_toinfo(mtd); > + > readsl(info->regs + S3C2440_NFDATA, buf, len / 4); > + if (unlikely(len & 3)) { > + u32 data; > + > + data = readl(info->regs + S3C2440_NFDATA); > + memcpy(buf + (len & ~3), &data, len & 3); > + } note you'll still fail for len in the 0..3 region, as you'll move len+4 bytes, and then copy them over the data you just read. > } > > static void s3c2410_nand_write_buf(struct mtd_info *mtd, const u_char *buf, int len) > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
Ben Dooks wrote: > As noted on the openmoko list, Sorry for starting two threads on the same topic. The joy of trying to do the right thing with lists that don't let you cross-post unless you're subscribed ... > I think we can do 256byte subpage reads > as long as they are aligned to 256bytes. We could make the ECC code > deal with non-256 byte power-of-two aligned blocks without huge > changes but my belief is that we cannot support anything that isn't > a power of two. In this case, the problem is a bit more subtle: the data blocks retrieved are perfectly normal, i.e., 256 bytes in size and properly aligned. However, nand_read_subpage optimizes retrieval of the OOB data. So instead of retrieving, say, 64 bytes, it only retrieves 24 (for a 2048 bytes page). Sometimes, not the entire page is retrieved, and then we get those accesses with an odd size. > I think the best thing to do is to either force the caller to read > a power of two (pref. >4 bytes), so either we need some form of flag > to say this, or change the behaviour of the callers to never try this. If it's considered generally objectionable to call read_buf for an amount of data that isn't a whole number of words, those two approaches would work as well. Making nand_read_subpage align to 4 bytes instead of 1/2 would be fairly simple. A flag would be a bit messier. - Werner
On Sat, Nov 01, 2008 at 02:11:48PM -0200, Werner Almesberger wrote: > Ben Dooks wrote: > > As noted on the openmoko list, > > Sorry for starting two threads on the same topic. The joy of trying > to do the right thing with lists that don't let you cross-post unless > you're subscribed ... > > > I think we can do 256byte subpage reads > > as long as they are aligned to 256bytes. We could make the ECC code > > deal with non-256 byte power-of-two aligned blocks without huge > > changes but my belief is that we cannot support anything that isn't > > a power of two. > > In this case, the problem is a bit more subtle: the data blocks > retrieved are perfectly normal, i.e., 256 bytes in size and > properly aligned. > > However, nand_read_subpage optimizes retrieval of the OOB data. > So instead of retrieving, say, 64 bytes, it only retrieves 24 > (for a 2048 bytes page). Sometimes, not the entire page is > retrieved, and then we get those accesses with an odd size. > > > I think the best thing to do is to either force the caller to read > > a power of two (pref. >4 bytes), so either we need some form of flag > > to say this, or change the behaviour of the callers to never try this. > > If it's considered generally objectionable to call read_buf for > an amount of data that isn't a whole number of words, those two > approaches would work as well. Making nand_read_subpage align to > 4 bytes instead of 1/2 would be fairly simple. A flag would be a > bit messier. I think that if it is only a problem of reading the correct number of bytes for stuff like the OOB, then there's no problem in adding an fractional fixup after the readsl.
Index: ktrack/drivers/mtd/nand/s3c2410.c =================================================================== --- ktrack.orig/drivers/mtd/nand/s3c2410.c 2008-11-01 00:29:45.000000000 -0200 +++ ktrack/drivers/mtd/nand/s3c2410.c 2008-11-01 00:38:10.000000000 -0200 @@ -530,7 +530,14 @@ static void s3c2440_nand_read_buf(struct mtd_info *mtd, u_char *buf, int len) { struct s3c2410_nand_info *info = s3c2410_nand_mtd_toinfo(mtd); + readsl(info->regs + S3C2440_NFDATA, buf, len / 4); + if (unlikely(len & 3)) { + u32 data; + + data = readl(info->regs + S3C2440_NFDATA); + memcpy(buf + (len & ~3), &data, len & 3); + } } static void s3c2410_nand_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
In recent kernels, the optimized OOB reads in nand_read_subpage often make requests which aren't an exact multiple of a word, which caused nand/s3c2410.c to do a partial read, yielding chaos and mayhem. I'm not entirely sure whether nand_read_subpage is to blame for making non-word requests, or whether nand/s3c2410.c is to blame for not supporting them, but here's a patch for the latter. Note that s3c2410_nand_write_buf has a similar issue. RNDIN is used in nand_write_oob_syndrome, so this may also be a real problems. - Werner ---------------------------------- cut here ----------------------------------- fix-s3c-nand-read-bytes.patch With the introduction of optimized OOB reads in nand_read_subpage, the length of the data requested may not be a multiple of four bytes. This caused a partial read on the 2440, leading to false ECC errors and, worse, attempts to "correct" them. Note that there is a similar issue in s3c2440_nand_write_buf, which doesn't seem to cause trouble yet. Signed-off-by: Werner Almesberger <werner@openmoko.org> ---