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