[PATCH] mtd: add MEMREAD ioctl

[PATCH] mtd: add MEMREAD ioctl

[Michał Kępień]

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.

Signed-off-by: Michał Kępień <kernel@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,
-- 
2.33.0

Re: [PATCH] mtd: add MEMREAD ioctl

[Miquel Raynal]

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@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@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,

Re: [PATCH] mtd: add MEMREAD ioctl

[Boris Brezillon]

Hi Miquel, Michal,

On Tue, 28 Sep 2021 15:58:59 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> 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 ;)

Couple of comment below.


> > Signed-off-by: Michał Kępień <kernel@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);

Hm, I know the write path does that, but I'm really not sure
kmalloc()-ing a buffer of the requested read length is a good
idea. Having a loop doing reads with an erasesize granularity would
avoid this unbounded allocation while keeping performance acceptable in
most cases.

> > +		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];
> > +};

I do agree that a new interface is needed, but if we're adding a new
entry point, let's make sure it covers all possible use cases we have
now. At the very least, I think we're missing info about the maximum
number of corrected bits per ECC region on the portion being read.
Propagating EUCLEAN errors is nice, but it's not precise enough IMHO.

I remember discussing search a new READ ioctl with Sascha Hauer a few
years back, but I can't find the discussion...

Regards,

Boris

Re: [PATCH] mtd: add MEMREAD ioctl

[Miquel Raynal]

Hi Boris,

boris.brezillon@collabora.com wrote on Tue, 28 Sep 2021 16:24:02 +0200:

> Hi Miquel, Michal,
> 
> On Tue, 28 Sep 2021 15:58:59 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > 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 ;)  
> 
> Couple of comment below.
> 
> 
> > > Signed-off-by: Michał Kępień <kernel@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);  
> 
> Hm, I know the write path does that, but I'm really not sure
> kmalloc()-ing a buffer of the requested read length is a good
> idea. Having a loop doing reads with an erasesize granularity would
> avoid this unbounded allocation while keeping performance acceptable in
> most cases.

Right.

> 
> > > +		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];
> > > +};  
> 
> I do agree that a new interface is needed, but if we're adding a new
> entry point, let's make sure it covers all possible use cases we have
> now. At the very least, I think we're missing info about the maximum
> number of corrected bits per ECC region on the portion being read.
> Propagating EUCLEAN errors is nice, but it's not precise enough IMHO.
> 
> I remember discussing search a new READ ioctl with Sascha Hauer a few
> years back, but I can't find the discussion...

We also discussed a mtd_io_op some time ago, which would equivalently
replace mtd_oob_ops at some point, including more information such as
the bitflips which happened on every chunk instead of the information
regarding the maximum number of bitflips in one of the chunks only.
IIRC the point was to get rid of the mtd_{read,write}{,_oob} hooks and
structures in favor of a more robust and complete set of operations.

Re: [PATCH] mtd: add MEMREAD ioctl

[Michał Kępień]

Miquel, Boris,

Thank you both for your input.

> > I do agree that a new interface is needed, but if we're adding a new
> > entry point, let's make sure it covers all possible use cases we have
> > now. At the very least, I think we're missing info about the maximum
> > number of corrected bits per ECC region on the portion being read.
> > Propagating EUCLEAN errors is nice, but it's not precise enough IMHO.
> > 
> > I remember discussing search a new READ ioctl with Sascha Hauer a few
> > years back, but I can't find the discussion...

I think this is the thread in question:

    https://www.infradead.org/pipermail/linux-mtd/2016-April/thread.html#67085

In fact, it looks like Boris beat me to preparing a draft patch adding a
MEMREAD ioctl by some five years:

    https://www.infradead.org/pipermail/linux-mtd/2016-April/067187.html

