Re: [PATCH] mtd: add MEMREAD ioctl

[Boris Brezillon <boris.brezillon@collabora.com>]

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