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

[Michael Walle]

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@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?
>
> 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