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

[Miquel Raynal]

On 11/09/2026 at 14:22:54 GMT, sashiko-bot@xxxxxxxxxx wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Spurious -EIO failure during write verification in
> spi_nor_write_sr1_and_sr2_and_check() for flashes with QER=4 or 5 (e.g.,
> BFPT_DWORD15_QER_SR2_BIT1_NO_RD).
> --
>
> commit 65f885da367814b98529830d97438f96b6f29aa0
> Author: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
>
> mtd: spi-nor: Fix quad-enable for flashes with QER bit in SR1
>
> This commit attempts to fix quad-enable for flashes lacking an SR2 register
> by having the generic helper return 0 when SR2 reads are unsupported. It also
> updates the SFDP parsing step to assert that no chip has an SR2 write path
> without an SR2 read path.
>
> Reported-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> 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")
>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index e2b6efafdd8d0..62b53933927f2 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -864,7 +865,12 @@ 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]);
>> +	else
>> +		sr[1] = 0;
>
> [Severity: High]
> This is a pre-existing issue, but does forcing sr[1] to 0 here cause a
> spurious -EIO failure downstream during write verification?
>
> For flashes with QER=4 or 5 (e.g., BFPT_DWORD15_QER_SR2_BIT1_NO_RD), SFDP
> parsing disables reading SR2 (read_sr2 = 0) but retains a 16-bit write path
> via write_sr1_and_sr2 and sets qe_mask[1] = BIT(1).
>
> During quad enable, sr[1] gets populated with BIT(1) and is written using the
> 16-bit command. However, spi_nor_write_sr1_and_sr2_and_check() verifies the
> write by calling this function, which forcefully sets tmp[1] = 0.
>
> The check if (sr[0] != tmp[0] || sr[1] != tmp[1]) then compares sr[1]=0x02
> against tmp[1]=0x00, fails, and returns -EIO. This leaves probing completely
> broken for these flashes.

Michael you thought we had circled back to the previous version, you
were almost right, but with this new feedback, now we have :-)

But I see an easy way forward that can keep the atrocious hack in swp.c
like you want:

--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -966,7 +966,10 @@ int spi_nor_write_sr1_and_sr2_and_check(struct spi_nor *nor, const u8 *sr)
        if (ret)
                return ret;
 
-       if (sr[0] != tmp[0] || sr[1] != tmp[1])
+       if (sr[0] != tmp[0])
+               return -EIO;
+
+       if (nor->params->opcodes.read_sr2 && sr[1] != tmp[1])
                return -EIO;
 
        return 0;

Basically we can just state that if we cannot read SR2 there is just no
point in comparing its content. It's probably more accurate anyway to do
it this way.

>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index c21a6953db960..641f17ad51a09 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));
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this sanity check fail to
> account for write_sr1_and_sr2?
>
> A chip can lack read_sr2 (read_sr2 = 0) and write_sr2 (write_sr2 = 0), passing
> this WARN_ON, but still possess a valid SR2 write path via
> write_sr1_and_sr2.
>
> For QER=4 or 5, this means we can write to SR2 without being able to read it,
> which the WARN_ON fails to catch, giving a false sense of safety.

This one however is a false positive. We already handle that case
properly.

Miquèl