It is apparently true that "everything that can be invented has been
invented"... :-)  I did search the web for existing mentions of a
MEMREAD ioctl before submitting my patch, but this thread did not turn
up in the results :(

Anyway, back in 2016, Sascha hinted that he might move forward with the
draft prepared by Boris:

    https://www.infradead.org/pipermail/linux-mtd/2016-April/067215.html

but I cannot find any related submissions from Sascha in linux-mtd's
Patchwork.

> We also discussed a mtd_io_op some time ago, which would equivalently
> replace mtd_oob_ops at some point, including more information such as
> the bitflips which happened on every chunk instead of the information
> regarding the maximum number of bitflips in one of the chunks only.

Is that discussion available online?  Search engines seem to be
oblivious to that term, which makes it hard for me to get acquainted
with that idea and/or to comment on it ;)

> IIRC the point was to get rid of the mtd_{read,write}{,_oob} hooks and
> structures in favor of a more robust and complete set of operations.

That sounds like a major overhaul, right?

I guess the big question from my perspective is: should I revive Boris'
original effort on the MEMREAD ioctl (which returns more detailed
bitflip stats in the structure passed by user space) or would that be a
waste of time because the subsystem will be switched over wholesale to a
new way of doing I/O (mtd_io_op) in the foreseeable future and therefore
exposing yet another ioctl to user space today would be frowned upon?

-- 
Best regards,
Michał Kępień

Re: [PATCH] mtd: add MEMREAD ioctl

[Boris Brezillon]

Hu Michal,

On Wed, 29 Sep 2021 21:42:24 +0200
Michał Kępień <kernel@kempniu.pl> wrote:

> Miquel, Boris,
> 
> Thank you both for your input.
> 
> > > I do agree that a new interface is needed, but if we're adding a new
> > > entry point, let's make sure it covers all possible use cases we have
> > > now. At the very least, I think we're missing info about the maximum
> > > number of corrected bits per ECC region on the portion being read.
> > > Propagating EUCLEAN errors is nice, but it's not precise enough IMHO.
> > > 
> > > I remember discussing search a new READ ioctl with Sascha Hauer a few
> > > years back, but I can't find the discussion...  
> 
> I think this is the thread in question:
> 
>     https://www.infradead.org/pipermail/linux-mtd/2016-April/thread.html#67085
> 
> In fact, it looks like Boris beat me to preparing a draft patch adding a
> MEMREAD ioctl by some five years:
> 
>     https://www.infradead.org/pipermail/linux-mtd/2016-April/067187.html

Exactly the one I was referring to. Note that this patch still contains
the unbounded malloc which I think is worth fixing, but other than
that and the addition of ECC stats, it looks pretty similar to yours.

> 
> It is apparently true that "everything that can be invented has been
> invented"... :-)  I did search the web for existing mentions of a
> MEMREAD ioctl before submitting my patch, but this thread did not turn
> up in the results :(
> 
> Anyway, back in 2016, Sascha hinted that he might move forward with the
> draft prepared by Boris:
> 
>     https://www.infradead.org/pipermail/linux-mtd/2016-April/067215.html
> 
> but I cannot find any related submissions from Sascha in linux-mtd's
> Patchwork.
> 
> > We also discussed a mtd_io_op some time ago, which would equivalently
> > replace mtd_oob_ops at some point, including more information such as
> > the bitflips which happened on every chunk instead of the information
> > regarding the maximum number of bitflips in one of the chunks only.  
> 
> Is that discussion available online?  Search engines seem to be
> oblivious to that term, which makes it hard for me to get acquainted
> with that idea and/or to comment on it ;)

Not sure this has been discussed publicly, but I remember suggesting
that to Miquel a while ago to simplify the in-kernel MTD interface.

> 
> > IIRC the point was to get rid of the mtd_{read,write}{,_oob} hooks and
> > structures in favor of a more robust and complete set of operations.  
> 
> That sounds like a major overhaul, right?
> 
> I guess the big question from my perspective is: should I revive Boris'
> original effort on the MEMREAD ioctl (which returns more detailed
> bitflip stats in the structure passed by user space) or would that be a
> waste of time because the subsystem will be switched over wholesale to a
> new way of doing I/O (mtd_io_op) in the foreseeable future and therefore
> exposing yet another ioctl to user space today would be frowned upon?
> 

That's not my call to make, but I think those 2 things are orthogonal
and can be addressed separately.

