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

[Jungseung Lee <js07.lee@samsung.com>]

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] 
> > 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 ?)

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

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/