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

[Jungseung Lee <js07.lee@xxxxxxxxx>]

Hi, chenxiang,

On Sat, Mar 14, 2020 at 6:58 PM chenxiang (M) <chenxiang66@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx/
> >>>> ),
> >>>>
> >>>> 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.

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@xxxxxxxxxxx>
> >>>>> ---
> >>>>>    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/