[PATCH 00/19] mtd: spi-nor: Enhance software protection

[PATCH 00/19] mtd: spi-nor: Enhance software protection

[Miquel Raynal]

Hello,

As recently raised on the mailing-list (link below), it seems that the
"locking" support in SPI NOR could benefit from some enhancements. As I
myself had to dig into it recently, here is a proposal.

First issue that I see, the MEMLOCK ioctl is not behaving correctly
in some cases, as addressed in:

    mtd: spi-nor: swp: Improve locking user experience

Then there is no clear explanation of the shortcuts taken by the kernel
in terms of uAPI, so there is an attempt to list them in:

    mtd: spi-nor: swp: Explain the MEMLOCK ioctl implementation behaviour

Plus, Tudor also asked if we could cover locking in the testing
procedure, which is done in:

    mtd: spi-nor: Add steps for testing locking support

In order to simplify this procedure, and because it got very helpful
during my testing/development, I want to propose additions to the
debugfs output:

    mtd: spi-nor: debugfs: Add locking support TODO: make the captures again

Finally, I am providing an implementation for the complement (CMP)
feature in order to allow finer control of the regions locked. This
feature is for instance available on Winbond chips:

    [core] mtd: spi-nor: swp: Add support for the complement feature
    [doc]  mtd: spi-nor: Add steps for testing locking with CMP
    [use]  mtd: spi-nor: winbond: Add CMP locking support

Disclaimer: it was much less straightforward than I initially thought to
get the CMP feature working correctly. I tested it with as much focus as
I could, and I am improving the test coverage for the new cases, I am
also providing extra test cases in the metadata of the commit (which do
not make sense to test for chip additions, but may be sensible show when
making core additions like this one), but honestly there are so many
possibilities, I may still be missing corner cases. I hope this will
anyway be helpful to others!

All the other patches are misc improvements or style fixes which I faced
and fixed during my development.

Link: https://lore.kernel.org/linux-mtd/92e99a96-5582-48a5-a4f9-e9b33fcff171@linux.dev/

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
Miquel Raynal (19):
      mtd: spi-nor: debugfs: Fix the flags list
      mtd: spi-nor: swp: Improve locking user experience
      mtd: spi-nor: Improve opcodes documentation
      mtd: spi-nor: debugfs: Align variable access with the rest of the file
      mtd: spi-nor: debugfs: Enhance output
      mtd: spi-nor: swp: Explain the MEMLOCK ioctl implementation behaviour
      mtd: spi-nor: swp: Clarify a comment
      mtd: spi-nor: swp: Use a pointer for SR instead of a single byte
      mtd: spi-nor: swp: Create a helper that writes SR, CR and checks
      mtd: spi-nor: swp: Rename a mask
      mtd: spi-nor: swp: Create a TB intermediate variable
      mtd: spi-nor: swp: Create helpers for building the SR register
      mtd: spi-nor: swp: Simplify checking the locked/unlocked range
      mtd: spi-nor: swp: Cosmetic changes
      mtd: spi-nor: debugfs: Add locking support
      mtd: spi-nor: Add steps for testing locking support
      mtd: spi-nor: swp: Add support for the complement feature
      mtd: spi-nor: Add steps for testing locking with CMP
      mtd: spi-nor: winbond: Add CMP locking support

 Documentation/driver-api/mtd/spi-nor.rst | 154 +++++++++++++++
 drivers/mtd/spi-nor/core.c               |  68 +++++++
 drivers/mtd/spi-nor/core.h               |   9 +
 drivers/mtd/spi-nor/debugfs.c            |  51 ++++-
 drivers/mtd/spi-nor/swp.c                | 313 +++++++++++++++++++++++--------
 drivers/mtd/spi-nor/winbond.c            |  18 +-
 include/linux/mtd/spi-nor.h              |   5 +-
 7 files changed, 533 insertions(+), 85 deletions(-)
---
base-commit: 1f046901101e2933086b76b1e75b0b53a8a39bc0
change-id: 20251114-winbond-v6-18-rc1-spi-nor-swp-865d36f4f695

Best regards,
-- 
Miquel Raynal <miquel.raynal@bootlin.com>

[PATCH 01/19] mtd: spi-nor: debugfs: Fix the flags list

[Miquel Raynal]

As mentioned above the spi_nor_option_flags enumeration in core.h, this
list should be kept in sync with the one in the core.

Add the missing flag.

Fixes: 6a42bc97ccda ("mtd: spi-nor: core: Allow specifying the byte order in Octal DTR mode")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/spi-nor/debugfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
index fa6956144d2e4458ff0e78c5c317a52748b02047..d700e0b271822d034e56903e7b437d89c653f3e7 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -28,6 +28,7 @@ static const char *const snor_f_names[] = {
 	SNOR_F_NAME(RWW),
 	SNOR_F_NAME(ECC),
 	SNOR_F_NAME(NO_WP),
+	SNOR_F_NAME(SWAP16),
 };
 #undef SNOR_F_NAME
 

-- 
2.51.0

[PATCH 02/19] mtd: spi-nor: swp: Improve locking user experience

[Miquel Raynal]

In the case of a single block being locked, if the user want to fully
unlock the device it has two possibilities:
- either it asks to unlock the entire device, and this works;
- or it asks to unlock just the blocks that are currently locked, which
fails.

It fails because the conditions "can_be_top" and "can_be_bottom" are
true. Indeed, in this case, we unlock everything, to the TB bit does not
matter. However in the current implementation, use_top would be true (as
this is the favourite option) and lock_len, which in practice should be
reduced down to 0, is set to "nor->params->size - (ofs + len)" which is
a positive number. This is wrong.

An easy way is to simply add an extra condition. In the unlock() path,
if we can achieve the results from both sides, it means we unlock
everything and lock_len must simply be 0.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
For me, this result was clearly unexpected, but I am not sure this
qualifies as a fix.
---
 drivers/mtd/spi-nor/swp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index 9b07f83aeac76dce2109f90dfa1534c9bd93330d..9bc5a356444665ad8824e9e12d679fd551b3e67d 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -281,7 +281,9 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 	use_top = can_be_top;
 
 	/* lock_len: length of region that should remain locked */
-	if (use_top)
+	if (can_be_top && can_be_bottom)
+		lock_len = 0;
+	else if (use_top)
 		lock_len = nor->params->size - (ofs + len);
 	else
 		lock_len = ofs;

-- 
2.51.0

[PATCH 03/19] mtd: spi-nor: Improve opcodes documentation

[Miquel Raynal]

There are two status registers, named 1 and 2, all the opcodes imply a 1
byte access. Make it clear by aligning all comments on the same pattern,
for the four "{read,write} status {1,2} registers" definitions.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/mtd/spi-nor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index cdcfe0fd2e7d624bbb66fefcb87823bce300268e..90a0cf58351295c63baea4f064b49b7390337d37 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -21,8 +21,8 @@
 /* Flash opcodes. */
 #define SPINOR_OP_WRDI		0x04	/* Write disable */
 #define SPINOR_OP_WREN		0x06	/* Write enable */
-#define SPINOR_OP_RDSR		0x05	/* Read status register */
-#define SPINOR_OP_WRSR		0x01	/* Write status register 1 byte */
+#define SPINOR_OP_RDSR		0x05	/* Read status register 1 */
+#define SPINOR_OP_WRSR		0x01	/* Write status register 1 */
 #define SPINOR_OP_RDSR2		0x3f	/* Read status register 2 */
 #define SPINOR_OP_WRSR2		0x3e	/* Write status register 2 */
 #define SPINOR_OP_READ		0x03	/* Read data bytes (low frequency) */

-- 
2.51.0

[PATCH 04/19] mtd: spi-nor: debugfs: Align variable access with the rest of the file

[Miquel Raynal]

The "params" variable is used everywhere else, align this particular
line of the file to use "params" directly rather than the "nor" pointer.

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

diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
index d700e0b271822d034e56903e7b437d89c653f3e7..69830ad4399009185983549647f3be61ad5d1c84 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -139,7 +139,7 @@ static int spi_nor_params_show(struct seq_file *s, void *data)
 
 	if (!(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
 		string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
-		seq_printf(s, " %02x (%s)\n", nor->params->die_erase_opcode, buf);
+		seq_printf(s, " %02x (%s)\n", params->die_erase_opcode, buf);
 	}
 
 	seq_puts(s, "\nsector map\n");

-- 
2.51.0

[PATCH 05/19] mtd: spi-nor: debugfs: Enhance output

[Miquel Raynal]

Align the number of dashes to the bigger column width (the title in this
case) to make the output more pleasant and aligned with what is done
in the "params" file output.

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

diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
index 69830ad4399009185983549647f3be61ad5d1c84..d0191eb9f87956418dfd964fc1f16b21e3345049 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -144,7 +144,7 @@ static int spi_nor_params_show(struct seq_file *s, void *data)
 
 	seq_puts(s, "\nsector map\n");
 	seq_puts(s, " region (in hex)   | erase mask | overlaid\n");
-	seq_puts(s, " ------------------+------------+----------\n");
+	seq_puts(s, " ------------------+------------+---------\n");
 	for (i = 0; i < erase_map->n_regions; i++) {
 		u64 start = region[i].offset;
 		u64 end = start + region[i].size - 1;

-- 
2.51.0

[PATCH 06/19] mtd: spi-nor: swp: Explain the MEMLOCK ioctl implementation behaviour

[Miquel Raynal]

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

[PATCH 07/19] mtd: spi-nor: swp: Clarify a comment

[Miquel Raynal]

The comment states that all power of two sizes are not supported. This
is very device dependent (based on the size), so modulate a bit the
sentence to make it more accurate.

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

diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index ede03f26de3c65ff53c1cb888c2c43aea268b85a..350fb8cd67dbafa3c62201c8c06bff7131143c04 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -298,7 +298,7 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
 			val = (val & ~SR_BP3) | SR_BP3_BIT6;
 
-		/* Some power-of-two sizes are not supported */
+		/* Some power-of-two sizes may not be supported */
 		if (val & ~mask)
 			return -EINVAL;
 	}

-- 
2.51.0

[PATCH 08/19] mtd: spi-nor: swp: Use a pointer for SR instead of a single byte

[Miquel Raynal]

At this stage, the Status Register is most often seen as a single
byte. This is subject to change when we will need to read the CMP bit
which is located in the Control Register (kind of secondary status
register). Both will need to be carried.

Change a few prototypes to carry a u8 pointer. This way it also makes it
very clear where we access the first register, and where we will access
the second.

There is no functional change.

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

diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index 350fb8cd67dbafa3c62201c8c06bff7131143c04..bac07287ada036f49c25237549e4900f76a0247d 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -53,13 +53,13 @@ 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, u8 sr, loff_t *ofs,
+static void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs,
 					u64 *len)
 {
 	u64 min_prot_len;
 	u8 mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
-	u8 bp, val = sr & mask;
+	u8 bp, val = sr[0] & mask;
 
 	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
 		val = (val & ~SR_BP3_BIT6) | SR_BP3;
@@ -79,7 +79,7 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
 	if (*len > nor->params->size)
 		*len = nor->params->size;
 
-	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
+	if (nor->flags & SNOR_F_HAS_SR_TB && sr[0] & tb_mask)
 		*ofs = 0;
 	else
 		*ofs = nor->params->size - *len;
@@ -90,7 +90,7 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
  * (if @locked is false); false otherwise.
  */
 static bool spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
-					 u64 len, u8 sr, bool locked)
+					 u64 len, const u8 *sr, bool locked)
 {
 	loff_t lock_offs, lock_offs_max, offs_max;
 	u64 lock_len;
@@ -111,13 +111,13 @@ 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, u8 sr)
+static 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);
 }
 
 static bool spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, u64 len,