Regards,

Boris

Re: [PATCH] mtd: add MEMREAD ioctl

[Miquel Raynal]

boris.brezillon@collabora.com wrote on Thu, 30 Sep 2021 08:51:33 +0200:

> Hu Michal,
> 
> On Wed, 29 Sep 2021 21:42:24 +0200
> Michał Kępień <kernel@kempniu.pl> wrote:
> 
> > Miquel, Boris,
> > 
> > Thank you both for your input.
> >   
> > > > I do agree that a new interface is needed, but if we're adding a new
> > > > entry point, let's make sure it covers all possible use cases we have
> > > > now. At the very least, I think we're missing info about the maximum
> > > > number of corrected bits per ECC region on the portion being read.
> > > > Propagating EUCLEAN errors is nice, but it's not precise enough IMHO.
> > > > 
> > > > I remember discussing search a new READ ioctl with Sascha Hauer a few
> > > > years back, but I can't find the discussion...    
> > 
> > I think this is the thread in question:
> > 
> >     https://www.infradead.org/pipermail/linux-mtd/2016-April/thread.html#67085
> > 
> > In fact, it looks like Boris beat me to preparing a draft patch adding a
> > MEMREAD ioctl by some five years:
> > 
> >     https://www.infradead.org/pipermail/linux-mtd/2016-April/067187.html  
> 
> Exactly the one I was referring to. Note that this patch still contains
> the unbounded malloc which I think is worth fixing, but other than
> that and the addition of ECC stats, it looks pretty similar to yours.
> 
> > 
> > It is apparently true that "everything that can be invented has been
> > invented"... :-)  I did search the web for existing mentions of a
> > MEMREAD ioctl before submitting my patch, but this thread did not turn
> > up in the results :(
> > 
> > Anyway, back in 2016, Sascha hinted that he might move forward with the
> > draft prepared by Boris:
> > 
> >     https://www.infradead.org/pipermail/linux-mtd/2016-April/067215.html
> > 
> > but I cannot find any related submissions from Sascha in linux-mtd's
> > Patchwork.
> >   
> > > We also discussed a mtd_io_op some time ago, which would equivalently
> > > replace mtd_oob_ops at some point, including more information such as
> > > the bitflips which happened on every chunk instead of the information
> > > regarding the maximum number of bitflips in one of the chunks only.    
> > 
> > Is that discussion available online?  Search engines seem to be
> > oblivious to that term, which makes it hard for me to get acquainted
> > with that idea and/or to comment on it ;)  
> 
> Not sure this has been discussed publicly, but I remember suggesting
> that to Miquel a while ago to simplify the in-kernel MTD interface.

It certainly happened on IRC indeed.

> > > IIRC the point was to get rid of the mtd_{read,write}{,_oob} hooks and
> > > structures in favor of a more robust and complete set of operations.    
> > 
> > That sounds like a major overhaul, right?
> > 
> > I guess the big question from my perspective is: should I revive Boris'
> > original effort on the MEMREAD ioctl (which returns more detailed
> > bitflip stats in the structure passed by user space) or would that be a
> > waste of time because the subsystem will be switched over wholesale to a
> > new way of doing I/O (mtd_io_op) in the foreseeable future and therefore
> > exposing yet another ioctl to user space today would be frowned upon?
> >   
> 
> That's not my call to make, but I think those 2 things are orthogonal
> and can be addressed separately.

Agreed.

Thanks,
Miquèl

Re: [PATCH] mtd: add MEMREAD ioctl

[Michał Kępień]

> > > > > I remember discussing search a new READ ioctl with Sascha Hauer a few
> > > > > years back, but I can't find the discussion...    
> > > 
> > > I think this is the thread in question:
> > > 
> > >     https://www.infradead.org/pipermail/linux-mtd/2016-April/thread.html#67085
> > > 
> > > In fact, it looks like Boris beat me to preparing a draft patch adding a
> > > MEMREAD ioctl by some five years:
> > > 
> > >     https://www.infradead.org/pipermail/linux-mtd/2016-April/067187.html  
> > 
> > Exactly the one I was referring to. Note that this patch still contains
> > the unbounded malloc which I think is worth fixing, but other than
> > that and the addition of ECC stats, it looks pretty similar to yours.

