All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
@ 2021-03-02 11:09 ` Michael Walle
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2021-03-02 11:09 UTC (permalink / raw)
  To: linux-mtd, linux-kernel, linux-api
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Tudor.Ambarus, Michael Walle

This may sound like a contradiction but some SPI-NOR flashes really
support erasing their OTP region until it is finally locked. Having the
possibility to erase an OTP region might come in handy during
development.

The ioctl argument follows the OTPLOCK style.

Signed-off-by: Michael Walle <michael@walle.cc>
---
OTP support for SPI-NOR flashes may be merged soon:
https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/

Tudor suggested to add support for the OTP erase operation most SPI-NOR
flashes have:
https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/

Therefore, this is an RFC to get some feedback on the MTD side, once this
is finished, I can post a patch for mtd-utils. Then we'll have a foundation
to add the support to SPI-NOR.

 drivers/mtd/mtdchar.c      |  7 ++++++-
 drivers/mtd/mtdcore.c      | 12 ++++++++++++
 include/linux/mtd/mtd.h    |  3 +++
 include/uapi/mtd/mtd-abi.h |  2 ++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 323035d4f2d0..da423dd031ae 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 	case OTPGETREGIONCOUNT:
 	case OTPGETREGIONINFO:
 	case OTPLOCK:
+	case OTPERASE:
 	case ECCGETLAYOUT:
 	case ECCGETSTATS:
 	case MTDFILEMODE:
@@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 	}
 
 	case OTPLOCK:
+	case OTPERASE:
 	{
 		struct otp_info oinfo;
 
@@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 			return -EINVAL;
 		if (copy_from_user(&oinfo, argp, sizeof(oinfo)))
 			return -EFAULT;
-		ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
+		if (cmd == OTPLOCK)
+			ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
+		else
+			ret = mtd_erase_user_prot_reg(mtd, oinfo.start, oinfo.length);
 		break;
 	}
 
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 2d6423d89a17..d844d718ef77 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1918,6 +1918,18 @@ 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_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
+{
+	struct mtd_info *master = mtd_get_master(mtd);
+
+	if (!master->_erase_user_prot_reg)
+		return -EOPNOTSUPP;
+	if (!len)
+		return 0;
+	return master->_erase_user_prot_reg(master, from, len);
+}
+EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
+
 /* Chip-supported device locking */
 int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 157357ec1441..734ad7a8c353 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -336,6 +336,8 @@ struct mtd_info {
 				     size_t len, size_t *retlen, u_char *buf);
 	int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
 				    size_t len);
+	int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
+				     size_t len);
 	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);
@@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len,
 int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len,
 			    size_t *retlen, u_char *buf);
 int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
+int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
 
 int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
 	       unsigned long count, loff_t to, size_t *retlen);
diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
index 65b9db936557..242015f60d10 100644
--- a/include/uapi/mtd/mtd-abi.h
+++ b/include/uapi/mtd/mtd-abi.h
@@ -205,6 +205,8 @@ struct otp_info {
  * without OOB, e.g., NOR flash.
  */
 #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
+/* Erase a given range of user data (must be in mode %MTD_FILE_MODE_OTP_USER) */
+#define OTPERASE		_IOR('M', 25, struct otp_info)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
@ 2021-03-02 11:09 ` Michael Walle
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2021-03-02 11:09 UTC (permalink / raw)
  To: linux-mtd, linux-kernel, linux-api
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Tudor.Ambarus, Michael Walle

This may sound like a contradiction but some SPI-NOR flashes really
support erasing their OTP region until it is finally locked. Having the
possibility to erase an OTP region might come in handy during
development.

The ioctl argument follows the OTPLOCK style.

Signed-off-by: Michael Walle <michael@walle.cc>
---
OTP support for SPI-NOR flashes may be merged soon:
https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/

Tudor suggested to add support for the OTP erase operation most SPI-NOR
flashes have:
https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/

Therefore, this is an RFC to get some feedback on the MTD side, once this
is finished, I can post a patch for mtd-utils. Then we'll have a foundation
to add the support to SPI-NOR.

 drivers/mtd/mtdchar.c      |  7 ++++++-
 drivers/mtd/mtdcore.c      | 12 ++++++++++++
 include/linux/mtd/mtd.h    |  3 +++
 include/uapi/mtd/mtd-abi.h |  2 ++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 323035d4f2d0..da423dd031ae 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 	case OTPGETREGIONCOUNT:
 	case OTPGETREGIONINFO:
 	case OTPLOCK:
+	case OTPERASE:
 	case ECCGETLAYOUT:
 	case ECCGETSTATS:
 	case MTDFILEMODE:
@@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 	}
 
 	case OTPLOCK:
+	case OTPERASE:
 	{
 		struct otp_info oinfo;
 
@@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 			return -EINVAL;
 		if (copy_from_user(&oinfo, argp, sizeof(oinfo)))
 			return -EFAULT;
-		ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
+		if (cmd == OTPLOCK)
+			ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
+		else
+			ret = mtd_erase_user_prot_reg(mtd, oinfo.start, oinfo.length);
 		break;
 	}
 
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 2d6423d89a17..d844d718ef77 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1918,6 +1918,18 @@ 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_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
+{
+	struct mtd_info *master = mtd_get_master(mtd);
+
+	if (!master->_erase_user_prot_reg)
+		return -EOPNOTSUPP;
+	if (!len)
+		return 0;
+	return master->_erase_user_prot_reg(master, from, len);
+}
+EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
+
 /* Chip-supported device locking */
 int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 157357ec1441..734ad7a8c353 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -336,6 +336,8 @@ struct mtd_info {
 				     size_t len, size_t *retlen, u_char *buf);
 	int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
 				    size_t len);
+	int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
+				     size_t len);
 	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);
@@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len,
 int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len,
 			    size_t *retlen, u_char *buf);
 int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
+int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
 
 int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
 	       unsigned long count, loff_t to, size_t *retlen);
diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
index 65b9db936557..242015f60d10 100644
--- a/include/uapi/mtd/mtd-abi.h
+++ b/include/uapi/mtd/mtd-abi.h
@@ -205,6 +205,8 @@ struct otp_info {
  * without OOB, e.g., NOR flash.
  */
 #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
+/* Erase a given range of user data (must be in mode %MTD_FILE_MODE_OTP_USER) */
+#define OTPERASE		_IOR('M', 25, struct otp_info)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
  2021-03-02 11:09 ` Michael Walle
@ 2021-03-02 12:46   ` Miquel Raynal
  -1 siblings, 0 replies; 14+ messages in thread
From: Miquel Raynal @ 2021-03-02 12:46 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-mtd, linux-kernel, linux-api, Richard Weinberger,
	Vignesh Raghavendra, Tudor.Ambarus

Hi Michael,

Michael Walle <michael@walle.cc> wrote on Tue,  2 Mar 2021 12:09:27
+0100:

> This may sound like a contradiction but some SPI-NOR flashes really
> support erasing their OTP region until it is finally locked. Having the
> possibility to erase an OTP region might come in handy during
> development.
> 
> The ioctl argument follows the OTPLOCK style.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> OTP support for SPI-NOR flashes may be merged soon:
> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
> 
> Tudor suggested to add support for the OTP erase operation most SPI-NOR
> flashes have:
> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
> 
> Therefore, this is an RFC to get some feedback on the MTD side, once this
> is finished, I can post a patch for mtd-utils. Then we'll have a foundation
> to add the support to SPI-NOR.
> 
>  drivers/mtd/mtdchar.c      |  7 ++++++-
>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>  include/linux/mtd/mtd.h    |  3 +++
>  include/uapi/mtd/mtd-abi.h |  2 ++
>  4 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 323035d4f2d0..da423dd031ae 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  	case OTPGETREGIONCOUNT:
>  	case OTPGETREGIONINFO:
>  	case OTPLOCK:
> +	case OTPERASE:
>  	case ECCGETLAYOUT:
>  	case ECCGETSTATS:
>  	case MTDFILEMODE:
> @@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  	}
>  
>  	case OTPLOCK:
> +	case OTPERASE:
>  	{
>  		struct otp_info oinfo;
>  
> @@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  			return -EINVAL;
>  		if (copy_from_user(&oinfo, argp, sizeof(oinfo)))
>  			return -EFAULT;
> -		ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
> +		if (cmd == OTPLOCK)
> +			ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
> +		else
> +			ret = mtd_erase_user_prot_reg(mtd, oinfo.start, oinfo.length);
>  		break;
>  	}
>  
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 2d6423d89a17..d844d718ef77 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1918,6 +1918,18 @@ 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_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> +	struct mtd_info *master = mtd_get_master(mtd);
> +
> +	if (!master->_erase_user_prot_reg)
> +		return -EOPNOTSUPP;
> +	if (!len)
> +		return 0;

Should we add a sanity check enforcing that we don't try to access
beyond the end of the OTP area?