-				   u8 sr)
+				   const u8 *sr)
 {
 	return spi_nor_check_lock_status_sr(nor, ofs, len, sr, false);
 }
@@ -158,7 +158,8 @@ static bool spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, u64 len,
 static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 {
 	u64 min_prot_len;
-	int ret, status_old, status_new;
+	int ret;
+	u8 status_old[1] = {}, status_new[1] = {};
 	u8 mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
 	u8 pow, val;
@@ -170,7 +171,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	if (ret)
 		return ret;
 
-	status_old = nor->bouncebuf[0];
+	status_old[0] = nor->bouncebuf[0];
 
 	/* If nothing in our range is unlocked, we don't need to do anything */
 	if (spi_nor_is_locked_sr(nor, ofs, len, status_old))
@@ -215,7 +216,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 			return -EINVAL;
 	}
 
-	status_new = (status_old & ~mask & ~tb_mask) | val;
+	status_new[0] = (status_old[0] & ~mask & ~tb_mask) | val;
 
 	/*
 	 * Disallow further writes if WP# pin is neither left floating nor
@@ -223,20 +224,20 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	 * WP# pin hard strapped to GND can be a valid use case.
 	 */
 	if (!(nor->flags & SNOR_F_NO_WP))
-		status_new |= SR_SRWD;
+		status_new[0] |= SR_SRWD;
 
 	if (!use_top)
-		status_new |= tb_mask;
+		status_new[0] |= tb_mask;
 
 	/* Don't bother if they're the same */
-	if (status_new == status_old)
+	if (status_new[0] == status_old[0])
 		return 0;
 
 	/* Only modify protection if it will not unlock other areas */
-	if ((status_new & mask) < (status_old & mask))
+	if ((status_new[0] & mask) < (status_old[0] & mask))
 		return -EINVAL;
 
-	return spi_nor_write_sr_and_check(nor, status_new);
+	return spi_nor_write_sr_and_check(nor, status_new[0]);
 }
 
 /*
@@ -247,7 +248,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 {
 	u64 min_prot_len;
-	int ret, status_old, status_new;
+	int ret;
+	u8 status_old[1], status_new[1];
 	u8 mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
 	u8 pow, val;
@@ -259,7 +261,7 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 	if (ret)
 		return ret;
 
-	status_old = nor->bouncebuf[0];
+	status_old[0] = nor->bouncebuf[0];
 
 	/* If nothing in our range is locked, we don't need to do anything */
 	if (spi_nor_is_unlocked_sr(nor, ofs, len, status_old))
@@ -303,24 +305,24 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 			return -EINVAL;
 	}
 
-	status_new = (status_old & ~mask & ~tb_mask) | val;
+	status_new[0] = (status_old[0] & ~mask & ~tb_mask) | val;
 
 	/* Don't protect status register if we're fully unlocked */
 	if (lock_len == 0)
-		status_new &= ~SR_SRWD;
+		status_new[0] &= ~SR_SRWD;
 
 	if (!use_top)
-		status_new |= tb_mask;
+		status_new[0] |= tb_mask;
 
 	/* Don't bother if they're the same */
-	if (status_new == status_old)
+	if (status_new[0] == status_old[0])
 		return 0;
 
 	/* Only modify protection if it will not lock other areas */
-	if ((status_new & mask) > (status_old & mask))
+	if ((status_new[0] & mask) > (status_old[0] & mask))
 		return -EINVAL;
 
-	return spi_nor_write_sr_and_check(nor, status_new);
+	return spi_nor_write_sr_and_check(nor, status_new[0]);
 }
 
 /*
@@ -338,7 +340,7 @@ static int spi_nor_sr_is_locked(struct spi_nor *nor, loff_t ofs, u64 len)
 	if (ret)
 		return ret;
 
-	return spi_nor_is_locked_sr(nor, ofs, len, nor->bouncebuf[0]);
+	return spi_nor_is_locked_sr(nor, ofs, len, nor->bouncebuf);
 }
 
 /*

-- 
2.51.0

[PATCH 09/19] mtd: spi-nor: swp: Create a helper that writes SR, CR and checks

[Miquel Raynal]

There are many helpers already to either read and/or write SR and/or CR,
as well as sometimes check the returned values. In order to be able to
switch from a 1 byte status register to a 2 bytes status register while
keeping the same level of verification, let's introduce a new helper
that writes them both (atomically) and then reads them back (separated)
to compare the values.

In case 2 bytes registers are not supported, we still have the usual
fallback available in the helper being exported to the rest of the core.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/spi-nor/core.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/core.h |  1 +
 2 files changed, 66 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 20ea80450f222242761878ebdb3dcdcca1e8877a..f56c92fd405062d93b601e7096e82e9e456cd276 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -976,6 +976,54 @@ int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr)
 	return 0;
 }
 
+/**
+ * spi_nor_write_16bit_sr_cr_and_check() - Write the Status Register 1 and the
+ * Configuration Register in one shot. Ensure that the bytes written in both
+ * registers match the received value.
+ * @nor:	pointer to a 'struct spi_nor'.
+ * @regs:	two-byte array with values to be written to the status and
+ *		configuration registers.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_write_16bit_sr_cr_and_check(struct spi_nor *nor, const u8 *regs)
+{
+	u8 written_regs[2];
+	int ret;
+
+	written_regs[0] = regs[0];
+	written_regs[1] = regs[1];
+	nor->bouncebuf[0] = regs[0];
+	nor->bouncebuf[1] = regs[1];
+
+	ret = spi_nor_write_sr(nor, nor->bouncebuf, 2);
+	if (ret)
+		return ret;
+
+	ret = spi_nor_read_sr(nor, &nor->bouncebuf[0]);
+	if (ret)
+		return ret;
+
+	if (written_regs[0] != nor->bouncebuf[0]) {
+		dev_dbg(nor->dev, "SR: Read back test failed\n");
+		return -EIO;
+	}
+
+	if (nor->flags & SNOR_F_NO_READ_CR)
+		return 0;
+
+	ret = spi_nor_read_cr(nor, &nor->bouncebuf[1]);
+	if (ret)
+		return ret;
+
+	if (written_regs[1] != nor->bouncebuf[1]) {
+		dev_dbg(nor->dev, "CR: read back test failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
 /**
  * spi_nor_write_sr_and_check() - Write the Status Register 1 and ensure that
  * the byte written match the received value without affecting other bits in the
@@ -993,6 +1041,23 @@ int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1)
 	return spi_nor_write_sr1_and_check(nor, sr1);
 }
 
+/**
+ * spi_nor_write_sr_cr_and_check() - Write the Status Register 1 and ensure that
+ * the byte written match the received value. Same for the Control Register if
+ * available.
+ * @nor:	pointer to a 'struct spi_nor'.
+ * @regs:	byte array to be written to the registers.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_write_sr_cr_and_check(struct spi_nor *nor, const u8 *regs)
+{
+	if (nor->flags & SNOR_F_HAS_16BIT_SR)
+		return spi_nor_write_16bit_sr_cr_and_check(nor, regs);
+
+	return spi_nor_write_sr1_and_check(nor, regs[0]);
+}
+
 /**
  * spi_nor_write_sr2() - Write the Status Register 2 using the
  * SPINOR_OP_WRSR2 (3eh) command.
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index ceff412f7d65ab7b856795ca5c092cddbf598cd6..516ab19dc7b86a5c6ba8729d2ba18904b922df23 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -626,6 +626,7 @@ int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
 int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
 int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
 int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr);
+int spi_nor_write_sr_cr_and_check(struct spi_nor *nor, const u8 *regs);
 
 ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 			  u8 *buf);

-- 
2.51.0

[PATCH 10/19] mtd: spi-nor: swp: Rename a mask

[Miquel Raynal]

"mask" is not very descriptive when we already manipulate two masks, and
soon will manipulate three. Rename it "bp_mask" to align with the
existing "tb_mask" and soon "cmp_mask".

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

diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index bac07287ada036f49c25237549e4900f76a0247d..971aac0581db2830a4bbee5603661ec1e6612e6f 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -57,9 +57,9 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_
 					u64 *len)
 {
 	u64 min_prot_len;
-	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
-	u8 bp, val = sr[0] & mask;
+	u8 bp, val = sr[0] & bp_mask;
 
 	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
 		val = (val & ~SR_BP3_BIT6) | SR_BP3;
@@ -160,7 +160,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	u64 min_prot_len;
 	int ret;
 	u8 status_old[1] = {}, status_new[1] = {};
-	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
 	u8 pow, val;
 	loff_t lock_len;
@@ -199,7 +199,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 		lock_len = ofs + len;
 
 	if (lock_len == nor->params->size) {
-		val = mask;
+		val = bp_mask;
 	} else {
 		min_prot_len = spi_nor_get_min_prot_length_sr(nor);
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
@@ -208,15 +208,15 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
 			val = (val & ~SR_BP3) | SR_BP3_BIT6;
 
-		if (val & ~mask)
+		if (val & ~bp_mask)
 			return -EINVAL;
 
 		/* Don't "lock" with no region! */
-		if (!(val & mask))
+		if (!(val & bp_mask))
 			return -EINVAL;
 	}
 
-	status_new[0] = (status_old[0] & ~mask & ~tb_mask) | val;
+	status_new[0] = (status_old[0] & ~bp_mask & ~tb_mask) | val;
 
 	/*
 	 * Disallow further writes if WP# pin is neither left floating nor
@@ -234,7 +234,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 		return 0;
 
 	/* Only modify protection if it will not unlock other areas */
-	if ((status_new[0] & mask) < (status_old[0] & mask))
+	if ((status_new[0] & bp_mask) < (status_old[0] & bp_mask))
 		return -EINVAL;
 
 	return spi_nor_write_sr_and_check(nor, status_new[0]);
@@ -250,7 +250,7 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 	u64 min_prot_len;
 	int ret;
 	u8 status_old[1], status_new[1];
-	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
 	u8 pow, val;
 	loff_t lock_len;
@@ -301,11 +301,11 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 			val = (val & ~SR_BP3) | SR_BP3_BIT6;
 
 		/* Some power-of-two sizes may not be supported */
-		if (val & ~mask)
+		if (val & ~bp_mask)
 			return -EINVAL;
 	}
 
-	status_new[0] = (status_old[0] & ~mask & ~tb_mask) | val;
+	status_new[0] = (status_old[0] & ~bp_mask & ~tb_mask) | val;
 
 	/* Don't protect status register if we're fully unlocked */
 	if (lock_len == 0)
@@ -319,7 +319,7 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 		return 0;
 
 	/* Only modify protection if it will not lock other areas */
-	if ((status_new[0] & mask) > (status_old[0] & mask))
+	if ((status_new[0] & bp_mask) > (status_old[0] & bp_mask))
 		return -EINVAL;
 
 	return spi_nor_write_sr_and_check(nor, status_new[0]);

-- 
2.51.0

[PATCH 11/19] mtd: spi-nor: swp: Create a TB intermediate variable

[Miquel Raynal]

Ease the future reuse of the tb (Top/Bottom) boolean by creating an
intermediate variable.

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

diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index 971aac0581db2830a4bbee5603661ec1e6612e6f..61d899b4fbf42b8bd9d86dc4e6b3ffcfe91a90e8 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -60,6 +60,7 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_
 	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
 	u8 bp, val = sr[0] & bp_mask;
+	bool tb = (nor->flags & SNOR_F_HAS_SR_TB) ? sr[0] & tb_mask : 0;
 
 	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
 		val = (val & ~SR_BP3_BIT6) | SR_BP3;
@@ -79,7 +80,7 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_
 	if (*len > nor->params->size)
 		*len = nor->params->size;
 
-	if (nor->flags & SNOR_F_HAS_SR_TB && sr[0] & tb_mask)
+	if (tb)
 		*ofs = 0;
 	else
 		*ofs = nor->params->size - *len;

-- 
2.51.0

[PATCH 12/19] mtd: spi-nor: swp: Create helpers for building the SR register

[Miquel Raynal]

The status register contains 3 or 4 BP (Block Protect) bits, 0 or 1
TB (Top/Bottom) bit, soon 0 or 1 CMP (Complement) bit. The last BP bit
and the TB bit locations change between vendors. The whole logic of
buildling the content of the status register based on some input
conditions is used two times and soon will be used 4 times.

Create dedicated helpers for these steps.

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

diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index 61d899b4fbf42b8bd9d86dc4e6b3ffcfe91a90e8..48c4f76db793a17dedb0f904d57e446de8fd04cd 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -123,6 +123,43 @@ static bool spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, u64 len,
 	return spi_nor_check_lock_status_sr(nor, ofs, len, sr, false);
 }
 
+static int spi_nor_sr_set_bp_mask(struct spi_nor *nor, u8 *sr, u8 pow)
+{
+	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 val = pow << SR_BP_SHIFT;
+
+	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
+		val = (val & ~SR_BP3) | SR_BP3_BIT6;
+
+	if (val & ~mask)
+		return -EINVAL;
+
+	sr[0] = val;
+
+	return 0;
+}
+
+static int spi_nor_build_sr(struct spi_nor *nor, const u8 *old_sr, u8 *new_sr,
+			    u8 pow, bool use_top)
+{
+	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
+	int ret;
+
+	new_sr[0] = old_sr[0] & ~bp_mask & ~tb_mask;
+
+	/* Build BP field*/
+	ret = spi_nor_sr_set_bp_mask(nor, &new_sr[0], pow);
+	if (ret)
+		return ret;
+
+	/* Build TB field */
+	if (!use_top)
+		new_sr[0] |= tb_mask;
+
+	return 0;
+}
+
 /*
  * Lock a region of the flash. Compatible with ST Micro and similar flash.
  * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in the status
@@ -162,11 +199,10 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	int ret;
 	u8 status_old[1] = {}, status_new[1] = {};
 	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
-	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
-	u8 pow, val;
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
+	u8 pow;
 
 	ret = spi_nor_read_sr(nor, nor->bouncebuf);
 	if (ret)
@@ -200,24 +236,19 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 		lock_len = ofs + len;
 
 	if (lock_len == nor->params->size) {
-		val = bp_mask;
+		pow = (nor->flags & SNOR_F_HAS_4BIT_BP) ? GENMASK(3, 0) : GENMASK(2, 0);
 	} else {
 		min_prot_len = spi_nor_get_min_prot_length_sr(nor);
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
-		val = pow << SR_BP_SHIFT;
-
-		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
-			val = (val & ~SR_BP3) | SR_BP3_BIT6;
-
-		if (val & ~bp_mask)
-			return -EINVAL;
-
-		/* Don't "lock" with no region! */
-		if (!(val & bp_mask))
-			return -EINVAL;
 	}
 
-	status_new[0] = (status_old[0] & ~bp_mask & ~tb_mask) | val;
+	ret = spi_nor_build_sr(nor, status_old, status_new, pow, use_top);
+	if (ret)
+		return ret;
+
+	/* Don't "lock" with no region! */
+	if (!(status_new[0] & bp_mask))
+		return -EINVAL;
 
 	/*
 	 * Disallow further writes if WP# pin is neither left floating nor
@@ -227,9 +258,6 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	if (!(nor->flags & SNOR_F_NO_WP))
 		status_new[0] |= SR_SRWD;
 
-	if (!use_top)
-		status_new[0] |= tb_mask;
-
 	/* Don't bother if they're the same */
 	if (status_new[0] == status_old[0])
 		return 0;
@@ -252,11 +280,10 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 	int ret;
 	u8 status_old[1], status_new[1];
 	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
-	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
-	u8 pow, val;
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
+	u8 pow;
 
 	ret = spi_nor_read_sr(nor, nor->bouncebuf);
 	if (ret)
@@ -292,29 +319,21 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 		lock_len = ofs;
 
 	if (lock_len == 0) {
-		val = 0; /* fully unlocked */
+		pow = 0; /* fully unlocked */
 	} else {
 		min_prot_len = spi_nor_get_min_prot_length_sr(nor);
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
-		val = pow << SR_BP_SHIFT;
 
-		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
-			val = (val & ~SR_BP3) | SR_BP3_BIT6;
-
-		/* Some power-of-two sizes may not be supported */
-		if (val & ~bp_mask)
-			return -EINVAL;
 	}
 
-	status_new[0] = (status_old[0] & ~bp_mask & ~tb_mask) | val;
+	ret = spi_nor_build_sr(nor, status_old, status_new, pow, use_top);
+	if (ret)
+		return ret;
 
 	/* Don't protect status register if we're fully unlocked */
 	if (lock_len == 0)
 		status_new[0] &= ~SR_SRWD;
 
-	if (!use_top)
-		status_new[0] |= tb_mask;
-
 	/* Don't bother if they're the same */
 	if (status_new[0] == status_old[0])
 		return 0;

-- 
2.51.0

[PATCH 13/19] mtd: spi-nor: swp: Simplify checking the locked/unlocked range

[Miquel Raynal]

In both the locking/unlocking steps, at the end we verify whether we do
not lock/unlock more than requested (in which case an error must be
returned).

While being possible to do that with very simple mask comparisons, it
does not scale when adding extra locking features such as the CMP
possibility. In order to make these checks slightly easier to read and
more future proof, use existing helpers to read the (future) status
register, extract the covered range, and compare it with very usual
algebric comparisons.

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

diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index 48c4f76db793a17dedb0f904d57e446de8fd04cd..c0226b13d85b3f0f340ffca347e847c17fcc727f 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -198,7 +198,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	u64 min_prot_len;
 	int ret;
 	u8 status_old[1] = {}, status_new[1] = {};
-	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
+	loff_t ofs_old, ofs_new;
+	u64 len_old, len_new;
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
@@ -246,10 +247,6 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	if (ret)
 		return ret;
 
-	/* Don't "lock" with no region! */
-	if (!(status_new[0] & bp_mask))
-		return -EINVAL;
-
 	/*
 	 * Disallow further writes if WP# pin is neither left floating nor
 	 * wrongly tied to GND (that includes internal pull-downs).
@@ -262,8 +259,16 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	if (status_new[0] == status_old[0])
 		return 0;
 
+	spi_nor_get_locked_range_sr(nor, status_old, &ofs_old, &len_old);
+	spi_nor_get_locked_range_sr(nor, status_new, &ofs_new, &len_new);
+
+	/* Don't "lock" with no region! */
+	if (!len_new)
+		return -EINVAL;
+
 	/* Only modify protection if it will not unlock other areas */
-	if ((status_new[0] & bp_mask) < (status_old[0] & bp_mask))
+	if (len_old &&
+	    (ofs_old < ofs_new || (ofs_new + len_new) < (ofs_old + len_old)))
 		return -EINVAL;
 
 	return spi_nor_write_sr_and_check(nor, status_new[0]);
@@ -279,7 +284,8 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 	u64 min_prot_len;
 	int ret;
 	u8 status_old[1], status_new[1];
-	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
+	loff_t ofs_old, ofs_new;
+	u64 len_old, len_new;
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
@@ -339,7 +345,10 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 		return 0;
 
 	/* Only modify protection if it will not lock other areas */
-	if ((status_new[0] & bp_mask) > (status_old[0] & bp_mask))
+	spi_nor_get_locked_range_sr(nor, status_old, &ofs_old, &len_old);
+	spi_nor_get_locked_range_sr(nor, status_new, &ofs_new, &len_new);
+	if (len_old && len_new &&
+	    (ofs_new < ofs_old || (ofs_old + len_old) < (ofs_new + len_new)))
 		return -EINVAL;
 
 	return spi_nor_write_sr_and_check(nor, status_new[0]);

-- 
2.51.0

[PATCH 14/19] mtd: spi-nor: swp: Cosmetic changes

[Miquel Raynal]

As a final preparation step for the introduction of CMP support, make
a few more cosmetic changes to simplify the reading of the diff when
adding the CMP feature. In particular, define "min_prot_len" earlier as
it will be reused and move the definition of the "ret" variable at the
end of the stack just because it looks better.

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

diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index c0226b13d85b3f0f340ffca347e847c17fcc727f..f5ec05d6f2680113332f47e0efbcb4d88f0d3e3c 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -195,14 +195,14 @@ static int spi_nor_build_sr(struct spi_nor *nor, const u8 *old_sr, u8 *new_sr,
  */
 static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 {
-	u64 min_prot_len;
-	int ret;
+	u64 min_prot_len = spi_nor_get_min_prot_length_sr(nor);
 	u8 status_old[1] = {}, status_new[1] = {};
 	loff_t ofs_old, ofs_new;
 	u64 len_old, len_new;
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
+	int ret;
 	u8 pow;
 
 	ret = spi_nor_read_sr(nor, nor->bouncebuf);
@@ -236,12 +236,10 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	else
 		lock_len = ofs + len;
 
-	if (lock_len == nor->params->size) {
+	if (lock_len == nor->params->size)
 		pow = (nor->flags & SNOR_F_HAS_4BIT_BP) ? GENMASK(3, 0) : GENMASK(2, 0);
-	} else {
-		min_prot_len = spi_nor_get_min_prot_length_sr(nor);
+	else
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
-	}
 
 	ret = spi_nor_build_sr(nor, status_old, status_new, pow, use_top);
 	if (ret)
@@ -281,7 +279,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
  */
 static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 {
-	u64 min_prot_len;
+	u64 min_prot_len = spi_nor_get_min_prot_length_sr(nor);
 	int ret;
 	u8 status_old[1], status_new[1];
 	loff_t ofs_old, ofs_new;
@@ -324,14 +322,11 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 	else
 		lock_len = ofs;
 
-	if (lock_len == 0) {
+	if (lock_len == 0)
 		pow = 0; /* fully unlocked */
-	} else {
-		min_prot_len = spi_nor_get_min_prot_length_sr(nor);
+	else
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
 
-	}
-
 	ret = spi_nor_build_sr(nor, status_old, status_new, pow, use_top);
 	if (ret)
 		return ret;

-- 
2.51.0

[PATCH 15/19] mtd: spi-nor: debugfs: Add locking support

[Miquel Raynal]

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

[PATCH 16/19] mtd: spi-nor: Add steps for testing locking support

[Miquel Raynal]

As recently raised on the mailing list, it may be useful to propose a
list of steps to go through in order to proove the devices have been
described correctly, especially since all the block protection
information is not stored in any kind of table and is instead filled
manually by developpers.

Use the debugfs output to ease the comparison between expectations and
reality.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 Documentation/driver-api/mtd/spi-nor.rst | 118 +++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/Documentation/driver-api/mtd/spi-nor.rst b/Documentation/driver-api/mtd/spi-nor.rst
index 148fa4288760b6ba47d530ed72c5ef81397d598f..d56ff5c42a98af23a65097c9b77cd20ef2504a49 100644
--- a/Documentation/driver-api/mtd/spi-nor.rst
+++ b/Documentation/driver-api/mtd/spi-nor.rst
@@ -203,3 +203,121 @@ section, after the ``---`` marker.
     mtd.writesize = 1
     mtd.oobsize = 0
     regions = 0
+
+5) If your flash supports locking, also follow the following test
+   procedure to make sure it correctly behaves. These tests must be
+   conducted with #WP high (no hardware protection) or the `no-wp`
+   property in the DT node.
+
+   Test full chip locking and make sure expectations, the MEMISLOCKED
+   ioctl output, the debugfs output and experimental results are all
+   aligned::
+
+    root@1:~# alias show_sectors='grep -A4 "locked sectors" /sys/kernel/debug/spi-nor/spi0.0/params'
+    root@1:~# flash_lock -u /dev/mtd0
+    root@1:~# flash_lock -i /dev/mtd0
+    Device: /dev/mtd0
+    Start: 0
+    Len: 0x4000000
+    Lock status: unlocked
+    Return code: 0
+    root@1:~# mtd_debug erase /dev/mtd0 0 2097152
+    Erased 2097152 bytes from address 0x00000000 in flash
+    root@1:~# mtd_debug write /dev/mtd0 0 2097152 spi_test
+    Copied 2097152 bytes from spi_test to address 0x00000000 in flash
+    root@1:~# mtd_debug read /dev/mtd0 0 2097152 spi_read
+    Copied 2097152 bytes from address 0x00000000 in flash to spi_read
+    root@1:~# sha256sum spi*
+    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_read
+    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_test
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-03ffffff | unlocked | 1024
+
+    root@1:~# flash_lock -l /dev/mtd0
+    root@1:~# flash_lock -i /dev/mtd0
+    Device: /dev/mtd0
+    Start: 0
+    Len: 0x4000000
+    Lock status: locked
+    Return code: 1
+    root@1:~# mtd_debug erase /dev/mtd0 0 2097152
+    Erased 2097152 bytes from address 0x00000000 in flash
+    root@1:~# mtd_debug read /dev/mtd0 0 2097152 spi_read
+    Copied 2097152 bytes from address 0x00000000 in flash to spi_read
+    root@1:~# sha256sum spi*
+    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_read
+    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_test
+    root@1:~# dd if=/dev/urandom of=./spi_test2 bs=1M count=2
+    2+0 records in
+    2+0 records out
+    root@1:~# mtd_debug write /dev/mtd0 0 2097152 spi_test2
+    Copied 2097152 bytes from spi_test to address 0x00000000 in flash
+    root@1:~# mtd_debug read /dev/mtd0 0 2097152 spi_read2
+    Copied 2097152 bytes from address 0x00000000 in flash to spi_read
+    root@1:~# sha256sum spi*
+    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_read
+    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_read2
+    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_test
+    bea9334df51c620440f86751cba0799214a016329f1736f9456d40cf40efdc88  spi_test2
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-03ffffff |   locked | 1024
+
+   Once we trust the debugfs output we can use it to test various
+   situations. Check top locking/unlocking (end of the device)::
+
+    root@1:~# bs=$(cat /sys/class/mtd/mtd0/erasesize)
+    root@1:~# size=$(cat /sys/class/mtd/mtd0/size)
+
+    root@1:~# flash_lock -u /dev/mtd0
+    root@1:~# flash_lock -l /dev/mtd0 $(($size - (2 * $bs))) 2 # last two
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-03fdffff | unlocked | 1022
+     03fe0000-03ffffff |   locked | 2
+    root@1:~# flash_lock -u /dev/mtd0 $(($size - (2 * $bs))) 1 # last one
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-03feffff | unlocked | 1023
+     03ff0000-03ffffff |   locked | 1
+
+   If the flash features 4 block protection bits (BP), we can protect
+   more than 4MB (typically 128 64kiB-blocks or more), with a finer
+   grain than locking the entire device::
+
+    root@1:~# flash_lock -u /dev/mtd0
+    root@1:~# flash_lock -l /dev/mtd0 $(($size - (2**7 * $bs))) $((2**7))
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-037fffff | unlocked | 896
+     03800000-03ffffff |   locked | 128
+
+   If the flash features a Top/Bottom (TB) bit, we can protect the
+   beginning of the flash::
+
+    root@1:~# flash_lock -u /dev/mtd0
+    root@1:~# flash_lock -l /dev/mtd0 0 2 # first two
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-0001ffff |   locked | 2
+     00020000-03ffffff | unlocked | 1022
+    root@1:~# flash_lock -u /dev/mtd0 $bs 1 # first one
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-0000ffff |   locked | 1
+     00010000-03ffffff | unlocked | 1023

-- 
2.51.0

[PATCH 17/19] mtd: spi-nor: swp: Add support for the complement feature

[Miquel Raynal]

The current locking implementation allows to select a power of two
number of blocks, which is going to be the protected amount, as well as
telling whether this is the data at the top (end of the device) or the
bottom (beginning of the device). This means at most we can cover half
of the device or the entire device, but nothing in between.

The complement feature allows a much finer grain of configuration, by
allowing to invert what is considered locked and unlocked.

Add support for this feature. The only known position for the CMP bit is
bit 6 of the configuration register.

The locking and unlocking logics are kept unchanged if the CMP bit is
unavailable. Otherwise, once the regular logic has been applied, we
check if we already found an optimal configuration. If not, we try with
the CMP bit set. If the coverage is closer to the request, we use it.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/spi-nor/core.c    |   3 +
 drivers/mtd/spi-nor/core.h    |   4 +
 drivers/mtd/spi-nor/debugfs.c |   1 +
 drivers/mtd/spi-nor/swp.c     | 184 +++++++++++++++++++++++++++++++++++-------
 include/linux/mtd/spi-nor.h   |   1 +
 5 files changed, 163 insertions(+), 30 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f56c92fd405062d93b601e7096e82e9e456cd276..86011defc0dc5a5a2b4959070e4227adcd4a9214 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2964,6 +2964,9 @@ static void spi_nor_init_flags(struct spi_nor *nor)
 			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
 	}
 
+	if (flags & SPI_NOR_HAS_CMP)
+		nor->flags |= SNOR_F_HAS_SR2_CMP_BIT6;
+
 	if (flags & SPI_NOR_RWW && nor->params->n_banks > 1 &&
 	    !nor->controller_ops)
 		nor->flags |= SNOR_F_RWW;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 8a95592994f749a62b2cc70ab85f54d36681e760..f88c3bb236674672fb15ffa989501edaf38f2ea6 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -141,6 +141,7 @@ enum spi_nor_option_flags {
 	SNOR_F_ECC		= BIT(15),
 	SNOR_F_NO_WP		= BIT(16),
 	SNOR_F_SWAP16		= BIT(17),
+	SNOR_F_HAS_SR2_CMP_BIT6	= BIT(18),
 };
 
 struct spi_nor_read_command {
@@ -477,6 +478,8 @@ struct spi_nor_id {
  *   SPI_NOR_NO_ERASE:        no erase command needed.
  *   SPI_NOR_QUAD_PP:         flash supports Quad Input Page Program.
  *   SPI_NOR_RWW:             flash supports reads while write.
+ *   SPI_NOR_HAS_CMP:         flash SR2 has complement (CMP) protect bit. Must
+ *                            be used with SPI_NOR_HAS_LOCK.
  *
  * @no_sfdp_flags:  flags that indicate support that can be discovered via SFDP.
  *                  Used when SFDP tables are not defined in the flash. These
@@ -525,6 +528,7 @@ struct flash_info {
 #define SPI_NOR_NO_ERASE		BIT(6)
 #define SPI_NOR_QUAD_PP			BIT(8)
 #define SPI_NOR_RWW			BIT(9)
+#define SPI_NOR_HAS_CMP			BIT(10)
 
 	u8 no_sfdp_flags;
 #define SPI_NOR_SKIP_SFDP		BIT(0)
diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
index d2af4c189aad68bab78c1c68688b5865eebef9b9..14e166c15e7f90e330e75817410dfe2c479d7fad 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -29,6 +29,7 @@ static const char *const snor_f_names[] = {
 	SNOR_F_NAME(ECC),
 	SNOR_F_NAME(NO_WP),
 	SNOR_F_NAME(SWAP16),
+	SNOR_F_NAME(HAS_SR2_CMP_BIT6),
 };
 #undef SNOR_F_NAME
 
diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index 0e685aa3a4fdc3100b5259659a3083c14a2cf127..3969ba82ad7ae8084183052ba046dba5bb0bdf03 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -32,6 +32,14 @@ static u8 spi_nor_get_sr_tb_mask(struct spi_nor *nor)
 		return SR_TB_BIT5;
 }
 
+static u8 spi_nor_get_sr_cmp_mask(struct spi_nor *nor)
+{
+	if (nor->flags & SNOR_F_HAS_SR2_CMP_BIT6)
+		return SR2_CMP_BIT6;
+	else
+		return 0;
+}
+
 u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
 {
 	unsigned int bp_slots, bp_slots_needed;
@@ -59,8 +67,10 @@ void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs,
 	u64 min_prot_len;
 	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
+	u8 cmp_mask = spi_nor_get_sr_tb_mask(nor);
 	u8 bp, val = sr[0] & bp_mask;
 	bool tb = (nor->flags & SNOR_F_HAS_SR_TB) ? sr[0] & tb_mask : 0;
+	bool cmp = sr[1] & cmp_mask;
 
 	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
 		val = (val & ~SR_BP3_BIT6) | SR_BP3;
@@ -68,22 +78,37 @@ void spi_nor_get_locked_range_sr(struct spi_nor *nor, const u8 *sr, loff_t *ofs,
 	bp = val >> SR_BP_SHIFT;
 
 	if (!bp) {
-		/* No protection */
-		*ofs = 0;
-		*len = 0;
-		return;
+		if (!cmp) {
+			/* No protection */
+			*ofs = 0;
+			*len = 0;
+			return;
+		} else {
+			/* Full protection */
+			*ofs = 0;
+			*len = nor->params->size;
+		}
 	}
 
 	min_prot_len = spi_nor_get_min_prot_length_sr(nor);
 	*len = min_prot_len << (bp - 1);
-
 	if (*len > nor->params->size)
 		*len = nor->params->size;
 
-	if (tb)
-		*ofs = 0;
-	else
-		*ofs = nor->params->size - *len;
+	if (cmp)
+		*len = nor->params->size - *len;
+
+	if (!cmp) {
+		if (tb)
+			*ofs = 0;
+		else
+			*ofs = nor->params->size - *len;
+	} else {
+		if (tb)
+			*ofs = nor->params->size - *len;
+		else
+			*ofs = 0;
+	}
 }
 
 /*
@@ -140,13 +165,15 @@ static int spi_nor_sr_set_bp_mask(struct spi_nor *nor, u8 *sr, u8 pow)
 }
 
 static int spi_nor_build_sr(struct spi_nor *nor, const u8 *old_sr, u8 *new_sr,
-			    u8 pow, bool use_top)
+			    u8 pow, bool use_top, bool cmp)
 {
 	u8 bp_mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
+	u8 cmp_mask = spi_nor_get_sr_cmp_mask(nor);
 	int ret;
 
 	new_sr[0] = old_sr[0] & ~bp_mask & ~tb_mask;
+	new_sr[1] = old_sr[1] & ~cmp_mask;
 
 	/* Build BP field*/
 	ret = spi_nor_sr_set_bp_mask(nor, &new_sr[0], pow);
@@ -154,9 +181,13 @@ static int spi_nor_build_sr(struct spi_nor *nor, const u8 *old_sr, u8 *new_sr,
 		return ret;
 
 	/* Build TB field */
-	if (!use_top)
+	if ((!cmp && !use_top) || (cmp && use_top))
 		new_sr[0] |= tb_mask;
 
+	/* Build CMP field */
+	if (cmp)
+		new_sr[1] |= cmp_mask;
+
 	return 0;
 }
 
@@ -166,10 +197,11 @@ static int spi_nor_build_sr(struct spi_nor *nor, const u8 *old_sr, u8 *new_sr,
  * register
  * (SR). Does not support these features found in newer SR bitfields:
  *   - SEC: sector/block protect - only handle SEC=0 (block protect)
- *   - CMP: complement protect - only support CMP=0 (range is not complemented)
  *
  * Support for the following is provided conditionally for some flash:
  *   - TB: top/bottom protect
+ *   - CMP: complement protect (BP and TP describe the unlocked part, while
+ *          the reminder is locked)
  *
  * Sample table portion for 8MB flash (Winbond w25q64fw):
  *
@@ -196,11 +228,13 @@ static int spi_nor_build_sr(struct spi_nor *nor, const u8 *old_sr, u8 *new_sr,
 static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 {
 	u64 min_prot_len = spi_nor_get_min_prot_length_sr(nor);
-	u8 status_old[1] = {}, status_new[1] = {};
-	loff_t ofs_old, ofs_new;
-	u64 len_old, len_new;
+	u8 status_old[2] = {}, status_new[2] = {}, status_new_cmp[2] = {};
+	u8 *best_status_new = status_new;
+	loff_t ofs_old, ofs_new, ofs_new_cmp;
+	u64 len_old, len_new, len_new_cmp;
 	loff_t lock_len;
-	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
+	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB,
+		can_be_cmp = spi_nor_get_sr_cmp_mask(nor);
 	bool use_top;
 	int ret;
 	u8 pow;
@@ -211,6 +245,14 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 
 	status_old[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;
+
+		status_old[1] = nor->bouncebuf[1];
+	}
+
 	/* If nothing in our range is unlocked, we don't need to do anything */
 	if (spi_nor_is_locked_sr(nor, ofs, len, status_old))
 		return 0;
@@ -241,24 +283,56 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	else
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
 
-	ret = spi_nor_build_sr(nor, status_old, status_new, pow, use_top);
+	ret = spi_nor_build_sr(nor, status_old, status_new, pow, use_top, false);
 	if (ret)
 		return ret;
 
+	/*
+	 * In case the region asked is not fully met, maybe we can try with the
+	 * complement feature
+	 */
+	spi_nor_get_locked_range_sr(nor, status_new, &ofs_new, &len_new);
+	if (can_be_cmp && len_new != lock_len) {
+		pow = ilog2(nor->params->size - lock_len) - ilog2(min_prot_len) + 1;
+		ret = spi_nor_build_sr(nor, status_old, status_new_cmp, pow, use_top, true);
+		if (ret)
+			return ret;
+
+		/*
+		 * ilog2() "floors" the result, which means in some cases we may have to
+		 * manually reduce the scope when the complement feature is used.
+		 * The uAPI is to never lock more than what is requested, but less is accepted.
+		 * Make sure we are not covering a too wide range, reduce it otherwise.
+		 */
+		spi_nor_get_locked_range_sr(nor, status_new_cmp, &ofs_new_cmp, &len_new_cmp);
+		if (len_new_cmp > lock_len) {
+			pow++;
+			ret = spi_nor_build_sr(nor, status_old, status_new_cmp, pow, use_top, true);
+			if (ret)
+				return ret;
+		}
+
+		/* Pick the CMP configuration if we cover a closer range */
+		spi_nor_get_locked_range_sr(nor, status_new, &ofs_new, &len_new);
+		spi_nor_get_locked_range_sr(nor, status_new_cmp, &ofs_new_cmp, &len_new_cmp);
+		if (len_new_cmp > len_new)
+			best_status_new = status_new_cmp;
+	}
+
 	/*
 	 * Disallow further writes if WP# pin is neither left floating nor
 	 * wrongly tied to GND (that includes internal pull-downs).
 	 * WP# pin hard strapped to GND can be a valid use case.
 	 */
 	if (!(nor->flags & SNOR_F_NO_WP))
-		status_new[0] |= SR_SRWD;
+		best_status_new[0] |= SR_SRWD;
 
 	/* Don't bother if they're the same */
-	if (status_new[0] == status_old[0])
+	if (best_status_new[0] == status_old[0] && best_status_new[1] == status_old[1])
 		return 0;
 
 	spi_nor_get_locked_range_sr(nor, status_old, &ofs_old, &len_old);
-	spi_nor_get_locked_range_sr(nor, status_new, &ofs_new, &len_new);
+	spi_nor_get_locked_range_sr(nor, best_status_new, &ofs_new, &len_new);
 
 	/* Don't "lock" with no region! */
 	if (!len_new)
@@ -269,7 +343,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 	    (ofs_old < ofs_new || (ofs_new + len_new) < (ofs_old + len_old)))
 		return -EINVAL;
 
-	return spi_nor_write_sr_and_check(nor, status_new[0]);
+	return spi_nor_write_sr_cr_and_check(nor, best_status_new);
 }
 
 /*
@@ -281,11 +355,13 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 {
 	u64 min_prot_len = spi_nor_get_min_prot_length_sr(nor);
 	int ret;
-	u8 status_old[1], status_new[1];
-	loff_t ofs_old, ofs_new;
-	u64 len_old, len_new;
+	u8 status_old[2], status_new[2], status_new_cmp[2];
+	u8 *best_status_new = status_new;
+	loff_t ofs_old, ofs_new, ofs_new_cmp;
+	u64 len_old, len_new, len_new_cmp;
 	loff_t lock_len;
-	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
+	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB,
+		can_be_cmp = spi_nor_get_sr_cmp_mask(nor);
 	bool use_top;
 	u8 pow;
 
@@ -295,6 +371,14 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 
 	status_old[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;
+
+		status_old[1] = nor->bouncebuf[1];
+	}
+
 	/* If nothing in our range is locked, we don't need to do anything */
 	if (spi_nor_is_unlocked_sr(nor, ofs, len, status_old))
 		return 0;
@@ -327,26 +411,58 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 	else
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
 
-	ret = spi_nor_build_sr(nor, status_old, status_new, pow, use_top);
+	ret = spi_nor_build_sr(nor, status_old, status_new, pow, use_top, false);
 	if (ret)
 		return ret;
 
+	/*
+	 * In case the region asked is not fully met, maybe we can try with the
+	 * complement feature
+	 */
+	spi_nor_get_locked_range_sr(nor, status_new, &ofs_new, &len_new);
+	if (can_be_cmp && len_new != lock_len) {
+		pow = ilog2(nor->params->size - lock_len) - ilog2(min_prot_len) + 1;
+		ret = spi_nor_build_sr(nor, status_old, status_new_cmp, pow, use_top, true);
+		if (ret)
+			return ret;
+
+		/*
+		 * ilog2() "floors" the result, which means in some cases we may have to
+		 * manually reduce the scope when the complement feature is used.
+		 * The uAPI is to never unlock more than what is requested, but less is accepted.
+		 * Make sure we are not covering a too small range, increase it otherwise.
+		 */
+		spi_nor_get_locked_range_sr(nor, status_new_cmp, &ofs_new_cmp, &len_new_cmp);
+		if (len_new_cmp < lock_len) {
+			pow--;
+			ret = spi_nor_build_sr(nor, status_old, status_new_cmp, pow, use_top, true);
+			if (ret)
+				return ret;
+		}
+
+		/* Pick the CMP configuration if we cover a closer range */
+		spi_nor_get_locked_range_sr(nor, status_new, &ofs_new, &len_new);
+		spi_nor_get_locked_range_sr(nor, status_new_cmp, &ofs_new_cmp, &len_new_cmp);
+		if (len_new_cmp > len_new)
+			best_status_new = status_new_cmp;
+	}
+
 	/* Don't protect status register if we're fully unlocked */
 	if (lock_len == 0)
-		status_new[0] &= ~SR_SRWD;
+		best_status_new[0] &= ~SR_SRWD;
 
 	/* Don't bother if they're the same */
-	if (status_new[0] == status_old[0])
+	if (best_status_new[0] == status_old[0] && best_status_new[1] == status_old[1])
 		return 0;
 
 	/* Only modify protection if it will not lock other areas */
 	spi_nor_get_locked_range_sr(nor, status_old, &ofs_old, &len_old);
-	spi_nor_get_locked_range_sr(nor, status_new, &ofs_new, &len_new);
+	spi_nor_get_locked_range_sr(nor, best_status_new, &ofs_new, &len_new);
 	if (len_old && len_new &&
 	    (ofs_new < ofs_old || (ofs_old + len_old) < (ofs_new + len_new)))
 		return -EINVAL;
 
-	return spi_nor_write_sr_and_check(nor, status_new[0]);
+	return spi_nor_write_sr_cr_and_check(nor, best_status_new);
 }
 
 /*
@@ -364,6 +480,14 @@ static int spi_nor_sr_is_locked(struct spi_nor *nor, loff_t ofs, u64 len)
 	if (ret)
 		return ret;
 
+	if (!(nor->flags & SNOR_F_NO_READ_CR)) {
+		ret = spi_nor_read_cr(nor, nor->bouncebuf + 1);
+		if (ret)
+			return ret;
+	} else {
+		nor->bouncebuf[1] = 0;
+	}
+
 	return spi_nor_is_locked_sr(nor, ofs, len, nor->bouncebuf);
 }
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 90a0cf58351295c63baea4f064b49b7390337d37..0277b0acf0620fe088782c982568794b87e56e5a 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -125,6 +125,7 @@
 #define SR2_LB1			BIT(3)	/* Security Register Lock Bit 1 */
 #define SR2_LB2			BIT(4)	/* Security Register Lock Bit 2 */
 #define SR2_LB3			BIT(5)	/* Security Register Lock Bit 3 */
+#define SR2_CMP_BIT6		BIT(6)
 #define SR2_QUAD_EN_BIT7	BIT(7)
 
 /* Supported SPI protocols */

-- 
2.51.0

[PATCH 18/19] mtd: spi-nor: Add steps for testing locking with CMP

[Miquel Raynal]

Extend the test coverage by giving guidelines to verify the CMP bit acts
according to our expectations.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
The instructions listed in this file target people adding support for
new chips, however here are below extra steps that I also ran with the
same W25H512NWxxAM chip. They are here to prove core correctness.

    $ flash_lock -u /dev/mtd0
    $ flash_lock -l /dev/mtd0 0 1008
    $ show_sectors
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-03efffff |   locked | 1008
     03f00000-03ffffff | unlocked | 16
    $ flash_lock -l /dev/mtd0 0 1009
    $ show_sectors # should not change
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-03efffff |   locked | 1008
     03f00000-03ffffff | unlocked | 16
    $ flash_lock -l /dev/mtd0 0 1015
    $ show_sectors # should not change
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-03efffff |   locked | 1008
     03f00000-03ffffff | unlocked | 16
    $ flash_lock -l /dev/mtd0 0 1016
    $ show_sectors # should cover more
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-03f7ffff |   locked | 1016
     03f80000-03ffffff | unlocked | 8
    $ flash_lock -u /dev/mtd0 $((1015 * $bs)) 1
    $ show_sectors # should not change
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-03f7ffff |   locked | 1016
     03f80000-03ffffff | unlocked | 8
    $ flash_lock -u /dev/mtd0 $((1009 * $bs)) 7
    $ show_sectors # should not change
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-03f7ffff |   locked | 1016
     03f80000-03ffffff | unlocked | 8
    $ flash_lock -u /dev/mtd0 $((1008 * $bs)) 8
    $ show_sectors # range should reduce down to initial value
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-03efffff |   locked | 1008
     03f00000-03ffffff | unlocked | 16

    [Similar situations, on the other side of the device]

    $ flash_lock -u /dev/mtd0
    $ flash_lock -l /dev/mtd0 $((16 * $bs)) 1008
    $ show_sectors
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-000fffff | unlocked | 16
     00100000-03ffffff |   locked | 1008
    $ flash_lock -l /dev/mtd0 $((15 * $bs)) 1009
    $ show_sectors # should not change
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-000fffff | unlocked | 16
     00100000-03ffffff |   locked | 1008
    $ flash_lock -l /dev/mtd0 $((9 * $bs)) 1015
    $ show_sectors # should not change
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-000fffff | unlocked | 16
     00100000-03ffffff |   locked | 1008
    $ flash_lock -l /dev/mtd0 $((8 * $bs)) 1016
    $ show_sectors # should cover more
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-0007ffff | unlocked | 8
     00080000-03ffffff |   locked | 1016
    $ flash_lock -u /dev/mtd0 $((8 * $bs)) 1
    $ show_sectors # should not change
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-0007ffff | unlocked | 8
     00080000-03ffffff |   locked | 1016
    $ flash_lock -u /dev/mtd0 $((8 * $bs)) 7
    $ show_sectors # should not change
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-0007ffff | unlocked | 8
     00080000-03ffffff |   locked | 1016
    $ flash_lock -u /dev/mtd0 $((8 * $bs)) 8
    $ show_sectors # range should reduce down to initial value
    software locked sectors
     region (in hex)   | status   | #blocks
     ------------------+----------+--------
     00000000-000fffff | unlocked | 16
     00100000-03ffffff |   locked | 1008
---
 Documentation/driver-api/mtd/spi-nor.rst | 36 ++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/driver-api/mtd/spi-nor.rst b/Documentation/driver-api/mtd/spi-nor.rst
index d56ff5c42a98af23a65097c9b77cd20ef2504a49..e00ca19e7dd6cb3a118b81f8a1fe36470355fa42 100644
--- a/Documentation/driver-api/mtd/spi-nor.rst
+++ b/Documentation/driver-api/mtd/spi-nor.rst
@@ -321,3 +321,39 @@ section, after the ``---`` marker.
      ------------------+----------+--------
      00000000-0000ffff |   locked | 1
      00010000-03ffffff | unlocked | 1023
+
+   If the flash features a Complement (CMP) bit, we can protect with
+   more granularity above half of the capacity. Let's lock all but one
+   block, then unlock one more block::
+
+    root@1:~# all_but_one=$((($size / $bs) - 1))
+    root@1:~# flash_lock -u /dev/mtd0
+    root@1:~# flash_lock -l /dev/mtd0 $bs $all_but_one # all but the first
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-0000ffff | unlocked | 1
+     00010000-03ffffff |   locked | 1023
+    root@1:~# flash_lock -u /dev/mtd0 $bs 1 # all but the two first
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-0001ffff | unlocked | 2
+     00020000-03ffffff |   locked | 1022
+    root@1:~# flash_lock -u /dev/mtd0
+    root@1:~# flash_lock -l /dev/mtd0 0 $all_but_one # same from the other side
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-03feffff |   locked | 1023
+     03ff0000-03ffffff | unlocked | 1
+    root@1:~# flash_lock -u /dev/mtd0 $(($size - (2 * $bs))) 1 # all but two
+    root@1:~# show_sectors
+    software locked sectors
+     region (in hex)   | status   | #blocks
+     ------------------+----------+--------
+     00000000-03fdffff |   locked | 1022
+     03fe0000-03ffffff | unlocked | 2

-- 
2.51.0

[PATCH 19/19] mtd: spi-nor: winbond: Add CMP locking support

[Miquel Raynal]

All W25Q01NWxxIQ, W25Q{01,02}NWxxIM and W25H{512,01,02}NWxxAM chips have
support for the locking complement (CMP) feature. Add the relevant bit
to enable it.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
The documentation about testing the CMP feature has been written based
on a W25H512NWxxAM test run. I would like to gather feedback on this
entire series before providing a very detailed output for the 5 other
almost identical chips.
---
 drivers/mtd/spi-nor/winbond.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index fb855fe44733db5664c200520d19a6be33edc323..22a923e6b9d7d8f1571cd6b45a688c08ff400b63 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -346,27 +346,33 @@ static const struct flash_info winbond_nor_parts[] = {
 	}, {
 		/* W25Q01NWxxIQ */
 		.id = SNOR_ID(0xef, 0x60, 0x21),
-		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 | SPI_NOR_4BIT_BP,
+		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 |
+			 SPI_NOR_4BIT_BP | SPI_NOR_HAS_CMP,
 	}, {
 		/* W25Q01NWxxIM */
 		.id = SNOR_ID(0xef, 0x80, 0x21),
-		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 | SPI_NOR_4BIT_BP,
+		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 |
+			 SPI_NOR_4BIT_BP | SPI_NOR_HAS_CMP,
 	}, {
 		/* W25Q02NWxxIM */
 		.id = SNOR_ID(0xef, 0x80, 0x22),
-		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 | SPI_NOR_4BIT_BP,
+		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 |
+			 SPI_NOR_4BIT_BP | SPI_NOR_HAS_CMP,
 	}, {
 		/* W25H512NWxxAM */
 		.id = SNOR_ID(0xef, 0xa0, 0x20),
-		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 | SPI_NOR_4BIT_BP,
+		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 |
+			 SPI_NOR_4BIT_BP | SPI_NOR_HAS_CMP,
 	}, {
 		/* W25H01NWxxAM */
 		.id = SNOR_ID(0xef, 0xa0, 0x21),
-		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 | SPI_NOR_4BIT_BP,
+		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 |
+			 SPI_NOR_4BIT_BP | SPI_NOR_HAS_CMP,
 	}, {
 		/* W25H02NWxxAM */
 		.id = SNOR_ID(0xef, 0xa0, 0x22),
-		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 | SPI_NOR_4BIT_BP,
+		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 |
+			 SPI_NOR_4BIT_BP | SPI_NOR_HAS_CMP,
 	},
 };
 

-- 
2.51.0

Re: [PATCH 01/19] mtd: spi-nor: debugfs: Fix the flags list

[Michael Walle]

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> As mentioned above the spi_nor_option_flags enumeration in core.h, this
> list should be kept in sync with the one in the core.
>
> Add the missing flag.
>
> Fixes: 6a42bc97ccda ("mtd: spi-nor: core: Allow specifying the byte order in Octal DTR mode")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

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

Re: [PATCH 02/19] mtd: spi-nor: swp: Improve locking user experience

[Michael Walle]

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

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> In the case of a single block being locked, if the user want to fully
> unlock the device it has two possibilities:
> - either it asks to unlock the entire device, and this works;
> - or it asks to unlock just the blocks that are currently locked, which
> fails.
>
> It fails because the conditions "can_be_top" and "can_be_bottom" are
> true. Indeed, in this case, we unlock everything, to the TB bit does not
> matter. However in the current implementation, use_top would be true (as
> this is the favourite option) and lock_len, which in practice should be
> reduced down to 0, is set to "nor->params->size - (ofs + len)" which is
> a positive number. This is wrong.

This only happens if you try to unlock the first sector, correct? If
my maths are correct, trying it on the last sector, lock_len should
be 0, i.e in that case "ofs + len == size".

If it's the first sector (or sectors), lock_len will end up with
"size - N * 64k", which is clearly wrong.

> An easy way is to simply add an extra condition. In the unlock() path,
> if we can achieve the results from both sides, it means we unlock
> everything and lock_len must simply be 0.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> For me, this result was clearly unexpected, but I am not sure this
> qualifies as a fix.

That's definetly a bug, esp. because it will lock an entire
unrelated region. And it seems to go back all the to commit
3dd8012a8eeb "mtd: spi-nor: add TB (Top/Bottom) protect support").

> ---
>  drivers/mtd/spi-nor/swp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
> index 9b07f83aeac76dce2109f90dfa1534c9bd93330d..9bc5a356444665ad8824e9e12d679fd551b3e67d 100644
> --- a/drivers/mtd/spi-nor/swp.c
> +++ b/drivers/mtd/spi-nor/swp.c
> @@ -281,7 +281,9 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
>  	use_top = can_be_top;
>  
>  	/* lock_len: length of region that should remain locked */
> -	if (use_top)
> +	if (can_be_top && can_be_bottom)
> +		lock_len = 0;

Could you please add a comment stating that if both are true, it
means that both adjacent regions are unlocked and thus the entire
flash will be unlocked.

-michael

> +	else if (use_top)
>  		lock_len = nor->params->size - (ofs + len);
>  	else
>  		lock_len = ofs;


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

Re: [PATCH 03/19] mtd: spi-nor: Improve opcodes documentation

[Michael Walle]

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

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> There are two status registers, named 1 and 2, all the opcodes imply a 1
> byte access. Make it clear by aligning all comments on the same pattern,
> for the four "{read,write} status {1,2} registers" definitions.

Not sure, what you mean. The current comment implies 1 byte access.
But that is wrong because the WRSR can be used to write both the SR1
and SR2 (or sometimes called CR).

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

-michael

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/linux/mtd/spi-nor.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index cdcfe0fd2e7d624bbb66fefcb87823bce300268e..90a0cf58351295c63baea4f064b49b7390337d37 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -21,8 +21,8 @@
>  /* Flash opcodes. */
>  #define SPINOR_OP_WRDI		0x04	/* Write disable */
>  #define SPINOR_OP_WREN		0x06	/* Write enable */
> -#define SPINOR_OP_RDSR		0x05	/* Read status register */
> -#define SPINOR_OP_WRSR		0x01	/* Write status register 1 byte */
> +#define SPINOR_OP_RDSR		0x05	/* Read status register 1 */
> +#define SPINOR_OP_WRSR		0x01	/* Write status register 1 */
>  #define SPINOR_OP_RDSR2		0x3f	/* Read status register 2 */
>  #define SPINOR_OP_WRSR2		0x3e	/* Write status register 2 */
>  #define SPINOR_OP_READ		0x03	/* Read data bytes (low frequency) */


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

Re: [PATCH 04/19] mtd: spi-nor: debugfs: Align variable access with the rest of the file

[Michael Walle]

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

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> The "params" variable is used everywhere else, align this particular
> line of the file to use "params" directly rather than the "nor" pointer.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

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

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

Re: [PATCH 05/19] mtd: spi-nor: debugfs: Enhance output

[Michael Walle]

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

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> Align the number of dashes to the bigger column width (the title in this
> case) to make the output more pleasant and aligned with what is done
> in the "params" file output.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

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

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

Re: [PATCH 06/19] mtd: spi-nor: swp: Explain the MEMLOCK ioctl implementation behaviour

[Michael Walle]

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

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

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

Re: [PATCH 07/19] mtd: spi-nor: swp: Clarify a comment

[Michael Walle]

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

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> The comment states that all power of two sizes are not supported. This

No it says "some power-of-two". That's clearly not all :)

I'm fine with either:
Reviewed-by: Michael Walle <mwalle@kernel.org>

-michael

> is very device dependent (based on the size), so modulate a bit the
> sentence to make it more accurate.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/spi-nor/swp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
> index ede03f26de3c65ff53c1cb888c2c43aea268b85a..350fb8cd67dbafa3c62201c8c06bff7131143c04 100644
> --- a/drivers/mtd/spi-nor/swp.c
> +++ b/drivers/mtd/spi-nor/swp.c
> @@ -298,7 +298,7 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
>  		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
>  			val = (val & ~SR_BP3) | SR_BP3_BIT6;
>  
> -		/* Some power-of-two sizes are not supported */
> +		/* Some power-of-two sizes may not be supported */
>  		if (val & ~mask)
>  			return -EINVAL;
>  	}


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

Re: [PATCH 16/19] mtd: spi-nor: Add steps for testing locking support

[Michael Walle]

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

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> As recently raised on the mailing list, it may be useful to propose a
> list of steps to go through in order to proove the devices have been
> described correctly, especially since all the block protection
> information is not stored in any kind of table and is instead filled
> manually by developpers.
>
> Use the debugfs output to ease the comparison between expectations and
> reality.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  Documentation/driver-api/mtd/spi-nor.rst | 118 +++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
>
> diff --git a/Documentation/driver-api/mtd/spi-nor.rst b/Documentation/driver-api/mtd/spi-nor.rst
> index 148fa4288760b6ba47d530ed72c5ef81397d598f..d56ff5c42a98af23a65097c9b77cd20ef2504a49 100644
> --- a/Documentation/driver-api/mtd/spi-nor.rst
> +++ b/Documentation/driver-api/mtd/spi-nor.rst
> @@ -203,3 +203,121 @@ section, after the ``---`` marker.
>      mtd.writesize = 1
>      mtd.oobsize = 0
>      regions = 0
> +
> +5) If your flash supports locking, also follow the following test
> +   procedure to make sure it correctly behaves. These tests must be
> +   conducted with #WP high (no hardware protection) or the `no-wp`
> +   property in the DT node.

