[PATCH] mtd: spi-nor: Enable locking for n25q00a

Fri Oct 10 08:45:26 PDT 2025

On 10/9/25 19:07, Pratyush Yadav wrote:
> On Thu, Oct 09 2025, Sean Anderson wrote:
> 
>> On 10/8/25 08:30, Pratyush Yadav wrote:
>>> On Tue, Oct 07 2025, Sean Anderson wrote:
>>> 
>>>> On 10/7/25 09:15, Pratyush Yadav wrote:
>>>>> On Mon, Oct 06 2025, Sean Anderson wrote:
>>>>> 
>>>>>> The datasheet for n25q00a shows that the status register has the same
>>>>>> layout as for n25q00, so use the same flags to enable locking support.
>>>>>> These flags should have been added back in commit 150ccc181588 ("mtd:
>>>>>> spi-nor: Enable locking for n25q128a11"), but they were removed by the
>>>>>> maintainer...
>>>>> 
>>>>> This makes it sound like the maintainer did something wrong, which is
>>>>> not true. Tudor had a good reason for removing them.
>>>>
>>>> I disagree. The maintainer used his position of authority to make the
>>>> submitter second-guess their correct patch.
>>> 
>>> Sean, you are being very combative over such a small issue. You must
>>> test your changes. This is one of the most basic principles in software
>>> engineering. It was perfectly reasonable from Tudor to push back on
>>> untested changes.
>>> 
>>> There is no abuse of "position of authority" here. When things break, we
>>> get to do the work of putting the pieces back together. So of course, we
>>> are reluctant to take things that increase this burden for us. Having
>>> contributors test their changes is the simplest of things we ask for to
>>> keep the quality bar.
>>> 
>>> Beyond that, I'd say that a little politeness goes a long way in life.
>>> Especially towards the people maintaining the software for free that you
>>> (or your employer) use. We are both wasting our energy on this debate.
>>> Please stop. Take a step back and think from the other side's
>>> perspective. And try to work _with_ people, not against them.
>>> 
>>>>
>>>> These flashes have capacity of greater than the 8 MiB that can be
>>>> protected using 3 BP bits. Micron (and ST before them?) addressed this
>>>> by adding a fourth BP bit. This is consistent across every flash in this
>>>> series, and is clearly documented in every datasheet. Defaulting to 3
>>>> bits is buggy behavior: we should assume flashes behave per their
>>>> datasheets until proven otherwise, especially for less-popular features
>>> 
>>> If I had a euro every time I found a bug in a datasheet, well, I would
>>> have enough money to at least buy a nice dinner. My point is, datasheets
>>> are not perfect. Only running on real hardware gets you the true
>>> picture.
>>
>> Well, it's even *more* buggy to pretend that the datasheet doesn't exist
>> and just do whatever you please. Might as well reverse-engineer every
>> chip that comes across your desk from first principles with that
>> attitude.
> 
> ... or, you know, read the data sheet, write the driver, and _test_ if
> it actually works?

Which I did.

But apparently it's not enough to confirm that the datasheet does
describe the hardware.

Which is frankly absurd because the first generation chips have been
obsolete for a decade and there are literally dozens of variants of
these chips and none of them have any documented difference in the
status register.

The datasheet is innocent until proven guilty. While it's important to
verify its claims, it's up to *you* to prove it's wrong. You have given
me no evidence to believe it is incorrect.

And you would prefer to believe that the existing (broken!) behavior is
better? The one that clearly no one tested locking with because:

- Locking literally can't work with the "default" BP semantics. BP holds
  the log2 size of the protected area in eraseblocks plus one. The maximum number
  of eraseblocks that can be protected with 3 bits is 1 << (7 - 1) = 64. So
  if you have more than 64 eraseblocks (the smallest flash in this
  series has 128) you have to come up with different semantics:

  - Enlarge the protection granularity (e.g. to some multiple of the
    eraseblock size or to 
  - Add another bit to BP (what Micron did)

  Both of these are incompatible with existing behavior and need
  feature flags since they can't be discovered via SFDP. If you pretend
  that BP only has 3 bits, you will either not lock the whole flash (if
  you wanted to lock the whole thing) or you will lock too much of the
  flash (if you only wanted to lock part of it).

- Locking is broken based on the documentation. Every datasheet for
  flashes in this series clearly shows the 4th BP bit in the status
  register:

    https://media.digikey.com/pdf/Data%20Sheets/Micron%20Technology%20Inc%20PDFs/N25Q032A_RevJ.pdf
    This is the only exception because it has 64 eraseblocks and doesn't
    need an extra BP bit.
    https://media.digikey.com/pdf/Data%20Sheets/Micron%20Technology%20Inc%20PDFs/N25Q064Ax3.pdf
    https://media.digikey.com/pdf/Data%20Sheets/Micron%20Technology%20Inc%20PDFs/N25Q128A.pdf
    https://mm.digikey.com/Volume0/opasdata/d220001/medias/docus/6657/MT25QL256ABA8ESF-0SIT.pdf
    https://mm.digikey.com/Volume0/opasdata/d220001/medias/docus/789/N25Q256A.pdf
    https://mm.digikey.com/Volume0/opasdata/d220001/medias/docus/7156/mt25q-qlkt-l-512-abb-0.pdf
    https://www.digikey.com/htmldatasheets/production/1952775/0/0/1/N25Q512Ax3.pdf
    https://www.alldatasheet.com/datasheet-pdf/download/585916/MICRON/N25Q00AA.html
    https://mm.digikey.com/Volume0/opasdata/d220001/medias/docus/6242/MT25QL02GCBB.pdf
    https://www.digikey.com/htmldatasheets/production/2058522/0/0/1/n25q016a.pdf

    etc.

- Locking is broken based on my testing.

- Locking is broken whenever anyone else tests it on one of these
  flashes. And they all fix it in exactly the same way:

    f3f2b7eb2f1c ("mtd: spi-nor: Enable locking for n25q512ax3/n25q512a")
    f80ff13135cb ("mtd: spi-nor: micron-st: Enable locking for n25q00")
    bcc0c61e6134 ("mtd: spi-nor: micron-st: Enable locking for mt25qu256a")
    a2a3e5430e7b ("mtd: spi-nor: micron-st: enable lock/unlock for mt25qu512a")

- Locking is not in your suggested test procedure for new flashes
  (although it probably should be if you're so gung ho about mistrusting
  datasheets).

- None of the commits adding support for these chips report testing
  locking:

    c692ba6de1c5 ("mtd: spi-nor: micron-st: Add support for mt25qu01g")
    7f412111e276 ("mtd: spi-nor: Add entries for mt25q variants")
    bd8a6e31b87b ("mtd: spi-nor: Split mt25qu512a (n25q512a) entry into two")
    9607af6f857f ("mtd: spi-nor: Rename "n25q512a" to "mt25qu512a (n25q512a)"")
    21ed90acd178 ("mtd: spi-nor: Add Micron MT25QL02 support")
    a98086e00420 ("mtd: spi-nor: add entry for mt35xu512aba flash")
    56c6855c81c8 ("mtd: spi-nor: Add Micron MT25QU02 support")
    835ed7bf1260 ("mtd: spi-nor: Add support for N25Q256A11")
    61e4611864b3 ("mtd: spi-nor: Add support for N25Q016A")
    cebc1fd06907 ("mtd: spi-nor: Added support for n25q00a.")
    f9bcb6dc8013 ("mtd: spi-nor: Add support for Micron n25q032a")
    2a06c7b1fd23 ("mtd: spi-nor: Add support for Micron n25q064a serial flash")
    c14deddec1fb ("mtd: spi-nor: add support for flag status register on Micron chips")
    867f770de812 ("mtd: m25p80: Add support for Micron N25Q512A memory")
    98a9e2450667 ("mtd: m25p80: modify info for Micron N25Q128")
    3105875f6b89 ("mtd: m25p80: add support for Micron N25Q128")
    8da28681eb14 ("mtd: m25p80: add support for Micron N25Q256A")

There is no evidence that the status register has three bits (except
when the flash has 64 eraseblocks or fewer), and there overwhelming
evidence to the contrary.

--Sean

>>
>> The locking doesn't work on any of these flashes without these flags. If
>> you don't believe me you can try it yourself. The people who submitted
>> the original patches certainly didn't test it.
> 
> Right. So can you send the patches you _did_ test on the hardware you do
> have? So this time we are sure we got it right. And reply to the other
> review comments? Without that, I don't think this patch can make
> progress.
> 