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

[PATCH] 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 we do not attempt to read SR2 if the QE
bit is in SR1.

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>
---
 drivers/mtd/spi-nor/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index e2b6efafdd8d..ac909dfb9657 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -985,7 +985,10 @@ static int spi_nor_generic_quad_enable(struct spi_nor *nor)
 	if (!qe_mask[0] && !qe_mask[1])
 		return 0;
 
-	ret = spi_nor_read_sr1_and_sr2(nor, sr);
+	if (qe_mask[0])
+		ret = spi_nor_read_sr1(nor, sr);
+	else
+		ret = spi_nor_read_sr1_and_sr2(nor, sr);
 	if (ret)
 		return ret;
 

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

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

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

[Michael Walle]

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

On Fri Sep 11, 2026 at 10:55 AM CEST, Miquel Raynal wrote:
> 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 we do not attempt to read SR2 if the QE
> bit is in SR1.
>
> 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>
> ---
>  drivers/mtd/spi-nor/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index e2b6efafdd8d..ac909dfb9657 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -985,7 +985,10 @@ static int spi_nor_generic_quad_enable(struct spi_nor *nor)
>  	if (!qe_mask[0] && !qe_mask[1])
>  		return 0;
>  
> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
> +	if (qe_mask[0])
> +		ret = spi_nor_read_sr1(nor, sr);
> +	else
> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);

Shouldn't that be the other way around?

+	if (qe_mask[1])
+		ret = spi_nor_read_sr1_and_sr2(nor, sr);
+	else
+		ret = spi_nor_read_sr1(nor, sr);

I know qe_mask won't be spread across two SRs. Just to be
correct and make it easier to grok.

-michael

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

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

[Miquel Raynal]

>> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
>> +	if (qe_mask[0])
>> +		ret = spi_nor_read_sr1(nor, sr);
>> +	else
>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>
> Shouldn't that be the other way around?
>
> +	if (qe_mask[1])
> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
> +	else
> +		ret = spi_nor_read_sr1(nor, sr);
>
> I know qe_mask won't be spread across two SRs. Just to be
> correct and make it easier to grok.

Why is the second version easier to parse? First version is maybe more
future proof since we are treating a specific case. Because either it is
a specific chip (Mxic) or we end up in the more common case. There is
only one QER entry for qe_mask[0], many for qe_mask[1], and chances are
that in the future we will mostly see new chips only using a qe_mask[1]
mask. So having this in the else doesn't sound so strange to me? I also
don't think we will ever see masks spanning the two registers.

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

[Michael Walle]

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

On Fri Sep 11, 2026 at 11:19 AM CEST, Miquel Raynal wrote:
>
>>> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>> +	if (qe_mask[0])
>>> +		ret = spi_nor_read_sr1(nor, sr);
>>> +	else
>>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>
>> Shouldn't that be the other way around?
>>
>> +	if (qe_mask[1])
>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>> +	else
>> +		ret = spi_nor_read_sr1(nor, sr);
>>
>> I know qe_mask won't be spread across two SRs. Just to be
>> correct and make it easier to grok.
>
> Why is the second version easier to parse? First version is maybe more
> future proof since we are treating a specific case. Because either it is
> a specific chip (Mxic) or we end up in the more common case. There is
> only one QER entry for qe_mask[0], many for qe_mask[1], and chances are
> that in the future we will mostly see new chips only using a qe_mask[1]
> mask. So having this in the else doesn't sound so strange to me? I also
> don't think we will ever see masks spanning the two registers.

Because for the first one you'll have to know that qe_mask wont span
two different registers, where with my proposal, you don't have to
have that knowledge, no?

If qe_mask[1] is used regardless of qe_mask[0], you know you have to
write sr2, otherwise you'd be skipping something.

With the former, qe_mask[1] might or might not be 0 and you could
end up in spi_nor_read_sr1().

What do you mean with future prove? In the end it's the very same
logic iff there will only be one bit set in qe_mask[] and
sizeof(qe_mask) == 2.

-michael

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

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

[Miquel Raynal]

On 11/09/2026 at 11:19:39 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

>>> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>> +	if (qe_mask[0])
>>> +		ret = spi_nor_read_sr1(nor, sr);
>>> +	else
>>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>
>> Shouldn't that be the other way around?
>>
>> +	if (qe_mask[1])
>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>> +	else
>> +		ret = spi_nor_read_sr1(nor, sr);
>>
>> I know qe_mask won't be spread across two SRs. Just to be
>> correct and make it easier to grok.
>
> Why is the second version easier to parse? First version is maybe more
> future proof since we are treating a specific case. Because either it is
> a specific chip (Mxic) or we end up in the more common case. There is
> only one QER entry for qe_mask[0], many for qe_mask[1], and chances are
> that in the future we will mostly see new chips only using a qe_mask[1]
> mask. So having this in the else doesn't sound so strange to me? I also
> don't think we will ever see masks spanning the two registers.

The problem still persists because there is the check which also fails
reading SR2.

