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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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.

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