[RFC PATCH] mmc: block: Add new ioctl to send combo commands

[RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Jon Hunter]

From: Seshagiri Holi <sholi@nvidia.com>

Certain eMMC devices allow vendor specific device information to be read
via a sequence of vendor commands. These vendor commands must be issued
in sequence and an atomic fashion. One way to support this would be to
add an ioctl function for sending a sequence of commands to the device
atomically as proposed here. These combo commands are simple array of
the existing mmc_ioc_cmd structure.

Signed-off-by: Seshagiri Holi <sholi@nvidia.com>
[ jonathanh@nvidia.com: Rebased on linux-next from v3.18 and updated
  commit message ]
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
I am not sure if there are other/better ways to support this, and so
if this approach is not acceptable, other suggestions would be great.
Thanks!

I still need to test this updated version for -next but wanted to get
some feedback on the approach.

 drivers/mmc/card/block.c       | 199 ++++++++++++++++++++++++++++++++---------
 include/uapi/linux/mmc/ioctl.h |  17 +++-
 2 files changed, 171 insertions(+), 45 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c742cfd7674e..fe2bca75b997 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -387,6 +387,22 @@ out:
 	return ERR_PTR(err);
 }
 
+static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
+				      struct mmc_blk_ioc_data *idata)
+{
+	if (copy_to_user(&(ic_ptr->response), idata->ic.response,
+			 sizeof(idata->ic.response)))
+		return -EFAULT;
+
+	if (!idata->ic.write_flag) {
+		if (copy_to_user((void __user *)(unsigned long)idata->ic.data_ptr,
+				 idata->buf, idata->buf_bytes))
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
 static int ioctl_rpmb_card_status_poll(struct mmc_card *card, u32 *status,
 				       u32 retries_max)
 {
@@ -447,12 +463,9 @@ out:
 	return err;
 }
 
-static int mmc_blk_ioctl_cmd(struct block_device *bdev,
-	struct mmc_ioc_cmd __user *ic_ptr)
+static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
+			       struct mmc_blk_ioc_data *idata)
 {
-	struct mmc_blk_ioc_data *idata;
-	struct mmc_blk_data *md;
-	struct mmc_card *card;
 	struct mmc_command cmd = {0};
 	struct mmc_data data = {0};
 	struct mmc_request mrq = {NULL};
@@ -461,33 +474,12 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 	int is_rpmb = false;
 	u32 status = 0;
 
-	/*
-	 * The caller must have CAP_SYS_RAWIO, and must be calling this on the
-	 * whole block device, not on a partition.  This prevents overspray
-	 * between sibling partitions.
-	 */
-	if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
-		return -EPERM;
-
-	idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
-	if (IS_ERR(idata))
-		return PTR_ERR(idata);
-
-	md = mmc_blk_get(bdev->bd_disk);
-	if (!md) {
-		err = -EINVAL;
-		goto cmd_err;
-	}
+	if (!card || !md || !idata)
+		return -EINVAL;
 
 	if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
 		is_rpmb = true;
 
-	card = md->queue.card;
-	if (IS_ERR(card)) {
-		err = PTR_ERR(card);
-		goto cmd_done;
-	}
-
 	cmd.opcode = idata->ic.opcode;
 	cmd.arg = idata->ic.arg;
 	cmd.flags = idata->ic.flags;
@@ -582,18 +574,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 	if (idata->ic.postsleep_min_us)
 		usleep_range(idata->ic.postsleep_min_us, idata->ic.postsleep_max_us);
 
-	if (copy_to_user(&(ic_ptr->response), cmd.resp, sizeof(cmd.resp))) {
-		err = -EFAULT;
-		goto cmd_rel_host;
-	}
-
-	if (!idata->ic.write_flag) {
-		if (copy_to_user((void __user *)(unsigned long) idata->ic.data_ptr,
-						idata->buf, idata->buf_bytes)) {
-			err = -EFAULT;
-			goto cmd_rel_host;
-		}
-	}
+	memcpy(&(idata->ic.response), cmd.resp, sizeof(cmd.resp));
 
 	if (is_rpmb) {
 		/*
@@ -609,6 +590,48 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 
 cmd_rel_host:
 	mmc_put_card(card);
+	return err;
+}
+
+static int mmc_blk_ioctl_cmd(struct block_device *bdev,
+			struct mmc_ioc_cmd __user *ic_ptr)
+{
+	struct mmc_blk_ioc_data *idata;
+	struct mmc_blk_data *md;
+	struct mmc_card *card;
+	int err;
+
+	/*
+	 * The caller must have CAP_SYS_RAWIO, and must be calling this on the
+	 * whole block device, not on a partition.  This prevents overspray
+	 * between sibling partitions.
+	 */
+	if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
+		return -EPERM;
+
+	idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
+	if (IS_ERR(idata))
+		return PTR_ERR(idata);
+
+	md = mmc_blk_get(bdev->bd_disk);
+	if (!md) {
+		err = -EINVAL;
+		goto cmd_err;
+	}
+
+	card = md->queue.card;
+	if (IS_ERR(card)) {
+		err = PTR_ERR(card);
+		goto cmd_done;
+	}
+
+	mmc_claim_host(card->host);
+
+	err = __mmc_blk_ioctl_cmd(card, md, idata);
+
+	mmc_release_host(card->host);
+
+	err = mmc_blk_ioctl_copy_to_user(ic_ptr, idata);
 
 cmd_done:
 	mmc_blk_put(md);
@@ -618,13 +641,101 @@ cmd_err:
 	return err;
 }
 
+static int mmc_blk_ioctl_combo_cmd(struct block_device *bdev,
+				   struct mmc_ioc_combo_cmd __user *user)
+{
+	struct mmc_ioc_combo_cmd mcci = {0};
+	struct mmc_blk_ioc_data **ioc_data = NULL;
+	struct mmc_ioc_cmd __user *usr_ptr;
+	struct mmc_card *card;
+	struct mmc_blk_data *md;
+	int i, err = -EFAULT;
+	u8 n_cmds = 0;
+
+	/*
+	 * The caller must have CAP_SYS_RAWIO, and must be calling this on the
+	 * whole block device, not on a partition.  This prevents overspray
+	 * between sibling partitions.
+	 */
+	if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
+		return -EPERM;
+
+	if (copy_from_user(&mcci, user, sizeof(struct mmc_ioc_combo_cmd)))
+		return -EFAULT;
+
+	if (!mcci.num_of_cmds) {
+		err = -EINVAL;
+		goto cmd_err;
+	}
+
+	ioc_data = kcalloc(mcci.num_of_cmds, sizeof(*ioc_data), GFP_KERNEL);
+	if (!ioc_data) {
+		err = -ENOMEM;
+		goto cmd_err;
+	}
+
+	usr_ptr = (void * __user)mcci.mmc_ioc_cmd_list;
+	for (n_cmds = 0; n_cmds < mcci.num_of_cmds; n_cmds++) {
+		ioc_data[n_cmds] =
+			mmc_blk_ioctl_copy_from_user(&usr_ptr[n_cmds]);
+		if (IS_ERR(ioc_data[n_cmds])) {
+			err = PTR_ERR(ioc_data[n_cmds]);
+			goto cmd_err;
+		}
+	}
+
+	md = mmc_blk_get(bdev->bd_disk);
+	if (!md)
+		goto cmd_err;
+
+	card = md->queue.card;
+	if (IS_ERR(card)) {
+		err = PTR_ERR(card);
+		goto cmd_done;
+	}
+
+	mmc_claim_host(card->host);
+	for (i = 0; i < n_cmds; i++) {
+		err = __mmc_blk_ioctl_cmd(card, md, ioc_data[i]);
+		if (err) {
+			mmc_release_host(card->host);
+			goto cmd_done;
+		}
+	}
+
+	mmc_release_host(card->host);
+
+	/* copy to user if data and response */
+	for (i = 0; i < n_cmds; i++) {
+		err = mmc_blk_ioctl_copy_to_user(&usr_ptr[i], ioc_data[i]);
+		if (err)
+			break;
+	}
+
+cmd_done:
+	mmc_blk_put(md);
+cmd_err:
+	for (i = 0; i < n_cmds; i++) {
+		kfree(ioc_data[i]->buf);
+		kfree(ioc_data[i]);
+	}
+	kfree(ioc_data);
+	return err;
+}
+
 static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
 	unsigned int cmd, unsigned long arg)
 {
-	int ret = -EINVAL;
-	if (cmd == MMC_IOC_CMD)
-		ret = mmc_blk_ioctl_cmd(bdev, (struct mmc_ioc_cmd __user *)arg);
-	return ret;
+	switch (cmd) {
+	case MMC_COMBO_IOC_CMD:
+		return mmc_blk_ioctl_combo_cmd(bdev,
+				(struct mmc_ioc_combo_cmd __user *)arg);
+	case MMC_IOC_CMD:
+		return mmc_blk_ioctl_cmd(bdev,
+				(struct mmc_ioc_cmd __user *)arg);
+	default:
+		return -EINVAL;
+	}
 }
 
 #ifdef CONFIG_COMPAT
diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
index 1f5e68923929..5943e51f22b3 100644
--- a/include/uapi/linux/mmc/ioctl.h
+++ b/include/uapi/linux/mmc/ioctl.h
@@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
 };
 #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
 
