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

Jungseung Lee
2020-03-09T11:20:40Z

Hi,

2020-03-09 (Mon), 15:50 +0800, chenxiang (M):
> Hi Jungseung,
>
> 在 2020/3/7 16:24, Jungseung Lee 写道:
> > Hi,
> >
> > 2020-03-06 (Fri), 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).
>

> >
> > 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 locking portion of the
flash, lock the some of portion including the request area.

UNLOCK
- unlock operation return success if the requested area is already
unlocked.

- unlock operation try to unlock all portions includes the request
area. (But user will not knowing well)

> >
> > 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;
> > >
> > >
> >
> > .
> >