Re: [PATCH v4] mtd: spinand: cache the last read page to avoid redundant SPI operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Partial page reads with software/host ECC cause silent data corruption on subsequent cache hits due to incomplete cache updates.
- [Critical] Cache hit logic fails to check the Logical Unit Number (LUN), leading to cross-LUN cache aliasing.
- [High] Missing cache invalidation on OOB/OTP writes causes cached reads to return clobbered data.
- [High] Error handling leaves cache valid with uncorrected data on non-EBADMSG errors.
--

commit 5e46633e4cdf72f3fb112f28479efe0c2e5ed5d8
Author: Zongzhen Feng <1768315307@qq.com>

mtd: spinand: cache the last read page to avoid redundant SPI operations

This patch caches the last successfully read page in the SPI NAND driver to
avoid redundant SPI read operations when accessing smaller sectors (like 512
bytes) within a larger page. It serves repeated reads directly from a bounce
buffer and attempts to invalidate the cache during writes, erases, and
errors.

> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 43bbe2629b85b..a742a1cb572a4 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
[ ... ]
> @@ -577,6 +578,13 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
>  			       req->ooblen);
>  	}
>  
> +	if (req->datalen && !req->continuous && req->mode != MTD_OPS_RAW) {
> +		spinand->cur_target_cache = req->pos.target;
> +		spinand->cur_block_cache = req->pos.eraseblock;
> +		spinand->cur_page_cache = req->pos.page;
> +		spinand->cache_valid = true;
> +	}
> +

[Severity: Critical]
Is it necessary to include the Logical Unit Number (LUN) in this cache logic?

The SPI NAND subsystem supports devices with multiple LUNs per target. The
nand_pos structure includes a lun field to address these, but this mechanism
tracks identity using only the target, eraseblock, and page.

Could this cause a read from one LUN to falsely hit the cache and serve data
from a different LUN when accessing the exact same eraseblock and page index?

>  	return 0;
>  }
>  
> @@ -780,7 +788,12 @@ int spinand_read_page(struct spinand_device *spinand,
>  	if (ret)
>  		return ret;
>  
> -	return nand_ecc_finish_io_req(nand, (struct nand_page_io_req *)req);
> +	ret = nand_ecc_finish_io_req(nand, (struct nand_page_io_req *)req);
> +	if (ret > 0 && req->datalen && !req->continuous && req->mode != MTD_OPS_RAW)
> +		memcpy(spinand->databuf + req->dataoffs, req->databuf.in,
> +		       req->datalen);
> +
> +	return ret;
>  }

[Severity: Critical]
Are partial page reads handled correctly here?

When a partial page read occurs, the full page is loaded into the cache and
marked valid, but only the requested req->datalen bytes are corrected by the
ECC engine and explicitly copied back to spinand->databuf. The remainder of
the cached page remains raw and uncorrected.

Will subsequent reads requesting a different chunk of the same page hit the
cache, bypass the SPI read and ECC correction, and return uncorrected data?

[ ... ]
> @@ -849,6 +862,20 @@ static int spinand_mtd_regular_page_read(struct mtd_info *mtd, loff_t from,
>  		if (disable_ecc)
>  			iter.req.mode = MTD_OPS_RAW;
>  
> +		if (spinand->cache_valid && !disable_ecc &&
> +		    !iter.req.ooblen &&
> +		    iter.req.pos.target == spinand->cur_target_cache &&
> +		    iter.req.pos.eraseblock == spinand->cur_block_cache &&
> +		    iter.req.pos.page == spinand->cur_page_cache) {
> +			if (iter.req.datalen)
> +				memcpy(iter.req.databuf.in,
> +				       spinand->databuf + iter.req.dataoffs,
> +				       iter.req.datalen);
> +			ops->retlen += iter.req.datalen;
> +			ops->oobretlen += iter.req.ooblen;
> +			continue;
> +		}
> +
>  		ret = spinand_select_target(spinand, iter.req.pos.target);
>  		if (ret)
>  			break;
> @@ -858,6 +885,9 @@ static int spinand_mtd_regular_page_read(struct mtd_info *mtd, loff_t from,
>  		if (ret < 0 && ret != -EBADMSG)
>  			break;
>  
> +		if (ret == -EBADMSG)
> +			spinand->cache_valid = false;
> +

[Severity: High]
Could the cache be left valid with uncorrected data if a non-EBADMSG error
occurs?

In spinand_read_from_cache_op(), raw data is loaded into the cache buffer and
spinand->cache_valid is prematurely set to true. If the subsequent
nand_ecc_finish_io_req() fails with a non-EBADMSG error (such as -ERANGE from
software BCH ECC layout parsing, or -ETIMEDOUT from a hardware engine),
spinand_read_page() propagates this error.

When the loop breaks here on ret < 0 && ret != -EBADMSG, the cache is not
invalidated.

Will a subsequent access to the same page incorrectly hit the cache and return
the uncorrected data as a success?

[ ... ]
> @@ -1080,6 +1110,8 @@ static int spinand_mtd_write(struct mtd_info *mtd, loff_t to,
>  		if (ret)
>  			break;
>  
> +		spinand->cache_valid = false;
> +
>  		ret = spinand_write_page(spinand, &iter.req);
>  		if (ret)
>  			break;

[Severity: High]
Are other write paths missing cache invalidation?

While this patch correctly invalidates the cache in spinand_mtd_write() and
spinand_erase(), other write paths like spinand_markbad() and
spinand_otp_rw() also invoke spinand_write_page().

Because spinand_write_to_cache_op() unconditionally overwrites the shared
data buffer (spinand->databuf) with 0xFFs during any write operation, this
can cause the cache buffer to be clobbered while spinand->cache_valid
remains true.

Will subsequent reads to the cached coordinates falsely hit the cache and
return clobbered data instead of the actual page contents?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_522792EE6F63A5581C238FF17E9862F10A08@qq.com?part=1