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

[Michael Walle <michael@walle.cc>]

Hi,

Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> 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.

So instead of rounding up, it does round down now?

> 
> 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.

I don't know if that is really what a user really want. Because you'd
end up with regions which the user believes are locked but are not.
IMHO if you'd have to make a choice I'd prefer to have the remainder
locked. Not the other way around. Esp. since the user explicitly
express to have a region locked.

-michael

> 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@microchip.com>
> ---
>  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;
> @@ -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/