-#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
+/**
+ * struct mmc_ioc_combo_cmd - combo command information
+ * @num_of_cmds: number of commands to send
+ * @mmc_ioc_cmd_list: pointer to list of commands
+ */
+struct mmc_ioc_combo_cmd {
+	uint8_t num_of_cmds;
+	struct mmc_ioc_cmd *mmc_ioc_cmd_list;
+};
 
+#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
+/*
+ * MMC_COMBO_IOC_CMD: Used to send an array of MMC commands described by
+ *	the structure mmc_ioc_combo_cmd. The MMC driver will issue all
+ *	commands in array in sequence to card.
+ */
+#define MMC_COMBO_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct mmc_ioc_combo_cmd)
 /*
  * Since this ioctl is only meant to enhance (and not replace) normal access
  * to the mmc bus device, an upper data transfer limit of MMC_IOC_MAX_BYTES
-- 
2.1.4

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Olof Johansson]

Hi,

On Wed, Sep 2, 2015 at 7:21 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> From: Seshagiri Holi <sholi@nvidia.com>
>
> Certain eMMC devices allow vendor specific device information to be read
> via a sequence of vendor commands. These vendor commands must be issued
> in sequence and an atomic fashion. One way to support this would be to
> add an ioctl function for sending a sequence of commands to the device
> atomically as proposed here. These combo commands are simple array of
> the existing mmc_ioc_cmd structure.

This seems reasonable to me, being able to do a sequence of
back-to-back operations without system read/writes slipping through.



> diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
> index 1f5e68923929..5943e51f22b3 100644
> --- a/include/uapi/linux/mmc/ioctl.h
> +++ b/include/uapi/linux/mmc/ioctl.h
> @@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
>  };
>  #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
>
> -#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
> +/**
> + * struct mmc_ioc_combo_cmd - combo command information
> + * @num_of_cmds: number of commands to send
> + * @mmc_ioc_cmd_list: pointer to list of commands
> + */
> +struct mmc_ioc_combo_cmd {
> +       uint8_t num_of_cmds;
> +       struct mmc_ioc_cmd *mmc_ioc_cmd_list;
> +};

The size of this struct will depend on the pointer size of userspace.

It might be better to use a u64 for the pointer instead. Otherwise
you'd need a compat ioctl wrapper to copy a 32-bit pointer over on a
64-bit kernel.



-Olof

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Grant Grundler]

[resending text-only]

On Wed, Sep 2, 2015 at 3:07 PM, Grant Grundler <grundler@google.com> wrote:
>
>
> On Wed, Sep 2, 2015 at 11:28 AM, Olof Johansson <olof@lixom.net> wrote:
> ...
>> > +/**
>> > + * struct mmc_ioc_combo_cmd - combo command information
>> > + * @num_of_cmds: number of commands to send
>> > + * @mmc_ioc_cmd_list: pointer to list of commands
>> > + */
>> > +struct mmc_ioc_combo_cmd {
>> > +       uint8_t num_of_cmds;
>> > +       struct mmc_ioc_cmd *mmc_ioc_cmd_list;
>> > +};
>>
>> The size of this struct will depend on the pointer size of userspace.
>>
>> It might be better to use a u64 for the pointer instead. Otherwise
>> you'd need a compat ioctl wrapper to copy a 32-bit pointer over on a
>> 64-bit kernel.
>
> If alignment matters, then maybe swap the fields?
> Or declare num_of_cmds as u64 as well?
>
> grant
>

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Jon Hunter]

Hi Olof,

On 02/09/15 19:28, Olof Johansson wrote:
> Hi,
> 
> On Wed, Sep 2, 2015 at 7:21 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>> From: Seshagiri Holi <sholi@nvidia.com>
>>
>> Certain eMMC devices allow vendor specific device information to be read
>> via a sequence of vendor commands. These vendor commands must be issued
>> in sequence and an atomic fashion. One way to support this would be to
>> add an ioctl function for sending a sequence of commands to the device
>> atomically as proposed here. These combo commands are simple array of
>> the existing mmc_ioc_cmd structure.
> 
> This seems reasonable to me, being able to do a sequence of
> back-to-back operations without system read/writes slipping through.
> 
>> diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
>> index 1f5e68923929..5943e51f22b3 100644
>> --- a/include/uapi/linux/mmc/ioctl.h
>> +++ b/include/uapi/linux/mmc/ioctl.h
>> @@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
>>  };
>>  #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
>>
>> -#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
>> +/**
>> + * struct mmc_ioc_combo_cmd - combo command information
>> + * @num_of_cmds: number of commands to send
>> + * @mmc_ioc_cmd_list: pointer to list of commands
>> + */
>> +struct mmc_ioc_combo_cmd {
>> +       uint8_t num_of_cmds;
>> +       struct mmc_ioc_cmd *mmc_ioc_cmd_list;
>> +};
> 
> The size of this struct will depend on the pointer size of userspace.
> 
> It might be better to use a u64 for the pointer instead. Otherwise
> you'd need a compat ioctl wrapper to copy a 32-bit pointer over on a
> 64-bit kernel.

Oh, good point. I have made this change today. I have also tested
this now with a hacked version of mmc-utils to send a couple status
commands back-to-back. I have tested on -next with ARM32 and on
ARM64 with an older 3.18 kernel (due to lack of mmc support in the
mainline for my ARM64 board) and seems to be working fine. Here is
an updated version ...

Jon

>From fe5849a0d91ebbcfd092c74696f3fa7d7de3a156 Mon Sep 17 00:00:00 2001
From: Seshagiri Holi <sholi@nvidia.com>
Date: Mon, 4 Nov 2013 17:27:43 +0530
Subject: [PATCH] mmc: block: Add new ioctl to send combo commands

Certain eMMC devices allow vendor specific device information to be read
via a sequence of vendor commands. These vendor commands must be issued
in sequence and an atomic fashion. One way to support this would be to
add an ioctl function for sending a sequence of commands to the device
atomically as proposed here. These combo commands are simple array of
the existing mmc_ioc_cmd structure.

