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

Commit Message

Jon Hunter Sept. 2, 2015, 2:21 p.m. UTC

From: Seshagiri Holi <[e-mail]>

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 <[e-mail]>
[ [e-mail]: Rebased on linux-next from v3.18 and updated
  commit message ]
Signed-off-by: Jon Hunter <[e-mail]>
---
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(-)

Comments

Olof Johansson Sept. 2, 2015, 6:28 p.m. UTC | #1

Hi,

On Wed, Sep 2, 2015 at 7:21 AM, Jon Hunter <[e-mail]> wrote:
> From: Seshagiri Holi <[e-mail]>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Grundler Sept. 2, 2015, 10:08 p.m. UTC | #2

[resending text-only]

On Wed, Sep 2, 2015 at 3:07 PM, Grant Grundler <[e-mail]> wrote:
>
>
> On Wed, Sep 2, 2015 at 11:28 AM, Olof Johansson <[e-mail]> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jon Hunter Sept. 3, 2015, 3:10 p.m. UTC | #3

On 02/09/15 23:08, Grant Grundler wrote:
> [resending text-only]
> 
> On Wed, Sep 2, 2015 at 3:07 PM, Grant Grundler <[e-mail]> wrote:
>>
>>
>> On Wed, Sep 2, 2015 at 11:28 AM, Olof Johansson <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Grundler Sept. 4, 2015, 1:16 a.m. UTC | #4

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

On Thu, Sep 3, 2015 at 8:10 AM, Jon Hunter <[e-mail]> 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 <[e-mail]"

(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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jon Hunter Sept. 8, 2015, 9:18 a.m. UTC | #5

On 04/09/15 02:14, Grant Grundler wrote:
> On Thu, Sep 3, 2015 at 8:10 AM, Jon Hunter <[e-mail]> wrote:
> 
>>
>> On 02/09/15 23:08, Grant Grundler wrote:
>>> [resending text-only]
>>>
>>> On Wed, Sep 2, 2015 at 3:07 PM, Grant Grundler <[e-mail]>
>> wrote:
>>>>
>>>>
>>>> On Wed, Sep 2, 2015 at 11:28 AM, Olof Johansson <[e-mail]> 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 <[e-mail]"
> 
> (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

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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