[PATCH v3] mtd: spi-nor: Fix quad-enable for flashes with QER bit in SR1

[PATCH v3] mtd: spi-nor: Fix quad-enable for flashes with QER bit in SR1

[Miquel Raynal]

Some flashes (eg. from Macronix) do set BFPT_DWORD15_QER_SR1_BIT6, which
means they do not have an SR2 to read from/write to. The new generic QE
helper was supposed to accommodate this situation but in the last version
that got merged, parts of that specific handling has been moved to a
more contained location, swp.c (which needed most of the extra code),
yet the Macronix case has been forgotten about in that generic QE
handling helper. Booting with such flashes will always fail probing.

Fix the situation by making sure SR2 reads just return 0 if
unsupported. This is safe since there is no chip with a write SR2 path
but no read SR2 path (which is now enforced in the SFDP parsing step).

This way, callers still do not have to care about the internal device
capabilities. Calling sr1_and_sr2 read/write helpers is safe in both
directions (not risk to get a spurious error). The behavior for SR1-only
chips is respected, the complexity in the core kept to its minimum.

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Closes: https://lore.kernel.org/linux-mtd/178876719232.3543902.14451625037676421254.b4-ty@b4/T/#m5bc4ba6776436f2870ced0eb5789d229037ad840
Fixes: 63489002d397 ("mtd: spi-nor: Refactor Read Status/Write Status support")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
Changes in v3:
- Make sure the read helper returns a "valid" SR2 in the sense that it
  cannot be random data by returning 0 for chips that do not feature a
  read_sr2 opcode. The major thread was to get a wrong comparison
  (against the tmp variable) in the _and_check() helper.
- Link to v2: https://lore.kernel.org/r/20260911-perso-fix-spi-nor-qe-mxic-v2-1-70c324e9f30e@bootlin.com

