Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Jungseung Lee <js07.lee@xxxxxxxxxxx>]

Hi, Tudor,

On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> From: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> 
> Fix the gap for the SR block protection, the BP bits were set with
> a +1 value than actually needed. This patch does not change the
> behavior of the locking operations, just fixes the protected areas.
> 
> On a 16Mbit flash with 64KByte erase sector, the following changed
> for the lock operation:
> 
> Number of blocks | BP2:0 before | BP2:0 now |
>                1 | 010b         | 001b      |
>                2 | 110b         | 010b      |
>                3 | 110b         | 010b      |
>                4 | 100b         | 011b      |
>                5 | 100b         | 011b      |
>                6 | 100b         | 011b      |
>                7 | 100b         | 011b      |
>                8 | 101b         | 100b      |
>                9 | 101b         | 100b      |
>              ... | ...          | ...       |
> 
> For the lock operation, if one requests to lock an area that is not
> matching the upper boundary of a BP protected area, we round down
> the total length and lock less than the user requested, in order to
> not lock more than the user actually requested.
> 
> For the unlock operation, read the number of blocks column as
> "locked all but number of blocks value". On a 16Mbit flash with
> 64KByte erase sector, the following changed for the lock operation:
> 
> Number of blocks | BP2:0 before | BP2:0 now |
>                1 | 111b         | 101b      |
>              ... | ...          | ...       |
>               15 | 111b         | 101b      |
>               16 | 110b         | 101b      |
>               17 | 110b         | 100b      |
>              ... | ...          | ...       |
>               24 | 101b         | 100b      |
>               25 | 101b         | 011b      |
>               26 | 101b         | 011b      |
>               27 | 101b         | 011b      |
>               28 | 100b         | 011b      |
>               29 | 100b         | 010b      |
>               30 | 011b         | 010b      |
>               31 | 010b         | 001b      |
>               32 | 000b         | 000b      |
> 
> For the unlock operation, if one requests to unlock an area that is
> not matching the upper boundary of a BP protected area, we round up
> the total length and unlock more than the user actually requested.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> ---
>  drivers/mtd/spi-nor/core.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 877557dbda7f..36660068bc04 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	/*
>  	 * Need smallest pow such that:
>  	 *
> -	 *   1 / (2^pow) <= (len / size)
> +	 *   1 / ((2^pow) - 1) <= (len / size)
>  	 *
>  	 * so (assuming power-of-2 size) we do:
>  	 *
> -	 *   pow = ceil(log2(size / len)) = log2(size) -
> floor(log2(len))
> +	 *   pow = ceil(log2(size / len)) = log2(size) -
> floor(log2(len)) + 1
>  	 */
> -	pow = ilog2(mtd->size) - ilog2(lock_len);
> +	pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
>  	val = mask - (pow << SR_BP_SHIFT);
>  	if (val & ~mask)
>  		return -EINVAL;

    (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;

I think that current mainline implementation is only valid for flashes
in case (1). (for BP0-2 : 1/64, 1/32 ...) Isn't it?

This is current implementaion.
	pow = ilog2(mtd->size) - order_base_2(lock_len)
	val = mask - (pow << SR_BP_SHIFT);

1/64 6 = 110b -> 001b
1/32 5 = 101b -> 010b
1/16 4 = 100b -> 011b
1/8  3 = 011b -> 100b
1/4  2 = 010b -> 101b
1/2  1 = 001b -> 110b
ALL  0 = 000b -> 111b

It is handling case (1) admirably. In other cases, however, the
situation would be different.


A 16Mbit flash with 64KByte erase sector(which you mentioned on this
patch) should get 32 erase sectors and seems that it's smallest
protected portion is 1/32.

So supporting the flash, it needs some modification as you did.

	pow = ilog2(mtd->size) - order_base_2(lock_len) + 1
	val = mask - (pow << SR_BP_SHIFT);

1/64 6 = 110b 111b -> 000b (execption case)
1/32 5 = 101b 110b -> 001b
1/16 4 = 100b 101b -> 010b
1/8  3 = 011b 100b -> 011b
1/4  2 = 010b 011b -> 100b
1/2  1 = 001b 010b -> 101b
ALL  0 = 000b 001b -> 110b

It would works for the flash, but it will conflicts with flashes in
case (1) what has already been applied on mainline.

And there are too various flashes that has different protected portions
to handle with the above.

Btw, the description on this patch is exactly what I had been looking
for before and seems it's very useful.

Thanks,


> @@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	/*
>  	 * Need largest pow such that:
>  	 *
> -	 *   1 / (2^pow) >= (len / size)
> +	 *   1 / ((2^pow) - 1) >= (len / size)
>  	 *
>  	 * so (assuming power-of-2 size) we do:
>  	 *
> -	 *   pow = floor(log2(size / len)) = log2(size) -
> ceil(log2(len))
> +	 *   pow = floor(log2(size / len)) = log2(size) -
> ceil(log2(len)) + 1
>  	 */
> -	pow = ilog2(mtd->size) - order_base_2(lock_len);
> +	pow = ilog2(mtd->size) - order_base_2(lock_len) + 1;
>  	if (lock_len == 0) {
>  		val = 0; /* fully unlocked */
>  	} else {


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