Or? If WPn is low, the no-wp property doesn't matter. It's hardware
write protected. Also there is that corner case, where you can
actually fully lock your flash: WPn hard tied to low. Be aware, that
you can enable locking even if WPn is tied low. That has the use
case to initially program the flash and then lock it forever. So we
might add a warning note, that WPn should somehow be controllable
(or be sure that is tied high) before conducting the locking tests.

> +
> +   Test full chip locking and make sure expectations, the MEMISLOCKED
> +   ioctl output, the debugfs output and experimental results are all
> +   aligned::
> +
> +    root@1:~# alias show_sectors='grep -A4 "locked sectors" /sys/kernel/debug/spi-nor/spi0.0/params'
> +    root@1:~# flash_lock -u /dev/mtd0
> +    root@1:~# flash_lock -i /dev/mtd0
> +    Device: /dev/mtd0
> +    Start: 0
> +    Len: 0x4000000
> +    Lock status: unlocked
> +    Return code: 0
> +    root@1:~# mtd_debug erase /dev/mtd0 0 2097152
> +    Erased 2097152 bytes from address 0x00000000 in flash
> +    root@1:~# mtd_debug write /dev/mtd0 0 2097152 spi_test
> +    Copied 2097152 bytes from spi_test to address 0x00000000 in flash
> +    root@1:~# mtd_debug read /dev/mtd0 0 2097152 spi_read
> +    Copied 2097152 bytes from address 0x00000000 in flash to spi_read
> +    root@1:~# sha256sum spi*
> +    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_read
> +    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_test
> +    root@1:~# show_sectors
> +    software locked sectors
> +     region (in hex)   | status   | #blocks
> +     ------------------+----------+--------
> +     00000000-03ffffff | unlocked | 1024
> +
> +    root@1:~# flash_lock -l /dev/mtd0
> +    root@1:~# flash_lock -i /dev/mtd0
> +    Device: /dev/mtd0
> +    Start: 0
> +    Len: 0x4000000
> +    Lock status: locked
> +    Return code: 1
> +    root@1:~# mtd_debug erase /dev/mtd0 0 2097152
> +    Erased 2097152 bytes from address 0x00000000 in flash
> +    root@1:~# mtd_debug read /dev/mtd0 0 2097152 spi_read
> +    Copied 2097152 bytes from address 0x00000000 in flash to spi_read
> +    root@1:~# sha256sum spi*
> +    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_read
> +    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_test
> +    root@1:~# dd if=/dev/urandom of=./spi_test2 bs=1M count=2
> +    2+0 records in
> +    2+0 records out
> +    root@1:~# mtd_debug write /dev/mtd0 0 2097152 spi_test2
> +    Copied 2097152 bytes from spi_test to address 0x00000000 in flash
> +    root@1:~# mtd_debug read /dev/mtd0 0 2097152 spi_read2
> +    Copied 2097152 bytes from address 0x00000000 in flash to spi_read
> +    root@1:~# sha256sum spi*
> +    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_read
> +    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_read2
> +    c444216a6ba2a4a66cccd60a0dd062bce4b865dd52b200ef5e21838c4b899ac8  spi_test
> +    bea9334df51c620440f86751cba0799214a016329f1736f9456d40cf40efdc88  spi_test2
> +    root@1:~# show_sectors
> +    software locked sectors
> +     region (in hex)   | status   | #blocks
> +     ------------------+----------+--------
> +     00000000-03ffffff |   locked | 1024
> +
> +   Once we trust the debugfs output we can use it to test various
> +   situations. Check top locking/unlocking (end of the device)::
> +
> +    root@1:~# bs=$(cat /sys/class/mtd/mtd0/erasesize)
> +    root@1:~# size=$(cat /sys/class/mtd/mtd0/size)
> +
> +    root@1:~# flash_lock -u /dev/mtd0
> +    root@1:~# flash_lock -l /dev/mtd0 $(($size - (2 * $bs))) 2 # last two
> +    root@1:~# show_sectors
> +    software locked sectors
> +     region (in hex)   | status   | #blocks
> +     ------------------+----------+--------
> +     00000000-03fdffff | unlocked | 1022
> +     03fe0000-03ffffff |   locked | 2
> +    root@1:~# flash_lock -u /dev/mtd0 $(($size - (2 * $bs))) 1 # last one

