The ioctl output may be counter intuitive in some cases. Asking for a
"locked status" over a region that is only partially locked will return
"unlocked" whereas in practice maybe the biggest part is actually
locked.

Knowing what is the real software locking state through debugfs would be
very convenient for development/debugging purposes, hence this proposal
for adding two extra blocks at the end of the file:
- A "software locked sectors" array which lists every section, if it is
locked or not, showing both the address ranges and the sizes in numbers
of blocks.
- Some kind of mapping of the locked sectors, which pictures the entire
flash. It may be verbose, so perhaps we'll drop it in the end. I found
it very useful to really get a clearer mental model of what was
locked/unlocked, but the array just before is already a good source of
information.

Here is an example of output, what is after the "sector map" is new.

$ cat /sys/kernel/debug/spi-nor/spi0.0/params
name		(null)
id		ef a0 20 00 00 00
size		64.0 MiB
write size	1
page size	256
address nbytes	4
flags		HAS_SR_TB | 4B_OPCODES | HAS_4BAIT | HAS_LOCK | HAS_16BIT_SR | HAS_SR_TB_BIT6 | HAS_4BIT_BP | SOFT_RESET | NO_WP

opcodes
 read		0xec
  dummy cycles	6
 erase		0xdc
 program	0x34
 8D extension	none

protocols
 read		1S-4S-4S
 write		1S-1S-4S
 register	1S-1S-1S

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

sector map
 region (in hex)   | erase mask | overlaid
 ------------------+------------+---------
 00000000-03ffffff |     [   3] | no

software locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-03ffffff | unlocked | 1024

64kiB-sectors locking map (x: locked, .: unlocked)
|.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
 ...........................|

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
Here are below more examples of output with various situations. The full
output of the "params" content has been manually removed to show just
the two added blocks.

$ flash_lock -l /dev/mtd0 0x3f00000 16
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
software locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-03efffff | unlocked | 1008
 03f00000-03ffffff |   locked | 16

64kiB-sectors locking map (x: locked, .: unlocked)
|.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
 ...........xxxxxxxxxxxxxxxx|
$ flash_lock -u /dev/mtd0 0x3f00000 8
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
software locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-03f7ffff | unlocked | 1016
 03f80000-03ffffff |   locked | 8

64kiB-sectors locking map (x: locked, .: unlocked)
|.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
 ...................xxxxxxxx|
$ flash_lock -u /dev/mtd0
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
software locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-03ffffff | unlocked | 1024

64kiB-sectors locking map (x: locked, .: unlocked)
|.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
 ...........................|
$ flash_lock -l /dev/mtd0
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
software locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-03ffffff |   locked | 1024

64kiB-sectors locking map (x: locked, .: unlocked)
|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
 xxxxxxxxxxxxxxxxxxxxxxxxxxx|
$ flash_lock -u /dev/mtd0 0x20000 1022
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
software locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-0001ffff |   locked | 2
 00020000-03ffffff | unlocked | 1022

64kiB-sectors locking map (x: locked, .: unlocked)
|xx...................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
 ...........................|
---
 drivers/mtd/spi-nor/core.h    |  4 ++++
 drivers/mtd/spi-nor/debugfs.c | 45 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/swp.c     | 11 +++++++----
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 516ab19dc7b86a5c6ba8729d2ba18904b922df23..8a95592994f749a62b2cc70ab85f54d36681e760 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -700,6 +700,10 @@ static inline bool spi_nor_needs_sfdp(const struct spi_nor *nor)
 	return !nor->info->size;
 }
 
+u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor);
+void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs, u64 *len);
+bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, u64 len, const u8 *sr);
+
 #ifdef CONFIG_DEBUG_FS
 void spi_nor_debugfs_register(struct spi_nor *nor);
 void spi_nor_debugfs_shutdown(void);
diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
index d0191eb9f87956418dfd964fc1f16b21e3345049..d2af4c189aad68bab78c1c68688b5865eebef9b9 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -77,12 +77,16 @@ static void spi_nor_print_flags(struct seq_file *s, unsigned long flags,
 static int spi_nor_params_show(struct seq_file *s, void *data)
 {
 	struct spi_nor *nor = s->private;
+	unsigned int min_prot_len = spi_nor_get_min_prot_length_sr(nor);
 	struct spi_nor_flash_parameter *params = nor->params;
 	struct spi_nor_erase_map *erase_map = &params->erase_map;
 	struct spi_nor_erase_region *region = erase_map->regions;
 	const struct flash_info *info = nor->info;
+	loff_t lock_start, lock_length;
 	char buf[16], *str;
 	unsigned int i;
+	u8 sr[2] = {};
+	int ret;
 
 	seq_printf(s, "name\t\t%s\n", info->name);
 	seq_printf(s, "id\t\t%*ph\n", SPI_NOR_MAX_ID_LEN, nor->id);
@@ -159,6 +163,47 @@ static int spi_nor_params_show(struct seq_file *s, void *data)
 			   region[i].overlaid ? "yes" : "no");
 	}
 
+	seq_puts(s, "\nsoftware locked sectors\n");
+	seq_puts(s, " region (in hex)   | status   | #blocks\n");
+	seq_puts(s, " ------------------+----------+--------\n");
+
+	ret = spi_nor_read_sr(nor, nor->bouncebuf);
+	if (ret)
+		return ret;
+
+	sr[0] = nor->bouncebuf[0];
+
+	if (!(nor->flags & SNOR_F_NO_READ_CR)) {
+		ret = spi_nor_read_cr(nor, nor->bouncebuf + 1);
+		if (ret)
+			return ret;
+	}
+
+	sr[1] = nor->bouncebuf[1];
+
+	spi_nor_get_locked_range_sr(nor, sr, &lock_start, &lock_length);
+	if (!lock_length || lock_length == params->size) {
+		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, params->size - 1,
+			   lock_length ? "  locked" : "unlocked", params->size / min_prot_len);
+	} else if (!lock_start) {
+		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_length - 1,
+			   "  locked", lock_length / min_prot_len);
+		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_length, params->size - 1,
+			   "unlocked", (params->size - lock_length) / min_prot_len);
+	} else {
+		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_start - 1,
+			   "unlocked", lock_start / min_prot_len);
+		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_start, params->size - 1,
+			   "  locked", lock_length / min_prot_len);
+	}
+
+	seq_printf(s, "\n%dkiB-sectors locking map (x: locked, .: unlocked)\n",
+		   min_prot_len / 1024);
+	seq_puts(s, "|");
+	for (i = 0; i < params->size; i += min_prot_len)
+		seq_printf(s, spi_nor_is_locked_sr(nor, i, min_prot_len, sr) ? "x" : ".");
+	seq_puts(s, "|\n");
+
 	return 0;
 }
 DEFINE_SHOW_ATTRIBUTE(spi_nor_params);
diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index f5ec05d6f2680113332f47e0efbcb4d88f0d3e3c..0e685aa3a4fdc3100b5259659a3083c14a2cf127 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -32,7 +32,7 @@ static u8 spi_nor_get_sr_tb_mask(struct spi_nor *nor)
 		return SR_TB_BIT5;
 }
 
