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