Right, thanks.

> > > I guess the big question from my perspective is: should I revive Boris'
> > > original effort on the MEMREAD ioctl (which returns more detailed
> > > bitflip stats in the structure passed by user space) or would that be a
> > > waste of time because the subsystem will be switched over wholesale to a
> > > new way of doing I/O (mtd_io_op) in the foreseeable future and therefore
> > > exposing yet another ioctl to user space today would be frowned upon?
> > >   
> > 
> > That's not my call to make, but I think those 2 things are orthogonal
> > and can be addressed separately.
> 
> Agreed.

Thank you both - it sounds like I should start working on a v2 that will
make the new MEMREAD ioctl return more detailed ECC statistics to user
space.

Boris, I think a Suggested-by tag crediting you is in order for both the
unbounded malloc issue and the MEMREAD ioctl, but submitting-patches.rst
says I should not add this tag without your permission.  So, are you
okay with me adding it?

Miquel, as for the unbounded malloc issue, should I address this in a
separate (preliminary) patch or rather submit a two-patch v2 series
(unbounded malloc fix + new MEMREAD ioctl)?

-- 
Best regards,
Michał Kępień

Re: [PATCH] mtd: add MEMREAD ioctl

[Miquel Raynal]

Hi Michał,

kernel@kempniu.pl wrote on Thu, 30 Sep 2021 15:54:07 +0200:

> > > > > > I remember discussing search a new READ ioctl with Sascha Hauer a few
> > > > > > years back, but I can't find the discussion...      
> > > > 
> > > > I think this is the thread in question:
> > > > 
> > > >     https://www.infradead.org/pipermail/linux-mtd/2016-April/thread.html#67085
> > > > 
> > > > In fact, it looks like Boris beat me to preparing a draft patch adding a
> > > > MEMREAD ioctl by some five years:
> > > > 
> > > >     https://www.infradead.org/pipermail/linux-mtd/2016-April/067187.html    
> > > 
> > > Exactly the one I was referring to. Note that this patch still contains
> > > the unbounded malloc which I think is worth fixing, but other than
> > > that and the addition of ECC stats, it looks pretty similar to yours.  
> 
> Right, thanks.
> 
> > > > I guess the big question from my perspective is: should I revive Boris'
> > > > original effort on the MEMREAD ioctl (which returns more detailed
> > > > bitflip stats in the structure passed by user space) or would that be a
> > > > waste of time because the subsystem will be switched over wholesale to a
> > > > new way of doing I/O (mtd_io_op) in the foreseeable future and therefore
> > > > exposing yet another ioctl to user space today would be frowned upon?
> > > >     
> > > 
> > > That's not my call to make, but I think those 2 things are orthogonal
> > > and can be addressed separately.  
> > 
> > Agreed.  
> 
> Thank you both - it sounds like I should start working on a v2 that will
> make the new MEMREAD ioctl return more detailed ECC statistics to user
> space.
> 
> Boris, I think a Suggested-by tag crediting you is in order for both the
> unbounded malloc issue and the MEMREAD ioctl, but submitting-patches.rst
> says I should not add this tag without your permission.  So, are you
> okay with me adding it?
> 
> Miquel, as for the unbounded malloc issue, should I address this in a
> separate (preliminary) patch or rather submit a two-patch v2 series
> (unbounded malloc fix + new MEMREAD ioctl)?

Both work as long as you keep the changes in different commits :)

Thanks,
Miquèl

Re: [PATCH] mtd: add MEMREAD ioctl

[Boris Brezillon]

On Thu, 30 Sep 2021 15:54:07 +0200
Michał Kępień <kernel@kempniu.pl> wrote:

> Boris, I think a Suggested-by tag crediting you is in order for both the
> unbounded malloc issue and the MEMREAD ioctl, but submitting-patches.rst
> says I should not add this tag without your permission.  So, are you
> okay with me adding it?

Sure, no problem.