-static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
+u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
 {
 	unsigned int bp_slots, bp_slots_needed;
 	/*
@@ -53,8 +53,8 @@ static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
 		return sector_size;
 }
 
-static void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs,
-					u64 *len)
+void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs,
+				 u64 *len)
 {
 	u64 min_prot_len;
 	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
@@ -112,7 +112,7 @@ static bool spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
 		return (ofs >= lock_offs_max) || (offs_max <= lock_offs);
 }
 
-static bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, u64 len, const u8 *sr)
+bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, u64 len, const u8 *sr)
 {
 	return spi_nor_check_lock_status_sr(nor, ofs, len, sr, true);
 }
@@ -374,6 +374,9 @@ static int spi_nor_sr_is_locked(struct spi_nor *nor, loff_t ofs, u64 len)
  * -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.
+ *
+ * If in doubt during development, check-out the debugfs output which tries to
+ * be more user friendly.
  */
 static const struct spi_nor_locking_ops spi_nor_sr_locking_ops = {
 	.lock = spi_nor_sr_lock,

-- 
2.51.0

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> The ioctl output may be counter intuitive in some cases. Asking for a
> "locked status" over a region that is only partially locked will return
> "unlocked" whereas in practice maybe the biggest part is actually
> locked.
>
> Knowing what is the real software locking state through debugfs would be
> very convenient for development/debugging purposes, hence this proposal
> for adding two extra blocks at the end of the file:
> - A "software locked sectors" array which lists every section, if it is
> locked or not, showing both the address ranges and the sizes in numbers
> of blocks.

I know the file is called software write protection (or swp) but
it's really a hardware write protection, isn't it?

> - Some kind of mapping of the locked sectors, which pictures the entire
> flash. It may be verbose, so perhaps we'll drop it in the end. I found
> it very useful to really get a clearer mental model of what was
> locked/unlocked, but the array just before is already a good source of
> information.
>
> Here is an example of output, what is after the "sector map" is new.
>
> $ cat /sys/kernel/debug/spi-nor/spi0.0/params
> name		(null)
> id		ef a0 20 00 00 00
> size		64.0 MiB
> write size	1
> page size	256
> address nbytes	4
> flags		HAS_SR_TB | 4B_OPCODES | HAS_4BAIT | HAS_LOCK | HAS_16BIT_SR | HAS_SR_TB_BIT6 | HAS_4BIT_BP | SOFT_RESET | NO_WP
>
> opcodes
>  read		0xec
>   dummy cycles	6
>  erase		0xdc
>  program	0x34
>  8D extension	none
>
> protocols
>  read		1S-4S-4S
>  write		1S-1S-4S
>  register	1S-1S-1S
>
> erase commands
>  21 (4.00 KiB) [1]
>  dc (64.0 KiB) [3]
>  c7 (64.0 MiB)
>
> sector map
>  region (in hex)   | erase mask | overlaid
>  ------------------+------------+---------
>  00000000-03ffffff |     [   3] | no
>
> software locked sectors

drop "software" here.

>  region (in hex)   | status   | #blocks
>  ------------------+----------+--------
>  00000000-03ffffff | unlocked | 1024

I really like that.

>
> 64kiB-sectors locking map (x: locked, .: unlocked)
> |.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
>  ...........................|

Maybe put it into an own file. In any case, a sane line wrapping
would be good. And add a leading offset, ie, "0000: xxxx.....".

..

>  drivers/mtd/spi-nor/core.h    |  4 ++++
>  drivers/mtd/spi-nor/debugfs.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/swp.c     | 11 +++++++----
>  3 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 516ab19dc7b86a5c6ba8729d2ba18904b922df23..8a95592994f749a62b2cc70ab85f54d36681e760 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -700,6 +700,10 @@ static inline bool spi_nor_needs_sfdp(const struct spi_nor *nor)
>  	return !nor->info->size;
>  }
>  
> +u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor);
> +void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs, u64 *len);
> +bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, u64 len, const u8 *sr);
> +
>  #ifdef CONFIG_DEBUG_FS
>  void spi_nor_debugfs_register(struct spi_nor *nor);
>  void spi_nor_debugfs_shutdown(void);
> diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
> index d0191eb9f87956418dfd964fc1f16b21e3345049..d2af4c189aad68bab78c1c68688b5865eebef9b9 100644
> --- a/drivers/mtd/spi-nor/debugfs.c
> +++ b/drivers/mtd/spi-nor/debugfs.c
> @@ -77,12 +77,16 @@ static void spi_nor_print_flags(struct seq_file *s, unsigned long flags,
>  static int spi_nor_params_show(struct seq_file *s, void *data)
>  {
>  	struct spi_nor *nor = s->private;
> +	unsigned int min_prot_len = spi_nor_get_min_prot_length_sr(nor);
>  	struct spi_nor_flash_parameter *params = nor->params;
>  	struct spi_nor_erase_map *erase_map = &params->erase_map;
>  	struct spi_nor_erase_region *region = erase_map->regions;
>  	const struct flash_info *info = nor->info;
> +	loff_t lock_start, lock_length;
>  	char buf[16], *str;
>  	unsigned int i;
> +	u8 sr[2] = {};
> +	int ret;
>  
>  	seq_printf(s, "name\t\t%s\n", info->name);
>  	seq_printf(s, "id\t\t%*ph\n", SPI_NOR_MAX_ID_LEN, nor->id);
> @@ -159,6 +163,47 @@ static int spi_nor_params_show(struct seq_file *s, void *data)
>  			   region[i].overlaid ? "yes" : "no");
>  	}
>  
> +	seq_puts(s, "\nsoftware locked sectors\n");
> +	seq_puts(s, " region (in hex)   | status   | #blocks\n");
> +	seq_puts(s, " ------------------+----------+--------\n");
> +
> +	ret = spi_nor_read_sr(nor, nor->bouncebuf);
> +	if (ret)
> +		return ret;
> +
> +	sr[0] = nor->bouncebuf[0];
> +
> +	if (!(nor->flags & SNOR_F_NO_READ_CR)) {
> +		ret = spi_nor_read_cr(nor, nor->bouncebuf + 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	sr[1] = nor->bouncebuf[1];

Shouldn't that go into the former if conditional? bouncebuf[1] might
never be read.

Also, until now, reading the "params" debug file never interacts
with the flash, but with this patch it does. We don't do locking
here which looks wrong. Maybe we should just cache the protection
bits. Not sure.

> +
> +	spi_nor_get_locked_range_sr(nor, sr, &lock_start, &lock_length);
> +	if (!lock_length || lock_length == params->size) {
> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, params->size - 1,
> +			   lock_length ? "  locked" : "unlocked", params->size / min_prot_len);
> +	} else if (!lock_start) {
> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_length - 1,
> +			   "  locked", lock_length / min_prot_len);
> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_length, params->size - 1,
> +			   "unlocked", (params->size - lock_length) / min_prot_len);
> +	} else {
> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_start - 1,
> +			   "unlocked", lock_start / min_prot_len);
> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_start, params->size - 1,
> +			   "  locked", lock_length / min_prot_len);
> +	}
> +
> +	seq_printf(s, "\n%dkiB-sectors locking map (x: locked, .: unlocked)\n",
> +		   min_prot_len / 1024);
> +	seq_puts(s, "|");
> +	for (i = 0; i < params->size; i += min_prot_len)
> +		seq_printf(s, spi_nor_is_locked_sr(nor, i, min_prot_len, sr) ? "x" : ".");

