Re: [PATCH v2 1/2] mtd: spi-nand: Add read retry support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Cheng Ming,

On 14/11/2024 at 10:35:27 +08, Cheng Ming Lin <linchengming884@gmail.com> wrote:

> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
>
> When the host ECC fails to correct the data error of NAND device,
> there's a special read for data recovery method which host setups

Here is a suggestion for rewording the second part of your commit log:

... which can be setup by the host for the next read. There are several
retry levels that can be attempted until the lost data is recovered or
definitely assumed lost.

> for the next read retry mode and may recover the lost data by host
> ECC again.
>
> Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> ---
>  drivers/mtd/nand/spi/core.c | 35 +++++++++++++++++++++++++++++++++--
>  include/linux/mtd/spinand.h | 14 ++++++++++++++
>  2 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 4d76f9f71a0e..bd5339a308aa 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -672,10 +672,14 @@ static int spinand_mtd_regular_page_read(struct mtd_info *mtd, loff_t from,
>  	struct spinand_device *spinand = mtd_to_spinand(mtd);
>  	struct nand_device *nand = mtd_to_nanddev(mtd);
>  	struct nand_io_iter iter;
> +	struct mtd_ecc_stats old_stats;

Reverse christmas tree is nicer :)

>  	bool disable_ecc = false;
>  	bool ecc_failed = false;
> +	unsigned int retry_mode = 0;
>  	int ret;
>  
> +	old_stats = mtd->ecc_stats;
> +
>  	if (ops->mode == MTD_OPS_RAW || !mtd->ooblayout)
>  		disable_ecc = true;
>  
> @@ -687,18 +691,43 @@ static int spinand_mtd_regular_page_read(struct mtd_info *mtd, loff_t from,
>  		if (ret)
>  			break;
>  
> +read_retry:
>  		ret = spinand_read_page(spinand, &iter.req);
>  		if (ret < 0 && ret != -EBADMSG)
>  			break;
>  
> -		if (ret == -EBADMSG)
> +		if (ret == -EBADMSG && spinand->set_read_retry) {
> +			if (spinand->read_retries && (++retry_mode < spinand->read_retries)) {
> +				ret = spinand->set_read_retry(spinand, retry_mode);
> +				if (ret < 0) {
> +					ecc_failed = true;
> +					break;

What is this break gonna do? You're not in a loop. I don't think breaks
have any effect on if blocks.

> +				}
> +
> +				/* Reset ecc_stats; retry */
> +				mtd->ecc_stats = old_stats;
> +				goto read_retry;
> +			} else {
> +				/* No more retry modes; real failure */
> +				ecc_failed = true;
> +			}
> +		} else if (ret == -EBADMSG) {
>  			ecc_failed = true;
> -		else
> +		} else {
>  			*max_bitflips = max_t(unsigned int, *max_bitflips, ret);
> +		}
>  

Thanks,
Miquèl