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

[Jungseung Lee <js07.lee@xxxxxxxxxxx>]

Hi,

On Mon, 2020-03-16 at 15:21 +0800, chenxiang (M) wrote:
> Hi Jungseung,
> 
> 在 2020/3/14 21:50, Jungseung Lee 写道:
> > 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://protect2.fireeye.com/url?k=ed8659ca-b0544ec3-ed87d285-0cc47a31cdf8-aa60cbf507f7bb2c&u=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.
> > > > 
https://protect2.fireeye.com/url?k=a6aef5a7-fb7ce2ae-a6af7ee8-0cc47a31cdf8-1b34841aa21abc3e&u=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@xxxxxxxxxxxxx>.
> If there would be next version, i will test them also.
> 

Good, I'll post new version by the end of the day.

Thanks,

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