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

[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.

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