Re: [PATCH v2] mtd: spi-nor: clear the default RDCR opcode when entering Octal DTR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mtd: spi-nor: compilation error accessing non-existent opcodes.read_sr2
- [High] mtd: spi-nor: modifying hallucinated opcode does not skip SR2 reads
- [Medium] mtd: spi-nor: permanent breakage of SR2 reads due to missing state restore
--

commit b3ebcb1264c5eaab828caba13b5a6ee56af80aa5
Author: Haibo Chen <haibo.chen@nxp.com>

mtd: spi-nor: clear the default RDCR opcode when entering Octal DTR

This commit attempts to skip invalid SR2 reads when switching to Octal DTR
mode by clearing a default RDCR opcode. It updates spi_nor_set_octal_dtr()
to clear read_sr2 on success.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 8bc117b46e028..0f47d869cbb48 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3238,10 +3238,16 @@ static int spi_nor_set_octal_dtr(struct spi_nor *nor, bool enable)
>  	if (ret)
>  		return ret;
>  
> -	if (enable)
> +	if (enable) {
>  		nor->reg_proto = SNOR_PROTO_8_8_8_DTR;
> -	else
> +		/*
> +		 * The default RDCR is not a valid SR2 read in Octal DTR mode;
> +		 * clear it so SR2 reads are skipped instead of being issued.
> +		 */
> +		nor->params->opcodes.read_sr2 = 0;

[Severity: High]
Can this code compile successfully? struct spi_nor_flash_parameter (which
nor->params points to) does not appear to contain an opcodes field, which
would cause a build failure here.

[Severity: High]
If this dynamic opcode field did exist, would modifying it actually prevent
the SR2 reads?

Looking at spi_nor_read_sr2(), it seems the reads rely entirely on statically
hardcoded macros rather than dynamic configuration fields:

drivers/mtd/spi-nor/core.c:spi_nor_read_sr2() {
    ...
	if (nor->spimem) {
		struct spi_mem_op op = SPI_NOR_RDSR2_OP(sr2);
    ...
	} else {
		ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_RDSR2, sr2,
						      1);
	}
    ...
}

Since SPI_NOR_RDSR2_OP and SPINOR_OP_RDSR2 are static constants, wouldn't
the reads still be issued regardless of this variable's state?

> +	} else {
>  		nor->reg_proto = SNOR_PROTO_1_1_1;
> +	}

[Severity: Medium]
When disabling Octal DTR, does this path also need to restore the cleared
opcode state?

If the opcode was zeroed when enabling the mode, dropping back to
SNOR_PROTO_1_1_1 without restoring it might leave SR2 reads permanently
disabled for the remaining lifetime of the device.

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-spi-nor-fix-v2-1-75440cfe4e76@nxp.com?part=1

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