Signed-off-by: Seshagiri Holi <sholi@nvidia.com>
[ jonathanh@nvidia.com: Rebased on linux-next from v3.18. Changed
  userspace pointer type for combo command to be a u64. Updated
  commit message ]
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/mmc/card/block.c       | 199 ++++++++++++++++++++++++++++++++---------
 include/uapi/linux/mmc/ioctl.h |  17 +++-
 2 files changed, 171 insertions(+), 45 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c742cfd7674e..0423d95ea020 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -387,6 +387,22 @@ out:
 	return ERR_PTR(err);
 }
 
+static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
+				      struct mmc_blk_ioc_data *idata)
+{
+	if (copy_to_user(&(ic_ptr->response), idata->ic.response,
+			 sizeof(idata->ic.response)))
+		return -EFAULT;
+
+	if (!idata->ic.write_flag) {
+		if (copy_to_user((void __user *)(unsigned long)idata->ic.data_ptr,
+				 idata->buf, idata->buf_bytes))
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
 static int ioctl_rpmb_card_status_poll(struct mmc_card *card, u32 *status,
 				       u32 retries_max)
 {
@@ -447,12 +463,9 @@ out:
 	return err;
 }
 
-static int mmc_blk_ioctl_cmd(struct block_device *bdev,
-	struct mmc_ioc_cmd __user *ic_ptr)
+static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
+			       struct mmc_blk_ioc_data *idata)
 {
-	struct mmc_blk_ioc_data *idata;
-	struct mmc_blk_data *md;
-	struct mmc_card *card;
 	struct mmc_command cmd = {0};
 	struct mmc_data data = {0};
 	struct mmc_request mrq = {NULL};
@@ -461,33 +474,12 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 	int is_rpmb = false;
 	u32 status = 0;
 
-	/*
-	 * The caller must have CAP_SYS_RAWIO, and must be calling this on the
-	 * whole block device, not on a partition.  This prevents overspray
-	 * between sibling partitions.
-	 */
-	if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
-		return -EPERM;
-
-	idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
-	if (IS_ERR(idata))
-		return PTR_ERR(idata);
-
-	md = mmc_blk_get(bdev->bd_disk);
-	if (!md) {
-		err = -EINVAL;
-		goto cmd_err;
-	}
+	if (!card || !md || !idata)
+		return -EINVAL;
 
 	if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
 		is_rpmb = true;
 
-	card = md->queue.card;
-	if (IS_ERR(card)) {
-		err = PTR_ERR(card);
-		goto cmd_done;
-	}
-
 	cmd.opcode = idata->ic.opcode;
 	cmd.arg = idata->ic.arg;
 	cmd.flags = idata->ic.flags;
@@ -582,18 +574,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 	if (idata->ic.postsleep_min_us)
 		usleep_range(idata->ic.postsleep_min_us, idata->ic.postsleep_max_us);
 
-	if (copy_to_user(&(ic_ptr->response), cmd.resp, sizeof(cmd.resp))) {
-		err = -EFAULT;
-		goto cmd_rel_host;
-	}
-
-	if (!idata->ic.write_flag) {
-		if (copy_to_user((void __user *)(unsigned long) idata->ic.data_ptr,
-						idata->buf, idata->buf_bytes)) {
-			err = -EFAULT;
-			goto cmd_rel_host;
-		}
-	}
+	memcpy(&(idata->ic.response), cmd.resp, sizeof(cmd.resp));
 
 	if (is_rpmb) {
 		/*
@@ -609,6 +590,48 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 
 cmd_rel_host:
 	mmc_put_card(card);
+	return err;
+}
+
+static int mmc_blk_ioctl_cmd(struct block_device *bdev,
+			struct mmc_ioc_cmd __user *ic_ptr)
+{
+	struct mmc_blk_ioc_data *idata;
+	struct mmc_blk_data *md;
+	struct mmc_card *card;
+	int err;
+
+	/*
+	 * The caller must have CAP_SYS_RAWIO, and must be calling this on the
+	 * whole block device, not on a partition.  This prevents overspray
+	 * between sibling partitions.
+	 */
+	if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
+		return -EPERM;
+
+	idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
+	if (IS_ERR(idata))
+		return PTR_ERR(idata);
+
+	md = mmc_blk_get(bdev->bd_disk);
+	if (!md) {
+		err = -EINVAL;
+		goto cmd_err;
+	}
+
+	card = md->queue.card;
+	if (IS_ERR(card)) {
+		err = PTR_ERR(card);
+		goto cmd_done;
+	}
+
+	mmc_claim_host(card->host);
+
+	err = __mmc_blk_ioctl_cmd(card, md, idata);
+
+	mmc_release_host(card->host);
+
+	err = mmc_blk_ioctl_copy_to_user(ic_ptr, idata);
 
 cmd_done:
 	mmc_blk_put(md);
@@ -618,13 +641,101 @@ cmd_err:
 	return err;
 }
 
+static int mmc_blk_ioctl_combo_cmd(struct block_device *bdev,
+				   struct mmc_ioc_combo_cmd __user *user)
+{
+	struct mmc_ioc_combo_cmd mcci = {0};
+	struct mmc_blk_ioc_data **ioc_data = NULL;
+	struct mmc_ioc_cmd __user *usr_ptr;
+	struct mmc_card *card;
+	struct mmc_blk_data *md;
+	int i, err = -EFAULT;
+	u8 n_cmds = 0;
+
+	/*
+	 * The caller must have CAP_SYS_RAWIO, and must be calling this on the
+	 * whole block device, not on a partition.  This prevents overspray
+	 * between sibling partitions.
+	 */
+	if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
+		return -EPERM;
+
+	if (copy_from_user(&mcci, user, sizeof(struct mmc_ioc_combo_cmd)))
+		return -EFAULT;
+
+	if (!mcci.num_of_cmds) {
+		err = -EINVAL;
+		goto cmd_err;
+	}
+
+	ioc_data = kcalloc(mcci.num_of_cmds, sizeof(*ioc_data), GFP_KERNEL);
+	if (!ioc_data) {
+		err = -ENOMEM;
+		goto cmd_err;
+	}
+
+	usr_ptr = (void * __user)(unsigned long)mcci.cmds_ptr;
+	for (n_cmds = 0; n_cmds < mcci.num_of_cmds; n_cmds++) {
+		ioc_data[n_cmds] =
+			mmc_blk_ioctl_copy_from_user(&usr_ptr[n_cmds]);
+		if (IS_ERR(ioc_data[n_cmds])) {
+			err = PTR_ERR(ioc_data[n_cmds]);
+			goto cmd_err;
+		}
+	}
+
+	md = mmc_blk_get(bdev->bd_disk);
+	if (!md)
+		goto cmd_err;
+
+	card = md->queue.card;
+	if (IS_ERR(card)) {
+		err = PTR_ERR(card);
+		goto cmd_done;
+	}
+
+	mmc_claim_host(card->host);
+	for (i = 0; i < n_cmds; i++) {
+		err = __mmc_blk_ioctl_cmd(card, md, ioc_data[i]);
+		if (err) {
+			mmc_release_host(card->host);
+			goto cmd_done;
+		}
+	}
+
+	mmc_release_host(card->host);
+
+	/* copy to user if data and response */
+	for (i = 0; i < n_cmds; i++) {
+		err = mmc_blk_ioctl_copy_to_user(&usr_ptr[i], ioc_data[i]);
+		if (err)
+			break;
+	}
+
+cmd_done:
+	mmc_blk_put(md);
+cmd_err:
+	for (i = 0; i < n_cmds; i++) {
+		kfree(ioc_data[i]->buf);
+		kfree(ioc_data[i]);
+	}
+	kfree(ioc_data);
+	return err;
+}
+
 static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
 	unsigned int cmd, unsigned long arg)
 {
-	int ret = -EINVAL;
-	if (cmd == MMC_IOC_CMD)
-		ret = mmc_blk_ioctl_cmd(bdev, (struct mmc_ioc_cmd __user *)arg);
-	return ret;
+	switch (cmd) {
+	case MMC_IOC_CMD:
+		return mmc_blk_ioctl_cmd(bdev,
+				(struct mmc_ioc_cmd __user *)arg);
+	case MMC_IOC_COMBO_CMD:
+		return mmc_blk_ioctl_combo_cmd(bdev,
+				(struct mmc_ioc_combo_cmd __user *)arg);
+	default:
+		return -EINVAL;
+	}
 }
 
 #ifdef CONFIG_COMPAT
diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
index 1f5e68923929..593e665177e2 100644
--- a/include/uapi/linux/mmc/ioctl.h
+++ b/include/uapi/linux/mmc/ioctl.h
@@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
 };
 #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
 
