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

[PATCH v2] 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 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 | 8 ++++++--
 drivers/mtd/spi-nor/sfdp.c | 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index e2b6efafdd8d..7bb197df5585 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,10 @@ 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]);
+
+	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 v2] 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:
- [High] spi_nor_read_sr1_and_sr2() leaves sr[1] uninitialized when read_sr2 is unsupported, leading to uninitialized stack memory use in callers and spurious -EIO errors.
--

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

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

Some flashes do set BFPT_DWORD15_QER_SR1_BIT6, which means they do not
have an SR2 to read from/write to. The generic QE helper has forgotten
about the Macronix case, which causes booting to fail. This fix makes
sure SR2 reads just return 0 if unsupported to accommodate this situation.

Link: https://lore.kernel.org/linux-mtd/178876719232.3543902.14451625037676421254.b4-ty@b4/T/#m5bc4ba6776436f2870ced0eb5789d229037ad840

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index e2b6efafdd8d0..7bb197df5585b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -864,7 +865,10 @@ 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]);
> +
> +	return ret;
>  }

[Severity: High]
Does this code leave sr[1] uninitialized when read_sr2 is unsupported?

In spi_nor_write_sr1_and_sr2_and_check(), an uninitialized u8 tmp[2] is
passed to spi_nor_read_sr1_and_sr2():

drivers/mtd/spi-nor/core.c:spi_nor_write_sr1_and_sr2_and_check() {
	u8 tmp[2];
	...
	ret = spi_nor_read_sr1_and_sr2(nor, tmp);
	if (ret)
		return ret;

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

For chips without SR2, tmp[1] remains uninitialized stack garbage and the
comparison sr[1] != tmp[1] can evaluate to true, returning a spurious -EIO
error. This causes quad-enable and device probing to fail for the exact
flashes the patch is attempting to fix.

Also, does this affect spi_nor_otp_lock_sr2()?

drivers/mtd/spi-nor/otp.c:spi_nor_otp_lock_sr2() {
	u8 sr[2];
	...
	ret = spi_nor_read_sr1_and_sr2(nor, sr);
	if (ret)
		return ret;

	/* no need to write the register if region is already locked */
	if (sr[1] & lock_bit)
		...
}

Here, sr[1] contains garbage, which is evaluated in the lock condition
check and potentially modified and written back to the device.

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

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

[Miquel Raynal]

Hello Michael,

On 11/09/2026 at 10:52:44 GMT, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] spi_nor_read_sr1_and_sr2() leaves sr[1] uninitialized when
> read_sr2 is unsupported, leading to uninitialized stack memory use in
> callers and spurious -EIO errors.

The annoyingly right Sashiko robot is correct :-)

The best way I see to make sure this does not appear, is to just add
this fallback to make sure when we read both registers we just get zero
instead of random data in the buffer. Again, the idea is to make sure
callers do not need to be "QER aware".

--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -867,6 +867,8 @@ int spi_nor_read_sr1_and_sr2(struct spi_nor *nor, u8 *sr)
 
        if (nor->params->opcodes.read_sr2)
                ret = spi_nor_read_sr2(nor, &sr[1]);
+       else
+               sr[1] = 0;
 
        return ret;
 }

What do you think?

Thanks,
Miquèl

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

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]

On Fri Sep 11, 2026 at 2:45 PM CEST, Miquel Raynal wrote:
> Hello Michael,
>
> On 11/09/2026 at 10:52:44 GMT, sashiko-bot@kernel.org wrote:
>
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] spi_nor_read_sr1_and_sr2() leaves sr[1] uninitialized when
>> read_sr2 is unsupported, leading to uninitialized stack memory use in
>> callers and spurious -EIO errors.
>
> The annoyingly right Sashiko robot is correct :-)

I actually had the same feedback, but then discarded it, because of
your comment in the function doc.

Here's what I wrote:

  But now we are lying to the user of spi_nor_read_sr1_and_sr2()
  because we might actually not read sr2 at all and just return 0 -
  or even worse any garbage the sr[1] was initialized with. And the
  user cannot even know if sr2 was actually read or not.

But can this actually happen somewhere? Except for the WIP bit,
otp.c and swp.c I don't see where we actually check for a bit in
the SRs. Everything else is for RMW and that shouldn't be writing
garbage as the expectation is that there is no flash with !read_sr2
&& write_sr2. 
I agree, that it might be uninitialized, but if that uninitialized
value is actually used somewhere, we'd have a bigger problem, as
that new value is now just made up by us.

> The best way I see to make sure this does not appear, is to just add
> this fallback to make sure when we read both registers we just get zero
> instead of random data in the buffer. Again, the idea is to make sure
> callers do not need to be "QER aware".
>
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -867,6 +867,8 @@ int spi_nor_read_sr1_and_sr2(struct spi_nor *nor, u8 *sr)
>  
>         if (nor->params->opcodes.read_sr2)
>                 ret = spi_nor_read_sr2(nor, &sr[1]);
> +       else
> +               sr[1] = 0;

Almost back to the original one :) At this point, I'm fine with
either.

-michael

>         return ret;
>  }
>
> What do you think?
>
> Thanks,
> Miquèl


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

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

[Miquel Raynal]

On 11/09/2026 at 15:18:21 +02, "Michael Walle" <mwalle@kernel.org> wrote:

> On Fri Sep 11, 2026 at 2:45 PM CEST, Miquel Raynal wrote:
>> Hello Michael,
>>
>> On 11/09/2026 at 10:52:44 GMT, sashiko-bot@kernel.org wrote:
>>
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>> - [High] spi_nor_read_sr1_and_sr2() leaves sr[1] uninitialized when
>>> read_sr2 is unsupported, leading to uninitialized stack memory use in
>>> callers and spurious -EIO errors.
>>
>> The annoyingly right Sashiko robot is correct :-)
>
> I actually had the same feedback, but then discarded it, because of
> your comment in the function doc.
>
> Here's what I wrote:
>
>   But now we are lying to the user of spi_nor_read_sr1_and_sr2()
>   because we might actually not read sr2 at all and just return 0 -
>   or even worse any garbage the sr[1] was initialized with. And the
>   user cannot even know if sr2 was actually read or not.
>
> But can this actually happen somewhere? Except for the WIP bit,
> otp.c and swp.c I don't see where we actually check for a bit in
> the SRs. Everything else is for RMW and that shouldn't be writing
> garbage as the expectation is that there is no flash with !read_sr2
> && write_sr2. 
> I agree, that it might be uninitialized, but if that uninitialized
> value is actually used somewhere, we'd have a bigger problem, as
> that new value is now just made up by us.

It's just annoying for the comparisons we make in the _and_check()
helpers.

>> The best way I see to make sure this does not appear, is to just add
>> this fallback to make sure when we read both registers we just get zero
>> instead of random data in the buffer. Again, the idea is to make sure
>> callers do not need to be "QER aware".
>>
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -867,6 +867,8 @@ int spi_nor_read_sr1_and_sr2(struct spi_nor *nor, u8 *sr)
>>  
>>         if (nor->params->opcodes.read_sr2)
>>                 ret = spi_nor_read_sr2(nor, &sr[1]);
>> +       else
>> +               sr[1] = 0;
>
> Almost back to the original one :) At this point, I'm fine with
> either.

This is in addition to this patch. Just to make sure the "_and_check"
comparisons are not broken because of stale stack variables.

The rest should be good.