| Message ID | 20210531181757.19458-6-p.yadav@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid odd length/address read/writes in 8D-8D-8D mode. | expand |
On 5/31/21 9:17 PM, Pratyush Yadav wrote: Hi! > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Octal DTR capable flashes like Micron Xcella reads cannot start or > end at an odd address in Octal DTR mode. Extra bytes need to be read at > the start or end to make sure both the start address and length remain > even. > > To avoid allocating too much extra memory, thereby putting unnecessary > memory pressure on the system, the temporary buffer containing the extra > padding bytes is capped at PAGE_SIZE bytes. The rest of the 2-byte > aligned part should be read directly in the main buffer. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > Reviewed-by: Michael Walle <michael@walle.cc> > > --- > > Changes in v2: > - Replace semicolon in subject with colon. > - Add a comment that ret < 0 after adjusting for extra bytes is not > possible, and add a WARN_ON() on the condition to make sure it gets > spotted quickly when some change triggers this bug. > - Add Michael's R-by. > > drivers/mtd/spi-nor/core.c | 82 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 81 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index d521ca577884..a696af6a1b71 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1904,6 +1904,83 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > return ERR_PTR(-ENODEV); > } > > +/* > + * On Octal DTR capable flashes like Micron Xcella reads cannot start or > + * end at an odd address in Octal DTR mode. Extra bytes need to be read > + * at the start or end to make sure both the start address and length > + * remain even. > + */ > +static int spi_nor_octal_dtr_read(struct spi_nor *nor, loff_t from, size_t len, > + u_char *buf) > +{ > + u_char *tmp_buf; > + size_t tmp_len; > + loff_t start, end; > + int ret, bytes_read; > + > + if (IS_ALIGNED(from, 2) && IS_ALIGNED(len, 2)) > + return spi_nor_read_data(nor, from, len, buf); > + else if (IS_ALIGNED(from, 2) && len > PAGE_SIZE) > + return spi_nor_read_data(nor, from, round_down(len, PAGE_SIZE), > + buf); > + > + tmp_buf = kmalloc(PAGE_SIZE, GFP_KERNEL); > + if (!tmp_buf) > + return -ENOMEM; > + > + start = round_down(from, 2); > + end = round_up(from + len, 2); > + > + /* > + * Avoid allocating too much memory. The requested read length might be > + * quite large. Allocating a buffer just as large (slightly bigger, in > + * fact) would put unnecessary memory pressure on the system. > + * > + * For example if the read is from 3 to 1M, then this will read from 2 > + * to 4098. The reads from 4098 to 1M will then not need a temporary > + * buffer so they can proceed as normal. > + */ > + tmp_len = min_t(size_t, end - start, PAGE_SIZE); > + > + ret = spi_nor_read_data(nor, start, tmp_len, tmp_buf); > + if (ret == 0) { > + ret = -EIO; > + goto out; > + } > + if (ret < 0) > + goto out; > + > + /* > + * More bytes are read than actually requested, but that number can't be > + * reported to the calling function or it will confuse its calculations. > + * Calculate how many of the _requested_ bytes were read. > + */ > + bytes_read = ret; > + > + if (from != start) > + ret -= from - start; > + > + /* > + * Only account for extra bytes at the end if they were actually read. > + * For example, if the total length was truncated because of temporary > + * buffer size limit then the adjustment for the extra bytes at the end > + * is not needed. > + */ > + if (start + bytes_read == end) > + ret -= end - (from + len); > + > + /* Should not be possible. */ > + if (WARN_ON(ret < 0)) { > + ret = -EIO; > + goto out; > + } then why do we keep it? What are the cases in which ret < 0? Cheers, ta > + > + memcpy(buf, tmp_buf + (from - start), ret); > +out: > + kfree(tmp_buf); > + return ret; > +} > + > static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, > size_t *retlen, u_char *buf) > { > @@ -1921,7 +1998,10 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, > > addr = spi_nor_convert_addr(nor, addr); > > - ret = spi_nor_read_data(nor, addr, len, buf); > + if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) > + ret = spi_nor_octal_dtr_read(nor, addr, len, buf); > + else > + ret = spi_nor_read_data(nor, addr, len, buf); > if (ret == 0) { > /* We shouldn't see 0-length reads */ > ret = -EIO; > -- > 2.30.0 >
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index d521ca577884..a696af6a1b71 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1904,6 +1904,83 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) return ERR_PTR(-ENODEV); } +/* + * On Octal DTR capable flashes like Micron Xcella reads cannot start or + * end at an odd address in Octal DTR mode. Extra bytes need to be read + * at the start or end to make sure both the start address and length + * remain even. + */ +static int spi_nor_octal_dtr_read(struct spi_nor *nor, loff_t from, size_t len, + u_char *buf) +{ + u_char *tmp_buf; + size_t tmp_len; + loff_t start, end; + int ret, bytes_read; + + if (IS_ALIGNED(from, 2) && IS_ALIGNED(len, 2)) + return spi_nor_read_data(nor, from, len, buf); + else if (IS_ALIGNED(from, 2) && len > PAGE_SIZE) + return spi_nor_read_data(nor, from, round_down(len, PAGE_SIZE), + buf); + + tmp_buf = kmalloc(PAGE_SIZE, GFP_KERNEL); + if (!tmp_buf) + return -ENOMEM; + + start = round_down(from, 2); + end = round_up(from + len, 2); + + /* + * Avoid allocating too much memory. The requested read length might be + * quite large. Allocating a buffer just as large (slightly bigger, in + * fact) would put unnecessary memory pressure on the system. + * + * For example if the read is from 3 to 1M, then this will read from 2 + * to 4098. The reads from 4098 to 1M will then not need a temporary + * buffer so they can proceed as normal. + */ + tmp_len = min_t(size_t, end - start, PAGE_SIZE); + + ret = spi_nor_read_data(nor, start, tmp_len, tmp_buf); + if (ret == 0) { + ret = -EIO; + goto out; + } + if (ret < 0) + goto out; + + /* + * More bytes are read than actually requested, but that number can't be + * reported to the calling function or it will confuse its calculations. + * Calculate how many of the _requested_ bytes were read. + */ + bytes_read = ret; + + if (from != start) + ret -= from - start; + + /* + * Only account for extra bytes at the end if they were actually read. + * For example, if the total length was truncated because of temporary + * buffer size limit then the adjustment for the extra bytes at the end + * is not needed. + */ + if (start + bytes_read == end) + ret -= end - (from + len); + + /* Should not be possible. */ + if (WARN_ON(ret < 0)) { + ret = -EIO; + goto out; + } + + memcpy(buf, tmp_buf + (from - start), ret); +out: + kfree(tmp_buf); + return ret; +} + static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) { @@ -1921,7 +1998,10 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, addr = spi_nor_convert_addr(nor, addr); - ret = spi_nor_read_data(nor, addr, len, buf); + if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) + ret = spi_nor_octal_dtr_read(nor, addr, len, buf); + else + ret = spi_nor_read_data(nor, addr, len, buf); if (ret == 0) { /* We shouldn't see 0-length reads */ ret = -EIO;