Changes in v2:
- Change the approach, see v1 thread below.
- Link to v1: https://lore.kernel.org/r/20260911-perso-fix-spi-nor-qe-mxic-v1-1-fd6d91416a2a@bootlin.com
---
 drivers/mtd/spi-nor/core.c | 10 ++++++++--
 drivers/mtd/spi-nor/sfdp.c |  3 ++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index e2b6efafdd8d..62b53933927f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -849,7 +849,8 @@ int spi_nor_read_sr2(struct spi_nor *nor, u8 *sr2)
 
 /**
  * spi_nor_read_sr1_and_sr2() - Read SR1 then SR2
- * General purpose helper.
+ * General purpose helper, always safe to call. Will expectedly ignore
+ * SR2 on certain chips.
  *
  * @nor: the spi_nor structure
  * @sr: pointer to a valid 2-byte array
@@ -864,7 +865,12 @@ int spi_nor_read_sr1_and_sr2(struct spi_nor *nor, u8 *sr)
 	if (ret)
 		return ret;
 
-	return spi_nor_read_sr2(nor, &sr[1]);
+	if (nor->params->opcodes.read_sr2)
+		ret = spi_nor_read_sr2(nor, &sr[1]);
+	else
+		sr[1] = 0;
+
+	return ret;
 }
 
 /**
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index c21a6953db96..641f17ad51a0 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -656,7 +656,8 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 
 	/* opcodes sanity check */
 	WARN_ON(!params->opcodes.read_sr1 ||
-		(!params->opcodes.write_sr1 && !params->opcodes.write_sr1_and_sr2));
+		(!params->opcodes.write_sr1 && !params->opcodes.write_sr1_and_sr2) ||
+		(!params->opcodes.read_sr2 && params->opcodes.write_sr2));
 
 	dword = bfpt.dwords[SFDP_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK;
 	if (SFDP_MASK_CHECK(dword, BFPT_DWORD16_4B_ADDR_MODE_BRWR))

---
base-commit: 700bf34058ca7cd792236b1ba5caad3770d66208
change-id: 20260911-perso-fix-spi-nor-qe-mxic-d0513e9f4421

Best regards,
-- 
Miquel Raynal <miquel.raynal@bootlin.com>

Re: [PATCH v3] mtd: spi-nor: Fix quad-enable for flashes with QER bit in SR1

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Spurious -EIO failure during write verification in spi_nor_write_sr1_and_sr2_and_check() for flashes with QER=4 or 5 (e.g., BFPT_DWORD15_QER_SR2_BIT1_NO_RD).
--

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

mtd: spi-nor: Fix quad-enable for flashes with QER bit in SR1

This commit attempts to fix quad-enable for flashes lacking an SR2 register
by having the generic helper return 0 when SR2 reads are unsupported. It also
updates the SFDP parsing step to assert that no chip has an SR2 write path
without an SR2 read path.

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Closes: https://lore.kernel.org/linux-mtd/178876719232.3543902.14451625037676421254.b4-ty@b4/T/#m5bc4ba6776436f2870ced0eb5789d229037ad840
Fixes: 63489002d397 ("mtd: spi-nor: Refactor Read Status/Write Status support")

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index e2b6efafdd8d0..62b53933927f2 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -864,7 +865,12 @@ int spi_nor_read_sr1_and_sr2(struct spi_nor *nor, u8 *sr)
>  	if (ret)
>  		return ret;
>  
> -	return spi_nor_read_sr2(nor, &sr[1]);
> +	if (nor->params->opcodes.read_sr2)
> +		ret = spi_nor_read_sr2(nor, &sr[1]);
> +	else
> +		sr[1] = 0;

[Severity: High]
This is a pre-existing issue, but does forcing sr[1] to 0 here cause a
spurious -EIO failure downstream during write verification?

For flashes with QER=4 or 5 (e.g., BFPT_DWORD15_QER_SR2_BIT1_NO_RD), SFDP
parsing disables reading SR2 (read_sr2 = 0) but retains a 16-bit write path
via write_sr1_and_sr2 and sets qe_mask[1] = BIT(1).

During quad enable, sr[1] gets populated with BIT(1) and is written using the
16-bit command. However, spi_nor_write_sr1_and_sr2_and_check() verifies the
write by calling this function, which forcefully sets tmp[1] = 0.

The check if (sr[0] != tmp[0] || sr[1] != tmp[1]) then compares sr[1]=0x02
against tmp[1]=0x00, fails, and returns -EIO. This leaves probing completely
broken for these flashes.

> +
> +	return ret;
>  }
>  
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index c21a6953db960..641f17ad51a09 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -656,7 +656,8 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  
>  	/* opcodes sanity check */
>  	WARN_ON(!params->opcodes.read_sr1 ||
> -		(!params->opcodes.write_sr1 && !params->opcodes.write_sr1_and_sr2));
> +		(!params->opcodes.write_sr1 && !params->opcodes.write_sr1_and_sr2) ||
> +		(!params->opcodes.read_sr2 && params->opcodes.write_sr2));

[Severity: High]
This isn't a bug introduced by this patch, but does this sanity check fail to
account for write_sr1_and_sr2?

A chip can lack read_sr2 (read_sr2 = 0) and write_sr2 (write_sr2 = 0), passing
this WARN_ON, but still possess a valid SR2 write path via write_sr1_and_sr2.

For QER=4 or 5, this means we can write to SR2 without being able to read it,
which the WARN_ON fails to catch, giving a false sense of safety.

