Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl

[Michael Walle <michael@xxxxxxxx>]

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@xxxxxxxx>
> > ---
> > OTP support for SPI-NOR flashes may be merged soon:
> > https://lore.kernel.org/linux-mtd/20210216162807.13509-1-michael@xxxxxxxx/
> >
> > 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@xxxxxxxxxxxxx/
> >
> > 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