Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling

"chenxiang (M)" <chenxiang66@xxxxxxxxxxxxx> · Mon, 9 Mar 2020 15:50:19 +0800

Hi Jungseung,

在 2020/3/7 16:24, Jungseung Lee 写道:
Hi,

2020-03-06 (금), 20:19 +0800, chenxiang (M):
Hi Jungseung,

在 2020/3/4 19:07, Jungseung Lee 写道:
The current mainline locking was restricted and could only be
applied
to flashes that has 3 block protection bit and fixed locking ratio.

A new method of normalization was reached at the end of the
discussion [1].

     (1) - if bp slot is insufficient.
     (2) - if bp slot is sufficient.

     if (bp_slots_needed > bp_slots)    // (1)
         min_prot_length = sector_size << (bp_slots_needed -
bp_slots);
     else                               // (2)
         min_prot_length = sector_size;

This patch changes block protection handling logic based on
min_prot_length.
It is suitable for the overall flashes with exception of some
corner case
and easy to extend and apply for the case of 2bit or 4bit block
protection.

[1]
http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
  
I have tested the patchset on one of my board (there is micron flash
n25q128a11 which supports 4bit BP, and also bp3 is on bit6 of SR, TB
bit is on bit5 of SR), so
i modify the code as follows to enable the lock/unlock of n25q128a11.
-       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K |
SPI_NOR_QUAD_READ) },
+       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
+                       SECT_4K | SPI_NOR_QUAD_READ |
+                       USE_FSR | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
+                       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },

There are two issues i met:
(1) i lock/unlock the full region of the flash, lock is valid, but
there is error when unlock the flash, i query the status of it is
unlock (the issue i think it is
the same as the issue John has reported before
https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@xxxxxxxxxxxxx/),
  
i screenshot the log of the operation as follows:

Looks like the unlock operation was actually done (as can be checked
from the following query of the status) but an error is coming with
EIO.

I think another part of sr handling is related with your case. (maybe
SR read back test ?)

Yes,  it is the issue of SR read back test:  it writes 0X2 (bit WEL is 
set), but it reads back 0x0 (bit WEL is cleared).


If you can dump the sr value & dev_dbg msg, it will be helpful to
define this issue.

(2) i try to lock part of the flash region such as ./flash_lock
/dev/mtd0 0xc00000 10, it reports invalid argument,
and i am not sure whether it is something wrong with my operation.

It is unable to lock such region since the spi flash doesn't support
it. only we can lock it from the top or the bottom.

like this for n25q128a11,

flash_lock /dev/mtd0 0xff0000 0x10
flash_lock /dev/mtd0 0x0 0x10

Do you mean if lock/unlcok from top,  the address of lock/unlock 
commands should be the address of 255th block (0xff0000), 254th 
block(0xfe0000), 252nd block(0xfc0000), ...., 128th block (0x800000)?
If lock/unlock from bottom, the address of lock/unlock commands should 
be always the address of 0th block (0x0)?


Note the block count of examples is 0x10 not 10. The locking try with
block count under minimum block protection length will be failed.

Thanks,

Signed-off-by: Jungseung Lee <js07.lee@xxxxxxxxxxx>
---
  drivers/mtd/spi-nor/spi-nor.c | 110 ++++++++++++++++++++--------
------
  1 file changed, 66 insertions(+), 44 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
nor/spi-nor.c
index caf0c109cca0..c58c27552a74 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct mtd_info
*mtd, struct erase_info *instr)
  	return ret;
  }
  
