Re: [PATCH v3 08/17] mtd: spi-nor: Fix WRSR with ODTR chips - sashiko-bot

From: sashiko-bot@kernel.org
To: "Miquel Raynal" <miquel.raynal@bootlin.com>,
	"Pratyush Yadav" <pratyush@kernel.org>,
	"Michael Walle" <mwalle@kernel.org>,
	"Takahiro Kuwano" <takahiro.kuwano@infineon.com>,
	"Richard Weinberger" <richard@nod.at>,
	"Vignesh Raghavendra" <vigneshr@ti.com>
Cc: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Steam Lin" <STLin2@winbond.com>,
	linux-mtd@lists.infradead.org
Subject: Re: [PATCH v3 08/17] mtd: spi-nor: Fix WRSR with ODTR chips
Date: Tue, 13 Oct 2026 16:51:49 +0000	[thread overview]
Message-ID: <sashiko-outbox-170165@kernel.org> (raw)
In-Reply-To: <20261013-winbond-master-spi-nor-w35t-v3-8-6dde1d513df8@bootlin.com>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spi-nor: Unconditional -EINVAL for 2-byte writes in DTR mode breaks flash unlocking
--

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

mtd: spi-nor: Fix WRSR with ODTR chips

The ODTR interface requires an even number of bytes in each section of
the operation. spi_nor_spimem_setup_op() takes care of the opcode phase,
but we have to adapt the rest of the operation ourself.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0399e193a896d..a91e47eae2789 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -778,7 +778,8 @@ static int spi_nor_write_sr_ll(struct spi_nor *nor, u8 opcode, const u8 *sr,
>  {
>  	int ret, i;
>  
> -	if (len > 2)
> +	/* There is no sr1_and_sr2 in ODTR mode: this is a quad-only corner case */
> +	if (len > 2  || (nor->reg_proto == SNOR_PROTO_8_8_8_DTR && len > 1))
>  		return -EINVAL;

[Severity: High]
Does this unconditional return of -EINVAL break flash unlocking for ODTR chips?

When ODTR is enabled, this forces spi_nor_write_sr_ll() to return -EINVAL for
any write longer than 1 byte. However, spi_nor_write_sr1_and_sr2() directly
propagates this -EINVAL without falling back to individual spi_nor_write_sr1()
and spi_nor_write_sr2() calls if params->opcodes.write_sr1_and_sr2 is set:

drivers/mtd/spi-nor/core.c:spi_nor_write_sr1_and_sr2() {
    ...
    if (params->opcodes.write_sr1_and_sr2)
        return spi_nor_write_sr_ll(nor,
                                   params->opcodes.write_sr1_and_sr2,
                                   sr, 2);
    ...
}

Since not all ODTR chips explicitly clear write_sr1_and_sr2 (for example,
Spansion S28 parts, unless their SFDP table sets a QER that implicitly clears
it), operations like spi_nor_sr_unlock() will unconditionally fail with -EINVAL.

While the write_sr1_and_sr2 flag may be cleared manually later for certain
manufacturers (like Winbond), leaving the structural bug in
spi_nor_write_sr1_and_sr2() causes a complete failure instead of a fallback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-winbond-master-spi-nor-w35t-v3-0-6dde1d513df8@bootlin.com?part=8

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