Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Jungseung Lee <js07.lee@samsung.com>
To: Tudor.Ambarus@microchip.com, michael@walle.cc, vigneshr@ti.com,
	js07.lee@samsung.com
Cc: linux-mtd@lists.infradead.org
Subject: Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking
Date: Tue, 24 Mar 2020 12:52:57 +0900
Message-ID: <794ff38f1976ed842d13abec7b0a87e0727cb5f8.camel@samsung.com> (raw)
In-Reply-To: <20200323092430.1466234-2-tudor.ambarus@microchip.com>

Hi, Tudor,

On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote:
> 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.
> 
> 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@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;

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

  parent reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-23  9:24 [PATCH v3 0/5] mtd: spi-nor: Add SR 4bit block protection support Tudor.Ambarus
2020-03-23  9:24 ` [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking Tudor.Ambarus
2020-03-23 18:27   ` Michael Walle
2020-03-23 19:20     ` Tudor.Ambarus
2020-03-23 19:54       ` Michael Walle
2020-03-23 20:26         ` Tudor.Ambarus
2020-03-23 21:14           ` Michael Walle
2020-03-23 21:30             ` Tudor.Ambarus
2020-03-23 21:33               ` Tudor.Ambarus
2020-03-23 22:35               ` Michael Walle
2020-03-24  5:37                 ` Tudor.Ambarus
2020-03-24  3:52   ` Jungseung Lee [this message]
2020-03-25  9:44   ` Tudor.Ambarus
2020-03-23  9:24 ` [PATCH v3 2/5] mtd: spi-nor: Set all BP bits to one when lock_len == mtd->size Tudor.Ambarus
2020-03-23 14:08   ` Jungseung Lee
2020-03-23 18:28   ` Michael Walle
2020-03-23  9:24 ` [PATCH v3 3/5] mtd: spi-nor: Add new formula for SR block protection handling Tudor.Ambarus
     [not found]   ` <000001d600ff$063a8fd0$12afaf70$@samsung.com>
2020-03-23 13:32     ` Jungseung Lee
2020-03-23  9:24 ` [PATCH v3 4/5] mtd: spi-nor: Add SR 4bit block protection support Tudor.Ambarus
2020-03-23 12:43   ` Jungseung Lee
2020-03-23 12:55     ` Tudor.Ambarus
2020-03-23 13:16       ` Jungseung Lee
2020-03-23 18:33   ` Michael Walle
2020-03-23 18:51     ` Tudor.Ambarus
2020-03-23  9:24 ` [PATCH v3 4/5] mtd: spi-nor: Add 4bit SR " Tudor.Ambarus
2020-03-23  9:46   ` Tudor.Ambarus
2020-03-23  9:24 ` [PATCH v3 5/5] mtd: spi-nor: Enable locking for n25q512ax3/n25q512a Tudor.Ambarus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=794ff38f1976ed842d13abec7b0a87e0727cb5f8.camel@samsung.com \
    --to=js07.lee@samsung.com \
    --cc=Tudor.Ambarus@microchip.com \
    --cc=linux-mtd@lists.infradead.org \
    --cc=michael@walle.cc \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git