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

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

[Sean Anderson]

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...

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
Tested with a mt25qu01gbbb, which shares the same flash ID.

 drivers/mtd/spi-nor/micron-st.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index 187239ccd549..17c7d6322508 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -486,6 +486,8 @@ static const struct flash_info st_nor_parts[] = {
 		.id = SNOR_ID(0x20, 0xbb, 0x21),
 		.name = "n25q00a",
 		.size = SZ_128M,
+		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
+			 SPI_NOR_BP3_SR_BIT6,
 		.no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ,
 		.mfr_flags = USE_FSR,
 		.fixups = &n25q00_fixups,
-- 
2.35.1.1320.gc452695387.dirty

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

[Sean Anderson]

On 10/6/25 18:34, 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

Sorry, this should be commit f80ff13135cb ("mtd: spi-nor: micron-st: Enable locking for n25q00")

https://lore.kernel.org/all/20200421063313.32655-1-js07.lee@samsung.com/

> maintainer...
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> Tested with a mt25qu01gbbb, which shares the same flash ID.
> 
>  drivers/mtd/spi-nor/micron-st.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index 187239ccd549..17c7d6322508 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -486,6 +486,8 @@ static const struct flash_info st_nor_parts[] = {
>  		.id = SNOR_ID(0x20, 0xbb, 0x21),
>  		.name = "n25q00a",
>  		.size = SZ_128M,
> +		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
> +			 SPI_NOR_BP3_SR_BIT6,
>  		.no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ,
>  		.mfr_flags = USE_FSR,
>  		.fixups = &n25q00_fixups,

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

[Tudor Ambarus]

Hi, Sean,

On 10/6/25 11:38 PM, Sean Anderson wrote:
> On 10/6/25 18:34, 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:

Were the flags removed upstream and then not added back?

>> spi-nor: Enable locking for n25q128a11"), but they were removed by the
> 
> Sorry, this should be commit f80ff13135cb ("mtd: spi-nor: micron-st: Enable locking for n25q00")
> 
> https://lore.kernel.org/all/20200421063313.32655-1-js07.lee@samsung.com/

The rule is still true today: I don't queue patches that are not 
functionally tested, even if they are based on datasheet info.

> 
>> maintainer...

Don't point fingers please. If you feel the context is worth
mentioning, specify it in an impersonal way and add a link to the
discussion in the commit message.

>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> Tested with a mt25qu01gbbb, which shares the same flash ID.

Would you please let us know how you tested the support?

If you feel generous and want to give back to the community, you can also
describe your testing steps in the documentation from:
https://docs.kernel.org/driver-api/mtd/spi-nor.html

Also, if there's going to be a v2, please dump the SPI NOR sysfs and
debugfs data, see how in the link from above. We're keeping a database
and it will help us differentiate flashes that have the same flash ID
but different functionalities.

Cheers,
ta

>>
>>  drivers/mtd/spi-nor/micron-st.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
>> index 187239ccd549..17c7d6322508 100644
>> --- a/drivers/mtd/spi-nor/micron-st.c
>> +++ b/drivers/mtd/spi-nor/micron-st.c
>> @@ -486,6 +486,8 @@ static const struct flash_info st_nor_parts[] = {
>>  		.id = SNOR_ID(0x20, 0xbb, 0x21),
>>  		.name = "n25q00a",
>>  		.size = SZ_128M,
>> +		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
>> +			 SPI_NOR_BP3_SR_BIT6,
>>  		.no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ,
>>  		.mfr_flags = USE_FSR,
>>  		.fixups = &n25q00_fixups,
> 

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

[Pratyush Yadav]

On Wed, Oct 08 2025, Tudor Ambarus wrote:

> Hi, Sean,
>
> On 10/6/25 11:38 PM, Sean Anderson wrote:
>> On 10/6/25 18:34, 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:
>
> Were the flags removed upstream and then not added back?
>
>>> spi-nor: Enable locking for n25q128a11"), but they were removed by the
>> 
>> Sorry, this should be commit f80ff13135cb ("mtd: spi-nor: micron-st: Enable locking for n25q00")
>> 
>> https://lore.kernel.org/all/20200421063313.32655-1-js07.lee@samsung.com/
>
> The rule is still true today: I don't queue patches that are not 
> functionally tested, even if they are based on datasheet info.
>
>> 
>>> maintainer...
>
> Don't point fingers please. If you feel the context is worth
> mentioning, specify it in an impersonal way and add a link to the
> discussion in the commit message.

+1.

>
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>> ---
>>> Tested with a mt25qu01gbbb, which shares the same flash ID.
>
> Would you please let us know how you tested the support?
>
> If you feel generous and want to give back to the community, you can also
> describe your testing steps in the documentation from:
> https://docs.kernel.org/driver-api/mtd/spi-nor.html
>
> Also, if there's going to be a v2, please dump the SPI NOR sysfs and
> debugfs data, see how in the link from above. We're keeping a database
> and it will help us differentiate flashes that have the same flash ID
> but different functionalities.

There will need to be a v2. I'm not applying the commit message in its
current form.

Would be nice to have a sysfs and debugfs dump too. Sean, the data we
need can be found in
https://docs.kernel.org/driver-api/mtd/spi-nor.html.

[...]

-- 
Regards,
Pratyush Yadav

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

[Pratyush Yadav]

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. Jungseung did not
have the flash at hand and Tudor didn't want to apply patches that
weren't tested. Both were in agreement for removing the n25q00a changes.

If you are going to mention that commit, then mention the full context,
and then also mention what has changed since that makes it possible to
add those changes back in. Having tested them on the real hardware for
example.

>
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> Tested with a mt25qu01gbbb, which shares the same flash ID.

Ughh, is this another case of flash ID reuse? Do mt25qu and n25q00a
flashes behave exactly the same and only have two names? If not, then
how do you know if n25q00a will also work with these changes?

>
>  drivers/mtd/spi-nor/micron-st.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index 187239ccd549..17c7d6322508 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -486,6 +486,8 @@ static const struct flash_info st_nor_parts[] = {
>  		.id = SNOR_ID(0x20, 0xbb, 0x21),
>  		.name = "n25q00a",
>  		.size = SZ_128M,
> +		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
> +			 SPI_NOR_BP3_SR_BIT6,
>  		.no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ,
>  		.mfr_flags = USE_FSR,
>  		.fixups = &n25q00_fixups,

-- 
Regards,
Pratyush Yadav

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

[Sean Anderson]

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.

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
that the original submitter may not have tested.

The original patch was entirely correct, and the maintainer's
justification for removing the second hunk is flawed.

> Jungseung did not
> have the flash at hand and Tudor didn't want to apply patches that
> weren't tested. Both were in agreement for removing the n25q00a changes.
> 
> If you are going to mention that commit, then mention the full context,
> and then also mention what has changed since that makes it possible to
> add those changes back in. Having tested them on the real hardware for
> example.
> 
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> Tested with a mt25qu01gbbb, which shares the same flash ID.
> 
> Ughh, is this another case of flash ID reuse? Do mt25qu and n25q00a
> flashes behave exactly the same and only have two names? If not, then
> how do you know if n25q00a will also work with these changes?

I examined the datasheet for the n25q00a and determined that it has the
same status register layout.

In fact, every n25q and mt25q flash has the same status register layout,
which (as noted above) is necessary to support capacities greater than 8
MiB (and all flashes in this series have such capacity).

--Sea

>>
>>  drivers/mtd/spi-nor/micron-st.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
>> index 187239ccd549..17c7d6322508 100644
>> --- a/drivers/mtd/spi-nor/micron-st.c
>> +++ b/drivers/mtd/spi-nor/micron-st.c
>> @@ -486,6 +486,8 @@ static const struct flash_info st_nor_parts[] = {
>>  		.id = SNOR_ID(0x20, 0xbb, 0x21),
>>  		.name = "n25q00a",
>>  		.size = SZ_128M,
>> +		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
>> +			 SPI_NOR_BP3_SR_BIT6,
>>  		.no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ,
>>  		.mfr_flags = USE_FSR,
>>  		.fixups = &n25q00_fixups,
> 

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

[Pratyush Yadav]

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.

> that the original submitter may not have tested.
>
> The original patch was entirely correct, and the maintainer's
> justification for removing the second hunk is flawed.
>
>> Jungseung did not
>> have the flash at hand and Tudor didn't want to apply patches that
>> weren't tested. Both were in agreement for removing the n25q00a changes.
>> 
>> If you are going to mention that commit, then mention the full context,
>> and then also mention what has changed since that makes it possible to
>> add those changes back in. Having tested them on the real hardware for
>> example.
>> 
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>> ---
>>> Tested with a mt25qu01gbbb, which shares the same flash ID.
>> 
>> Ughh, is this another case of flash ID reuse? Do mt25qu and n25q00a
>> flashes behave exactly the same and only have two names? If not, then
>> how do you know if n25q00a will also work with these changes?
>
> I examined the datasheet for the n25q00a and determined that it has the
> same status register layout.

Can you share the links to the datasheets?

Also, test logs would be nice to have.

>
> In fact, every n25q and mt25q flash has the same status register layout,
> which (as noted above) is necessary to support capacities greater than 8
> MiB (and all flashes in this series have such capacity).

Do they behave the same? If not, do you know how they differ? If they
behave differently, we might need to have some code that detects which
one is running. Not necessarily as part of this patch though.

>
> --Sea
>
>>>
>>>  drivers/mtd/spi-nor/micron-st.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
>>> index 187239ccd549..17c7d6322508 100644
>>> --- a/drivers/mtd/spi-nor/micron-st.c
>>> +++ b/drivers/mtd/spi-nor/micron-st.c
>>> @@ -486,6 +486,8 @@ static const struct flash_info st_nor_parts[] = {
>>>  		.id = SNOR_ID(0x20, 0xbb, 0x21),
>>>  		.name = "n25q00a",
>>>  		.size = SZ_128M,
>>> +		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
>>> +			 SPI_NOR_BP3_SR_BIT6,
>>>  		.no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ,
>>>  		.mfr_flags = USE_FSR,
>>>  		.fixups = &n25q00_fixups,
>> 

-- 
Regards,
Pratyush Yadav

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

[Pratyush Yadav]

On Wed, Oct 08 2025, Pratyush Yadav wrote:

> On Tue, Oct 07 2025, Sean Anderson wrote:
>
[...]
>>>> Tested with a mt25qu01gbbb, which shares the same flash ID.
>>> 
>>> Ughh, is this another case of flash ID reuse? Do mt25qu and n25q00a
>>> flashes behave exactly the same and only have two names? If not, then
>>> how do you know if n25q00a will also work with these changes?
>>
>> I examined the datasheet for the n25q00a and determined that it has the
>> same status register layout.
>
> Can you share the links to the datasheets?
>
> Also, test logs would be nice to have.
>
>>
>> In fact, every n25q and mt25q flash has the same status register layout,
>> which (as noted above) is necessary to support capacities greater than 8
>> MiB (and all flashes in this series have such capacity).
>
> Do they behave the same? If not, do you know how they differ? If they

To clarify, I mean behave the same in things other than the status
register.

> behave differently, we might need to have some code that detects which
> one is running. Not necessarily as part of this patch though.
[...]

-- 
Regards,
Pratyush Yadav

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

[Sean Anderson]

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.

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.

--Sean

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

[Pratyush Yadav]

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?

>
> 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.

-- 
Regards,
Pratyush Yadav

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

[Sean Anderson]

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.
> 

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

[Tudor Ambarus]

Sean,

On 10/10/25 4:45 PM, Sean Anderson wrote:

>> ... or, you know, read the data sheet, write the driver, and _test_ if
>> it actually works?
> 
> Which I did.

cut

> - 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). 

cut

> 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.

Nobody challenged that your flash has 4 BP bits. I/we just want the proof
that you did the tests, i.e. show the output of the mtd-utils locking tests.

I also need you to dump the sysfs/debugs entries.

It's true that the locking tests are not yet described in
https://docs.kernel.org/driver-api/mtd/spi-nor.html, that's why I encouraged
you to show us how you did the testing and maybe to contribute and extend
the documentation.

Cheers,
ta

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

[Sean Anderson]

On 10/13/25 03:30, Tudor Ambarus wrote:
> Sean,
> 
> On 10/10/25 4:45 PM, Sean Anderson wrote:
> 
>>> ... or, you know, read the data sheet, write the driver, and _test_ if
>>> it actually works?
>> 
>> Which I did.
> 
> cut
> 
>> - 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). 
> 
> cut
> 
>> 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.
> 
> Nobody challenged that your flash has 4 BP bits.

Pratyush just did! And what I mean is that ALL of these flashes should
have these flags added. It shouldn't be the case that random developers
have to slowly trickle in patches every other year as they discover that
locking is broken across the board.

> I/we just want the proof
> that you did the tests, i.e. show the output of the mtd-utils locking tests.

# flash_lock -u /dev/mtd/by-name/spi0.1
# flash_lock -i /dev/mtd/by-name/spi0.1
Device: /dev/mtd/by-name/spi0.1
Start: 0
Len: 0x8000000
Lock status: unlocked
Return code: 0
# test() {
> mtd=/dev/mtd/by-name/$1
> start=$(($2 * 64 * 1024))
> size=$(($3 * 64 * 1024))
> dd if=/dev/urandom of=$1 bs=64k count=$3 status=none && \
> mtd_debug erase $mtd $start $size && \
> mtd_debug write $mtd $start $size $1 && \
> dd if=$mtd bs=64k skip=$2 count=$3 status=none | sha256sum $1 - && \
> rm $1
> }
# test spi0.1 0 64
Erased 4194304 bytes from address 0x00000000 in flash
Copied 4194304 bytes from spi0.1 to address 0x00000000 in flash
8cb208e9ae82e57fb192e1bb6cabc6573e1de23abcba5d65d95217a8aca64f3f  spi0.1
8cb208e9ae82e57fb192e1bb6cabc6573e1de23abcba5d65d95217a8aca64f3f  -
# flash_lock -l /dev/mtd/by-name/spi0.1
# flash_lock -i /dev/mtd/by-name/spi0.1
Device: /dev/mtd/by-name/spi0.1
Start: 0
Len: 0x8000000
Lock status: locked
Return code: 1
# test
[95099.230842] spi-nor spi0.1: Erase operation failed.
[95099.235746] spi-nor spi0.1: Attempted to modify a protected sector.
While erasing blocks from 0x00000000-0x00400000 on /dev/mtd/by-name/spi0.1: Input/output error
# flash_lock -u /dev/mtd/by-name/spi0.1
# test spi0.1 64
83a8dc6125ec9672d18f7f18f92e16f867354dbe8e2f3b0aca52b9d0ad7d8ffe  spi0.1
83a8dc6125ec9672d18f7f18f92e16f867354dbe8e2f3b0aca52b9d0ad7d8ffe  -
# flash_lock -l /dev/mtd/by-name/spi0.1 $((1024 * 64 * 1024)) 1024
# flash_lock -i /dev/mtd/by-name/spi0.1 
Device: /dev/mtd/by-name/spi0.1
Start: 0
Len: 0x8000000
Lock status: unlocked <<<< Wrong!
Return code: 0
# test spi0.1 0 64
Erased 4194304 bytes from address 0x00000000 in flash
Copied 4194304 bytes from spi0.1 to address 0x00000000 in flash
cd56cebf0dce3276c595f7787d1b3b35db467786e15f9c401d5f11570073d41f spi0.1
cd56cebf0dce3276c595f7787d1b3b35db467786e15f9c401d5f11570073d41f -
# test spi0.1 1024 64
[97011.257102] spi-nor spi0.1: Erase operation failed.
[97011.262006] spi-nor spi0.1: Attempted to modify a protected sector.
MEMERASE: Input/output error
# flash_lock -u /dev/mtd/by-name/spi0.1
# flash_lock -l /dev/mtd/by-name/spi0.1 0 1024
# flash_lock -i /dev/mtd/by-name/spi0.1 
Device: /dev/mtd/by-name/spi0.1
Start: 0
Len: 0x8000000
Lock status: unlocked <<<< Wrong again!
Return code: 0
# test spi0.1 0 64
[98127.697206] spi-nor spi0.1: Erase operation failed.
[98127.702116] spi-nor spi0.1: Attempted to modify a protected sector.
MEMERASE: Input/output error
# test spi0.1 1024 64
Erased 4194304 bytes from address 0x04000000 in flash
Copied 4194304 bytes from spi0.1 to address 0x04000000 in flash
3c0fb61b64b3d0a8eb9503b39180b45bccc6d3e840482ccf36faa194a80c4b8f  spi0.1
3c0fb61b64b3d0a8eb9503b39180b45bccc6d3e840482ccf36faa194a80c4b8f  -

So MEMISLOCKED is not really working properly, but the actual lock
functionality is fine. All of the above was done with the write-protect
input high. Setting it low, unlocking fails:

# flash_lock -u /dev/mtd/by-name/spi0.1 
flash_lock: error!: could not unlock device: /dev/mtd/by-name/spi0.1

            error 5 (Input/output error)

> I also need you to dump the sysfs/debugs entries.

# cat /sys/bus/spi/devices/spi0.1/spi-nor/partname
n25q00a
# cat /sys/bus/spi/devices/spi0.1/spi-nor/jedec_id 
20bb21
# cat /sys/bus/spi/devices/spi0.1/spi-nor/manufacturer 
st
# hexdump -C /sys/bus/spi/devices/spi0.1/spi-nor/sfdp         
00000000  53 46 44 50 06 01 01 ff  00 06 01 10 30 00 00 ff  |SFDP........0...|
00000010  84 00 01 02 80 00 00 ff  ff ff ff ff ff ff ff ff  |................|
00000020  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000030  e5 20 fb ff ff ff ff 3f  29 eb 27 6b 27 3b 27 bb  |. .....?).'k';'.|
00000040  ff ff ff ff ff ff 27 bb  ff ff 29 eb 0c 20 10 d8  |......'...).. ..|
00000050  0f 52 00 00 24 4a 99 00  8b 8e 03 e1 ac 01 27 38  |.R..$J........'8|
00000060  7a 75 7a 75 fb bd d5 5c  4a 0f 82 ff 81 bd 3d 36  |zuzu...\J.....=6|
00000070  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000080  ff e7 ff ff 21 dc ff ff                           |....!...|
00000088
# sha256sum /sys/bus/spi/devices/spi0.1/spi-nor/sfdp 
e49dfee6eeb73c55e94c07a8c7d352dd7d8774b830a64ed1059ef6e7bc833668  /sys/bus/spi/devices/spi0.1/spi-nor/sfdp

# cat /sys/kernel/debug/spi-nor/spi0.1/capabilities
Supported read modes by the flash
 1S-1S-1S
  opcode	0x13
  mode cycles	0
  dummy cycles	0
 1S-1S-2S
  opcode	0x3c
  mode cycles	1
  dummy cycles	7
 1S-2S-2S
  opcode	0xbc
  mode cycles	1
  dummy cycles	7
 2S-2S-2S
  opcode	0xbc
  mode cycles	1
  dummy cycles	7
 1S-1S-4S
  opcode	0x6c
  mode cycles	1
  dummy cycles	7
 1S-4S-4S
  opcode	0xec
  mode cycles	1
  dummy cycles	9
 4S-4S-4S
  opcode	0xec
  mode cycles	1
  dummy cycles	9

Supported page program modes by the flash
 1S-1S-1S
  opcode	0x12
 1S-1S-4S
  opcode	0x34
 1S-4S-4S
  opcode	0x3e
# cat /sys/kernel/debug/spi-nor/spi0.1/params      
name		n25q00a
id		20 bb 21 10 44 00
size		128 MiB
write size	1
page size	256
address nbytes	4
flags		HAS_SR_TB | NO_OP_CHIP_ERASE | 4B_OPCODES | HAS_4BAIT | HAS_LOCK | HAS_4BIT_BP | HAS_SR_BP3_BIT6 | SOFT_RESET

opcodes
 read		0xbc
  dummy cycles	8
 erase		0xdc
 program	0x12
 8D extension	none

protocols
 read		1S-2S-2S
 write		1S-1S-1S
 register	1S-1S-1S

erase commands
 21 (4.00 KiB) [1]
 dc (64.0 KiB) [3]

sector map
 region (in hex)   | erase mask | flags
 ------------------+------------+----------
 00000000-07ffffff |     [ 123] | 

# mtd_debug info /dev/mtd/by-name/spi0.1
mtd.type = MTD_NORFLASH
mtd.flags = MTD_CAP_NORFLASH
mtd.size = 134217728 (128M)
mtd.erasesize = 65536 (64K)
mtd.writesize = 1 
mtd.oobsize = 0 
regions = 0

> It's true that the locking tests are not yet described in
> https://docs.kernel.org/driver-api/mtd/spi-nor.html, that's why I encouraged
> you to show us how you did the testing and maybe to contribute and extend
> the documentation.

And my point is that the original submitters did not test locking. So
you should assume that the locking flags are not working, rather than
assume that they are tested and intentionally omitted.

--Sean

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

[Tudor Ambarus]

The locking tests look fine to me:

Acked-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Sean, if you care you can extend the documentation on how to test locking.
Also, you may drop the non SFDP data from the flash's static definition as
a follow up patch.

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

[Pratyush Yadav]

On Mon, Nov 10 2025, Tudor Ambarus wrote:

> The locking tests look fine to me:
>
> Acked-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Thanks for reviewing Tudor.

I would like a re-roll with the commit message updated before applying
this.

>
> Sean, if you care you can extend the documentation on how to test locking.
> Also, you may drop the non SFDP data from the flash's static definition as
> a follow up patch.


-- 
Regards,
Pratyush Yadav

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

[Sean Anderson]

On 11/10/25 02:08, Tudor Ambarus wrote:
> 
> The locking tests look fine to me:
> 
> Acked-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> 
> Sean, if you care you can extend the documentation on how to test locking.

I'll take a look.

> Also, you may drop the non SFDP data from the flash's static definition as
> a follow up patch.

I don't understand what you mean by "non SFDP data".

--Sean

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

[Tudor Ambarus]

On 11/10/25 6:36 PM, Sean Anderson wrote:
> On 11/10/25 02:08, Tudor Ambarus wrote:
>>
>> The locking tests look fine to me:
>>
>> Acked-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>
>> Sean, if you care you can extend the documentation on how to test locking.
> 
> I'll take a look.
> 
>> Also, you may drop the non SFDP data from the flash's static definition as
>> a follow up patch.
> 
> I don't understand what you mean by "non SFDP data".
> 

Remove static definitions from the flash entry that can be discovered by SFDP.

For example in your case:

 		.id = SNOR_ID(0x20, 0xbb, 0x21),
 		.name = "n25q00a",
 		.size = SZ_128M,
		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
			 SPI_NOR_BP3_SR_BIT6,
 		.no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ,
 		.mfr_flags = USE_FSR,
 		.fixups = &n25q00_fixups,

you can drop size and no_sfdp_flags as this info is discovered when parsing SFDP.

Also, you can drop the name, and add is as a comment.
Cheers,
ta

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

[Miquel Raynal]

Hello Sean,

> # flash_lock -u /dev/mtd/by-name/spi0.1
> # flash_lock -i /dev/mtd/by-name/spi0.1
> Device: /dev/mtd/by-name/spi0.1
> Start: 0
> Len: 0x8000000
> Lock status: unlocked
> Return code: 0
> # test() {
>> mtd=/dev/mtd/by-name/$1
>> start=$(($2 * 64 * 1024))
>> size=$(($3 * 64 * 1024))
>> dd if=/dev/urandom of=$1 bs=64k count=$3 status=none && \
>> mtd_debug erase $mtd $start $size && \
>> mtd_debug write $mtd $start $size $1 && \
>> dd if=$mtd bs=64k skip=$2 count=$3 status=none | sha256sum $1 - && \
>> rm $1
>> }

I am also working on locking these days, have you already started
writing extra SPI NOR Documentation/process based on this thread?

I am also trying to clarify what is expected and what the API somehow
does, as it was not fully clear for me at first sight.

[...]

> # flash_lock -u /dev/mtd/by-name/spi0.1
> # test spi0.1 64
> 83a8dc6125ec9672d18f7f18f92e16f867354dbe8e2f3b0aca52b9d0ad7d8ffe  spi0.1
> 83a8dc6125ec9672d18f7f18f92e16f867354dbe8e2f3b0aca52b9d0ad7d8ffe  -
> # flash_lock -l /dev/mtd/by-name/spi0.1 $((1024 * 64 * 1024)) 1024
> # flash_lock -i /dev/mtd/by-name/spi0.1 
> Device: /dev/mtd/by-name/spi0.1
> Start: 0
> Len: 0x8000000
> Lock status: unlocked <<<< Wrong!

This isn't wrong, even though at a first glance the output is
misleading. The API apparently does not gives you all the
locked/unlocked sectors, it is way more basic than that: it tells you
whether the full range you asked for is indeed locked.

When you run "# flash_lock -i /dev/mtd/by-name/spi0.1", you privide no
start/length values to the command. Hence, the defaults are picked: the
entire device is considered for the check. The tool asks the kernel
whether the range 0-0x7ffffff is *fully* locked. Answer is no, it is not
fully locked.

In the kernel there are two helpers for that, and they won't give you
opposite results all the time:
- is locked:
    - returns true if the given range is fully locked
    - returns false otherwise
- is unlocked:
    - returns yes if the given range is fully unlocked
    - returns false otherwise

So if you want the tool to tell you "yes", you should instead use the
exact range you locked (1024-2047) or any subset of it.

Thanks, Miquèl

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

[Miquel Raynal]

> When you run "# flash_lock -i /dev/mtd/by-name/spi0.1", you privide no
> start/length values to the command. Hence, the defaults are picked: the
> entire device is considered for the check. The tool asks the kernel
> whether the range 0-0x7ffffff is *fully* locked. Answer is no, it is not
> fully locked.
>
> In the kernel there are two helpers for that, and they won't give you
> opposite results all the time:
> - is locked:
>     - returns true if the given range is fully locked
>     - returns false otherwise
> - is unlocked:
>     - returns yes if the given range is fully unlocked
>     - returns false otherwise
>
> So if you want the tool to tell you "yes", you should instead use the
> exact range you locked (1024-2047) or any subset of it.

I forgot to mention: I don't like this interface because it is not very
user friendly, but this is uAPI, so set in stone. As part of my journey
in the SPI NOR swp.c file, I wrote a debugfs interface to help
visualizing what is actually locked. It is absolutely trivial to do and
helps a lot. We might want to use that for writing some kind of testing
procedure — I will share it soon.

Thanks,
Miquèl

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

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Wed Nov 12, 2025 at 2:20 PM CET, Miquel Raynal wrote:
> I forgot to mention: I don't like this interface because it is not very
> user friendly, but this is uAPI, so set in stone. As part of my journey
> in the SPI NOR swp.c file, I wrote a debugfs interface to help
> visualizing what is actually locked. It is absolutely trivial to do and
> helps a lot. We might want to use that for writing some kind of testing
> procedure — I will share it soon.

Yes please, that sounds very good. We might also want to just raw
dump the SR (and maybe CR/SR2 if we somehow can infer it's
existence).

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

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

[Sean Anderson]

On 11/12/25 08:10, Miquel Raynal wrote:
> Hello Sean,
> 
>> # flash_lock -u /dev/mtd/by-name/spi0.1
>> # flash_lock -i /dev/mtd/by-name/spi0.1
>> Device: /dev/mtd/by-name/spi0.1
>> Start: 0
>> Len: 0x8000000
>> Lock status: unlocked
>> Return code: 0
>> # test() {
>>> mtd=/dev/mtd/by-name/$1
>>> start=$(($2 * 64 * 1024))
>>> size=$(($3 * 64 * 1024))
>>> dd if=/dev/urandom of=$1 bs=64k count=$3 status=none && \
>>> mtd_debug erase $mtd $start $size && \
>>> mtd_debug write $mtd $start $size $1 && \
>>> dd if=$mtd bs=64k skip=$2 count=$3 status=none | sha256sum $1 - && \
>>> rm $1
>>> }
> 
> I am also working on locking these days, have you already started
> writing extra SPI NOR Documentation/process based on this thread?