huh? shouldn't that be 1 * $bs?

-michael

> +    root@1:~# show_sectors
> +    software locked sectors
> +     region (in hex)   | status   | #blocks
> +     ------------------+----------+--------
> +     00000000-03feffff | unlocked | 1023
> +     03ff0000-03ffffff |   locked | 1
> +
> +   If the flash features 4 block protection bits (BP), we can protect
> +   more than 4MB (typically 128 64kiB-blocks or more), with a finer
> +   grain than locking the entire device::
> +
> +    root@1:~# flash_lock -u /dev/mtd0
> +    root@1:~# flash_lock -l /dev/mtd0 $(($size - (2**7 * $bs))) $((2**7))
> +    root@1:~# show_sectors
> +    software locked sectors
> +     region (in hex)   | status   | #blocks
> +     ------------------+----------+--------
> +     00000000-037fffff | unlocked | 896
> +     03800000-03ffffff |   locked | 128
> +
> +   If the flash features a Top/Bottom (TB) bit, we can protect the
> +   beginning of the flash::
> +
> +    root@1:~# flash_lock -u /dev/mtd0
> +    root@1:~# flash_lock -l /dev/mtd0 0 2 # first two
> +    root@1:~# show_sectors
> +    software locked sectors
> +     region (in hex)   | status   | #blocks
> +     ------------------+----------+--------
> +     00000000-0001ffff |   locked | 2
> +     00020000-03ffffff | unlocked | 1022
> +    root@1:~# flash_lock -u /dev/mtd0 $bs 1 # first one
> +    root@1:~# show_sectors
> +    software locked sectors
> +     region (in hex)   | status   | #blocks
> +     ------------------+----------+--------
> +     00000000-0000ffff |   locked | 1
> +     00010000-03ffffff | unlocked | 1023


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