As mentioned above, newlines as well as a leading offset counter
would be nice :)

-michael

> +	seq_puts(s, "|\n");
> +
>  	return 0;
>  }
>  DEFINE_SHOW_ATTRIBUTE(spi_nor_params);
> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
> index f5ec05d6f2680113332f47e0efbcb4d88f0d3e3c..0e685aa3a4fdc3100b5259659a3083c14a2cf127 100644
> --- a/drivers/mtd/spi-nor/swp.c
> +++ b/drivers/mtd/spi-nor/swp.c
> @@ -32,7 +32,7 @@ static u8 spi_nor_get_sr_tb_mask(struct spi_nor *nor)
>  		return SR_TB_BIT5;
>  }
>  
> -static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
> +u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
>  {
>  	unsigned int bp_slots, bp_slots_needed;
>  	/*
> @@ -53,8 +53,8 @@ static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
>  		return sector_size;
>  }
>  
> -static void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs,
> -					u64 *len)
> +void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs,
> +				 u64 *len)
>  {
>  	u64 min_prot_len;
>  	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
> @@ -112,7 +112,7 @@ static bool spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
>  		return (ofs >= lock_offs_max) || (offs_max <= lock_offs);
>  }
>  
> -static bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, u64 len, const u8 *sr)
> +bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, u64 len, const u8 *sr)
>  {
>  	return spi_nor_check_lock_status_sr(nor, ofs, len, sr, true);
>  }
> @@ -374,6 +374,9 @@ static int spi_nor_sr_is_locked(struct spi_nor *nor, loff_t ofs, u64 len)
>   * -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.
> + *
> + * If in doubt during development, check-out the debugfs output which tries to
> + * be more user friendly.
>   */
>  static const struct spi_nor_locking_ops spi_nor_sr_locking_ops = {
>  	.lock = spi_nor_sr_lock,

Hello,

On 18/11/2025 at 13:46:52 +01, "Michael Walle" <mwalle@kernel.org> wrote:

> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>> The ioctl output may be counter intuitive in some cases. Asking for a
>> "locked status" over a region that is only partially locked will return
>> "unlocked" whereas in practice maybe the biggest part is actually
>> locked.
>>
>> Knowing what is the real software locking state through debugfs would be
>> very convenient for development/debugging purposes, hence this proposal
>> for adding two extra blocks at the end of the file:
>> - A "software locked sectors" array which lists every section, if it is
>> locked or not, showing both the address ranges and the sizes in numbers
>> of blocks.
>
> I know the file is called software write protection (or swp) but
> it's really a hardware write protection, isn't it?

Well, it depends on your configuration I guess? Without #WP pin I don't
know how to call that. I had in mind that software meant "using the BP
pins" and "hardware" meant "toggling #WP". But I have no strong opinion
about this wording.

>> - Some kind of mapping of the locked sectors, which pictures the entire
>> flash. It may be verbose, so perhaps we'll drop it in the end. I found
>> it very useful to really get a clearer mental model of what was
>> locked/unlocked, but the array just before is already a good source of
>> information.
>>
>> Here is an example of output, what is after the "sector map" is new.
>>
>> $ cat /sys/kernel/debug/spi-nor/spi0.0/params
>> name		(null)
>> id		ef a0 20 00 00 00
>> size		64.0 MiB
>> write size	1
>> page size	256
>> address nbytes	4
>> flags		HAS_SR_TB | 4B_OPCODES | HAS_4BAIT | HAS_LOCK | HAS_16BIT_SR | HAS_SR_TB_BIT6 | HAS_4BIT_BP | SOFT_RESET | NO_WP
>>
>> opcodes
>>  read		0xec
>>   dummy cycles	6
>>  erase		0xdc
>>  program	0x34
>>  8D extension	none
>>
>> protocols
>>  read		1S-4S-4S
>>  write		1S-1S-4S
>>  register	1S-1S-1S
>>
>> erase commands
>>  21 (4.00 KiB) [1]
>>  dc (64.0 KiB) [3]
>>  c7 (64.0 MiB)
>>
>> sector map
>>  region (in hex)   | erase mask | overlaid
>>  ------------------+------------+---------
>>  00000000-03ffffff |     [   3] | no
>>
>> software locked sectors
>
> drop "software" here.

Okay.

>
>>  region (in hex)   | status   | #blocks
>>  ------------------+----------+--------
>>  00000000-03ffffff | unlocked | 1024
>
> I really like that.

:-)

>> 64kiB-sectors locking map (x: locked, .: unlocked)
>> |.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
>>  ...........................|
>
> Maybe put it into an own file. In any case, a sane line wrapping
> would be good. And add a leading offset, ie, "0000: xxxx.....".

I was unsure about doing that, but yes that makes sense. May I call it
"locked_sectors_map"?

[...]

>> +	sr[0] = nor->bouncebuf[0];
>> +
>> +	if (!(nor->flags & SNOR_F_NO_READ_CR)) {
>> +		ret = spi_nor_read_cr(nor, nor->bouncebuf + 1);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	sr[1] = nor->bouncebuf[1];
>
> Shouldn't that go into the former if conditional? bouncebuf[1] might
> never be read.

Yes, that's correct. I don't remember why I did it this way, probably a
bug, I'll move that line.

> Also, until now, reading the "params" debug file never interacts
> with the flash, but with this patch it does. We don't do locking
> here which looks wrong. Maybe we should just cache the protection
> bits. Not sure.

I guess caching the status registers makes sense, but we'll still have a
possible race when accessing the 2 registers. Is it okay to
ignore this very unlikely case in debugfs? Otherwise I might just lock
the entire device for the time we access the cached registers.

>> +	spi_nor_get_locked_range_sr(nor, sr, &lock_start, &lock_length);
>> +	if (!lock_length || lock_length == params->size) {
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, params->size - 1,
>> +			   lock_length ? "  locked" : "unlocked", params->size / min_prot_len);
>> +	} else if (!lock_start) {
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_length - 1,
>> +			   "  locked", lock_length / min_prot_len);
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_length, params->size - 1,
>> +			   "unlocked", (params->size - lock_length) / min_prot_len);
>> +	} else {
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_start - 1,
>> +			   "unlocked", lock_start / min_prot_len);
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_start, params->size - 1,
>> +			   "  locked", lock_length / min_prot_len);
>> +	}
>> +
>> +	seq_printf(s, "\n%dkiB-sectors locking map (x: locked, .: unlocked)\n",
>> +		   min_prot_len / 1024);
>> +	seq_puts(s, "|");
>> +	for (i = 0; i < params->size; i += min_prot_len)
>> +		seq_printf(s, spi_nor_is_locked_sr(nor, i, min_prot_len, sr) ? "x" : ".");
>
> As mentioned above, newlines as well as a leading offset counter
> would be nice :)

