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

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

Hi Jungseung,

在 2020/3/14 21:50, Jungseung Lee 写道:
> Hi, chenxiang,
>
> On Sat, Mar 14, 2020 at 6:58 PM chenxiang (M) <chenxiang66@hisilicon.com> wrote:
>> 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).
>>
> Yes it is. The spi flash consequently controls the region that will be
> locked through only one bp value on sr register.
> I wrote only some of the patterns I checked in the current mainline
> code, and frankly, I don't know if even this is always right in all
> combinations.

Ok, thanks.
So i have tested those patchset + (enabled n25q128a11 private patch) on 
flash n25q128a11, and it is ok, so you can add : Tested-by: Xiang Chen 
<chenxiang66@hisilicon.com>.
If there would be next version, i will test them also.

> Thanks,
>
>> 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/