Re: [PATCH v2 10/17] mtd: spi-nor: winbond: Prepare the introduction of W35TxxNW parts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: spi-nor: winbond: missing Flag Register polling ignores P/E errors
--

commit eef1bdae97bf8ec6c5ee8f47f2358cc4351e258b
Author: Miquel Raynal <miquel.raynal@bootlin.com>

mtd: spi-nor: winbond: Prepare the introduction of W35TxxNW parts

This commit prepares for a new octal DTR family by adding callbacks for octal
DTR modes and dummy cycles configuration. It also mentions these chips feature
SECDEC and lack SR2, but they do have a flag register.

> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 51f6bcd3f8e5b..058a2951c184e 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -11,6 +11,13 @@
>  #define WINBOND_NOR_OP_RDEAR	0xc8	/* Read Extended Address Register */
>  #define WINBOND_NOR_OP_WREAR	0xc5	/* Write Extended Address Register */
>  #define WINBOND_NOR_OP_SELDIE	0xc2	/* Select active die */
> +#define WINBOND_NOR_OP_WR_VCR	0x81	/* Write VCR registers */
> +#define   WINBOND_NOR_VCR_IO_MODE_CFG_REG 0x00 /* IO mode configuration address (VCR) */
> +#define     WINBOND_NOR_VCR_IO_MODE_SSDR 0xFF
> +#define     WINBOND_NOR_VCR_IO_MODE_ODDR 0xE7
> +#define   WINBOND_NOR_VCR_DUMMY_CFG_REG 0x01 /* Dummy cycle configuration address (VCR) */
> +#define WINBOND_NOR_OP_RD_FR	0x70	/* Read Flag registers */
> +#define WINBOND_NOR_OP_CL_FR	0x50	/* Clear Flag registers */

[Severity: High]
Are these flag register opcodes omitted from the driver logic?

The opcodes WINBOND_NOR_OP_RD_FR and WINBOND_NOR_OP_CL_FR are defined here
for interacting with the Flag Register, but they do not appear to be used
anywhere.

[ ... ]

> @@ -252,6 +265,87 @@ static const struct spi_nor_fixups winbond_nor_ecc_configuration_fixups = {
>  	.post_sfdp = winbond_nor_ecc_configuration_post_sfdp_fixups,
>  };
>  

[ ... ]

> +static int winbond_w35txxnw_nor_late_init(struct spi_nor *nor)
> +{
> +	nor->params->set_octal_dtr = winbond_w35txxnw_nor_set_octal_dtr;
> +	nor->params->set_dummy = winbond_w35txxnw_nor_set_dummy;
> +	nor->params->opcodes.read_sr2 = 0;
> +	nor->params->opcodes.write_sr1_and_sr2 = 0;

[Severity: High]
Should a custom ready callback be assigned here to poll the Flag Register?

Since no custom callback is assigned, the driver falls back to the default
spi_nor_sr_ready, which only checks the SR1 WIP bit and does not check the 
Flag Register for errors.

Could this cause a regression where any program or erase operation that fails
on a W35TxxNW part (e.g., from wear or protection), as well as ECC errors,
are silently ignored? In some cases, uncleared errors leave the flash stuck,
and the OS will incorrectly assume data is persisted, leading to data
corruption.

Could we assign a custom ready callback (such as winbond_w35txxnw_nor_ready)
to correctly utilize the Flag Register?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004-winbond-master-spi-nor-w35t-v2-0-592f3300a70f@bootlin.com?part=10