-#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
+/**
+ * struct mmc_ioc_combo_cmd - combo command information
+ * @cmds_ptr: Address of the location where the list of commands resides
+ * @num_of_cmds: number of commands to send
+ */
+struct mmc_ioc_combo_cmd {
+	__u64 cmds_ptr;
+	uint8_t num_of_cmds;
+};
 
+#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
+/*
+ * MMC_COMBO_IOC_CMD: Used to send an array of MMC commands described by
+ *	the structure mmc_ioc_combo_cmd. The MMC driver will issue all
+ *	commands in array in sequence to card.
+ */
+#define MMC_IOC_COMBO_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct mmc_ioc_combo_cmd)
 /*
  * Since this ioctl is only meant to enhance (and not replace) normal access
  * to the mmc bus device, an upper data transfer limit of MMC_IOC_MAX_BYTES
-- 
2.1.4

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Jon Hunter]

On 02/09/15 23:08, Grant Grundler wrote:
> [resending text-only]
> 
> On Wed, Sep 2, 2015 at 3:07 PM, Grant Grundler <grundler@google.com> wrote:
>>
>>
>> On Wed, Sep 2, 2015 at 11:28 AM, Olof Johansson <olof@lixom.net> wrote:
>> ...
>>>> +/**
>>>> + * struct mmc_ioc_combo_cmd - combo command information
>>>> + * @num_of_cmds: number of commands to send
>>>> + * @mmc_ioc_cmd_list: pointer to list of commands
>>>> + */
>>>> +struct mmc_ioc_combo_cmd {
>>>> +       uint8_t num_of_cmds;
>>>> +       struct mmc_ioc_cmd *mmc_ioc_cmd_list;
>>>> +};
>>>
>>> The size of this struct will depend on the pointer size of userspace.
>>>
>>> It might be better to use a u64 for the pointer instead. Otherwise
>>> you'd need a compat ioctl wrapper to copy a 32-bit pointer over on a
>>> 64-bit kernel.
>>
>> If alignment matters, then maybe swap the fields?
>> Or declare num_of_cmds as u64 as well?

Thanks. I did swap them in the updated version as this seems to make
sense. However, I left num_of_cmds as an 8-bit value unless someone has
a need for more than 256 commands ;-)

Jon

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Grant Grundler]

[resending...keep forgetting to switch back to text-only in gmail]

On Thu, Sep 3, 2015 at 8:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
...
>>> If alignment matters, then maybe swap the fields?
>>> Or declare num_of_cmds as u64 as well?
>
> Thanks. I did swap them in the updated version as this seems to make
> sense. However, I left num_of_cmds as an 8-bit value unless someone has
> a need for more than 256 commands ;-)

Awesome - thanks! I saw that in the new version you posted. LGTM.
You or Ulf can add "Reviewed by: Grant Grundler <grundler@chromium.org"

(I just switched the "reply-from:" on you ;)

Ulf, anything else stand out?

I normally don't like to rush but we'd at least like confirmation that
struct mmc_ioc_combo_cmd is acceptable as is (since it's the main part
of the API).

thanks,
grant

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Jon Hunter]

On 04/09/15 02:14, Grant Grundler wrote:
> On Thu, Sep 3, 2015 at 8:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> 
>>
>> On 02/09/15 23:08, Grant Grundler wrote:
>>> [resending text-only]
>>>
>>> On Wed, Sep 2, 2015 at 3:07 PM, Grant Grundler <grundler@google.com>
>> wrote:
>>>>
>>>>
>>>> On Wed, Sep 2, 2015 at 11:28 AM, Olof Johansson <olof@lixom.net> wrote:
>>>> ...
>>>>>> +/**
>>>>>> + * struct mmc_ioc_combo_cmd - combo command information
>>>>>> + * @num_of_cmds: number of commands to send
>>>>>> + * @mmc_ioc_cmd_list: pointer to list of commands
>>>>>> + */
>>>>>> +struct mmc_ioc_combo_cmd {
>>>>>> +       uint8_t num_of_cmds;
>>>>>> +       struct mmc_ioc_cmd *mmc_ioc_cmd_list;
>>>>>> +};
>>>>>
>>>>> The size of this struct will depend on the pointer size of userspace.
>>>>>
>>>>> It might be better to use a u64 for the pointer instead. Otherwise
>>>>> you'd need a compat ioctl wrapper to copy a 32-bit pointer over on a
>>>>> 64-bit kernel.
>>>>
>>>> If alignment matters, then maybe swap the fields?
>>>> Or declare num_of_cmds as u64 as well?
>>
>> Thanks. I did swap them in the updated version as this seems to make
>> sense. However, I left num_of_cmds as an 8-bit value unless someone has
>> a need for more than 256 commands ;-)
>>
> 
> Awesome - thanks! I saw that in the new version you posted. LGTM.
> You or Ulf can add my "Reviewed by: Grant Grundler <grundler@chromium.org"
> 
> (I just switched the "reply-from:" on you ;)
> 
> Ulf, anything else stand out?
> 
> I normally don't like to rush but we'd at least like confirmation that
> struct mmc_ioc_combo_cmd is acceptable as is (since it's the main part of
> the API).

Ulf,

Sorry to pester you, but we wanted to get some feedback on whether this
proposal is ok or not.

Cheers
Jon

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Ulf Hansson]

On 3 September 2015 at 17:08, Jon Hunter <jonathanh@nvidia.com> wrote:
> Hi Olof,
>
> On 02/09/15 19:28, Olof Johansson wrote:
>> Hi,
>>
>> On Wed, Sep 2, 2015 at 7:21 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> From: Seshagiri Holi <sholi@nvidia.com>
>>>
>>> Certain eMMC devices allow vendor specific device information to be read
>>> via a sequence of vendor commands. These vendor commands must be issued
>>> in sequence and an atomic fashion. One way to support this would be to
>>> add an ioctl function for sending a sequence of commands to the device
>>> atomically as proposed here. These combo commands are simple array of
>>> the existing mmc_ioc_cmd structure.
>>
>> This seems reasonable to me, being able to do a sequence of
>> back-to-back operations without system read/writes slipping through.

I agree, this change seems reasonable!

Perhaps, from clarification point of view, we should change from
naming it "combo" commands to *sequence* of commands. Both in the code
and in the change log, please.

