Add comments about how these requests are actually handled in the SPI
NOR core. Their behaviour was not entirely clear to me at first, and
explaining them in plain English sounds the way to go.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/spi-nor/swp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index 9bc5a356444665ad8824e9e12d679fd551b3e67d..ede03f26de3c65ff53c1cb888c2c43aea268b85a 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -341,6 +341,14 @@ static int spi_nor_sr_is_locked(struct spi_nor *nor, loff_t ofs, u64 len)
 	return spi_nor_is_locked_sr(nor, ofs, len, nor->bouncebuf[0]);
 }
 
+/*
+ * These ioctls behave according to the following rules:
+ * ->lock(): Never locks more than what is requested, ie. may lock less
+ * ->unlock(): Never unlocks more than what is requested, ie. may unlock less
+ * -is_locked(): Checks if the region is *fully* locked, returns false otherwise.
+ *               This feeback may be misleading because users may get an "unlocked"
+ *               status even though a subpart of the region is effectively locked.
+ */
 static const struct spi_nor_locking_ops spi_nor_sr_locking_ops = {
 	.lock = spi_nor_sr_lock,
 	.unlock = spi_nor_sr_unlock,

-- 
2.51.0

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> Add comments about how these requests are actually handled in the SPI
> NOR core. Their behaviour was not entirely clear to me at first, and
> explaining them in plain English sounds the way to go.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/spi-nor/swp.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
> index 9bc5a356444665ad8824e9e12d679fd551b3e67d..ede03f26de3c65ff53c1cb888c2c43aea268b85a 100644
> --- a/drivers/mtd/spi-nor/swp.c
> +++ b/drivers/mtd/spi-nor/swp.c
> @@ -341,6 +341,14 @@ static int spi_nor_sr_is_locked(struct spi_nor *nor, loff_t ofs, u64 len)
>  	return spi_nor_is_locked_sr(nor, ofs, len, nor->bouncebuf[0]);
>  }
>  
> +/*
> + * These ioctls behave according to the following rules:
> + * ->lock(): Never locks more than what is requested, ie. may lock less

That behavior sounds so wrong... The user requests a region to be
locked, and it isn't actually locked.

> + * ->unlock(): Never unlocks more than what is requested, ie. may unlock less

That seems somewhat sane.

Maybe we should return -EINVAL if ofs or ofs+len aren't at sector
boundaries. Yeah it's a change in the UAPI, but I'm not sure the
current behavior is not harmful and misleading.

> + * -is_locked(): Checks if the region is *fully* locked, returns false otherwise.
> + *               This feeback may be misleading because users may get an "unlocked"
> + *               status even though a subpart of the region is effectively locked.
> + */
>  static const struct spi_nor_locking_ops spi_nor_sr_locking_ops = {
>  	.lock = spi_nor_sr_lock,
>  	.unlock = spi_nor_sr_unlock,

Anyway, as it is how it's currently behaving:

Reviewed-by: Michael Walle <mwalle@kernel.org>

-michael

On 18/11/2025 at 10:53:42 +01, "Michael Walle" <mwalle@kernel.org> wrote:

> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>> Add comments about how these requests are actually handled in the SPI
>> NOR core. Their behaviour was not entirely clear to me at first, and
>> explaining them in plain English sounds the way to go.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  drivers/mtd/spi-nor/swp.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
>> index 9bc5a356444665ad8824e9e12d679fd551b3e67d..ede03f26de3c65ff53c1cb888c2c43aea268b85a 100644
>> --- a/drivers/mtd/spi-nor/swp.c
>> +++ b/drivers/mtd/spi-nor/swp.c
>> @@ -341,6 +341,14 @@ static int spi_nor_sr_is_locked(struct spi_nor *nor, loff_t ofs, u64 len)
>>  	return spi_nor_is_locked_sr(nor, ofs, len, nor->bouncebuf[0]);
>>  }
>>  
>> +/*
>> + * These ioctls behave according to the following rules:
>> + * ->lock(): Never locks more than what is requested, ie. may lock less
>
> That behavior sounds so wrong... The user requests a region to be
> locked, and it isn't actually locked.

Agreed. I really got puzzled by that.

>> + * ->unlock(): Never unlocks more than what is requested, ie. may unlock less
>
> That seems somewhat sane.
>
> Maybe we should return -EINVAL if ofs or ofs+len aren't at sector
> boundaries. Yeah it's a change in the UAPI, but I'm not sure the
> current behavior is not harmful and misleading.

I would even go further and propose to return -EINVAL whenever the
request is not exactly doable. Being at a block boundary is not enough,
as there are many boundaries we cannot describe with just 4 protection
bits.

But this is somewhat a uAPI change indeed. So in the first place, I will
keep this comment. But if we feel like we should make the uAPI stricter,
it can come on top. Doing this would require a broad acknowledgement.

>> + * -is_locked(): Checks if the region is *fully* locked, returns false otherwise.
>> + *               This feeback may be misleading because users may get an "unlocked"
>> + *               status even though a subpart of the region is effectively locked.
>> + */
>>  static const struct spi_nor_locking_ops spi_nor_sr_locking_ops = {
>>  	.lock = spi_nor_sr_lock,
>>  	.unlock = spi_nor_sr_unlock,
>
> Anyway, as it is how it's currently behaving:
>
> Reviewed-by: Michael Walle <mwalle@kernel.org>

Thanks!
Miquèl