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 an extra block at the end of the file: a "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.

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

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

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 only show
what has been added and how it behaves.

$ flash_lock -l /dev/mtd0 0x3f00000 16
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-03efffff | unlocked | 1008
 03f00000-03ffffff |   locked | 16
$
$ flash_lock -u /dev/mtd0 0x3f00000 8
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-03f7ffff | unlocked | 1016
 03f80000-03ffffff |   locked | 8
$
$ flash_lock -u /dev/mtd0
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-03ffffff | unlocked | 1024
$
$ flash_lock -l /dev/mtd0
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-03ffffff |   locked | 1024
$
$ flash_lock -u /dev/mtd0 0x20000 1022
$ cat /sys/kernel/debug/spi-nor/spi0.0/params
locked sectors
 region (in hex)   | status   | #blocks
 ------------------+----------+--------
 00000000-0001ffff |   locked | 2
 00020000-03ffffff | unlocked | 1022
---
 drivers/mtd/spi-nor/core.h    |  4 ++++
 drivers/mtd/spi-nor/debugfs.c | 22 ++++++++++++++++++++++
 drivers/mtd/spi-nor/swp.c     | 11 +++++++----
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 091eb934abe4..99ed6c54b90f 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -707,6 +707,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 d0191eb9f879..821fbc9587dc 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -77,10 +77,12 @@ 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;
 
@@ -159,6 +161,26 @@ static int spi_nor_params_show(struct seq_file *s, void *data)
 			   region[i].overlaid ? "yes" : "no");
 	}
 
+	seq_puts(s, "\nlocked sectors\n");
+	seq_puts(s, " region (in hex)   | status   | #blocks\n");
+	seq_puts(s, " ------------------+----------+--------\n");
+
+	spi_nor_get_locked_range_sr(nor, nor->dfs_sr_cache, &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);
+	}
+
 	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 7a6c2b8ef921..8de8459e8e90 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);
 }
@@ -410,6 +410,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.1

Hi Miquel,

> 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 an extra block at the end of the file: a "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.
> 
> 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
> 
> locked sectors
>  region (in hex)   | status   | #blocks
>  ------------------+----------+--------
>  00000000-03ffffff | unlocked | 1024
> 
> 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 only show
> what has been added and how it behaves.
> 
> $ flash_lock -l /dev/mtd0 0x3f00000 16
> $ cat /sys/kernel/debug/spi-nor/spi0.0/params
> locked sectors
>  region (in hex)   | status   | #blocks
>  ------------------+----------+--------
>  00000000-03efffff | unlocked | 1008
>  03f00000-03ffffff |   locked | 16
> $
> $ flash_lock -u /dev/mtd0 0x3f00000 8
> $ cat /sys/kernel/debug/spi-nor/spi0.0/params
> locked sectors
>  region (in hex)   | status   | #blocks
>  ------------------+----------+--------
>  00000000-03f7ffff | unlocked | 1016
>  03f80000-03ffffff |   locked | 8
> $
> $ flash_lock -u /dev/mtd0
> $ cat /sys/kernel/debug/spi-nor/spi0.0/params
> locked sectors
>  region (in hex)   | status   | #blocks
>  ------------------+----------+--------
>  00000000-03ffffff | unlocked | 1024
> $
> $ flash_lock -l /dev/mtd0
> $ cat /sys/kernel/debug/spi-nor/spi0.0/params
> locked sectors
>  region (in hex)   | status   | #blocks
>  ------------------+----------+--------
>  00000000-03ffffff |   locked | 1024
> $
> $ flash_lock -u /dev/mtd0 0x20000 1022
> $ cat /sys/kernel/debug/spi-nor/spi0.0/params
> locked sectors
>  region (in hex)   | status   | #blocks
>  ------------------+----------+--------
>  00000000-0001ffff |   locked | 2
>  00020000-03ffffff | unlocked | 1022
> ---
>  drivers/mtd/spi-nor/core.h    |  4 ++++
>  drivers/mtd/spi-nor/debugfs.c | 22 ++++++++++++++++++++++
>  drivers/mtd/spi-nor/swp.c     | 11 +++++++----
>  3 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 091eb934abe4..99ed6c54b90f 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -707,6 +707,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);
> +

It would be better to have generic helper functions rather than using SR-based
functions directly. The locking_ops is vendor/chip specific and can provide
other locking mechanism than SR-based block protection. For instance, Infineon,
Micron, Macronix (and maybe other vendors) offer protection mechanisms that can
protect sectors individually with volatile or non-volatile manner.

>  #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 d0191eb9f879..821fbc9587dc 100644
> --- a/drivers/mtd/spi-nor/debugfs.c
> +++ b/drivers/mtd/spi-nor/debugfs.c
> @@ -77,10 +77,12 @@ 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;
> 
> @@ -159,6 +161,26 @@ static int spi_nor_params_show(struct seq_file *s, void *data)
>                            region[i].overlaid ? "yes" : "no");
>         }
> 

Don't we need to check 'SNOR_F_HAS_LOCK' flag here?

> +       seq_puts(s, "\nlocked sectors\n");
> +       seq_puts(s, " region (in hex)   | status   | #blocks\n");
> +       seq_puts(s, " ------------------+----------+--------\n");
> +
> +       spi_nor_get_locked_range_sr(nor, nor->dfs_sr_cache, &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);

div_u64() is needed. 
I got undefined reference to `__aeabi_uldivmod' for my 32-bit ARM platform.
Same for following four seq_printf().

> +       } 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);
> +       }
> +
>         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 7a6c2b8ef921..8de8459e8e90 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);
>  }
> @@ -410,6 +410,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.1

Thanks!
Takahiro

Hi Takahiro,

>> +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);
>> +
>
> It would be better to have generic helper functions rather than using SR-based
> functions directly. The locking_ops is vendor/chip specific and can provide
> other locking mechanism than SR-based block protection. For instance, Infineon,
> Micron, Macronix (and maybe other vendors) offer protection mechanisms that can
> protect sectors individually with volatile or non-volatile manner.

I get what you mean, thanks for pointing this out. Unfortunately it's
not that straightforward.

As of today, there are the "default" locking ops (SR based) and a few
others, chip specific. For debugging purposes, these "other" ops do not
provide any kind of useful feedback regarding what has actually been
locked. Only SR based chips provide this, it is useful, I want to use it.

So I'm going to expose a new boolean to filter out whether we have
locking support *and* if yes, whether we can use SR based functions, this
should also address this concern:

> Don't we need to check 'SNOR_F_HAS_LOCK' flag here?

We will still need to expose the SR specific helpers, though, but at
least we will no longer get inconsistencies with chips not featuring the
default SR approach.

>> +       seq_puts(s, "\nlocked sectors\n");
>> +       seq_puts(s, " region (in hex)   | status   | #blocks\n");
>> +       seq_puts(s, " ------------------+----------+--------\n");
>> +
>> +       spi_nor_get_locked_range_sr(nor, nor->dfs_sr_cache, &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);
>
> div_u64() is needed. 
> I got undefined reference to `__aeabi_uldivmod' for my 32-bit ARM platform.
> Same for following four seq_printf().

Good catch, I will fix this too.

Thanks,
Miquèl