>>
>>> diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
>>> index 1f5e68923929..5943e51f22b3 100644
>>> --- a/include/uapi/linux/mmc/ioctl.h
>>> +++ b/include/uapi/linux/mmc/ioctl.h
>>> @@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
>>>  };
>>>  #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
>>>
>>> -#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
>>> +/**
>>> + * struct mmc_ioc_combo_cmd - combo command information
>>> + * @num_of_cmds: number of commands to send
>>> + * @mmc_ioc_cmd_list: pointer to list of commands
>>> + */
>>> +struct mmc_ioc_combo_cmd {
>>> +       uint8_t num_of_cmds;
>>> +       struct mmc_ioc_cmd *mmc_ioc_cmd_list;
>>> +};
>>
>> The size of this struct will depend on the pointer size of userspace.
>>
>> It might be better to use a u64 for the pointer instead. Otherwise
>> you'd need a compat ioctl wrapper to copy a 32-bit pointer over on a
>> 64-bit kernel.
>
> Oh, good point. I have made this change today. I have also tested
> this now with a hacked version of mmc-utils to send a couple status
> commands back-to-back. I have tested on -next with ARM32 and on
> ARM64 with an older 3.18 kernel (due to lack of mmc support in the
> mainline for my ARM64 board) and seems to be working fine. Here is
> an updated version ...
>
> Jon
>
> From fe5849a0d91ebbcfd092c74696f3fa7d7de3a156 Mon Sep 17 00:00:00 2001
> From: Seshagiri Holi <sholi@nvidia.com>
> Date: Mon, 4 Nov 2013 17:27:43 +0530
> Subject: [PATCH] mmc: block: Add new ioctl to send combo commands
>
> Certain eMMC devices allow vendor specific device information to be read
> via a sequence of vendor commands. These vendor commands must be issued
> in sequence and an atomic fashion. One way to support this would be to
> add an ioctl function for sending a sequence of commands to the device
> atomically as proposed here. These combo commands are simple array of
> the existing mmc_ioc_cmd structure.
>
> Signed-off-by: Seshagiri Holi <sholi@nvidia.com>
> [ jonathanh@nvidia.com: Rebased on linux-next from v3.18. Changed
>   userspace pointer type for combo command to be a u64. Updated
>   commit message ]
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/mmc/card/block.c       | 199 ++++++++++++++++++++++++++++++++---------
>  include/uapi/linux/mmc/ioctl.h |  17 +++-
>  2 files changed, 171 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index c742cfd7674e..0423d95ea020 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -387,6 +387,22 @@ out:
>         return ERR_PTR(err);
>  }
>
> +static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
> +                                     struct mmc_blk_ioc_data *idata)
> +{
> +       if (copy_to_user(&(ic_ptr->response), idata->ic.response,
> +                        sizeof(idata->ic.response)))
> +               return -EFAULT;
> +
> +       if (!idata->ic.write_flag) {
> +               if (copy_to_user((void __user *)(unsigned long)idata->ic.data_ptr,
> +                                idata->buf, idata->buf_bytes))
> +                       return -EFAULT;
> +       }
> +
> +       return 0;
> +}
> +
>  static int ioctl_rpmb_card_status_poll(struct mmc_card *card, u32 *status,
>                                        u32 retries_max)
>  {
> @@ -447,12 +463,9 @@ out:
>         return err;
>  }
>
> -static int mmc_blk_ioctl_cmd(struct block_device *bdev,
> -       struct mmc_ioc_cmd __user *ic_ptr)
> +static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
> +                              struct mmc_blk_ioc_data *idata)
>  {
> -       struct mmc_blk_ioc_data *idata;
> -       struct mmc_blk_data *md;
> -       struct mmc_card *card;
>         struct mmc_command cmd = {0};
>         struct mmc_data data = {0};
>         struct mmc_request mrq = {NULL};
> @@ -461,33 +474,12 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>         int is_rpmb = false;
>         u32 status = 0;
>
> -       /*
> -        * The caller must have CAP_SYS_RAWIO, and must be calling this on the
> -        * whole block device, not on a partition.  This prevents overspray
> -        * between sibling partitions.
> -        */
> -       if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
> -               return -EPERM;
> -
> -       idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
> -       if (IS_ERR(idata))
> -               return PTR_ERR(idata);
> -
> -       md = mmc_blk_get(bdev->bd_disk);
> -       if (!md) {
> -               err = -EINVAL;
> -               goto cmd_err;
> -       }
> +       if (!card || !md || !idata)
> +               return -EINVAL;
>
>         if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
>                 is_rpmb = true;
>
> -       card = md->queue.card;
> -       if (IS_ERR(card)) {
> -               err = PTR_ERR(card);
> -               goto cmd_done;
> -       }
> -
>         cmd.opcode = idata->ic.opcode;
>         cmd.arg = idata->ic.arg;
>         cmd.flags = idata->ic.flags;
> @@ -582,18 +574,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>         if (idata->ic.postsleep_min_us)
>                 usleep_range(idata->ic.postsleep_min_us, idata->ic.postsleep_max_us);
>
> -       if (copy_to_user(&(ic_ptr->response), cmd.resp, sizeof(cmd.resp))) {
> -               err = -EFAULT;
> -               goto cmd_rel_host;
> -       }
> -
> -       if (!idata->ic.write_flag) {
> -               if (copy_to_user((void __user *)(unsigned long) idata->ic.data_ptr,
> -                                               idata->buf, idata->buf_bytes)) {
> -                       err = -EFAULT;
> -                       goto cmd_rel_host;
> -               }
> -       }
> +       memcpy(&(idata->ic.response), cmd.resp, sizeof(cmd.resp));
>
>         if (is_rpmb) {
>                 /*
> @@ -609,6 +590,48 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>
>  cmd_rel_host:
>         mmc_put_card(card);
> +       return err;
> +}
> +
> +static int mmc_blk_ioctl_cmd(struct block_device *bdev,
> +                       struct mmc_ioc_cmd __user *ic_ptr)
> +{
> +       struct mmc_blk_ioc_data *idata;
> +       struct mmc_blk_data *md;
> +       struct mmc_card *card;
> +       int err;
> +
> +       /*
> +        * The caller must have CAP_SYS_RAWIO, and must be calling this on the
> +        * whole block device, not on a partition.  This prevents overspray
> +        * between sibling partitions.
> +        */
> +       if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
> +               return -EPERM;
> +
> +       idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
> +       if (IS_ERR(idata))
> +               return PTR_ERR(idata);
> +
> +       md = mmc_blk_get(bdev->bd_disk);
> +       if (!md) {
> +               err = -EINVAL;
> +               goto cmd_err;
> +       }
> +
> +       card = md->queue.card;
> +       if (IS_ERR(card)) {
> +               err = PTR_ERR(card);
> +               goto cmd_done;
> +       }
> +
> +       mmc_claim_host(card->host);

As __mmc_blk_ioctl_cmd() already does mmc_get_card(), you don't need
mmc_claim_host() here.

> +
> +       err = __mmc_blk_ioctl_cmd(card, md, idata);
> +
> +       mmc_release_host(card->host);

As __mmc_blk_ioctl_cmd() already does mmc_put_card(), you don't need
mmc_release_host() here.

> +
> +       err = mmc_blk_ioctl_copy_to_user(ic_ptr, idata);
>
>  cmd_done:
>         mmc_blk_put(md);
> @@ -618,13 +641,101 @@ cmd_err:
>         return err;
>  }
>
> +static int mmc_blk_ioctl_combo_cmd(struct block_device *bdev,
> +                                  struct mmc_ioc_combo_cmd __user *user)
> +{
> +       struct mmc_ioc_combo_cmd mcci = {0};
> +       struct mmc_blk_ioc_data **ioc_data = NULL;
> +       struct mmc_ioc_cmd __user *usr_ptr;
> +       struct mmc_card *card;
> +       struct mmc_blk_data *md;
> +       int i, err = -EFAULT;
> +       u8 n_cmds = 0;
> +
> +       /*
> +        * The caller must have CAP_SYS_RAWIO, and must be calling this on the
> +        * whole block device, not on a partition.  This prevents overspray
> +        * between sibling partitions.
> +        */
> +       if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
> +               return -EPERM;
> +
> +       if (copy_from_user(&mcci, user, sizeof(struct mmc_ioc_combo_cmd)))
> +               return -EFAULT;
> +
> +       if (!mcci.num_of_cmds) {
> +               err = -EINVAL;
> +               goto cmd_err;
> +       }
> +
> +       ioc_data = kcalloc(mcci.num_of_cmds, sizeof(*ioc_data), GFP_KERNEL);
> +       if (!ioc_data) {
> +               err = -ENOMEM;
> +               goto cmd_err;
> +       }
> +
> +       usr_ptr = (void * __user)(unsigned long)mcci.cmds_ptr;
> +       for (n_cmds = 0; n_cmds < mcci.num_of_cmds; n_cmds++) {
> +               ioc_data[n_cmds] =
> +                       mmc_blk_ioctl_copy_from_user(&usr_ptr[n_cmds]);
> +               if (IS_ERR(ioc_data[n_cmds])) {
> +                       err = PTR_ERR(ioc_data[n_cmds]);
> +                       goto cmd_err;
> +               }
> +       }
> +
> +       md = mmc_blk_get(bdev->bd_disk);
> +       if (!md)
> +               goto cmd_err;
> +
> +       card = md->queue.card;
> +       if (IS_ERR(card)) {
> +               err = PTR_ERR(card);
> +               goto cmd_done;
> +       }
> +
> +       mmc_claim_host(card->host);

Change to mmc_get_card().

> +       for (i = 0; i < n_cmds; i++) {
> +               err = __mmc_blk_ioctl_cmd(card, md, ioc_data[i]);
> +               if (err) {
> +                       mmc_release_host(card->host);
> +                       goto cmd_done;
> +               }
> +       }
> +
> +       mmc_release_host(card->host);

Change to mmc_put_card().

> +
> +       /* copy to user if data and response */
> +       for (i = 0; i < n_cmds; i++) {
> +               err = mmc_blk_ioctl_copy_to_user(&usr_ptr[i], ioc_data[i]);
> +               if (err)
> +                       break;
> +       }
> +
> +cmd_done:
> +       mmc_blk_put(md);
> +cmd_err:
> +       for (i = 0; i < n_cmds; i++) {
> +               kfree(ioc_data[i]->buf);
> +               kfree(ioc_data[i]);
> +       }
> +       kfree(ioc_data);
> +       return err;
> +}
> +
>  static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
>         unsigned int cmd, unsigned long arg)
>  {
> -       int ret = -EINVAL;
> -       if (cmd == MMC_IOC_CMD)
> -               ret = mmc_blk_ioctl_cmd(bdev, (struct mmc_ioc_cmd __user *)arg);
> -       return ret;
> +       switch (cmd) {
> +       case MMC_IOC_CMD:
> +               return mmc_blk_ioctl_cmd(bdev,
> +                               (struct mmc_ioc_cmd __user *)arg);
> +       case MMC_IOC_COMBO_CMD:
> +               return mmc_blk_ioctl_combo_cmd(bdev,
> +                               (struct mmc_ioc_combo_cmd __user *)arg);
> +       default:
> +               return -EINVAL;
> +       }
>  }
>
>  #ifdef CONFIG_COMPAT