Re: [PATCH 15/19] mtd: spi-nor: debugfs: Add locking support

[Michael Walle]

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

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,


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

Re: [PATCH 02/19] mtd: spi-nor: swp: Improve locking user experience

[Miquel Raynal]

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

> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>> In the case of a single block being locked, if the user want to fully
>> unlock the device it has two possibilities:
>> - either it asks to unlock the entire device, and this works;
>> - or it asks to unlock just the blocks that are currently locked, which
>> fails.
>>
>> It fails because the conditions "can_be_top" and "can_be_bottom" are
>> true. Indeed, in this case, we unlock everything, to the TB bit does not
>> matter. However in the current implementation, use_top would be true (as
>> this is the favourite option) and lock_len, which in practice should be
>> reduced down to 0, is set to "nor->params->size - (ofs + len)" which is
>> a positive number. This is wrong.
>
> This only happens if you try to unlock the first sector, correct? If
> my maths are correct, trying it on the last sector, lock_len should
> be 0, i.e in that case "ofs + len == size".
>
> If it's the first sector (or sectors), lock_len will end up with
> "size - N * 64k", which is clearly wrong.

That's it. Actually I forgot to mention it was happening only with the
first sectors, not the last ones, so yes you are correct, it matches my
maths and experiments.

>> An easy way is to simply add an extra condition. In the unlock() path,
>> if we can achieve the results from both sides, it means we unlock
>> everything and lock_len must simply be 0.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>> For me, this result was clearly unexpected, but I am not sure this
>> qualifies as a fix.
>
> That's definetly a bug, esp. because it will lock an entire
> unrelated region. And it seems to go back all the to commit
> 3dd8012a8eeb "mtd: spi-nor: add TB (Top/Bottom) protect support").
>
>> ---
>>  drivers/mtd/spi-nor/swp.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
>> index 9b07f83aeac76dce2109f90dfa1534c9bd93330d..9bc5a356444665ad8824e9e12d679fd551b3e67d 100644
>> --- a/drivers/mtd/spi-nor/swp.c
>> +++ b/drivers/mtd/spi-nor/swp.c
>> @@ -281,7 +281,9 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
>>  	use_top = can_be_top;
>>  
>>  	/* lock_len: length of region that should remain locked */
>> -	if (use_top)
>> +	if (can_be_top && can_be_bottom)
>> +		lock_len = 0;
>
> Could you please add a comment stating that if both are true, it
> means that both adjacent regions are unlocked and thus the entire
> flash will be unlocked.

Ofc.

Thanks,
Miquèl

Re: [PATCH 06/19] mtd: spi-nor: swp: Explain the MEMLOCK ioctl implementation behaviour

[Miquel Raynal]

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

Re: [PATCH 07/19] mtd: spi-nor: swp: Clarify a comment

[Miquel Raynal]

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

> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>> The comment states that all power of two sizes are not supported. This
>
> No it says "some power-of-two". That's clearly not all :)

