From: Vignesh Raghavendra <vigneshr@ti.com> To: Michael Walle <michael@walle.cc> Cc: <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linux-api@vger.kernel.org>, Miquel Raynal <miquel.raynal@bootlin.com>, Richard Weinberger <richard@nod.at>, <Tudor.Ambarus@microchip.com> Subject: Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl Date: Tue, 2 Mar 2021 22:03:29 +0530 [thread overview] Message-ID: <a4464459-dc49-d5de-d969-b9ea96b025d6@ti.com> (raw) In-Reply-To: <74df918148be8c9820acc877e39adf3f@walle.cc> 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. 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
WARNING: multiple messages have this Message-ID (diff)
From: Vignesh Raghavendra <vigneshr@ti.com> To: Michael Walle <michael@walle.cc> Cc: <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linux-api@vger.kernel.org>, Miquel Raynal <miquel.raynal@bootlin.com>, Richard Weinberger <richard@nod.at>, <Tudor.Ambarus@microchip.com> Subject: Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl Date: Tue, 2 Mar 2021 22:03:29 +0530 [thread overview] Message-ID: <a4464459-dc49-d5de-d969-b9ea96b025d6@ti.com> (raw) In-Reply-To: <74df918148be8c9820acc877e39adf3f@walle.cc> 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. 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2021-03-02 21:30 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-02 11:09 [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl Michael Walle 2021-03-02 11:09 ` Michael Walle 2021-03-02 12:46 ` Miquel Raynal 2021-03-02 12:46 ` Miquel Raynal 2021-03-02 13:06 ` Michael Walle 2021-03-02 13:06 ` Michael Walle 2021-03-02 15:30 ` Vignesh Raghavendra 2021-03-02 15:30 ` Vignesh Raghavendra 2021-03-02 16:19 ` Michael Walle 2021-03-02 16:19 ` Michael Walle 2021-03-02 16:33 ` Vignesh Raghavendra [this message] 2021-03-02 16:33 ` Vignesh Raghavendra 2021-03-02 16:59 ` Michael Walle 2021-03-02 16:59 ` Michael Walle
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a4464459-dc49-d5de-d969-b9ea96b025d6@ti.com \ --to=vigneshr@ti.com \ --cc=Tudor.Ambarus@microchip.com \ --cc=linux-api@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=michael@walle.cc \ --cc=miquel.raynal@bootlin.com \ --cc=richard@nod.at \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.