Re: [PATCH v2 3/3] mtd: rawnand: vf610_nfc: return an error when a command times out

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mtd: rawnand: vf610_nfc: -ETIMEDOUT swallowed in
  vf610_nfc_read_page()
--

commit 74e97975b0439f2b30c793e3dba498249fb4705a
Author: Mehmet Fide <mehmet.fide@screeningeagle.com>

mtd: rawnand: vf610_nfc: return an error when a command times out

This commit returns -ETIMEDOUT from vf610_nfc_done() on command timeout and
propagates the error through vf610_nfc_run(), vf610_nfc_cmd(), and the page
read and write hooks.

> diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/vf610_nfc.c
> index 547eb15138939..d6d4a18432ce8 100644
> --- a/drivers/mtd/nand/raw/vf610_nfc.c
> +++ b/drivers/mtd/nand/raw/vf610_nfc.c

[ ... ]

> @@ -572,7 +577,7 @@ static int vf610_nfc_read_page(struct nand_chip *chip, uint8_t *buf,
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	int trfr_sz = mtd->writesize + vf610_nfc_spare_size(mtd);
>  	u32 row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> -	int stat;
> +	int stat, ret;
>  
>  	vf610_nfc_select_target(chip, chip->cur_cs);
>  
> @@ -587,8 +592,10 @@ static int vf610_nfc_read_page(struct nand_chip *chip, uint8_t *buf,
>  	cmd2 |= code << CMD_CODE_SHIFT;
>  
>  	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
> -	vf610_nfc_run(nfc, 0, row, cmd1, cmd2, trfr_sz);
> +	ret = vf610_nfc_run(nfc, 0, row, cmd1, cmd2, trfr_sz);
>  	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Does vf610_nfc_read_page() swallow -ETIMEDOUT when a timeout occurs during
the raw re-read in vf610_nfc_correct_data()?

When hardware ECC reports a decode failure (such as on an erased page),
vf610_nfc_correct_data() re-reads the page and OOB via nand_read_page_op()
and nand_read_oob_op(), which now return -ETIMEDOUT through vf610_nfc_cmd():

drivers/mtd/nand/raw/vf610_nfc.c:vf610_nfc_correct_data() {
	...
	nfc->data_access = true;
	ret = nand_read_page_op(&nfc->chip, page, 0, dat, mtd->writesize);
	if (!ret)
		ret = nand_read_oob_op(&nfc->chip, page, 0, oob,
				       vf610_nfc_spare_size(mtd));
	nfc->data_access = false;
	if (ret)
		return ret;
	...
}

drivers/mtd/nand/raw/vf610_nfc.c:vf610_nfc_read_page() {
	...
	stat = vf610_nfc_correct_data(chip, buf, chip->oob_poi, page);

	if (stat < 0) {
		mtd->ecc_stats.failed++;
		return 0;
	} else {
	...
}

Because vf610_nfc_read_page() treats any negative stat as an uncorrectable
ECC failure, a timeout in vf610_nfc_correct_data() increments
mtd->ecc_stats.failed and returns 0, leaving the bogus ECC-corrected SRAM
data in buf and reporting -EBADMSG to the caller instead of aborting with
-ETIMEDOUT.  Only stat == -EBADMSG should be counted as an ECC failure,
while other negative errors should be returned directly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114192031.959376-1-mehmet.fide@gmail.com?part=3

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/