Re: [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence

[Hendrik Donner <hd@os-cillation.de>]

Hello,

On 3/13/26 12:46, Pratyush Yadav wrote:
> On Fri, Mar 06 2026, Hendrik Donner wrote:
> 
>> Hello,
>>
>> On 2/23/26 10:29, Michael Walle wrote:
>>> Hi,
>>> On Mon Feb 23, 2026 at 10:17 AM CET, Sanjaikumar V S wrote:
>>>>> Raises concern about writes ending at odd offsets potentially
>>>>> having the same issue
>>>>
>>>> The odd end address case (trailing byte) is already handled in the
>>>> existing code at lines 243-255:
>>>>
>>>> /* Write out trailing byte if it exists. */
>>>> if (actual != len) {
>>>>       ret = spi_nor_write_enable(nor);
>>>>       ...
>>>>       ret = sst_nor_write_data(nor, to, 1, buf + actual);
>>>> }
>>> Ah, I must be blind. I stopped reading at the write_disable.
>>>
>>>> So write_enable is already called before writing the trailing
>>>> byte. My patch only addresses the odd start case where BP clears
>>>> WEL before the AAI sequence begins.
>>>>
>>>>> Suggests simplifying the conditional logic by removing the length
>>>>> check
>>>>
>>>> The condition `if (actual < len - 1)` avoids an unnecessary
>>>> write_enable when len == 1 (single byte write at odd address, no
>>>> AAI follows). But if you prefer unconditional write_enable for
>>>> simplicity, I can change it in v3.
>>> I know, but I actually don't like repeating the condition in the for
>>> loop. So I'd prefer to have a local "needs_write_enable" boolean
>>> which will be set to true. But then, I wouldn't care too much if
>>> there is a write enable followed by a write disable for a rare case.
>>>
>>>>> Notes the patch lacks runtime testing
>>>>
>>>> I don't have the hardware setup to test odd-address writes at the
>>>> moment. The fix is based on code analysis. I have tested patch 2/2
>>>> (dirmap fallback) on hardware.
>>> I'm hesitant - because like I said, if there is really a bug - it
>>> would have never worked correctly, since day 1. But yeah, I've also
>>> read the datasheet and it clearly states that the byte write will
>>> clear the write enable latch.
>>>
>>
>> i can confirm both patches fix real issues, i have similiar fixes
>> on a kernel tree i always wanted to clean up and upstream. Diffs
>> based on 6.6.127:
>>
>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>> index 197d2c1101ed5..eaa50561ede2c 100644
>> --- a/drivers/mtd/spi-nor/sst.c
>> +++ b/drivers/mtd/spi-nor/sst.c
>> @@ -155,6 +155,13 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>                  if (ret)
>>                          goto out;
>>
>> +               ret = spi_nor_write_enable(nor);
>> +               if (ret)
>> +                       goto out;
>> +               ret = spi_nor_wait_till_ready(nor);
>> +               if (ret)
>> +                       goto out;
>> +
>>                  to++;
>>                  actual++;
>>          }
>>
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 1b0c6770c14e4..646bfb2e91a65 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -276,7 +276,7 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
>>          if (spi_nor_spimem_bounce(nor, &op))
>>                  memcpy(nor->bouncebuf, buf, op.data.nbytes);
>>
>> -       if (nor->dirmap.wdesc) {
>> +       if (nor->dirmap.wdesc && nor->program_opcode != SPINOR_OP_AAI_WP) {
> 
> Why is this better? This removes the use of dirmap for all flashes other
> than SST.

i claim the opposite down below? That patch 2 of the posted patch series
looks better to me. Sorry if that was unclear.

Regards,
Hendrik

> 
>>                  nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, op.addr.val,
>>                                                op.data.nbytes, op.data.buf.out);
>>          } else {
>>
>>
>> I think patch 2 of this series is the better approach though.
> 
> There is a v4 here:
> https://lore.kernel.org/linux-mtd/20260311103057.29-1-sanjaikumarvs@gmail.com/T/#u
> 
> Can you please review and test it so we can apply it?
> 
>>
>> Regards,
>> Hendrik
>>
>>>> Please let me know if you'd like me to send a v3 with the
>>>> simplified unconditional write_enable.
>>> Please see above.
>>> -michael
>>> ______________________________________________________
>>> Linux MTD discussion mailing list
>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>