I haven't started writing anything.

> I am also trying to clarify what is expected and what the API somehow
> does, as it was not fully clear for me at first sight.

I agree, as you could probably have figured out.

>> # flash_lock -u /dev/mtd/by-name/spi0.1
>> # test spi0.1 64
>> 83a8dc6125ec9672d18f7f18f92e16f867354dbe8e2f3b0aca52b9d0ad7d8ffe  spi0.1
>> 83a8dc6125ec9672d18f7f18f92e16f867354dbe8e2f3b0aca52b9d0ad7d8ffe  -
>> # flash_lock -l /dev/mtd/by-name/spi0.1 $((1024 * 64 * 1024)) 1024
>> # flash_lock -i /dev/mtd/by-name/spi0.1 
>> Device: /dev/mtd/by-name/spi0.1
>> Start: 0
>> Len: 0x8000000
>> Lock status: unlocked <<<< Wrong!
> 
> This isn't wrong, even though at a first glance the output is
> misleading. The API apparently does not gives you all the
> locked/unlocked sectors, it is way more basic than that: it tells you
> whether the full range you asked for is indeed locked.

Yeah, I figured that out eventually.

Actually, the most surprising thing to me is that the lock/unlock APIs
are not incremental. That is, if I have a flash of 8 seconds, and
sectors 0-3 are locked and I lock sectors 0-1, it will say "well,
sectors 2-3 should be unlocked now, but we're not allowed to unlock
during a lock operation" and fail to lock. I would have expected it to
say "sectors 0-1 are already locked so we don't need to do anything".
The only way to go from sectors 0-3 to 0-1 being locked is to issue an
*unlock* on sectors 2-7.