+static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
+{
+	return SR_BP2 | SR_BP1 | SR_BP0;
+}
+
+static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
+{
+	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
+		return SR_TB_BIT6;
+	else
+		return SR_TB_BIT5;
+}
+
+static int stm_get_min_prot_length(struct spi_nor *nor)
+{
+	int bp_slots, bp_slots_needed;
+	u8 mask = spi_nor_get_bp_mask(nor);
+
+	bp_slots = (mask >> SR_BP_SHIFT) + 1;
+
+	/* Reserved one for "protect none" and one for "protect all".
*/
+	bp_slots = bp_slots - 2;
+
+	bp_slots_needed = ilog2(nor->info->n_sectors);
+
+	if (bp_slots_needed > bp_slots)
+		return nor->info->sector_size <<
+			(bp_slots_needed - bp_slots);
+	else
+		return nor->info->sector_size;
+}
+
  static void stm_get_locked_range(struct spi_nor *nor, u8 sr,
loff_t *ofs,
  				 uint64_t *len)
  {
  	struct mtd_info *mtd = &nor->mtd;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
-	int pow;
+	int min_prot_len;
+	u8 mask = spi_nor_get_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_tb_mask(nor);
+	u8 bp = (sr & mask) >> SR_BP_SHIFT;
  
-	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
-		tb_mask = SR_TB_BIT6;
-
-	if (!(sr & mask)) {
+	if (!bp) {
  		/* No protection */
  		*ofs = 0;
  		*len = 0;
-	} else {
-		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
-		*len = mtd->size >> pow;
-		if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
-			*ofs = 0;
-		else
-			*ofs = mtd->size - *len;
+		return;
  	}
+
+	min_prot_len = stm_get_min_prot_length(nor);
+	*len = min_prot_len << (bp - 1);
+
+	if (*len > mtd->size)
+		*len = mtd->size;
+
+	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
+		*ofs = 0;
+	else
+		*ofs = mtd->size - *len;
  }
  
  /*
@@ -1880,8 +1915,9 @@ static int stm_lock(struct spi_nor *nor,
loff_t ofs, uint64_t len)
  {
  	struct mtd_info *mtd = &nor->mtd;
  	int ret, status_old, status_new;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
+	int min_prot_len;
+	u8 mask = spi_nor_get_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_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;
@@ -1918,20 +1954,14 @@ static int stm_lock(struct spi_nor *nor,
loff_t ofs, uint64_t len)
  	else
  		lock_len = ofs + len;
  
-	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
-		tb_mask = SR_TB_BIT6;
+	if (lock_len == mtd->size) {
+		val = mask; /* fully locked */
+	} else {
+		min_prot_len = stm_get_min_prot_length(nor);
+		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
+		val = pow << SR_BP_SHIFT;
+	}
  
-	/*
-	 * Need smallest pow such that:
-	 *
-	 *   1 / (2^pow) <= (len / size)
-	 *
-	 * so (assuming power-of-2 size) we do:
-	 *
-	 *   pow = ceil(log2(size / len)) = log2(size) -
floor(log2(len))
-	 */
-	pow = ilog2(mtd->size) - ilog2(lock_len);
-	val = mask - (pow << SR_BP_SHIFT);
  	if (val & ~mask)
  		return -EINVAL;
  	/* Don't "lock" with no region! */
@@ -1966,8 +1996,9 @@ static int stm_unlock(struct spi_nor *nor,
loff_t ofs, uint64_t len)
  {
  	struct mtd_info *mtd = &nor->mtd;
  	int ret, status_old, status_new;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
+	int min_prot_len;
+	u8 mask = spi_nor_get_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_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;
@@ -2004,22 +2035,13 @@ static int stm_unlock(struct spi_nor *nor,
loff_t ofs, uint64_t len)
  	else
  		lock_len = ofs;
  
-	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
-		tb_mask = SR_TB_BIT6;
-	/*
-	 * Need largest pow such that:
-	 *
-	 *   1 / (2^pow) >= (len / size)
-	 *
-	 * so (assuming power-of-2 size) we do:
-	 *
-	 *   pow = floor(log2(size / len)) = log2(size) -
ceil(log2(len))
-	 */
-	pow = ilog2(mtd->size) - order_base_2(lock_len);
  	if (lock_len == 0) {
  		val = 0; /* fully unlocked */
  	} else {
-		val = mask - (pow << SR_BP_SHIFT);
+		min_prot_len = stm_get_min_prot_length(nor);
+		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
+		val = pow << SR_BP_SHIFT;
+
  		/* Some power-of-two sizes are not supported */
  		if (val & ~mask)
  			return -EINVAL;
  

.




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/







