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

["chenxiang (M)" <chenxiang66@hisilicon.com>]

Hi Jungseung,

在 2020/3/9 19:44, Jungseung Lee 写道:
> Hi,
>
> 2020-03-09 (월), 15:50 +0800, chenxiang (M):
>> 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]
>>>>>
> https://protect2.fireeye.com/url?k=e80b1f1a-b5db17f2-e80a9455-000babff32e3-dadc30d1176f6374&u=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@microchip.com/
>>>> ),
>>>>    
>>>> 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).
>>
> I am reviewing tudor's patches and it seems solve your issue
> effectively.
> http://lists.infradead.org/pipermail/linux-mtd/2020-March/094231.html

Yes, it solves my issue.

>
>>> 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)?
>>
> I'm not fully understanding the usage of flash_lock, but it would be
> better to use such addresses for lock/unlocking to make it under
> control.
>   
> There are some ambiguous parts to explain that since some lock/unlock
> operation is still working well without the addresses.
>
> LOCK
> - Return success if the requested area is already locked.
> - If requested area is not fully matched with lock portion of the
> flash, lock some of the portion including the request area as it could
> be.
>
> UNLOCK
> - Return success if the requested area is already unlocked.
> - If requested area is not fully matched with lock portion of the
> flash, unlock all locked portion including the request area. the
> portion would be bigger than requested area.

Thanks for you info.
I have tested above situations of lock and unlock, and still have a 
question about it:
For unlock function, as you said, it will unlock all the locked portion 
including the request area which would be bigger than requested area if 
requested area is not fully matched with lock portion of the flash.
But for lock function, it seem not lock some of portion including the 
request area as it could be, and it seems require the total locked area 
must be matched with
some portion of the flash (it seems not allow hole between those regions).

For example, 16MB in my envirnment, i do as follows:
- lock [0xff0000, 0x1000000] which is the 255th block   -> it is matched 
with lock portion of the flash (BP3~0 = 0001, TB=0)
- lock [0xc00000, 0xff0000] or [0xc00000, 0xff1000]   -> it also matched 
with lock portion of the flash (BP3~0 = 0111, TB=0)
but if do it as follows:
- lock [0xff0000, 0x1000000] which is the 255th block   -> it is matched 
with lock portion of the flash (BP3~0 = 0001, TB=0)
- lock [0xc00000, 0xc10000]   -> it will report invalid argument at the 
second time, in my thought it would lock [0xc00000, 0x1000000] which 
will including those two regions

>
> So, the lock/unlock would be able to work without the addresses. but in
> my case I don't use the way because it will makes hard to tracking the
> locked area.
>
> Thanks,
>
>>> 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@samsung.com>
>>>>> ---
>>>>>    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/