Re: [PATCH 09/15] 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: volatile dummy cycle config lost on resume
--

commit 61a4c1418798804759dbb5ceb316bd56196b68ad
Author: Miquel Raynal <miquel.raynal@bootlin.com>
mtd: spi-nor: winbond: Prepare the introduction of W35TxxNW parts

This commit introduces support for the Winbond W35TxxNW octal DTR flash
family. It implements callbacks for entering octal DTR mode and configuring
the number of dummy cycles via volatile VCR registers.

> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c

[ ... ]

> +static int winbond_w35txxnw_nor_set_dummy(struct spi_nor *nor, u8 dummy_cycles)
> +{
> +	return winbond_w35txxnw_nor_write_vcr(nor, WINBOND_NOR_VCR_DUMMY_CFG_REG,
> +					      dummy_cycles);
> +}
> +
> +static int winbond_w35txxnw_nor_set_octal_dtr(struct spi_nor *nor, bool enable)
> +{
> +	enum spi_nor_protocol proto_aft;
> +	u8 *buf = nor->bouncebuf;
> +	u8 rdid_dummy_aft, val;
> +	int ret;
> +
> +	if (enable) {
> +		val = WINBOND_NOR_VCR_IO_MODE_ODDR;
> +		proto_aft = SNOR_PROTO_8_8_8_DTR;
> +		rdid_dummy_aft = 16;
> +	} else {
> +		val = WINBOND_NOR_VCR_IO_MODE_SSDR;
> +		proto_aft = SNOR_PROTO_1_1_1;
> +		rdid_dummy_aft = 0;
> +	}
> +
> +	ret = winbond_w35txxnw_nor_write_vcr(nor, WINBOND_NOR_VCR_IO_MODE_CFG_REG, val);
> +	if (ret)
> +		return ret;

[Severity: High]
Will this lose the dummy cycle configuration upon system resume?

When the system resumes, the SPI NOR subsystem invokes spi_nor_resume(),
which calls spi_nor_init(). The spi_nor_init() function calls
spi_nor_set_octal_dtr() to re-enable octal DTR mode, which invokes
winbond_w35txxnw_nor_set_octal_dtr():

spi_nor_resume()
  spi_nor_init()
    spi_nor_set_octal_dtr()
      winbond_w35txxnw_nor_set_octal_dtr()

Since winbond_w35txxnw_nor_set_dummy() writes to a volatile register
(WINBOND_NOR_VCR_DUMMY_CFG_REG), that configuration is lost during sleep.

Because the core does not call the nor->params->set_dummy() callback on
resume, and winbond_w35txxnw_nor_set_octal_dtr() does not explicitly
reconfigure the dummy cycles when Octal DTR is re-enabled, will the SPI
controller and the flash chip disagree on the number of dummy cycles used
for read operations?

Can this cause data corruption on read operations after a system
suspend/resume cycle?

> +
> +	/* Read flash ID to make sure the switch was successful */
> +	ret = spi_nor_read_id(nor, 0, rdid_dummy_aft, buf, proto_aft);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-winbond-master-spi-nor-w35t-v1-0-a22a2f95c26e@bootlin.com?part=9