Don't forget to build with this configuration as well, I don't think
you have. :-)

> diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
> index 1f5e68923929..593e665177e2 100644
> --- a/include/uapi/linux/mmc/ioctl.h
> +++ b/include/uapi/linux/mmc/ioctl.h
> @@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
>  };
>  #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
>
> -#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
> +/**
> + * struct mmc_ioc_combo_cmd - combo command information
> + * @cmds_ptr: Address of the location where the list of commands resides
> + * @num_of_cmds: number of commands to send
> + */
> +struct mmc_ioc_combo_cmd {
> +       __u64 cmds_ptr;
> +       uint8_t num_of_cmds;
> +};
>
> +#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
> +/*
> + * MMC_COMBO_IOC_CMD: Used to send an array of MMC commands described by
> + *     the structure mmc_ioc_combo_cmd. The MMC driver will issue all
> + *     commands in array in sequence to card.
> + */
> +#define MMC_IOC_COMBO_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct mmc_ioc_combo_cmd)
>  /*
>   * Since this ioctl is only meant to enhance (and not replace) normal access
>   * to the mmc bus device, an upper data transfer limit of MMC_IOC_MAX_BYTES
> --
> 2.1.4
>

Kind regards
Uffe

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Jon Hunter]

Hi Ulf,

On 09/09/15 13:42, Ulf Hansson wrote:
> On 3 September 2015 at 17:08, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Hi Olof,
>>
>> On 02/09/15 19:28, Olof Johansson wrote:
>>> Hi,
>>>
>>> On Wed, Sep 2, 2015 at 7:21 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>> From: Seshagiri Holi <sholi@nvidia.com>
>>>>
>>>> Certain eMMC devices allow vendor specific device information to be read
>>>> via a sequence of vendor commands. These vendor commands must be issued
>>>> in sequence and an atomic fashion. One way to support this would be to
>>>> add an ioctl function for sending a sequence of commands to the device
>>>> atomically as proposed here. These combo commands are simple array of
>>>> the existing mmc_ioc_cmd structure.
>>>
>>> This seems reasonable to me, being able to do a sequence of
>>> back-to-back operations without system read/writes slipping through.
> 
> I agree, this change seems reasonable!
> 
> Perhaps, from clarification point of view, we should change from
> naming it "combo" commands to *sequence* of commands. Both in the code
> and in the change log, please.

No problem. Sorry to bike-shed, but do you prefer sequence or multi? I
was thinking multi could be shorter for the code.

>>>
>>>> diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
>>>> index 1f5e68923929..5943e51f22b3 100644
>>>> --- a/include/uapi/linux/mmc/ioctl.h
>>>> +++ b/include/uapi/linux/mmc/ioctl.h
>>>> @@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
>>>>  };
>>>>  #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
>>>>
>>>> -#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
>>>> +/**
>>>> + * struct mmc_ioc_combo_cmd - combo command information
>>>> + * @num_of_cmds: number of commands to send
>>>> + * @mmc_ioc_cmd_list: pointer to list of commands
>>>> + */
>>>> +struct mmc_ioc_combo_cmd {
>>>> +       uint8_t num_of_cmds;
>>>> +       struct mmc_ioc_cmd *mmc_ioc_cmd_list;
>>>> +};
>>>
>>> The size of this struct will depend on the pointer size of userspace.
>>>
>>> It might be better to use a u64 for the pointer instead. Otherwise
>>> you'd need a compat ioctl wrapper to copy a 32-bit pointer over on a
>>> 64-bit kernel.
>>
>> Oh, good point. I have made this change today. I have also tested
>> this now with a hacked version of mmc-utils to send a couple status
>> commands back-to-back. I have tested on -next with ARM32 and on
>> ARM64 with an older 3.18 kernel (due to lack of mmc support in the
>> mainline for my ARM64 board) and seems to be working fine. Here is
>> an updated version ...
>>
>> Jon
>>
>> From fe5849a0d91ebbcfd092c74696f3fa7d7de3a156 Mon Sep 17 00:00:00 2001
>> From: Seshagiri Holi <sholi@nvidia.com>
>> Date: Mon, 4 Nov 2013 17:27:43 +0530
>> Subject: [PATCH] mmc: block: Add new ioctl to send combo commands
>>
>> Certain eMMC devices allow vendor specific device information to be read
>> via a sequence of vendor commands. These vendor commands must be issued
>> in sequence and an atomic fashion. One way to support this would be to
>> add an ioctl function for sending a sequence of commands to the device
>> atomically as proposed here. These combo commands are simple array of
>> the existing mmc_ioc_cmd structure.
>>
>> Signed-off-by: Seshagiri Holi <sholi@nvidia.com>
>> [ jonathanh@nvidia.com: Rebased on linux-next from v3.18. Changed
>>   userspace pointer type for combo command to be a u64. Updated
>>   commit message ]
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/mmc/card/block.c       | 199 ++++++++++++++++++++++++++++++++---------
>>  include/uapi/linux/mmc/ioctl.h |  17 +++-
>>  2 files changed, 171 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index c742cfd7674e..0423d95ea020 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -387,6 +387,22 @@ out:
>>         return ERR_PTR(err);
>>  }
>>
>> +static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
>> +                                     struct mmc_blk_ioc_data *idata)
>> +{
>> +       if (copy_to_user(&(ic_ptr->response), idata->ic.response,
>> +                        sizeof(idata->ic.response)))
>> +               return -EFAULT;
>> +
>> +       if (!idata->ic.write_flag) {
>> +               if (copy_to_user((void __user *)(unsigned long)idata->ic.data_ptr,
>> +                                idata->buf, idata->buf_bytes))
>> +                       return -EFAULT;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static int ioctl_rpmb_card_status_poll(struct mmc_card *card, u32 *status,
>>                                        u32 retries_max)
>>  {
>> @@ -447,12 +463,9 @@ out:
>>         return err;
>>  }
>>
>> -static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>> -       struct mmc_ioc_cmd __user *ic_ptr)
>> +static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>> +                              struct mmc_blk_ioc_data *idata)
>>  {
>> -       struct mmc_blk_ioc_data *idata;
>> -       struct mmc_blk_data *md;
>> -       struct mmc_card *card;
>>         struct mmc_command cmd = {0};
>>         struct mmc_data data = {0};
>>         struct mmc_request mrq = {NULL};
>> @@ -461,33 +474,12 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>>         int is_rpmb = false;
>>         u32 status = 0;
>>
>> -       /*
>> -        * The caller must have CAP_SYS_RAWIO, and must be calling this on the
>> -        * whole block device, not on a partition.  This prevents overspray
>> -        * between sibling partitions.
>> -        */
>> -       if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
>> -               return -EPERM;
>> -
>> -       idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
>> -       if (IS_ERR(idata))
>> -               return PTR_ERR(idata);
>> -
>> -       md = mmc_blk_get(bdev->bd_disk);
>> -       if (!md) {
>> -               err = -EINVAL;
>> -               goto cmd_err;
>> -       }
>> +       if (!card || !md || !idata)
>> +               return -EINVAL;
>>
>>         if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
>>                 is_rpmb = true;
>>
>> -       card = md->queue.card;
>> -       if (IS_ERR(card)) {
>> -               err = PTR_ERR(card);
>> -               goto cmd_done;
>> -       }
>> -
>>         cmd.opcode = idata->ic.opcode;
>>         cmd.arg = idata->ic.arg;
>>         cmd.flags = idata->ic.flags;
>> @@ -582,18 +574,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>>         if (idata->ic.postsleep_min_us)
>>                 usleep_range(idata->ic.postsleep_min_us, idata->ic.postsleep_max_us);
>>
>> -       if (copy_to_user(&(ic_ptr->response), cmd.resp, sizeof(cmd.resp))) {
>> -               err = -EFAULT;
>> -               goto cmd_rel_host;
>> -       }
>> -
>> -       if (!idata->ic.write_flag) {
>> -               if (copy_to_user((void __user *)(unsigned long) idata->ic.data_ptr,
>> -                                               idata->buf, idata->buf_bytes)) {
>> -                       err = -EFAULT;
>> -                       goto cmd_rel_host;
>> -               }
>> -       }
>> +       memcpy(&(idata->ic.response), cmd.resp, sizeof(cmd.resp));
>>
>>         if (is_rpmb) {
>>                 /*
>> @@ -609,6 +590,48 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>>
>>  cmd_rel_host:
>>         mmc_put_card(card);
>> +       return err;
>> +}
>> +
>> +static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>> +                       struct mmc_ioc_cmd __user *ic_ptr)
>> +{
>> +       struct mmc_blk_ioc_data *idata;
>> +       struct mmc_blk_data *md;
>> +       struct mmc_card *card;
>> +       int err;
>> +
>> +       /*
>> +        * The caller must have CAP_SYS_RAWIO, and must be calling this on the
>> +        * whole block device, not on a partition.  This prevents overspray
>> +        * between sibling partitions.
>> +        */
>> +       if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
>> +               return -EPERM;
>> +
>> +       idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
>> +       if (IS_ERR(idata))
>> +               return PTR_ERR(idata);
>> +
>> +       md = mmc_blk_get(bdev->bd_disk);
>> +       if (!md) {
>> +               err = -EINVAL;
>> +               goto cmd_err;
>> +       }
>> +
>> +       card = md->queue.card;
>> +       if (IS_ERR(card)) {
>> +               err = PTR_ERR(card);
>> +               goto cmd_done;
>> +       }
>> +
>> +       mmc_claim_host(card->host);
> 
> As __mmc_blk_ioctl_cmd() already does mmc_get_card(), you don't need
> mmc_claim_host() here.

Ok.

>> +
>> +       err = __mmc_blk_ioctl_cmd(card, md, idata);
>> +
>> +       mmc_release_host(card->host);
> 
> As __mmc_blk_ioctl_cmd() already does mmc_put_card(), you don't need
> mmc_release_host() here.

Ok.

>> +
>> +       err = mmc_blk_ioctl_copy_to_user(ic_ptr, idata);
>>
>>  cmd_done:
>>         mmc_blk_put(md);
>> @@ -618,13 +641,101 @@ cmd_err:
>>         return err;
>>  }
>>
>> +static int mmc_blk_ioctl_combo_cmd(struct block_device *bdev,
>> +                                  struct mmc_ioc_combo_cmd __user *user)
>> +{
>> +       struct mmc_ioc_combo_cmd mcci = {0};
>> +       struct mmc_blk_ioc_data **ioc_data = NULL;
>> +       struct mmc_ioc_cmd __user *usr_ptr;
>> +       struct mmc_card *card;
>> +       struct mmc_blk_data *md;
>> +       int i, err = -EFAULT;
>> +       u8 n_cmds = 0;
>> +
>> +       /*
>> +        * The caller must have CAP_SYS_RAWIO, and must be calling this on the
>> +        * whole block device, not on a partition.  This prevents overspray
>> +        * between sibling partitions.
>> +        */
>> +       if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
>> +               return -EPERM;
>> +
>> +       if (copy_from_user(&mcci, user, sizeof(struct mmc_ioc_combo_cmd)))
>> +               return -EFAULT;
>> +
>> +       if (!mcci.num_of_cmds) {
>> +               err = -EINVAL;
>> +               goto cmd_err;
>> +       }
>> +
>> +       ioc_data = kcalloc(mcci.num_of_cmds, sizeof(*ioc_data), GFP_KERNEL);
>> +       if (!ioc_data) {
>> +               err = -ENOMEM;
>> +               goto cmd_err;
>> +       }
>> +
>> +       usr_ptr = (void * __user)(unsigned long)mcci.cmds_ptr;
>> +       for (n_cmds = 0; n_cmds < mcci.num_of_cmds; n_cmds++) {
>> +               ioc_data[n_cmds] =
>> +                       mmc_blk_ioctl_copy_from_user(&usr_ptr[n_cmds]);
>> +               if (IS_ERR(ioc_data[n_cmds])) {
>> +                       err = PTR_ERR(ioc_data[n_cmds]);
>> +                       goto cmd_err;
>> +               }
>> +       }
>> +
>> +       md = mmc_blk_get(bdev->bd_disk);
>> +       if (!md)
>> +               goto cmd_err;
>> +
>> +       card = md->queue.card;
>> +       if (IS_ERR(card)) {
>> +               err = PTR_ERR(card);
>> +               goto cmd_done;
>> +       }
>> +
>> +       mmc_claim_host(card->host);
> 
> Change to mmc_get_card().
> 
>> +       for (i = 0; i < n_cmds; i++) {
>> +               err = __mmc_blk_ioctl_cmd(card, md, ioc_data[i]);
>> +               if (err) {
>> +                       mmc_release_host(card->host);
>> +                       goto cmd_done;
>> +               }
>> +       }
>> +
>> +       mmc_release_host(card->host);
> 
> Change to mmc_put_card().
> 
>> +
>> +       /* copy to user if data and response */
>> +       for (i = 0; i < n_cmds; i++) {
>> +               err = mmc_blk_ioctl_copy_to_user(&usr_ptr[i], ioc_data[i]);
>> +               if (err)
>> +                       break;
>> +       }
>> +
>> +cmd_done:
>> +       mmc_blk_put(md);
>> +cmd_err:
>> +       for (i = 0; i < n_cmds; i++) {
>> +               kfree(ioc_data[i]->buf);
>> +               kfree(ioc_data[i]);
>> +       }
>> +       kfree(ioc_data);
>> +       return err;
>> +}
>> +
>>  static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
>>         unsigned int cmd, unsigned long arg)
>>  {
>> -       int ret = -EINVAL;
>> -       if (cmd == MMC_IOC_CMD)
>> -               ret = mmc_blk_ioctl_cmd(bdev, (struct mmc_ioc_cmd __user *)arg);
>> -       return ret;
>> +       switch (cmd) {
>> +       case MMC_IOC_CMD:
>> +               return mmc_blk_ioctl_cmd(bdev,
>> +                               (struct mmc_ioc_cmd __user *)arg);
>> +       case MMC_IOC_COMBO_CMD:
>> +               return mmc_blk_ioctl_combo_cmd(bdev,
>> +                               (struct mmc_ioc_combo_cmd __user *)arg);
>> +       default:
>> +               return -EINVAL;
>> +       }
>>  }
>>
>>  #ifdef CONFIG_COMPAT
> 
> Don't forget to build with this configuration as well, I don't think
> you have. :-)