> +	return master->_erase_user_prot_reg(master, from, len);
> +}
> +EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
> +
>  /* Chip-supported device locking */
>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 157357ec1441..734ad7a8c353 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -336,6 +336,8 @@ struct mtd_info {
>  				     size_t len, size_t *retlen, u_char *buf);
>  	int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>  				    size_t len);
> +	int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
> +				     size_t len);
>  	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);
> @@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len,
>  int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len,
>  			    size_t *retlen, u_char *buf);
>  int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
>  
>  int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>  	       unsigned long count, loff_t to, size_t *retlen);
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index 65b9db936557..242015f60d10 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -205,6 +205,8 @@ struct otp_info {
>   * without OOB, e.g., NOR flash.
>   */
>  #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
> +/* Erase a given range of user data (must be in mode %MTD_FILE_MODE_OTP_USER) */
> +#define OTPERASE		_IOR('M', 25, struct otp_info)
>  
>  /*
>   * Obsolete legacy interface. Keep it in order not to break userspace

Thanks,
Miquèl

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
@ 2021-03-02 12:46   ` Miquel Raynal
  0 siblings, 0 replies; 14+ messages in thread
From: Miquel Raynal @ 2021-03-02 12:46 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-mtd, linux-kernel, linux-api, Richard Weinberger,
	Vignesh Raghavendra, Tudor.Ambarus

Hi Michael,

Michael Walle <michael@walle.cc> wrote on Tue,  2 Mar 2021 12:09:27
+0100:

> This may sound like a contradiction but some SPI-NOR flashes really
> support erasing their OTP region until it is finally locked. Having the
> possibility to erase an OTP region might come in handy during
> development.
> 
> The ioctl argument follows the OTPLOCK style.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> OTP support for SPI-NOR flashes may be merged soon:
> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
> 
> Tudor suggested to add support for the OTP erase operation most SPI-NOR
> flashes have:
> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
> 
> Therefore, this is an RFC to get some feedback on the MTD side, once this
> is finished, I can post a patch for mtd-utils. Then we'll have a foundation
> to add the support to SPI-NOR.
> 
>  drivers/mtd/mtdchar.c      |  7 ++++++-
>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>  include/linux/mtd/mtd.h    |  3 +++
>  include/uapi/mtd/mtd-abi.h |  2 ++
>  4 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 323035d4f2d0..da423dd031ae 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  	case OTPGETREGIONCOUNT:
>  	case OTPGETREGIONINFO:
>  	case OTPLOCK:
> +	case OTPERASE:
>  	case ECCGETLAYOUT:
>  	case ECCGETSTATS:
>  	case MTDFILEMODE:
> @@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  	}
>  
>  	case OTPLOCK:
> +	case OTPERASE:
>  	{
>  		struct otp_info oinfo;
>  
> @@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  			return -EINVAL;
>  		if (copy_from_user(&oinfo, argp, sizeof(oinfo)))
>  			return -EFAULT;
> -		ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
> +		if (cmd == OTPLOCK)
> +			ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
> +		else
> +			ret = mtd_erase_user_prot_reg(mtd, oinfo.start, oinfo.length);
>  		break;
>  	}
>  
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 2d6423d89a17..d844d718ef77 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1918,6 +1918,18 @@ 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_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> +	struct mtd_info *master = mtd_get_master(mtd);
> +
> +	if (!master->_erase_user_prot_reg)
> +		return -EOPNOTSUPP;
> +	if (!len)
> +		return 0;

Should we add a sanity check enforcing that we don't try to access
beyond the end of the OTP area?

> +	return master->_erase_user_prot_reg(master, from, len);
> +}
> +EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
> +
>  /* Chip-supported device locking */
>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 157357ec1441..734ad7a8c353 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -336,6 +336,8 @@ struct mtd_info {
>  				     size_t len, size_t *retlen, u_char *buf);
>  	int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>  				    size_t len);
> +	int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
> +				     size_t len);
>  	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);
> @@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len,
>  int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len,
>  			    size_t *retlen, u_char *buf);
>  int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
>  
>  int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>  	       unsigned long count, loff_t to, size_t *retlen);
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index 65b9db936557..242015f60d10 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -205,6 +205,8 @@ struct otp_info {
>   * without OOB, e.g., NOR flash.
>   */
>  #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
> +/* Erase a given range of user data (must be in mode %MTD_FILE_MODE_OTP_USER) */
> +#define OTPERASE		_IOR('M', 25, struct otp_info)
>  
>  /*
>   * Obsolete legacy interface. Keep it in order not to break userspace

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
  2021-03-02 12:46   ` Miquel Raynal
@ 2021-03-02 13:06     ` Michael Walle
  -1 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2021-03-02 13:06 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: linux-mtd, linux-kernel, linux-api, Richard Weinberger,
	Vignesh Raghavendra, Tudor.Ambarus

Hi,

Am 2021-03-02 13:46, schrieb Miquel Raynal:
> Michael Walle <michael@walle.cc> wrote on Tue,  2 Mar 2021 12:09:27
> +0100:
> 
>> This may sound like a contradiction but some SPI-NOR flashes really
>> support erasing their OTP region until it is finally locked. Having 
>> the
>> possibility to erase an OTP region might come in handy during
>> development.
>> 
>> The ioctl argument follows the OTPLOCK style.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> OTP support for SPI-NOR flashes may be merged soon:
>> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
>> 
>> Tudor suggested to add support for the OTP erase operation most 
>> SPI-NOR
>> flashes have:
>> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
>> 
>> Therefore, this is an RFC to get some feedback on the MTD side, once 
>> this
>> is finished, I can post a patch for mtd-utils. Then we'll have a 
>> foundation
>> to add the support to SPI-NOR.
>> 

[..]

>> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t 
>> len)
>> +{
>> +	struct mtd_info *master = mtd_get_master(mtd);
>> +
>> +	if (!master->_erase_user_prot_reg)
>> +		return -EOPNOTSUPP;
>> +	if (!len)
>> +		return 0;
> 
> Should we add a sanity check enforcing that we don't try to access
> beyond the end of the OTP area?

This is checked in the op itself, as it is done for all the
other OTP read/write/lock ops.

Right at the moment, I don't see how this could be achieved in
an elegant way. Without additional changes, you'd have to call
mtd_get_user_prot_info() and iterate over the returned values
and figure out whether from and len are valid.

[..]

-michael

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
@ 2021-03-02 13:06     ` Michael Walle
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2021-03-02 13:06 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: linux-mtd, linux-kernel, linux-api, Richard Weinberger,
	Vignesh Raghavendra, Tudor.Ambarus

Hi,

Am 2021-03-02 13:46, schrieb Miquel Raynal:
> Michael Walle <michael@walle.cc> wrote on Tue,  2 Mar 2021 12:09:27
> +0100:
> 
>> This may sound like a contradiction but some SPI-NOR flashes really
>> support erasing their OTP region until it is finally locked. Having 
>> the
>> possibility to erase an OTP region might come in handy during
>> development.
>> 
>> The ioctl argument follows the OTPLOCK style.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> OTP support for SPI-NOR flashes may be merged soon:
>> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
>> 
>> Tudor suggested to add support for the OTP erase operation most 
>> SPI-NOR
>> flashes have:
>> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
>> 
>> Therefore, this is an RFC to get some feedback on the MTD side, once 
>> this
>> is finished, I can post a patch for mtd-utils. Then we'll have a 
>> foundation
>> to add the support to SPI-NOR.
>> 

[..]

>> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t 
>> len)
>> +{
>> +	struct mtd_info *master = mtd_get_master(mtd);
>> +
>> +	if (!master->_erase_user_prot_reg)
>> +		return -EOPNOTSUPP;
>> +	if (!len)
>> +		return 0;
> 
> Should we add a sanity check enforcing that we don't try to access
> beyond the end of the OTP area?

This is checked in the op itself, as it is done for all the
other OTP read/write/lock ops.

Right at the moment, I don't see how this could be achieved in
an elegant way. Without additional changes, you'd have to call
mtd_get_user_prot_info() and iterate over the returned values
and figure out whether from and len are valid.

[..]

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
  2021-03-02 11:09 ` Michael Walle
@ 2021-03-02 15:30   ` Vignesh Raghavendra
  -1 siblings, 0 replies; 14+ messages in thread
From: Vignesh Raghavendra @ 2021-03-02 15:30 UTC (permalink / raw)
  To: Michael Walle, linux-mtd, linux-kernel, linux-api
  Cc: Miquel Raynal, Richard Weinberger, Tudor.Ambarus

Hi,

On 3/2/21 4:39 PM, Michael Walle wrote:
> This may sound like a contradiction but some SPI-NOR flashes really
> support erasing their OTP region until it is finally locked. Having the
> possibility to erase an OTP region might come in handy during
> development.
> 
> The ioctl argument follows the OTPLOCK style.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> OTP support for SPI-NOR flashes may be merged soon:
> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
> 
> Tudor suggested to add support for the OTP erase operation most SPI-NOR
> flashes have:
> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
> 
> Therefore, this is an RFC to get some feedback on the MTD side, once this
> is finished, I can post a patch for mtd-utils. Then we'll have a foundation
> to add the support to SPI-NOR.
> 
>  drivers/mtd/mtdchar.c      |  7 ++++++-
>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>  include/linux/mtd/mtd.h    |  3 +++
>  include/uapi/mtd/mtd-abi.h |  2 ++
>  4 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 323035d4f2d0..da423dd031ae 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  	case OTPGETREGIONCOUNT:
>  	case OTPGETREGIONINFO:
>  	case OTPLOCK:
> +	case OTPERASE:

This is not a Safe IOCTL. We are destroying OTP data. Need to check for
write permission before allowing the ioctl right?

>  	case ECCGETLAYOUT:
>  	case ECCGETSTATS:
>  	case MTDFILEMODE:
> @@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  	}
>  
>  	case OTPLOCK:
> +	case OTPERASE:
>  	{
>  		struct otp_info oinfo;
>  
> @@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  			return -EINVAL;
>  		if (copy_from_user(&oinfo, argp, sizeof(oinfo)))
>  			return -EFAULT;
> -		ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
> +		if (cmd == OTPLOCK)
> +			ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
> +		else
> +			ret = mtd_erase_user_prot_reg(mtd, oinfo.start, oinfo.length);
>  		break;
>  	}
>  
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 2d6423d89a17..d844d718ef77 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1918,6 +1918,18 @@ 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_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> +	struct mtd_info *master = mtd_get_master(mtd);
> +
> +	if (!master->_erase_user_prot_reg)
> +		return -EOPNOTSUPP;
> +	if (!len)
> +		return 0;
> +	return master->_erase_user_prot_reg(master, from, len);
> +}
> +EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
> +
>  /* Chip-supported device locking */
>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 157357ec1441..734ad7a8c353 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -336,6 +336,8 @@ struct mtd_info {
>  				     size_t len, size_t *retlen, u_char *buf);
>  	int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>  				    size_t len);
> +	int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
> +				     size_t len);
>  	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);
> @@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len,
>  int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len,
>  			    size_t *retlen, u_char *buf);
>  int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
>  
>  int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>  	       unsigned long count, loff_t to, size_t *retlen);
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index 65b9db936557..242015f60d10 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -205,6 +205,8 @@ struct otp_info {
>   * without OOB, e.g., NOR flash.
>   */
>  #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
> +/* Erase a given range of user data (must be in mode %MTD_FILE_MODE_OTP_USER) */
> +#define OTPERASE		_IOR('M', 25, struct otp_info)
>  

Hmm, shouldn't this be:

#define OTPERASE		_IOW('M', 25, struct otp_info)

Userspace is writing struct otp_info to the driver. OTPLOCK should
probably be _IOW() as well.

>  /*
>   * Obsolete legacy interface. Keep it in order not to break userspace
> 

Regards
Vignesh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
@ 2021-03-02 15:30   ` Vignesh Raghavendra
  0 siblings, 0 replies; 14+ messages in thread
From: Vignesh Raghavendra @ 2021-03-02 15:30 UTC (permalink / raw)
  To: Michael Walle, linux-mtd, linux-kernel, linux-api
  Cc: Miquel Raynal, Richard Weinberger, Tudor.Ambarus

Hi,

On 3/2/21 4:39 PM, Michael Walle wrote:
> This may sound like a contradiction but some SPI-NOR flashes really
> support erasing their OTP region until it is finally locked. Having the
> possibility to erase an OTP region might come in handy during
> development.
> 
> The ioctl argument follows the OTPLOCK style.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> OTP support for SPI-NOR flashes may be merged soon:
> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
> 
> Tudor suggested to add support for the OTP erase operation most SPI-NOR
> flashes have:
> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
> 
> Therefore, this is an RFC to get some feedback on the MTD side, once this
> is finished, I can post a patch for mtd-utils. Then we'll have a foundation
> to add the support to SPI-NOR.
> 
>  drivers/mtd/mtdchar.c      |  7 ++++++-
>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>  include/linux/mtd/mtd.h    |  3 +++
>  include/uapi/mtd/mtd-abi.h |  2 ++
>  4 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 323035d4f2d0..da423dd031ae 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  	case OTPGETREGIONCOUNT:
>  	case OTPGETREGIONINFO:
>  	case OTPLOCK:
> +	case OTPERASE:

This is not a Safe IOCTL. We are destroying OTP data. Need to check for
write permission before allowing the ioctl right?

>  	case ECCGETLAYOUT:
>  	case ECCGETSTATS:
>  	case MTDFILEMODE:
> @@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  	}
>  
>  	case OTPLOCK:
> +	case OTPERASE:
>  	{
>  		struct otp_info oinfo;
>  
> @@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  			return -EINVAL;
>  		if (copy_from_user(&oinfo, argp, sizeof(oinfo)))
>  			return -EFAULT;
> -		ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
> +		if (cmd == OTPLOCK)
> +			ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
> +		else
> +			ret = mtd_erase_user_prot_reg(mtd, oinfo.start, oinfo.length);
>  		break;
>  	}
>  
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 2d6423d89a17..d844d718ef77 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1918,6 +1918,18 @@ 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_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> +	struct mtd_info *master = mtd_get_master(mtd);
> +
> +	if (!master->_erase_user_prot_reg)
> +		return -EOPNOTSUPP;
> +	if (!len)
> +		return 0;
> +	return master->_erase_user_prot_reg(master, from, len);
> +}
> +EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
> +
>  /* Chip-supported device locking */
>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 157357ec1441..734ad7a8c353 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -336,6 +336,8 @@ struct mtd_info {
>  				     size_t len, size_t *retlen, u_char *buf);
>  	int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>  				    size_t len);
> +	int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
> +				     size_t len);
>  	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);
> @@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len,
>  int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len,
>  			    size_t *retlen, u_char *buf);
>  int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
>  
>  int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>  	       unsigned long count, loff_t to, size_t *retlen);
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index 65b9db936557..242015f60d10 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -205,6 +205,8 @@ struct otp_info {
>   * without OOB, e.g., NOR flash.
>   */
>  #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
> +/* Erase a given range of user data (must be in mode %MTD_FILE_MODE_OTP_USER) */
> +#define OTPERASE		_IOR('M', 25, struct otp_info)
>  

Hmm, shouldn't this be:

#define OTPERASE		_IOW('M', 25, struct otp_info)

Userspace is writing struct otp_info to the driver. OTPLOCK should
probably be _IOW() as well.

>  /*
>   * Obsolete legacy interface. Keep it in order not to break userspace
> 

Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
  2021-03-02 15:30   ` Vignesh Raghavendra
@ 2021-03-02 16:19     ` Michael Walle
  -1 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2021-03-02 16:19 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: linux-mtd, linux-kernel, linux-api, Miquel Raynal,
	Richard Weinberger, Tudor.Ambarus

Am 2021-03-02 16:30, schrieb Vignesh Raghavendra:
> Hi,
> 
> On 3/2/21 4:39 PM, Michael Walle wrote:
>> This may sound like a contradiction but some SPI-NOR flashes really
>> support erasing their OTP region until it is finally locked. Having 
>> the
>> possibility to erase an OTP region might come in handy during
>> development.
>> 
>> The ioctl argument follows the OTPLOCK style.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> OTP support for SPI-NOR flashes may be merged soon:
>> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
>> 
>> Tudor suggested to add support for the OTP erase operation most 
>> SPI-NOR
>> flashes have:
>> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
>> 
>> Therefore, this is an RFC to get some feedback on the MTD side, once 
>> this
>> is finished, I can post a patch for mtd-utils. Then we'll have a 
>> foundation
>> to add the support to SPI-NOR.
>> 
>>  drivers/mtd/mtdchar.c      |  7 ++++++-
>>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>>  include/linux/mtd/mtd.h    |  3 +++
>>  include/uapi/mtd/mtd-abi.h |  2 ++
>>  4 files changed, 23 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
>> index 323035d4f2d0..da423dd031ae 100644
>> --- a/drivers/mtd/mtdchar.c
>> +++ b/drivers/mtd/mtdchar.c
>> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int 
>> cmd, u_long arg)
>>  	case OTPGETREGIONCOUNT:
>>  	case OTPGETREGIONINFO:
>>  	case OTPLOCK:
>> +	case OTPERASE:
> 
> This is not a Safe IOCTL. We are destroying OTP data. Need to check for
> write permission before allowing the ioctl right?

Ah yes, of course. But this makes me wonder why OTPLOCK
is considered a safe command. As well as MEMLOCK and
MEMUNLOCK. And MEMSETBADBLOCK. Shouldn't these also
require write permissions?

>>  	case ECCGETLAYOUT:
>>  	case ECCGETSTATS:
>>  	case MTDFILEMODE:
>> @@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int 
>> cmd, u_long arg)
>>  	}
>> 
>>  	case OTPLOCK:
>> +	case OTPERASE:
>>  	{
>>  		struct otp_info oinfo;
>> 
>> @@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int 
>> cmd, u_long arg)
>>  			return -EINVAL;
>>  		if (copy_from_user(&oinfo, argp, sizeof(oinfo)))
>>  			return -EFAULT;
>> -		ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
>> +		if (cmd == OTPLOCK)
>> +			ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
>> +		else
>> +			ret = mtd_erase_user_prot_reg(mtd, oinfo.start, oinfo.length);
>>  		break;
>>  	}
>> 
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index 2d6423d89a17..d844d718ef77 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -1918,6 +1918,18 @@ 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_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t 
>> len)
>> +{
>> +	struct mtd_info *master = mtd_get_master(mtd);
>> +
>> +	if (!master->_erase_user_prot_reg)
>> +		return -EOPNOTSUPP;
>> +	if (!len)
>> +		return 0;
>> +	return master->_erase_user_prot_reg(master, from, len);
>> +}
>> +EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
>> +
>>  /* Chip-supported device locking */
>>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>>  {
>> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
>> index 157357ec1441..734ad7a8c353 100644
>> --- a/include/linux/mtd/mtd.h
>> +++ b/include/linux/mtd/mtd.h
>> @@ -336,6 +336,8 @@ struct mtd_info {
>>  				     size_t len, size_t *retlen, u_char *buf);
>>  	int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>>  				    size_t len);
>> +	int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>> +				     size_t len);
>>  	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);
>> @@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, 
>> loff_t from, size_t len,
>>  int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t 
>> len,
>>  			    size_t *retlen, u_char *buf);
>>  int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t 
>> len);
>> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t 
>> len);
>> 
>>  int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>>  	       unsigned long count, loff_t to, size_t *retlen);
>> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
>> index 65b9db936557..242015f60d10 100644
>> --- a/include/uapi/mtd/mtd-abi.h
>> +++ b/include/uapi/mtd/mtd-abi.h
>> @@ -205,6 +205,8 @@ struct otp_info {
>>   * without OOB, e.g., NOR flash.
>>   */
>>  #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
>> +/* Erase a given range of user data (must be in mode 
>> %MTD_FILE_MODE_OTP_USER) */
>> +#define OTPERASE		_IOR('M', 25, struct otp_info)
>> 
> 
> Hmm, shouldn't this be:
> 
> #define OTPERASE		_IOW('M', 25, struct otp_info)
> 
> Userspace is writing struct otp_info to the driver. OTPLOCK should
> probably be _IOW() as well.

You're right.

NB. most OTP commands have a wrong direction flag.

>>  /*
>>   * Obsolete legacy interface. Keep it in order not to break userspace
>> 

-michael

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
@ 2021-03-02 16:19     ` Michael Walle
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2021-03-02 16:19 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: linux-mtd, linux-kernel, linux-api, Miquel Raynal,
	Richard Weinberger, Tudor.Ambarus

Am 2021-03-02 16:30, schrieb Vignesh Raghavendra:
> Hi,
> 
> On 3/2/21 4:39 PM, Michael Walle wrote:
>> This may sound like a contradiction but some SPI-NOR flashes really
>> support erasing their OTP region until it is finally locked. Having 
>> the
>> possibility to erase an OTP region might come in handy during
>> development.
>> 
>> The ioctl argument follows the OTPLOCK style.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> OTP support for SPI-NOR flashes may be merged soon:
>> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
>> 
>> Tudor suggested to add support for the OTP erase operation most 
>> SPI-NOR
>> flashes have:
>> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
>> 
>> Therefore, this is an RFC to get some feedback on the MTD side, once 
>> this
>> is finished, I can post a patch for mtd-utils. Then we'll have a 
>> foundation
>> to add the support to SPI-NOR.
>> 
>>  drivers/mtd/mtdchar.c      |  7 ++++++-
>>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>>  include/linux/mtd/mtd.h    |  3 +++
>>  include/uapi/mtd/mtd-abi.h |  2 ++
>>  4 files changed, 23 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
>> index 323035d4f2d0..da423dd031ae 100644
>> --- a/drivers/mtd/mtdchar.c
>> +++ b/drivers/mtd/mtdchar.c
>> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int 
>> cmd, u_long arg)
>>  	case OTPGETREGIONCOUNT:
>>  	case OTPGETREGIONINFO:
>>  	case OTPLOCK:
>> +	case OTPERASE:
> 
> This is not a Safe IOCTL. We are destroying OTP data. Need to check for
> write permission before allowing the ioctl right?

Ah yes, of course. But this makes me wonder why OTPLOCK
is considered a safe command. As well as MEMLOCK and
MEMUNLOCK. And MEMSETBADBLOCK. Shouldn't these also
require write permissions?

>>  	case ECCGETLAYOUT:
>>  	case ECCGETSTATS:
>>  	case MTDFILEMODE:
>> @@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int 
>> cmd, u_long arg)
>>  	}
>> 
>>  	case OTPLOCK:
>> +	case OTPERASE:
>>  	{
>>  		struct otp_info oinfo;
>> 
>> @@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int 
>> cmd, u_long arg)
>>  			return -EINVAL;
>>  		if (copy_from_user(&oinfo, argp, sizeof(oinfo)))
>>  			return -EFAULT;
>> -		ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
>> +		if (cmd == OTPLOCK)
>> +			ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
>> +		else
>> +			ret = mtd_erase_user_prot_reg(mtd, oinfo.start, oinfo.length);
>>  		break;
>>  	}
>> 
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index 2d6423d89a17..d844d718ef77 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -1918,6 +1918,18 @@ 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_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t 
>> len)
>> +{
>> +	struct mtd_info *master = mtd_get_master(mtd);
>> +
>> +	if (!master->_erase_user_prot_reg)
>> +		return -EOPNOTSUPP;
>> +	if (!len)
>> +		return 0;
>> +	return master->_erase_user_prot_reg(master, from, len);
>> +}
>> +EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
>> +
>>  /* Chip-supported device locking */
>>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>>  {
>> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
>> index 157357ec1441..734ad7a8c353 100644
>> --- a/include/linux/mtd/mtd.h
>> +++ b/include/linux/mtd/mtd.h
>> @@ -336,6 +336,8 @@ struct mtd_info {
>>  				     size_t len, size_t *retlen, u_char *buf);
>>  	int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>>  				    size_t len);
>> +	int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
>> +				     size_t len);
>>  	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);
>> @@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, 
>> loff_t from, size_t len,
>>  int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t 
>> len,
>>  			    size_t *retlen, u_char *buf);
>>  int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t 
>> len);
>> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t 
>> len);
>> 
>>  int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>>  	       unsigned long count, loff_t to, size_t *retlen);
>> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
>> index 65b9db936557..242015f60d10 100644
>> --- a/include/uapi/mtd/mtd-abi.h
>> +++ b/include/uapi/mtd/mtd-abi.h
>> @@ -205,6 +205,8 @@ struct otp_info {
>>   * without OOB, e.g., NOR flash.
>>   */
>>  #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
>> +/* Erase a given range of user data (must be in mode 
>> %MTD_FILE_MODE_OTP_USER) */
>> +#define OTPERASE		_IOR('M', 25, struct otp_info)
>> 
> 
> Hmm, shouldn't this be:
> 
> #define OTPERASE		_IOW('M', 25, struct otp_info)
> 
> Userspace is writing struct otp_info to the driver. OTPLOCK should
> probably be _IOW() as well.

You're right.

NB. most OTP commands have a wrong direction flag.

>>  /*
>>   * Obsolete legacy interface. Keep it in order not to break userspace
>> 

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
  2021-03-02 16:19     ` Michael Walle
@ 2021-03-02 16:33       ` Vignesh Raghavendra
  -1 siblings, 0 replies; 14+ messages in thread
From: Vignesh Raghavendra @ 2021-03-02 16:33 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-mtd, linux-kernel, linux-api, Miquel Raynal,
	Richard Weinberger, Tudor.Ambarus



On 3/2/21 9:49 PM, Michael Walle wrote:
> Am 2021-03-02 16:30, schrieb Vignesh Raghavendra:
>> Hi,
>>
>> On 3/2/21 4:39 PM, Michael Walle wrote:
>>> This may sound like a contradiction but some SPI-NOR flashes really
>>> support erasing their OTP region until it is finally locked. Having the
>>> possibility to erase an OTP region might come in handy during
>>> development.
>>>
>>> The ioctl argument follows the OTPLOCK style.
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>> OTP support for SPI-NOR flashes may be merged soon:
>>> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
>>>
>>>
>>> Tudor suggested to add support for the OTP erase operation most SPI-NOR
>>> flashes have:
>>> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
>>>
>>>
>>> Therefore, this is an RFC to get some feedback on the MTD side, once
>>> this
>>> is finished, I can post a patch for mtd-utils. Then we'll have a
>>> foundation
>>> to add the support to SPI-NOR.
>>>
>>>  drivers/mtd/mtdchar.c      |  7 ++++++-
>>>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>>>  include/linux/mtd/mtd.h    |  3 +++
>>>  include/uapi/mtd/mtd-abi.h |  2 ++
>>>  4 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
>>> index 323035d4f2d0..da423dd031ae 100644
>>> --- a/drivers/mtd/mtdchar.c
>>> +++ b/drivers/mtd/mtdchar.c
>>> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int
>>> cmd, u_long arg)
>>>      case OTPGETREGIONCOUNT:
>>>      case OTPGETREGIONINFO:
>>>      case OTPLOCK:
>>> +    case OTPERASE:
>>
>> This is not a Safe IOCTL. We are destroying OTP data. Need to check for
>> write permission before allowing the ioctl right?
> 
> Ah yes, of course. But this makes me wonder why OTPLOCK
> is considered a safe command. As well as MEMLOCK and
> MEMUNLOCK. And MEMSETBADBLOCK. Shouldn't these also
> require write permissions?
> 

Well, one argument would be that LOCK/UNLOCK in itself won't modify data
and thus does not need write permission.. Although can brick a flash
from ever being writable again and change content of flash registers.

I am fine with moving these to require write permissions as well
(probably OTPLOCK as well).

[...]
>>> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
>>> index 65b9db936557..242015f60d10 100644
>>> --- a/include/uapi/mtd/mtd-abi.h
>>> +++ b/include/uapi/mtd/mtd-abi.h
>>> @@ -205,6 +205,8 @@ struct otp_info {
>>>   * without OOB, e.g., NOR flash.
>>>   */
>>>  #define MEMWRITE        _IOWR('M', 24, struct mtd_write_req)
>>> +/* Erase a given range of user data (must be in mode
>>> %MTD_FILE_MODE_OTP_USER) */
>>> +#define OTPERASE        _IOR('M', 25, struct otp_info)
>>>
>>
>> Hmm, shouldn't this be:
>>
>> #define OTPERASE        _IOW('M', 25, struct otp_info)
>>
>> Userspace is writing struct otp_info to the driver. OTPLOCK should
>> probably be _IOW() as well.
> 
> You're right.
> 
> NB. most OTP commands have a wrong direction flag.
> 

Unfortunately, yes :(


Regards
Vignesh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
@ 2021-03-02 16:33       ` Vignesh Raghavendra
  0 siblings, 0 replies; 14+ messages in thread
From: Vignesh Raghavendra @ 2021-03-02 16:33 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-mtd, linux-kernel, linux-api, Miquel Raynal,
	Richard Weinberger, Tudor.Ambarus



On 3/2/21 9:49 PM, Michael Walle wrote:
> Am 2021-03-02 16:30, schrieb Vignesh Raghavendra:
>> Hi,
>>
>> On 3/2/21 4:39 PM, Michael Walle wrote:
>>> This may sound like a contradiction but some SPI-NOR flashes really
>>> support erasing their OTP region until it is finally locked. Having the
>>> possibility to erase an OTP region might come in handy during
>>> development.
>>>
>>> The ioctl argument follows the OTPLOCK style.
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>> OTP support for SPI-NOR flashes may be merged soon:
>>> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
>>>
>>>
>>> Tudor suggested to add support for the OTP erase operation most SPI-NOR
>>> flashes have:
>>> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
>>>
>>>
>>> Therefore, this is an RFC to get some feedback on the MTD side, once
>>> this
>>> is finished, I can post a patch for mtd-utils. Then we'll have a
>>> foundation
>>> to add the support to SPI-NOR.
>>>
>>>  drivers/mtd/mtdchar.c      |  7 ++++++-
>>>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>>>  include/linux/mtd/mtd.h    |  3 +++
>>>  include/uapi/mtd/mtd-abi.h |  2 ++
>>>  4 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
>>> index 323035d4f2d0..da423dd031ae 100644
>>> --- a/drivers/mtd/mtdchar.c
>>> +++ b/drivers/mtd/mtdchar.c
>>> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int
>>> cmd, u_long arg)
>>>      case OTPGETREGIONCOUNT:
>>>      case OTPGETREGIONINFO:
>>>      case OTPLOCK:
>>> +    case OTPERASE:
>>
>> This is not a Safe IOCTL. We are destroying OTP data. Need to check for
>> write permission before allowing the ioctl right?
> 
> Ah yes, of course. But this makes me wonder why OTPLOCK
> is considered a safe command. As well as MEMLOCK and
> MEMUNLOCK. And MEMSETBADBLOCK. Shouldn't these also
> require write permissions?
> 

Well, one argument would be that LOCK/UNLOCK in itself won't modify data
and thus does not need write permission.. Although can brick a flash
from ever being writable again and change content of flash registers.

I am fine with moving these to require write permissions as well
(probably OTPLOCK as well).

[...]
>>> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
>>> index 65b9db936557..242015f60d10 100644
>>> --- a/include/uapi/mtd/mtd-abi.h
>>> +++ b/include/uapi/mtd/mtd-abi.h
>>> @@ -205,6 +205,8 @@ struct otp_info {
>>>   * without OOB, e.g., NOR flash.
>>>   */
>>>  #define MEMWRITE        _IOWR('M', 24, struct mtd_write_req)
>>> +/* Erase a given range of user data (must be in mode
>>> %MTD_FILE_MODE_OTP_USER) */
>>> +#define OTPERASE        _IOR('M', 25, struct otp_info)
>>>
>>
>> Hmm, shouldn't this be:
>>
>> #define OTPERASE        _IOW('M', 25, struct otp_info)
>>
>> Userspace is writing struct otp_info to the driver. OTPLOCK should
>> probably be _IOW() as well.
> 
> You're right.
> 
> NB. most OTP commands have a wrong direction flag.
> 

Unfortunately, yes :(


Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
  2021-03-02 16:33       ` Vignesh Raghavendra
@ 2021-03-02 16:59         ` Michael Walle
  -1 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2021-03-02 16:59 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: linux-mtd, linux-kernel, linux-api, Miquel Raynal,
	Richard Weinberger, Tudor.Ambarus

Am 2021-03-02 17:33, schrieb Vignesh Raghavendra:
> On 3/2/21 9:49 PM, Michael Walle wrote:
>> Am 2021-03-02 16:30, schrieb Vignesh Raghavendra:
>>> Hi,
>>> 
>>> On 3/2/21 4:39 PM, Michael Walle wrote:
>>>> This may sound like a contradiction but some SPI-NOR flashes really
>>>> support erasing their OTP region until it is finally locked. Having 
>>>> the
>>>> possibility to erase an OTP region might come in handy during
>>>> development.
>>>> 
>>>> The ioctl argument follows the OTPLOCK style.
>>>> 
>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>>> ---
>>>> OTP support for SPI-NOR flashes may be merged soon:
>>>> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
>>>> 
>>>> 
>>>> Tudor suggested to add support for the OTP erase operation most 
>>>> SPI-NOR
>>>> flashes have:
>>>> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
>>>> 
>>>> 
>>>> Therefore, this is an RFC to get some feedback on the MTD side, once
>>>> this
>>>> is finished, I can post a patch for mtd-utils. Then we'll have a
>>>> foundation
>>>> to add the support to SPI-NOR.
>>>> 
>>>>  drivers/mtd/mtdchar.c      |  7 ++++++-
>>>>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>>>>  include/linux/mtd/mtd.h    |  3 +++
>>>>  include/uapi/mtd/mtd-abi.h |  2 ++
>>>>  4 files changed, 23 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
>>>> index 323035d4f2d0..da423dd031ae 100644
>>>> --- a/drivers/mtd/mtdchar.c
>>>> +++ b/drivers/mtd/mtdchar.c
>>>> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, 
>>>> u_int
>>>> cmd, u_long arg)
>>>>      case OTPGETREGIONCOUNT:
>>>>      case OTPGETREGIONINFO:
>>>>      case OTPLOCK:
>>>> +    case OTPERASE:
>>> 
>>> This is not a Safe IOCTL. We are destroying OTP data. Need to check 
>>> for
>>> write permission before allowing the ioctl right?
>> 
>> Ah yes, of course. But this makes me wonder why OTPLOCK
>> is considered a safe command. As well as MEMLOCK and
>> MEMUNLOCK. And MEMSETBADBLOCK. Shouldn't these also
>> require write permissions?
>> 
> 
> Well, one argument would be that LOCK/UNLOCK in itself won't modify 
> data
> and thus does not need write permission.. Although can brick a flash
> from ever being writable again and change content of flash registers.

Whether not you can brick a device (I agree with you), it is writing
to the protection bits. But what is more imporant is that OTPLOCK
is actually write-once.

> I am fine with moving these to require write permissions as well
> (probably OTPLOCK as well).

Ok, I'm unsure about MEMSETBADBLOCK, though.

-michael

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
@ 2021-03-02 16:59         ` Michael Walle
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2021-03-02 16:59 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: linux-mtd, linux-kernel, linux-api, Miquel Raynal,
	Richard Weinberger, Tudor.Ambarus

Am 2021-03-02 17:33, schrieb Vignesh Raghavendra:
> On 3/2/21 9:49 PM, Michael Walle wrote:
>> Am 2021-03-02 16:30, schrieb Vignesh Raghavendra:
>>> Hi,
>>> 
>>> On 3/2/21 4:39 PM, Michael Walle wrote:
>>>> This may sound like a contradiction but some SPI-NOR flashes really
>>>> support erasing their OTP region until it is finally locked. Having 
>>>> the
>>>> possibility to erase an OTP region might come in handy during
>>>> development.
>>>> 
>>>> The ioctl argument follows the OTPLOCK style.
>>>> 
>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>>> ---
>>>> OTP support for SPI-NOR flashes may be merged soon:
>>>> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@walle.cc/
>>>> 
>>>> 
>>>> Tudor suggested to add support for the OTP erase operation most 
>>>> SPI-NOR
>>>> flashes have:
>>>> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9e57@microchip.com/
>>>> 
>>>> 
>>>> Therefore, this is an RFC to get some feedback on the MTD side, once
>>>> this
>>>> is finished, I can post a patch for mtd-utils. Then we'll have a
>>>> foundation
>>>> to add the support to SPI-NOR.
>>>> 
>>>>  drivers/mtd/mtdchar.c      |  7 ++++++-
>>>>  drivers/mtd/mtdcore.c      | 12 ++++++++++++
>>>>  include/linux/mtd/mtd.h    |  3 +++
>>>>  include/uapi/mtd/mtd-abi.h |  2 ++
>>>>  4 files changed, 23 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
>>>> index 323035d4f2d0..da423dd031ae 100644
>>>> --- a/drivers/mtd/mtdchar.c
>>>> +++ b/drivers/mtd/mtdchar.c
>>>> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, 
>>>> u_int
>>>> cmd, u_long arg)
>>>>      case OTPGETREGIONCOUNT:
>>>>      case OTPGETREGIONINFO:
>>>>      case OTPLOCK:
>>>> +    case OTPERASE:
>>> 
>>> This is not a Safe IOCTL. We are destroying OTP data. Need to check 
>>> for
>>> write permission before allowing the ioctl right?
>> 
>> Ah yes, of course. But this makes me wonder why OTPLOCK
>> is considered a safe command. As well as MEMLOCK and
>> MEMUNLOCK. And MEMSETBADBLOCK. Shouldn't these also
>> require write permissions?
>> 
> 
> Well, one argument would be that LOCK/UNLOCK in itself won't modify 
> data
> and thus does not need write permission.. Although can brick a flash
> from ever being writable again and change content of flash registers.

Whether not you can brick a device (I agree with you), it is writing
to the protection bits. But what is more imporant is that OTPLOCK
is actually write-once.

> I am fine with moving these to require write permissions as well
> (probably OTPLOCK as well).

Ok, I'm unsure about MEMSETBADBLOCK, though.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-03-03 22:06 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-02 11:09 [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl Michael Walle
2021-03-02 11:09 ` Michael Walle
2021-03-02 12:46 ` Miquel Raynal
2021-03-02 12:46   ` Miquel Raynal
2021-03-02 13:06   ` Michael Walle
2021-03-02 13:06     ` Michael Walle
2021-03-02 15:30 ` Vignesh Raghavendra
2021-03-02 15:30   ` Vignesh Raghavendra
2021-03-02 16:19   ` Michael Walle
2021-03-02 16:19     ` Michael Walle
2021-03-02 16:33     ` Vignesh Raghavendra
2021-03-02 16:33       ` Vignesh Raghavendra
2021-03-02 16:59       ` Michael Walle
2021-03-02 16:59         ` Michael Walle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.