Michael, we moved to swp.c that handling, introducing a "careful"
revision, was this actually relevant? Shouldn't we have the handling of
the lack of SR2 read back into the main core like before?

Possible steps forward:
1- Add extra logic in the "and_check" helper, kind of duplicating what
   is in the swp.c file (not my favourite)
2- Move the "careful" handling (which does not return errors when a
   register read is absent) back from swp.c into core.c.

What do you prefer?

Thanks,
Miquèl

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

[Michael Walle]

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

On Fri Sep 11, 2026 at 11:34 AM CEST, Miquel Raynal wrote:
> On 11/09/2026 at 11:19:39 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>>>> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>>> +	if (qe_mask[0])
>>>> +		ret = spi_nor_read_sr1(nor, sr);
>>>> +	else
>>>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>>
>>> Shouldn't that be the other way around?
>>>
>>> +	if (qe_mask[1])
>>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>> +	else
>>> +		ret = spi_nor_read_sr1(nor, sr);
>>>
>>> I know qe_mask won't be spread across two SRs. Just to be
>>> correct and make it easier to grok.
>>
>> Why is the second version easier to parse? First version is maybe more
>> future proof since we are treating a specific case. Because either it is
>> a specific chip (Mxic) or we end up in the more common case. There is
>> only one QER entry for qe_mask[0], many for qe_mask[1], and chances are
>> that in the future we will mostly see new chips only using a qe_mask[1]
>> mask. So having this in the else doesn't sound so strange to me? I also
>> don't think we will ever see masks spanning the two registers.
>
> The problem still persists because there is the check which also fails
> reading SR2.

I don't get it. Why would you read SR2 if bit6 is set in qe_mask[0]?
Or do you mean in the spi_nor_write_sr1_and_sr2_and_check(). Yeah
that should also probably only write SR1 if qe_mask[1] == 0, no?

> Michael, we moved to swp.c that handling, introducing a "careful"
> revision, was this actually relevant?

Yes, because I eventually want to get rid of that sr2 QE bit
guessing. I really can't imagine that this is necessary.

> the lack of SR2 read back into the main core like before?
>
> Possible steps forward:
> 1- Add extra logic in the "and_check" helper, kind of duplicating what
>    is in the swp.c file (not my favourite)
> 2- Move the "careful" handling (which does not return errors when a
>    register read is absent) back from swp.c into core.c.
>
> What do you prefer?

3- make the generic_quad_enable differentiate between "qe bit is in
SR1" or "qe bit is in SR2". If it's in SR1, just use
spi_nor_{read,write}_sr1. if it's in SR2, keep the current handling.

Do I miss something?

-michael

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

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

[Miquel Raynal]

On 11/09/2026 at 12:00:08 +02, "Michael Walle" <mwalle@kernel.org> wrote:

> On Fri Sep 11, 2026 at 11:34 AM CEST, Miquel Raynal wrote:
>> On 11/09/2026 at 11:19:39 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>>>>> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>>>> +	if (qe_mask[0])
>>>>> +		ret = spi_nor_read_sr1(nor, sr);
>>>>> +	else
>>>>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>>>
>>>> Shouldn't that be the other way around?
>>>>
>>>> +	if (qe_mask[1])
>>>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>>> +	else
>>>> +		ret = spi_nor_read_sr1(nor, sr);
>>>>
>>>> I know qe_mask won't be spread across two SRs. Just to be
>>>> correct and make it easier to grok.
>>>
>>> Why is the second version easier to parse? First version is maybe more
>>> future proof since we are treating a specific case. Because either it is
>>> a specific chip (Mxic) or we end up in the more common case. There is
>>> only one QER entry for qe_mask[0], many for qe_mask[1], and chances are
>>> that in the future we will mostly see new chips only using a qe_mask[1]
>>> mask. So having this in the else doesn't sound so strange to me? I also
>>> don't think we will ever see masks spanning the two registers.
>>
>> The problem still persists because there is the check which also fails
>> reading SR2.
>
> I don't get it. Why would you read SR2 if bit6 is set in qe_mask[0]?
> Or do you mean in the spi_nor_write_sr1_and_sr2_and_check(). Yeah
> that should also probably only write SR1 if qe_mask[1] == 0, no?

Yes, probably. My problem is that I initially thought the big
refactoring in a way that would make the users of those helpers no
longer aware of the subtleties. But since you insisted on moving some
parts to swp.c, it broke my assumptions and also the kernel doc comments
which are no longer relevant. And now we are back to the initial
situation, where awareness is needed from the callers and the API is no
longer balanced:
- spi_nor_read_sr1_and_sr2 is not safe
- spi_nor_write_sr1_and_sr2 is safe
- spi_nor_write_sr1_and_sr2_and_check is no longer safe
I dislike this kind of lack of uniformity. One of my initial
motivations was to get rid of them.

