On Tue, Jan 30, 2024 at 10:14:16AM +0530, Venkatesh Yadav Abbarapu wrote:
> @@ -1444,28 +1465,66 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t
> from, size_t len,
> {
> struct spi_nor *nor = mtd_to_spi_nor(mtd);
> int ret;
> + u32 offset = from;
Declare offset as loff_t otherwise it's limited to 4GB. I don't know if
we support more than 4GB here but it's just weird and forces us to add
casts later...
> + u32 stack_shift = 0;
This variable is never used.
> + u32 read_len = 0;
> + u32 rem_bank_len = 0;
> + u8 bank;
> + u8 is_ofst_odd = 0;
If you wanted, you could make this bool true/false.
>
> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);
>
> + if ((nor->flags & SNOR_F_HAS_PARALLEL) && (offset & 1)) {
> + /* We can hit this case when we use file system like ubifs */
> + from = (loff_t)(from - 1);
> + len = (size_t)(len + 1);
These casts are unnecessary. Just write:
from--;
len++;
> + is_ofst_odd = 1;
> + }
> +
> while (len) {
> - loff_t addr = from;
> - size_t read_len = len;
> + if (nor->addr_width == 3) {
> + if (nor->flags & SNOR_F_HAS_PARALLEL) {
> + bank = (u32)from / (SZ_16M << 0x01);
> + rem_bank_len = ((SZ_16M << 0x01) *
> + (bank + 1)) - from;
> + } else {
> + bank = (u32)from / SZ_16M;
> + rem_bank_len = (SZ_16M * (bank + 1)) - from;
> + }
> + }
If nor->addr_width != 3 then rem_bank_len is zero and I think we're
going to have a problem...
> + offset = from;
> +
> + if (nor->flags & SNOR_F_HAS_STACKED) {
> + stack_shift = 1;
> + if (offset >= (mtd->size / 2)) {
> + offset = offset - (mtd->size / 2);
> + nor->spi->flags |= SPI_XFER_U_PAGE;
> + } else {
> + nor->spi->flags &= ~SPI_XFER_U_PAGE;
> + }
> + }
>
> -#ifdef CONFIG_SPI_FLASH_BAR
> - u32 remain_len;
> + if (nor->flags & SNOR_F_HAS_PARALLEL)
> + offset /= 2;
>
> - ret = write_bar(nor, addr);
> - if (ret < 0)
> - return log_ret(ret);
> - remain_len = (SZ_16M * (nor->bank_curr + 1)) - addr;
> + if (nor->addr_width == 3) {
> +#ifdef CONFIG_SPI_FLASH_BAR
> + ret = write_bar(nor, offset);
> + if (ret < 0)
> + return log_ret(ret);
> +#endif
> + }
>
> - if (len < remain_len)
> + if (len < rem_bank_len)
> read_len = len;
> else
> - read_len = remain_len;
> -#endif
> + read_len = rem_bank_len;
If rem_bank_len is zero then read_len is zero.
> +
> + ret = spi_nor_wait_till_ready(nor);
> + if (ret)
> + goto read_err;
>
> - ret = nor->read(nor, addr, read_len, buf);
> + ret = nor->read(nor, offset, read_len, buf);
> if (ret == 0) {
> /* We shouldn't see 0-length reads */
> ret = -EIO;
When read_len is zero we return -EIO.
> @@ -1474,8 +1533,15 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t
> from, size_t len,
> if (ret < 0)
> goto read_err;
>
> - *retlen += ret;
> - buf += ret;
> + if (is_ofst_odd == 1) {
> + memcpy(buf, (buf + 1), (len - 1));
This needs to be memmove(). memcpy() is undefined for overlapped
buffers.
> + *retlen += (ret - 1);
> + buf += ret - 1;
> + is_ofst_odd = 0;
> + } else {
> + *retlen += ret;
> + buf += ret;
> + }
> from += ret;
> len -= ret;
> }
regards,
dan carpenter
