[PATCH v4 3/3] spi-nor: allow setting the BPNV (default locked) bit

Vignesh Raghavendra vigneshr at ti.com
Mon Apr 1 22:27:58 PDT 2019


Hi,

On 20/03/19 12:46 PM, Jonas Bonn wrote:
> Setting the BPNV bit defaults the block protection bits BP0-2 to 1 at
> reset.  This means that all the flash sectors are protected until they
> are explicity unlocked by the user.
> 

s/explicity/explicitly

> Together with protection of the status register via the SRWD bit and the
> WP# signal, this allows a flash device to be effectively protected from
> erasure by unauthorized actors.
> 
> The patch has been tested with a Cypress s25fl512s and works as
> advertised.  The concern that I have with this, though, is that the BPNV
> bit is "write once":  if somebody inadvertently sets this feature on the
> device, it is irrevocable!  Whether or not this actually belongs in the
> mainline kernel is therefore up for debate...
>

I am fine with this in general as setting BPNV is OTP but wont
necessarily brick the flash. More comments below:

> Signed-off-by: Jonas Bonn <jonas at norrbonn.se>
> ---
>  drivers/mtd/mtdchar.c         |   6 ++
>  drivers/mtd/mtdcore.c         |   8 +++
>  drivers/mtd/spi-nor/spi-nor.c | 103 +++++++++++++++++++++++-----------
>  include/linux/mtd/mtd.h       |   2 +
>  include/linux/mtd/spi-nor.h   |   1 +
>  include/uapi/mtd/mtd-abi.h    |   1 +
>  6 files changed, 88 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 02389528f622..72842308087b 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -800,6 +800,12 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  		break;
>  	}
>  
> +	case MEMSETDEFAULTLOCKED:
> +	{
> +		ret = mtd_set_default_locked(mtd);
> +		break;
> +	}
> +
>  	case MEMLOCK:
>  	{
>  		struct erase_info_user einfo;
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 76b4264936ff..811fe0f3c24a 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1681,6 +1681,14 @@ int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
>  }
>  EXPORT_SYMBOL_GPL(mtd_lock_user_prot_reg);
>  
> +int mtd_set_default_locked(struct mtd_info *mtd)
> +{
> +	if (!mtd->_set_default_locked)
> +		return -EOPNOTSUPP;
> +	return mtd->_set_default_locked(mtd);
> +}
> +EXPORT_SYMBOL_GPL(mtd_set_default_locked);
> +
>  /* Chip-supported device locking */
>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 9abac9e326a1..8a30cdac6f88 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -250,7 +250,8 @@ struct flash_info {
>  	u16		page_size;
>  	u16		addr_width;
>  
> -	u16		flags;
> +	u16		__unused16;

Why is this needed?

> +	u32		flags;
>  #define SECT_4K			BIT(0)	/* SPINOR_OP_BE_4K works uniformly */
>  #define SPI_NOR_NO_ERASE	BIT(1)	/* No erase command needed */
>  #define SST_WRITE		BIT(2)	/* use SST byte programming */
> @@ -279,6 +280,10 @@ struct flash_info {
>  #define SPI_NOR_SKIP_SFDP	BIT(13)	/* Skip parsing of SFDP tables */
>  #define USE_CLSR		BIT(14)	/* use CLSR command */
>  #define SPI_NOR_OCTAL_READ	BIT(15)	/* Flash supports Octal Read */
> +#define SPI_NOR_HAS_BPNV	BIT(16) /* Block protection bits can be set
> +					 * non-volatile, meaning all blocks
> +					 * are protected by default at reset
> +					 */
>  

Setting BPNV actually makes the BP bits _volatile_ and therefore are
reset to 1 on power up. Above comment and commit log should reflect the
same.

>  	/* Part specific fixup hooks. */
>  	const struct spi_nor_fixups *fixups;
> @@ -1324,6 +1329,36 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	return write_sr_and_check(nor, status_new, mask);
>  }
>  
> +/*
> + * Write status Register and configuration register with 2 bytes
> + * The first byte will be written to the status register, while the
> + * second byte will be written to the configuration register.
> + * Return negative if error occurred.
> + */
> +static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr)
> +{
> +	int ret;
> +
> +	write_enable(nor);
> +
> +	ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2);
> +	if (ret < 0) {
> +		dev_err(nor->dev,
> +			"error while writing configuration register\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = spi_nor_wait_till_ready(nor);
> +	if (ret) {
> +		dev_err(nor->dev,
> +			"timeout while writing configuration register\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +

Is there a real need to move this function over here? This messes up
looking at history using git blame. Please do not move the code around
unless needed

>  /*
>   * Check if a region of the flash is (completely) locked. See stm_lock() for
>   * more info.
> @@ -1342,6 +1377,35 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	return stm_is_locked_sr(nor, ofs, len, status);
>  }
>  
> +static int spi_nor_set_default_locked(struct mtd_info *mtd)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	struct device *dev = nor->dev;
> +	u8 sr_cr[2];
> +	int ret;
> +
> +	ret = read_cr(nor);
> +	if (ret < 0) {
> +		dev_err(dev, "error while reading configuration register\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ret & CR_BPNV)
> +		return 0;
> +
> +	sr_cr[1] = ret | CR_BPNV;
> +
> +	/* Keep the current value of the Status Register. */
> +	ret = read_sr(nor);
> +	if (ret < 0) {
> +		dev_err(dev, "error while reading status register\n");
> +		return -EINVAL;
> +	}
> +	sr_cr[0] = ret;
> +
> +	return write_sr_cr(nor, sr_cr);
> +}
> +
>  static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> @@ -1387,35 +1451,6 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	return ret;
>  }
>  
> -/*
> - * Write status Register and configuration register with 2 bytes
> - * The first byte will be written to the status register, while the
> - * second byte will be written to the configuration register.
> - * Return negative if error occurred.
> - */
> -static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr)
> -{
> -	int ret;
> -
> -	write_enable(nor);
> -
> -	ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2);
> -	if (ret < 0) {
> -		dev_err(nor->dev,
> -			"error while writing configuration register\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = spi_nor_wait_till_ready(nor);
> -	if (ret) {
> -		dev_err(nor->dev,
> -			"timeout while writing configuration register\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  /**
>   * macronix_quad_enable() - set QE bit in Status Register.
>   * @nor:	pointer to a 'struct spi_nor'
> @@ -1898,9 +1933,7 @@ static const struct flash_info spi_nor_ids[] = {
>  			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>  	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
>  	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> -	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256,
> -			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) },
> +	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_HAS_BPNV | USE_CLSR) },

Please limit to 80 char as before. That is the preferred style going forward

>  	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>  	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>  	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
> @@ -4082,6 +4115,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  		mtd->_is_locked = spi_nor_is_locked;
>  	}
>  
> +	if (info->flags & SPI_NOR_HAS_BPNV) {
> +		mtd->_set_default_locked = spi_nor_set_default_locked;
> +	}
> +
>  	/* sst nor chips use AAI word program */
>  	if (info->flags & SST_WRITE)
>  		mtd->_write = sst_write;
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 677768b21a1d..30ee6f1c1142 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -313,6 +313,7 @@ struct mtd_info {
>  	int (*_writev) (struct mtd_info *mtd, const struct kvec *vecs,
>  			unsigned long count, loff_t to, size_t *retlen);
>  	void (*_sync) (struct mtd_info *mtd);
> +	int (*_set_default_locked) (struct mtd_info *mtd);
>  	int (*_lock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  	int (*_unlock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  	int (*_is_locked) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
> @@ -451,6 +452,7 @@ static inline void mtd_sync(struct mtd_info *mtd)
>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len);
> +int mtd_set_default_locked(struct mtd_info *mtd);
>  int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs);
>  int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs);
>  int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs);
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index b3d360b0ee3d..40d575a1431f 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -144,6 +144,7 @@
>  #define FSR_PT_ERR		BIT(1)	/* Protection error bit */
>  
>  /* Configuration Register bits. */
> +#define CR_BPNV			BIT(3)	/* Block protection non-volatile */
>  #define CR_QUAD_EN_SPAN		BIT(1)	/* Spansion Quad I/O */
>  
>  /* Status Register 2 bits. */
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index aff5b5e59845..27b752796efa 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -204,6 +204,7 @@ struct otp_info {
>   * without OOB, e.g., NOR flash.
>   */
>  #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
> +#define MEMSETDEFAULTLOCKED	_IO('M', 25)
>  

There should be a comment to warn users that above ioctl might end up
setting a OTP bit as this is not clear from the name of the macro.

There is MTD_POWERUP_LOCK flag to indicate flash device is locked after
reset. To be consistent, above ioctl would need to be named
MEMSETPOWERUPLOCKED.


Also, spi-nor framework should detect the state of BPNV on powerup and
set MTD_POWERUP_LOCK in mtd->flags accordingly to honor ABI here[1]:


What:		/sys/class/mtd/mtdX/flags
Date:		April 2009
KernelVersion:	2.6.29
Contact:	linux-mtd at lists.infradead.org
Description:
		A hexadecimal value representing the device flags, ORed
		together:

		0x0400: MTD_WRITEABLE - device is writable
		0x0800: MTD_BIT_WRITEABLE - single bits can be flipped
		0x1000: MTD_NO_ERASE - no erase necessary
		0x2000: MTD_POWERUP_LOCK - always locked after reset

[1]https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-mtd



-- 
Regards
Vignesh



More information about the linux-mtd mailing list