>  
>  	dword = bfpt.dwords[SFDP_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK;
>  	if (SFDP_MASK_CHECK(dword, BFPT_DWORD16_4B_ADDR_MODE_BRWR))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-perso-fix-spi-nor-qe-mxic-v3-1-2ecaefb2ef2c@bootlin.com?part=1

Re: [PATCH v3] mtd: spi-nor: Fix quad-enable for flashes with QER bit in SR1

[Miquel Raynal]

On 11/09/2026 at 14:22:54 GMT, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Spurious -EIO failure during write verification in
> spi_nor_write_sr1_and_sr2_and_check() for flashes with QER=4 or 5 (e.g.,
> BFPT_DWORD15_QER_SR2_BIT1_NO_RD).
> --
>
> commit 65f885da367814b98529830d97438f96b6f29aa0
> Author: Miquel Raynal <miquel.raynal@bootlin.com>
>
> mtd: spi-nor: Fix quad-enable for flashes with QER bit in SR1
>
> This commit attempts to fix quad-enable for flashes lacking an SR2 register
> by having the generic helper return 0 when SR2 reads are unsupported. It also
> updates the SFDP parsing step to assert that no chip has an SR2 write path
> without an SR2 read path.
>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Closes:
> https://lore.kernel.org/linux-mtd/178876719232.3543902.14451625037676421254.b4-ty@b4/T/#m5bc4ba6776436f2870ced0eb5789d229037ad840
> Fixes: 63489002d397 ("mtd: spi-nor: Refactor Read Status/Write Status support")
>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index e2b6efafdd8d0..62b53933927f2 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -864,7 +865,12 @@ int spi_nor_read_sr1_and_sr2(struct spi_nor *nor, u8 *sr)
>>  	if (ret)
>>  		return ret;
>>  
>> -	return spi_nor_read_sr2(nor, &sr[1]);
>> +	if (nor->params->opcodes.read_sr2)
>> +		ret = spi_nor_read_sr2(nor, &sr[1]);
>> +	else
>> +		sr[1] = 0;
>
> [Severity: High]
> This is a pre-existing issue, but does forcing sr[1] to 0 here cause a
> spurious -EIO failure downstream during write verification?
>
> For flashes with QER=4 or 5 (e.g., BFPT_DWORD15_QER_SR2_BIT1_NO_RD), SFDP
> parsing disables reading SR2 (read_sr2 = 0) but retains a 16-bit write path
> via write_sr1_and_sr2 and sets qe_mask[1] = BIT(1).
>
> During quad enable, sr[1] gets populated with BIT(1) and is written using the
> 16-bit command. However, spi_nor_write_sr1_and_sr2_and_check() verifies the
> write by calling this function, which forcefully sets tmp[1] = 0.
>
> The check if (sr[0] != tmp[0] || sr[1] != tmp[1]) then compares sr[1]=0x02
> against tmp[1]=0x00, fails, and returns -EIO. This leaves probing completely
> broken for these flashes.

Michael you thought we had circled back to the previous version, you
were almost right, but with this new feedback, now we have :-)

But I see an easy way forward that can keep the atrocious hack in swp.c
like you want:

--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -966,7 +966,10 @@ int spi_nor_write_sr1_and_sr2_and_check(struct spi_nor *nor, const u8 *sr)
        if (ret)
                return ret;
 
-       if (sr[0] != tmp[0] || sr[1] != tmp[1])
+       if (sr[0] != tmp[0])
+               return -EIO;
+
+       if (nor->params->opcodes.read_sr2 && sr[1] != tmp[1])
                return -EIO;
 
        return 0;

Basically we can just state that if we cannot read SR2 there is just no
point in comparing its content. It's probably more accurate anyway to do
it this way.

>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index c21a6953db960..641f17ad51a09 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -656,7 +656,8 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>>  
>>  	/* opcodes sanity check */
>>  	WARN_ON(!params->opcodes.read_sr1 ||
>> -		(!params->opcodes.write_sr1 && !params->opcodes.write_sr1_and_sr2));
>> +		(!params->opcodes.write_sr1 && !params->opcodes.write_sr1_and_sr2) ||
>> +		(!params->opcodes.read_sr2 && params->opcodes.write_sr2));
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this sanity check fail to
> account for write_sr1_and_sr2?
>
> A chip can lack read_sr2 (read_sr2 = 0) and write_sr2 (write_sr2 = 0), passing
> this WARN_ON, but still possess a valid SR2 write path via
> write_sr1_and_sr2.
>
> For QER=4 or 5, this means we can write to SR2 without being able to read it,
> which the WARN_ON fails to catch, giving a false sense of safety.

This one however is a false positive. We already handle that case
properly.

Miquèl