Yes will do.

>> diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h
>> index 1f5e68923929..593e665177e2 100644
>> --- a/include/uapi/linux/mmc/ioctl.h
>> +++ b/include/uapi/linux/mmc/ioctl.h
>> @@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
>>  };
>>  #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
>>
>> -#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
>> +/**
>> + * struct mmc_ioc_combo_cmd - combo command information
>> + * @cmds_ptr: Address of the location where the list of commands resides
>> + * @num_of_cmds: number of commands to send
>> + */
>> +struct mmc_ioc_combo_cmd {
>> +       __u64 cmds_ptr;
>> +       uint8_t num_of_cmds;
>> +};
>>
>> +#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
>> +/*
>> + * MMC_COMBO_IOC_CMD: Used to send an array of MMC commands described by
>> + *     the structure mmc_ioc_combo_cmd. The MMC driver will issue all
>> + *     commands in array in sequence to card.
>> + */
>> +#define MMC_IOC_COMBO_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct mmc_ioc_combo_cmd)
>>  /*
>>   * Since this ioctl is only meant to enhance (and not replace) normal access
>>   * to the mmc bus device, an upper data transfer limit of MMC_IOC_MAX_BYTES
>> --
>> 2.1.4
>>
> 
> Kind regards
> Uffe

Cheers
Jon

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Jon Hunter]

Hi Ulf,

On 09/09/15 13:42, Ulf Hansson wrote:

[snip]

>> +static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>> +                       struct mmc_ioc_cmd __user *ic_ptr)
>> +{
>> +       struct mmc_blk_ioc_data *idata;
>> +       struct mmc_blk_data *md;
>> +       struct mmc_card *card;
>> +       int err;
>> +
>> +       /*
>> +        * The caller must have CAP_SYS_RAWIO, and must be calling this on the
>> +        * whole block device, not on a partition.  This prevents overspray
>> +        * between sibling partitions.
>> +        */
>> +       if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
>> +               return -EPERM;
>> +
>> +       idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
>> +       if (IS_ERR(idata))
>> +               return PTR_ERR(idata);
>> +
>> +       md = mmc_blk_get(bdev->bd_disk);
>> +       if (!md) {
>> +               err = -EINVAL;
>> +               goto cmd_err;
>> +       }
>> +
>> +       card = md->queue.card;
>> +       if (IS_ERR(card)) {
>> +               err = PTR_ERR(card);
>> +               goto cmd_done;
>> +       }
>> +
>> +       mmc_claim_host(card->host);
> 
> As __mmc_blk_ioctl_cmd() already does mmc_get_card(), you don't need
> mmc_claim_host() here.