Ah crap, I failed at replicating the comment here. Well, the change I
believe is still correct but the commit is indeed wrong, I'll fix it.

> I'm fine with either:
> Reviewed-by: Michael Walle <mwalle@kernel.org>

Thanks,
Miquèl

Re: [PATCH 16/19] mtd: spi-nor: Add steps for testing locking support

[Miquel Raynal]

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

> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>> As recently raised on the mailing list, it may be useful to propose a
>> list of steps to go through in order to proove the devices have been
>> described correctly, especially since all the block protection
>> information is not stored in any kind of table and is instead filled
>> manually by developpers.
>>
>> Use the debugfs output to ease the comparison between expectations and
>> reality.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  Documentation/driver-api/mtd/spi-nor.rst | 118 +++++++++++++++++++++++++++++++
>>  1 file changed, 118 insertions(+)
>>
>> diff --git a/Documentation/driver-api/mtd/spi-nor.rst b/Documentation/driver-api/mtd/spi-nor.rst
>> index 148fa4288760b6ba47d530ed72c5ef81397d598f..d56ff5c42a98af23a65097c9b77cd20ef2504a49 100644
>> --- a/Documentation/driver-api/mtd/spi-nor.rst
>> +++ b/Documentation/driver-api/mtd/spi-nor.rst
>> @@ -203,3 +203,121 @@ section, after the ``---`` marker.
>>      mtd.writesize = 1
>>      mtd.oobsize = 0
>>      regions = 0
>> +
>> +5) If your flash supports locking, also follow the following test
>> +   procedure to make sure it correctly behaves. These tests must be
>> +   conducted with #WP high (no hardware protection) or the `no-wp`
>> +   property in the DT node.
>
> Or? If WPn is low, the no-wp property doesn't matter.
> It's hardware write protected.

This is only correct if the SRP bit is set. If the SRP bit is unset, #WP
low still has no impact. And this is why setting the no-wp property
matters, because otherwise the SR_SRWD bit gets set at the first locking
command.

I disliked this behaviour as I actually got almost locked with
a non drivable #WP pin and had to play with pinctrl in order to force it
high and allow me to overwrite the SR_SRWD bit back.

> Also there is that corner case, where you can
> actually fully lock your flash: WPn hard tied to low. Be aware, that
> you can enable locking even if WPn is tied low. That has the use
> case to initially program the flash and then lock it forever. So we
> might add a warning note, that WPn should somehow be controllable
> (or be sure that is tied high) before conducting the locking tests.

Yes, that is the situation I met, it is not documented anywhere except
the code.

My intent with this paragraph was to hint people not to conduct these
tests under certain situations, but we can definitely improve that. What
about:

5) If your flash supports locking, please got through the following test
   procedure to make sure it correctly behaves.

   Warning: These tests may hard lock your device! Make sure:
   - The device is not hard locked already (#WP strapped to low and
     SR_SRWD bit set)
   - If you have a #WP pin, you may want to set `no-wp` in your DT for
     the time of the test, to only make use of software protection.

Please amend this text if you still think it is missing his goal.

>> +    root@1:~# flash_lock -u /dev/mtd0
>> +    root@1:~# flash_lock -l /dev/mtd0 $(($size - (2 * $bs))) 2 # last two
>> +    root@1:~# show_sectors
>> +    software locked sectors
>> +     region (in hex)   | status   | #blocks
>> +     ------------------+----------+--------
>> +     00000000-03fdffff | unlocked | 1022
>> +     03fe0000-03ffffff |   locked | 2
>> +    root@1:~# flash_lock -u /dev/mtd0 $(($size - (2 * $bs))) 1 # last one
>
> huh? shouldn't that be 1 * $bs?

I don't think so:
- first we lock the last two blocks (offset: size - 2 blocks, length: 2
  blocks)
- then we unlock the penultimate block so that only the last block
  remains locked (offset: size - 2 blocks, length 1).

I we were doing flash_lock -u <mtd> $(($size - (1 * $bs))) 1, we would
be asking to unlock the very last block and keep only the penultimate
block locked, which would not be supported.

Thanks,
Miquèl

Re: [PATCH 15/19] mtd: spi-nor: debugfs: Add locking support

[Miquel Raynal]

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

Re: [PATCH 16/19] mtd: spi-nor: Add steps for testing locking support

[Michael Walle]

On Wed Nov 19, 2025 at 10:40 AM CET, Miquel Raynal wrote:
> On 18/11/2025 at 13:24:02 +01, "Michael Walle" <mwalle@kernel.org> wrote:
>
>> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>>> As recently raised on the mailing list, it may be useful to propose a
>>> list of steps to go through in order to proove the devices have been
>>> described correctly, especially since all the block protection
>>> information is not stored in any kind of table and is instead filled
>>> manually by developpers.
>>>
>>> Use the debugfs output to ease the comparison between expectations and
>>> reality.
>>>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> ---
>>>  Documentation/driver-api/mtd/spi-nor.rst | 118 +++++++++++++++++++++++++++++++
>>>  1 file changed, 118 insertions(+)
>>>
>>> diff --git a/Documentation/driver-api/mtd/spi-nor.rst b/Documentation/driver-api/mtd/spi-nor.rst
>>> index 148fa4288760b6ba47d530ed72c5ef81397d598f..d56ff5c42a98af23a65097c9b77cd20ef2504a49 100644
>>> --- a/Documentation/driver-api/mtd/spi-nor.rst
>>> +++ b/Documentation/driver-api/mtd/spi-nor.rst
>>> @@ -203,3 +203,121 @@ section, after the ``---`` marker.
>>>      mtd.writesize = 1
>>>      mtd.oobsize = 0
>>>      regions = 0
>>> +
>>> +5) If your flash supports locking, also follow the following test
>>> +   procedure to make sure it correctly behaves. These tests must be
>>> +   conducted with #WP high (no hardware protection) or the `no-wp`
>>> +   property in the DT node.
>>
>> Or? If WPn is low, the no-wp property doesn't matter.
>> It's hardware write protected.
>
> This is only correct if the SRP bit is set. If the SRP bit is unset, #WP
> low still has no impact. And this is why setting the no-wp property
> matters, because otherwise the SR_SRWD bit gets set at the first locking
> command.

Yes, but only if it's not write-protected before. The text read as
if you could ignore the hardware write protection. More below.

> I disliked this behaviour as I actually got almost locked with
> a non drivable #WP pin and had to play with pinctrl in order to force it
> high and allow me to overwrite the SR_SRWD bit back.

That no-wp property was recently introduced because of faulty
hardware. Before that, locking was always the "real" hardware
write-protection.

>> Also there is that corner case, where you can
>> actually fully lock your flash: WPn hard tied to low. Be aware, that
>> you can enable locking even if WPn is tied low. That has the use
>> case to initially program the flash and then lock it forever. So we
>> might add a warning note, that WPn should somehow be controllable
>> (or be sure that is tied high) before conducting the locking tests.
>
> Yes, that is the situation I met, it is not documented anywhere except
> the code.
>
> My intent with this paragraph was to hint people not to conduct these
> tests under certain situations, but we can definitely improve that. What
> about:
>
> 5) If your flash supports locking, please got through the following test
>    procedure to make sure it correctly behaves.
>
>    Warning: These tests may hard lock your device! Make sure:
>    - The device is not hard locked already (#WP strapped to low and
>      SR_SRWD bit set)
>    - If you have a #WP pin, you may want to set `no-wp` in your DT for
>      the time of the test, to only make use of software protection.

Yes that is much better. BTW, I've never seen "#signal" but only
SIG#, nSIG, SIGn or /SIG, maybe I haven't paid too much attention.

> Please amend this text if you still think it is missing his goal.

What about

    - If you have a WPn pin, you may want to set `no-wp` in your DT for
      the time of the test, to only make use of software protection.
      Otherwise, clearing the locking state depends on the WPn
      signal and if it is tied to low, the flash will be permanently
      locked.


>
>>> +    root@1:~# flash_lock -u /dev/mtd0
>>> +    root@1:~# flash_lock -l /dev/mtd0 $(($size - (2 * $bs))) 2 # last two
>>> +    root@1:~# show_sectors
>>> +    software locked sectors
>>> +     region (in hex)   | status   | #blocks
>>> +     ------------------+----------+--------
>>> +     00000000-03fdffff | unlocked | 1022
>>> +     03fe0000-03ffffff |   locked | 2
>>> +    root@1:~# flash_lock -u /dev/mtd0 $(($size - (2 * $bs))) 1 # last one
>>
>> huh? shouldn't that be 1 * $bs?
>
> I don't think so:
> - first we lock the last two blocks (offset: size - 2 blocks, length: 2
>   blocks)
> - then we unlock the penultimate block so that only the last block
>   remains locked (offset: size - 2 blocks, length 1).

Yes you're correct. I've read the -u for a -l (and somehow assumed
a complete unlock in between).

> I we were doing flash_lock -u <mtd> $(($size - (1 * $bs))) 1, we would
> be asking to unlock the very last block and keep only the penultimate
> block locked, which would not be supported.
>
> Thanks,
> Miquèl

Re: [PATCH 15/19] mtd: spi-nor: debugfs: Add locking support

[Michael Walle]

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

Re: [PATCH 16/19] mtd: spi-nor: Add steps for testing locking support

[Miquel Raynal]

>> 5) If your flash supports locking, please got through the following test
>>    procedure to make sure it correctly behaves.
>>
>>    Warning: These tests may hard lock your device! Make sure:
>>    - The device is not hard locked already (#WP strapped to low and
>>      SR_SRWD bit set)
>>    - If you have a #WP pin, you may want to set `no-wp` in your DT for
>>      the time of the test, to only make use of software protection.
>
> Yes that is much better. BTW, I've never seen "#signal" but only
> SIG#, nSIG, SIGn or /SIG, maybe I haven't paid too much attention.

Ah, right. I'm fine either ways. I'll look for occurrences and use one
of your suggestions.

>> Please amend this text if you still think it is missing his goal.
>
> What about
>
>     - If you have a WPn pin, you may want to set `no-wp` in your DT for
>       the time of the test, to only make use of software protection.
>       Otherwise, clearing the locking state depends on the WPn
>       signal and if it is tied to low, the flash will be permanently
>       locked.

Yep, I'll use that.

>>>> +    root@1:~# flash_lock -u /dev/mtd0
>>>> +    root@1:~# flash_lock -l /dev/mtd0 $(($size - (2 * $bs))) 2 # last two
>>>> +    root@1:~# show_sectors
>>>> +    software locked sectors
>>>> +     region (in hex)   | status   | #blocks
>>>> +     ------------------+----------+--------
>>>> +     00000000-03fdffff | unlocked | 1022
>>>> +     03fe0000-03ffffff |   locked | 2
>>>> +    root@1:~# flash_lock -u /dev/mtd0 $(($size - (2 * $bs))) 1 # last one
>>>
>>> huh? shouldn't that be 1 * $bs?
>>
>> I don't think so:
>> - first we lock the last two blocks (offset: size - 2 blocks, length: 2
>>   blocks)
>> - then we unlock the penultimate block so that only the last block
>>   remains locked (offset: size - 2 blocks, length 1).
>
> Yes you're correct. I've read the -u for a -l (and somehow assumed
> a complete unlock in between).

You cannot imagine how many times this happened to me while
writing/testing/documenting all this :-)

Thanks,
Miquèl

Re: [PATCH 15/19] mtd: spi-nor: debugfs: Add locking support

[Miquel Raynal]

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