[PATCH] mtd: add MEMREAD ioctl
Miquel Raynal
miquel.raynal at bootlin.com
Tue Sep 28 06:58:59 PDT 2021
Hi Michał,
+ Boris just in case you have anything obvious that pops up in your
head when reading the description, otherwise no need to thoroughfully
review this ;)
kernel at kempniu.pl wrote on Mon, 20 Sep 2021 09:02:21 +0200:
> User-space applications making use of MTD devices via /dev/mtd*
> character devices currently have limited capabilities for reading data:
>
> - only deprecated methods of accessing OOB layout information exist,
>
> - there is no way to explicitly specify MTD operation mode to use; it
> is auto-selected based on the MTD file mode (MTD_FILE_MODE_*) set
> for the character device; in particular, this prevents using
> MTD_OPS_AUTO_OOB for reads,
>
> - all existing user-space interfaces which cause mtd_read() or
> mtd_read_oob() to be called (via mtdchar_read() and
> mtdchar_read_oob(), respectively) return success even when those
> functions return -EUCLEAN or -EBADMSG; this renders user-space
> applications using these interfaces unaware of any corrected
> bitflips or uncorrectable ECC errors detected during reads.
>
> Note that the existing MEMWRITE ioctl allows the MTD operation mode to
> be explicitly set, allowing user-space applications to write page data
> and OOB data without requiring them to know anything about the OOB
> layout of the MTD device they are writing to (MTD_OPS_AUTO_OOB). Also,
> the MEMWRITE ioctl does not mangle the return value of mtd_write_oob().
>
> Add a new ioctl, MEMREAD, which addresses the above issues. It is
> intended to be a read-side counterpart of the existing MEMWRITE ioctl.
>
> Update include/uapi/mtd/mtd-abi.h accordingly.
I have to admit I am generally scared whenever something touches this
file. While I am really open to anything internally I know we need to be
careful with these additions, hence I will only merge this patch with
all other MTD maintainers acks.
On my side I am fine with the approach though.
Thanks,
Miquèl
> Signed-off-by: Michał Kępień <kernel at kempniu.pl>
> ---
> This patch is a shameless calque^W^W^Wheavily inspired by MEMWRITE code,
> so quite a lot of copy-pasting happened. I guess it is somewhat
> expected when adding a read-side counterpart of existing code which
> takes care of writes, but please excuse me if I went too far.
>
> Note that "scripts/checkpatch.pl --strict" returns two alignment
> warnings for this patch. Given that existing code triggers the same
> warnings, I assumed that local consistency trumps checkpatch.pl's
> complaints.
>
> drivers/mtd/mtdchar.c | 60 ++++++++++++++++++++++++++++++++++++++
> include/uapi/mtd/mtd-abi.h | 43 +++++++++++++++++++++++----
> 2 files changed, 98 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 155e991d9d75..92e0024bdcf7 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -621,6 +621,58 @@ static int mtdchar_write_ioctl(struct mtd_info *mtd,
> return ret;
> }
>
> +static int mtdchar_read_ioctl(struct mtd_info *mtd,
> + struct mtd_read_req __user *argp)
> +{
> + struct mtd_info *master = mtd_get_master(mtd);
> + struct mtd_read_req req;
> + struct mtd_oob_ops ops = {};
> + void __user *usr_data, *usr_oob;
> + int ret;
> +
> + if (copy_from_user(&req, argp, sizeof(req)))
> + return -EFAULT;
> +
> + usr_data = (void __user *)(uintptr_t)req.usr_data;
> + usr_oob = (void __user *)(uintptr_t)req.usr_oob;
> +
> + if (!master->_read_oob)
> + return -EOPNOTSUPP;
> + ops.mode = req.mode;
> + ops.len = (size_t)req.len;
> + ops.ooblen = (size_t)req.ooblen;
> + ops.ooboffs = 0;
> +
> + if (usr_data) {
> + ops.datbuf = kmalloc(ops.len, GFP_KERNEL);
> + if (IS_ERR(ops.datbuf))
> + return PTR_ERR(ops.datbuf);
> + } else {
> + ops.datbuf = NULL;
> + }
> +
> + if (usr_oob) {
> + ops.oobbuf = kmalloc(ops.ooblen, GFP_KERNEL);
> + if (IS_ERR(ops.oobbuf)) {
> + kfree(ops.datbuf);
> + return PTR_ERR(ops.oobbuf);
> + }
> + } else {
> + ops.oobbuf = NULL;
> + }
> +
> + ret = mtd_read_oob(mtd, (loff_t)req.start, &ops);
> +
> + if (copy_to_user(usr_data, ops.datbuf, ops.retlen) ||
> + copy_to_user(usr_oob, ops.oobbuf, ops.oobretlen))
> + ret = -EFAULT;
> +
> + kfree(ops.datbuf);
> + kfree(ops.oobbuf);
> +
> + return ret;
> +}
> +
> static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
> {
> struct mtd_file_info *mfi = file->private_data;
> @@ -643,6 +695,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
> case MEMGETINFO:
> case MEMREADOOB:
> case MEMREADOOB64:
> + case MEMREAD:
> case MEMISLOCKED:
> case MEMGETOOBSEL:
> case MEMGETBADBLOCK:
> @@ -817,6 +870,13 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
> break;
> }
>
> + case MEMREAD:
> + {
> + ret = mtdchar_read_ioctl(mtd,
> + (struct mtd_read_req __user *)arg);
> + break;
> + }
> +
> case MEMLOCK:
> {
> struct erase_info_user einfo;
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index b869990c2db2..337e6e597fad 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -55,9 +55,9 @@ struct mtd_oob_buf64 {
> * @MTD_OPS_RAW: data are transferred as-is, with no error correction;
> * this mode implies %MTD_OPS_PLACE_OOB
> *
> - * These modes can be passed to ioctl(MEMWRITE) and are also used internally.
> - * See notes on "MTD file modes" for discussion on %MTD_OPS_RAW vs.
> - * %MTD_FILE_MODE_RAW.
> + * These modes can be passed to ioctl(MEMWRITE) and ioctl(MEMREAD); they are
> + * also used internally. See notes on "MTD file modes" for discussion on
> + * %MTD_OPS_RAW vs. %MTD_FILE_MODE_RAW.
> */
> enum {
> MTD_OPS_PLACE_OOB = 0,
> @@ -91,6 +91,32 @@ struct mtd_write_req {
> __u8 padding[7];
> };
>
> +/**
> + * struct mtd_read_req - data structure for requesting a read operation
> + *
> + * @start: start address
> + * @len: length of data buffer
> + * @ooblen: length of OOB buffer
> + * @usr_data: user-provided data buffer
> + * @usr_oob: user-provided OOB buffer
> + * @mode: MTD mode (see "MTD operation modes")
> + * @padding: reserved, must be set to 0
> + *
> + * This structure supports ioctl(MEMREAD) operations, allowing data and/or OOB
> + * reads in various modes. To read from OOB-only, set @usr_data == NULL, and to
> + * read data-only, set @usr_oob == NULL. However, setting both @usr_data and
> + * @usr_oob to NULL is not allowed.
> + */
> +struct mtd_read_req {
> + __u64 start;
> + __u64 len;
> + __u64 ooblen;
> + __u64 usr_data;
> + __u64 usr_oob;
> + __u8 mode;
> + __u8 padding[7];
> +};
> +
> #define MTD_ABSENT 0
> #define MTD_RAM 1
> #define MTD_ROM 2
> @@ -207,6 +233,12 @@ struct otp_info {
> #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 _IOW('M', 25, struct otp_info)
> +/*
> + * Most generic read interface; can read in-band and/or out-of-band in various
> + * modes (see "struct mtd_read_req"). This ioctl is not supported for flashes
> + * without OOB, e.g., NOR flash.
> + */
> +#define MEMREAD _IOWR('M', 26, struct mtd_read_req)
>
> /*
> * Obsolete legacy interface. Keep it in order not to break userspace
> @@ -270,8 +302,9 @@ struct mtd_ecc_stats {
> * Note: %MTD_FILE_MODE_RAW provides the same functionality as %MTD_OPS_RAW -
> * raw access to the flash, without error correction or autoplacement schemes.
> * Wherever possible, the MTD_OPS_* mode will override the MTD_FILE_MODE_* mode
> - * (e.g., when using ioctl(MEMWRITE)), but in some cases, the MTD_FILE_MODE is
> - * used out of necessity (e.g., `write()', ioctl(MEMWRITEOOB64)).
> + * (e.g., when using ioctl(MEMWRITE) or ioctl(MEMREAD)), but in some cases, the
> + * MTD_FILE_MODE is used out of necessity (e.g., `write()',
> + * ioctl(MEMWRITEOOB64)).
> */
> enum mtd_file_modes {
> MTD_FILE_MODE_NORMAL = MTD_OTP_OFF,
More information about the linux-mtd
mailing list