>> Possible steps forward:
>> 1- Add extra logic in the "and_check" helper, kind of duplicating what
>>    is in the swp.c file (not my favourite)
>> 2- Move the "careful" handling (which does not return errors when a
>>    register read is absent) back from swp.c into core.c.
>>
>> What do you prefer?
>
> 3- make the generic_quad_enable differentiate between "qe bit is in
> SR1" or "qe bit is in SR2". If it's in SR1, just use
> spi_nor_{read,write}_sr1. if it's in SR2, keep the current handling.
>
> Do I miss something?

Well, no, but this is backwards. We reintroduce complexity in a path
which does not need this complexity.

I drafted another proposal:

4- Make spi_nor_read_sr1_and_sr2() immune to the SR2 absence. swp.c
still uses its _careful() version. Guard the fact that we cannot have
write_sr2 but no read_sr2. This way the API is even on the SR2 handling,
the code in the core stays very simple, we do not leak the awful swp
handling in the generic path and the SR1-only case is perfectly handled
in the generic helper without more care (which was my initial intent).

Link: https://lore.kernel.org/linux-mtd/20260911-perso-fix-spi-nor-qe-mxic-v2-1-70c324e9f30e@bootlin.com/T/#u # v2

Thanks,
Miquèl

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

[sashiko-bot]

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

New issues:
- [High] Unintended Register Corruption for Flashes with `qe_mask[0]` and an Active SR2.

Pre-existing issues:
- [High] spi_nor_write_sr1_and_sr2_and_check() still fails unconditionally for flashes without SR2 if the QE bit needs to be set.
--

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

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

The patch modifies spi_nor_generic_quad_enable to only read SR1 when
qe_mask[0] is set. This aims to fix probe failures on flashes lacking
an SR2.

Closes: 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..ac909dfb9657c 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -985,7 +985,10 @@ static int spi_nor_generic_quad_enable(struct spi_nor *nor)
>  	if (!qe_mask[0] && !qe_mask[1])
>  		return 0;
>  
> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
> +	if (qe_mask[0])
> +		ret = spi_nor_read_sr1(nor, sr);

[Severity: High]
If qe_mask[0] is set and the flash has an active SR2, does this leave sr[1]
unpopulated as 0x00 without ensuring the flash actually lacks a two-byte
write opcode?

Later in this function, if the QE bit is not already set, it calls:

    return spi_nor_write_sr1_and_sr2_and_check(nor, sr);

This will invoke the helper which executes the two-byte write_sr1_and_sr2
opcode if set, writing the unpopulated sr[1] (0x00) to the flash's SR2/CR.
Can this corrupt the flash state and fail the validation check if the second
byte reads back as non-zero?

> +	else
> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>  	if (ret)
>  		return ret;
>  

[Severity: High]
This is a pre-existing issue, but does this still fail unconditionally for
flashes without SR2 if the QE bit needs to be set?

If the QE bit is not already set, spi_nor_generic_quad_enable() proceeds to
call spi_nor_write_sr1_and_sr2_and_check(), which reads both registers for
verification:

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

Because read_sr2 is 0 for flashes lacking SR2, spi_nor_read_sr2() returns
-EINVAL. Doesn't this mean the generic helper still fails with -EINVAL during
verification?

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

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

[Jon Hunter]

On 11/09/2026 09:55, Miquel Raynal wrote:
> 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 we do not attempt to read SR2 if the QE
> bit is in SR1.
> 
> 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>
> ---
>   drivers/mtd/spi-nor/core.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index e2b6efafdd8d..ac909dfb9657 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -985,7 +985,10 @@ static int spi_nor_generic_quad_enable(struct spi_nor *nor)
>   	if (!qe_mask[0] && !qe_mask[1])
>   		return 0;
>   
> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
> +	if (qe_mask[0])
> +		ret = spi_nor_read_sr1(nor, sr);
> +	else
> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>   	if (ret)
>   		return ret;

I know that there is still some discussion going on about this
patch, but FWIW this does fix the issue I was seeing.

Thanks
Jon

-- 
nvpublic

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

[Michael Walle]

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

On Fri Sep 11, 2026 at 12:33 PM CEST, Jon Hunter wrote:
>
> On 11/09/2026 09:55, Miquel Raynal wrote:
>> 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 we do not attempt to read SR2 if the QE
>> bit is in SR1.
>> 
>> 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>
>> ---
>>   drivers/mtd/spi-nor/core.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index e2b6efafdd8d..ac909dfb9657 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -985,7 +985,10 @@ static int spi_nor_generic_quad_enable(struct spi_nor *nor)
>>   	if (!qe_mask[0] && !qe_mask[1])
>>   		return 0;
>>   
>> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
>> +	if (qe_mask[0])
>> +		ret = spi_nor_read_sr1(nor, sr);
>> +	else
>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>   	if (ret)
>>   		return ret;
>
> I know that there is still some discussion going on about this
> patch, but FWIW this does fix the issue I was seeing.

Thanks, could you give the new version [1] a quick test and a
Tested-by tag?

-michael

[1] https://lore.kernel.org/r/20260911-perso-fix-spi-nor-qe-mxic-v2-1-70c324e9f30e@bootlin.com/

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