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

[Dan Carpenter]

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