Conversely, if what you wanted to do was ensure sectors 2-3 were
unlocked, you can't do the naive thing and unlock sectors 2-3, since
that will try to lock sectors 0-1 and 4-7, the latter being disallowed
in an unlock operation. So you actually have to unlock sectors 2-7.

And knowing what to do is complicated by ISLOCKED only returning a
boolean instead of just telling userspace what sectors are locked (which
must be a small finite set of ranges (usually one) on all flashes I'm
familiar with).

> When you run "# flash_lock -i /dev/mtd/by-name/spi0.1", you privide no
> start/length values to the command. Hence, the defaults are picked: the
> entire device is considered for the check. The tool asks the kernel
> whether the range 0-0x7ffffff is *fully* locked. Answer is no, it is not
> fully locked.
> 
> In the kernel there are two helpers for that, and they won't give you
> opposite results all the time:
> - is locked:
>     - returns true if the given range is fully locked
>     - returns false otherwise
> - is unlocked:
>     - returns yes if the given range is fully unlocked
>     - returns false otherwise
> 
> So if you want the tool to tell you "yes", you should instead use the
> exact range you locked (1024-2047) or any subset of it.
> 
> Thanks, Miquèl

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

[Miquel Raynal]

Hello,

>> I am also working on locking these days, have you already started
>> writing extra SPI NOR Documentation/process based on this thread?
>
> I haven't started writing anything.
>
>> I am also trying to clarify what is expected and what the API somehow
>> does, as it was not fully clear for me at first sight.
>
> I agree, as you could probably have figured out.
>
>>> # flash_lock -u /dev/mtd/by-name/spi0.1
>>> # test spi0.1 64
>>> 83a8dc6125ec9672d18f7f18f92e16f867354dbe8e2f3b0aca52b9d0ad7d8ffe  spi0.1
>>> 83a8dc6125ec9672d18f7f18f92e16f867354dbe8e2f3b0aca52b9d0ad7d8ffe  -
>>> # flash_lock -l /dev/mtd/by-name/spi0.1 $((1024 * 64 * 1024)) 1024
>>> # flash_lock -i /dev/mtd/by-name/spi0.1 
>>> Device: /dev/mtd/by-name/spi0.1
>>> Start: 0
>>> Len: 0x8000000
>>> Lock status: unlocked <<<< Wrong!
>> 
>> This isn't wrong, even though at a first glance the output is
>> misleading. The API apparently does not gives you all the
>> locked/unlocked sectors, it is way more basic than that: it tells you
>> whether the full range you asked for is indeed locked.
>
> Yeah, I figured that out eventually.
>
> Actually, the most surprising thing to me is that the lock/unlock APIs
> are not incremental. That is, if I have a flash of 8 seconds, and
> sectors 0-3 are locked and I lock sectors 0-1, it will say "well,
> sectors 2-3 should be unlocked now, but we're not allowed to unlock
> during a lock operation" and fail to lock. I would have expected it to
> say "sectors 0-1 are already locked so we don't need to do anything".
> The only way to go from sectors 0-3 to 0-1 being locked is to issue an
> *unlock* on sectors 2-7.
>
> Conversely, if what you wanted to do was ensure sectors 2-3 were
> unlocked, you can't do the naive thing and unlock sectors 2-3, since
> that will try to lock sectors 0-1 and 4-7, the latter being disallowed
> in an unlock operation. So you actually have to unlock sectors 2-7.
>
> And knowing what to do is complicated by ISLOCKED only returning a
> boolean instead of just telling userspace what sectors are locked (which
> must be a small finite set of ranges (usually one) on all flashes I'm
> familiar with).

I've tried to address most of this, see:

Link: https://lore.kernel.org/linux-mtd/20251114-winbond-v6-18-rc1-spi-nor-swp-v1-0-487bc7129931@bootlin.com/T/#mbae8b874181eb0401b30142f423b73b6389a0c54

Kind regards,
Miquèl