Re: [PATCH v2 2/2] mtd: spi-nor: issi: Add support for is25wx01g

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Locking operations (spi_nor_sr_lock/unlock) unconditionally fail with -EINVAL because read_sr2 is set to 0.
- [High] Missing Flag Status Register (FSR) handling leaves the WEL bit set, causing lock operations to fail with spurious -EIO errors.
--

commit e7f55264e73008ad9ab4912b2a3228b2d1b7b7bd
Author: Nuno Sá <nuno.sa@analog.com>

mtd: spi-nor: issi: Add support for is25wx01g

Add support for the ISSI IS25WX01G, a 1 Gbit (128 MiB) octal NOR flash.
The part is fully described by SFDP, so only the flash ID and a small
post_bfpt fixup are needed.

> diff --git a/drivers/mtd/spi-nor/issi.c b/drivers/mtd/spi-nor/issi.c
> index 2f057d731df2a..a0b46c298d53c 100644
> --- a/drivers/mtd/spi-nor/issi.c
> +++ b/drivers/mtd/spi-nor/issi.c
> @@ -29,6 +29,24 @@ static const struct spi_nor_fixups is25lp256_fixups = {
>  	.post_bfpt = is25lp256_post_bfpt_fixups,
>  };
>  
> +static int is25wx01g_post_bfpt_fixups(struct spi_nor *nor,
> +				      const struct sfdp_parameter_header *bfpt_header,
> +				      const struct sfdp_bfpt *bfpt)
> +{
> +	/*
> +	 * There is no Status Register 2 and no 35h command. And WRITE STATUS
> +	 * REGISTER takes a single data byte.
> +	 */
> +	nor->params->opcodes.write_sr1_and_sr2 = 0;
> +	nor->params->opcodes.read_sr2 = 0;

[Severity: High]
Does explicitly zeroing read_sr2 break the generic locking support?

When locking or unlocking the flash, the core calls
spi_nor_write_sr1_and_sr2_and_check(), which delegates to
spi_nor_read_sr1_and_sr2() to verify the lock bits. This helper
unconditionally calls spi_nor_read_sr2().

If read_sr2 is 0, spi_nor_read_sr2() immediately returns -EINVAL, causing
any attempt to lock or unlock the flash to unconditionally fail.

> +
> +	return 0;
> +}
> +
> +static const struct spi_nor_fixups is25wx01g_fixups = {
> +	.post_bfpt = is25wx01g_post_bfpt_fixups,
> +};
> +
[ ... ]
> @@ -120,6 +138,12 @@ static const struct flash_info issi_nor_parts[] = {
>  		.id = SNOR_ID(0x9d, 0x70, 0x19),
>  		.name = "is25wp256",
>  		.flags = SPI_NOR_QUAD_PP,
> +	}, {
> +		/* is25wx01g */
> +		.id = SNOR_ID(0x9d, 0x5b, 0x1b),
> +		.sector_size = SZ_128K,
> +		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
> +			 SPI_NOR_4BIT_BP | SPI_NOR_BP3_SR_BIT6,

[Severity: High]
Does enabling locking support without a Flag Status Register (FSR) clearing
mechanism cause spurious -EIO errors?

Because the driver enables SPI_NOR_HAS_LOCK without adding a custom late_init
or ready hook to clear the FSR (unlike the Micron implementations), the Write
Enable Latch (WEL) bit can remain stuck at 1 in the hardware register.

When spi_nor_write_sr1_and_sr2_and_check() reads back the status register to
verify the lock bits, it reads WEL=1 while the intended buffer has WEL=0. This
strict verification fails and returns -EIO, masking actual hardware status
errors.

For reference, the strict check in spi_nor_write_sr1_and_sr2_and_check()
looks like this:

    if (sr[0] != tmp[0] || sr[1] != tmp[1])
        return -EIO;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-mtd-spi-nor-new-issi-chip-v2-0-3cd4d7e434b2@analog.com?part=2

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