Arf, I was hoping I could escape that step, but ok, fair enough :-)

Miquèl

On Wed Nov 19, 2025 at 10:49 AM CET, Miquel Raynal wrote:
> Hello,
>
> On 18/11/2025 at 13:46:52 +01, "Michael Walle" <mwalle@kernel.org> wrote:
>
>> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>>> The ioctl output may be counter intuitive in some cases. Asking for a
>>> "locked status" over a region that is only partially locked will return
>>> "unlocked" whereas in practice maybe the biggest part is actually
>>> locked.
>>>
>>> Knowing what is the real software locking state through debugfs would be
>>> very convenient for development/debugging purposes, hence this proposal
>>> for adding two extra blocks at the end of the file:
>>> - A "software locked sectors" array which lists every section, if it is
>>> locked or not, showing both the address ranges and the sizes in numbers
>>> of blocks.
>>
>> I know the file is called software write protection (or swp) but
>> it's really a hardware write protection, isn't it?
>
> Well, it depends on your configuration I guess? Without #WP pin I don't
> know how to call that. I had in mind that software meant "using the BP
> pins" and "hardware" meant "toggling #WP". But I have no strong opinion
> about this wording.

See my previous mail and commit 18d7d01a0a0e ("mtd: spi-nor: Avoid
setting SRWD bit in SR if WP# signal not connected"). Personally, I
really don't like the "software" write protection, I mean you can
just use read-only for that partition or whatever. Locking for me is
really backed by the hardware. I.e. we use that to be really sure,
that we have a bootable bootloader and no one can break it.

>>> 64kiB-sectors locking map (x: locked, .: unlocked)
>>> |.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
>>>  ...........................|
>>
>> Maybe put it into an own file. In any case, a sane line wrapping
>> would be good. And add a leading offset, ie, "0000: xxxx.....".
>
> I was unsure about doing that, but yes that makes sense. May I call it
> "locked_sectors_map"?

I don't have a strong opinion here, but locking might be on a finer
granularity than sectors. Not with the BP scheme but IIRC I've seen
locking schemes with 4k blocks for example. So maybe just something
more general like "locked_erase_blocks_map" or just
"locked_blocks_map", up to you.  It's just debugfs ;)

> [...]
>
>>> +	sr[0] = nor->bouncebuf[0];
>>> +
>>> +	if (!(nor->flags & SNOR_F_NO_READ_CR)) {
>>> +		ret = spi_nor_read_cr(nor, nor->bouncebuf + 1);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	sr[1] = nor->bouncebuf[1];
>>
>> Shouldn't that go into the former if conditional? bouncebuf[1] might
>> never be read.
>
> Yes, that's correct. I don't remember why I did it this way, probably a
> bug, I'll move that line.
>
>> Also, until now, reading the "params" debug file never interacts
>> with the flash, but with this patch it does. We don't do locking
>> here which looks wrong. Maybe we should just cache the protection
>> bits. Not sure.
>
> I guess caching the status registers makes sense, but we'll still have a
> possible race when accessing the 2 registers. Is it okay to
> ignore this very unlikely case in debugfs? Otherwise I might just lock
> the entire device for the time we access the cached registers.

Oh I meant caching it in the core/swp.c (and invalidating/updating
when the bits are written) and just display it here. That way we
just keep that reading this file won't actually trigger any SPI
xfers.

>>> +	spi_nor_get_locked_range_sr(nor, sr, &lock_start, &lock_length);
>>> +	if (!lock_length || lock_length == params->size) {
>>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, params->size - 1,
>>> +			   lock_length ? "  locked" : "unlocked", params->size / min_prot_len);
>>> +	} else if (!lock_start) {
>>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_length - 1,
>>> +			   "  locked", lock_length / min_prot_len);
>>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_length, params->size - 1,
>>> +			   "unlocked", (params->size - lock_length) / min_prot_len);
>>> +	} else {
>>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_start - 1,
>>> +			   "unlocked", lock_start / min_prot_len);
>>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_start, params->size - 1,
>>> +			   "  locked", lock_length / min_prot_len);
>>> +	}
>>> +
>>> +	seq_printf(s, "\n%dkiB-sectors locking map (x: locked, .: unlocked)\n",
>>> +		   min_prot_len / 1024);
>>> +	seq_puts(s, "|");
>>> +	for (i = 0; i < params->size; i += min_prot_len)
>>> +		seq_printf(s, spi_nor_is_locked_sr(nor, i, min_prot_len, sr) ? "x" : ".");
>>
>> As mentioned above, newlines as well as a leading offset counter
>> would be nice :)
>
> Arf, I was hoping I could escape that step, but ok, fair enough :-)

