[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 at 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 at 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