Thinking about this some more, does it make sense to have a
mmc_get_card() above and then remove the one from __mmc_blk_ioctl_cmd()?
The mmc_blk_ioctl_multi_cmd() needs to call mmc_get_card() before
calling __mmc_blk_ioctl_cmd() and so currently we are calling
mmc_get_card() twice in the case of mmc_blk_ioctl_multi_cmd() which
seems unnecessary.

Cheers
Jon

Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands

[Ulf Hansson]

On 10 September 2015 at 10:43, Jon Hunter <jonathanh@nvidia.com> wrote:
> Hi Ulf,
>
> On 09/09/15 13:42, Ulf Hansson wrote:
>
> [snip]
>
>>> +static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>>> +                       struct mmc_ioc_cmd __user *ic_ptr)
>>> +{
>>> +       struct mmc_blk_ioc_data *idata;
>>> +       struct mmc_blk_data *md;
>>> +       struct mmc_card *card;
>>> +       int err;
>>> +
>>> +       /*
>>> +        * The caller must have CAP_SYS_RAWIO, and must be calling this on the
>>> +        * whole block device, not on a partition.  This prevents overspray
>>> +        * between sibling partitions.
>>> +        */
>>> +       if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
>>> +               return -EPERM;
>>> +
>>> +       idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
>>> +       if (IS_ERR(idata))
>>> +               return PTR_ERR(idata);
>>> +
>>> +       md = mmc_blk_get(bdev->bd_disk);
>>> +       if (!md) {
>>> +               err = -EINVAL;
>>> +               goto cmd_err;
>>> +       }
>>> +
>>> +       card = md->queue.card;
>>> +       if (IS_ERR(card)) {
>>> +               err = PTR_ERR(card);
>>> +               goto cmd_done;
>>> +       }
>>> +
>>> +       mmc_claim_host(card->host);
>>
>> As __mmc_blk_ioctl_cmd() already does mmc_get_card(), you don't need
>> mmc_claim_host() here.
>
> Thinking about this some more, does it make sense to have a
> mmc_get_card() above and then remove the one from __mmc_blk_ioctl_cmd()?
> The mmc_blk_ioctl_multi_cmd() needs to call mmc_get_card() before
> calling __mmc_blk_ioctl_cmd() and so currently we are calling
> mmc_get_card() twice in the case of mmc_blk_ioctl_multi_cmd() which
> seems unnecessary.

I agree.

We can follow your suggestion, but then we also need to add
mmc_get|put_card() in the case for non-multi commands.

Kind regards
Uffe