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

[Michael Walle <michael@walle.cc>]

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