Re: [PATCH v10 1/8] mtd: spi-nor: Add parallel and stacked memories support

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