:)

-michael

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

> On Wed Nov 19, 2025 at 10:49 AM CET, Miquel Raynal wrote:
>> Hello,
>>
>> On 18/11/2025 at 13:46:52 +01, "Michael Walle" <mwalle@kernel.org> wrote:
>>
>>> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>>>> The ioctl output may be counter intuitive in some cases. Asking for a
>>>> "locked status" over a region that is only partially locked will return
>>>> "unlocked" whereas in practice maybe the biggest part is actually
>>>> locked.
>>>>
>>>> Knowing what is the real software locking state through debugfs would be
>>>> very convenient for development/debugging purposes, hence this proposal
>>>> for adding two extra blocks at the end of the file:
>>>> - A "software locked sectors" array which lists every section, if it is
>>>> locked or not, showing both the address ranges and the sizes in numbers
>>>> of blocks.
>>>
>>> I know the file is called software write protection (or swp) but
>>> it's really a hardware write protection, isn't it?
>>
>> Well, it depends on your configuration I guess? Without #WP pin I don't
>> know how to call that. I had in mind that software meant "using the BP
>> pins" and "hardware" meant "toggling #WP". But I have no strong opinion
>> about this wording.
>
> See my previous mail and commit 18d7d01a0a0e ("mtd: spi-nor: Avoid
> setting SRWD bit in SR if WP# signal not connected"). Personally, I
> really don't like the "software" write protection, I mean you can
> just use read-only for that partition or whatever. Locking for me is
> really backed by the hardware. I.e. we use that to be really sure,
> that we have a bootable bootloader and no one can break it.
>
>>>> 64kiB-sectors locking map (x: locked, .: unlocked)
>>>> |.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
>>>>  ...........................|
>>>
>>> Maybe put it into an own file. In any case, a sane line wrapping
>>> would be good. And add a leading offset, ie, "0000: xxxx.....".
>>
>> I was unsure about doing that, but yes that makes sense. May I call it
>> "locked_sectors_map"?
>
> I don't have a strong opinion here, but locking might be on a finer
> granularity than sectors. Not with the BP scheme but IIRC I've seen
> locking schemes with 4k blocks for example. So maybe just something
> more general like "locked_erase_blocks_map" or just
> "locked_blocks_map", up to you.  It's just debugfs ;)

I find "sector" more generic than "erase block" or even "blocks". A
"sector" meaning is intimately related to what the vendor wants a sector
to be, unlike a block that carries the historical flash-related meaning
of an erase block. I also have the maths definition in mind, which is
basically a start and end.

We can go for locked_blocks_map. Technically speaking, the size of a
block as defined in the BP bits is left to the vendor, it could very
well be any size I guess? So that sounds fine.

>>>> +	sr[0] = nor->bouncebuf[0];
>>>> +
>>>> +	if (!(nor->flags & SNOR_F_NO_READ_CR)) {
>>>> +		ret = spi_nor_read_cr(nor, nor->bouncebuf + 1);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>> +	sr[1] = nor->bouncebuf[1];
>>>
>>> Shouldn't that go into the former if conditional? bouncebuf[1] might
>>> never be read.
>>
>> Yes, that's correct. I don't remember why I did it this way, probably a
>> bug, I'll move that line.
>>
>>> Also, until now, reading the "params" debug file never interacts
>>> with the flash, but with this patch it does. We don't do locking
>>> here which looks wrong. Maybe we should just cache the protection
>>> bits. Not sure.
>>
>> I guess caching the status registers makes sense, but we'll still have a
>> possible race when accessing the 2 registers. Is it okay to
>> ignore this very unlikely case in debugfs? Otherwise I might just lock
>> the entire device for the time we access the cached registers.
>
> Oh I meant caching it in the core/swp.c (and invalidating/updating
> when the bits are written) and just display it here. That way we
> just keep that reading this file won't actually trigger any SPI
> xfers.

I understand that but there is still a race:
- swp.c writes cached_st[0]
- debugfs.c reads cached_st[0]
- swp.c writes cached_st[1]
- debugfs.c reads cached_st[1]

debugfs would get old st[0], new st[1]. The presence of the CMP bit in
st[1] really changes what st[0] means.

Questions is, do we care?

If yes, we can probably protect these cached registers inside the spi-nor
device lock.

Thanks,
Miquèl