Re: [PATCH v2 5/6] mtd: spi-nor: core: avoid odd length/address reads on 8D-8D-8D mode

[Tudor.Ambarus]

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@xxxxxx>
> Reviewed-by: Michael Walle <michael@xxxxxxxx>
> 
> ---
> 
> 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
>