mmc: block: Add new ioctl to send combo commands

From: Seshagiri Holi <[email protected]>

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.

include/uapi/linux/mmc/ioctl.h +0 -0

index 1f5e68923929..5943e51f22b3 100644
@@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
 };
 #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
 

On Wed, Sep 2, 2015 at 11:28 AM, Olof Johansson <[email protected]> wrote:
...

Quoted 12 lines

include/uapi/linux/mmc/ioctl.h struct mmc_ioc_cmd { · line 56

Quoted 9 lines

include/uapi/linux/mmc/ioctl.h +9 -0

+/**
+ * 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.

Quoted 3 lines

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

[resending text-only]

Quoted 22 lines · Grant Grundler

On Wed, Sep 2, 2015 at 11:28 AM, Olof Johansson <[email protected]> wrote:
...

Quoted 12 lines

include/uapi/linux/mmc/ioctl.h struct mmc_ioc_cmd { · line 56

Quoted 9 lines

include/uapi/linux/mmc/ioctl.h +9 -0

+/**
+ * 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.

Quoted 3 lines

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?

Quoted 2 lines

Quoted 2 lines

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

Quoted 26 lines · Jon Hunter

Quoted 3 lines · Grant Grundler

[resending text-only]

On Wed, Sep 2, 2015 at 3:07 PM, Grant Grundler <[email protected]>

Quoted 20 lines · wrote:

Quoted 20 lines · Olof Johansson

...

Quoted 15 lines

include/uapi/linux/mmc/ioctl.h struct mmc_ioc_cmd { · line 56

Quoted 9 lines

include/uapi/linux/mmc/ioctl.h +9 -0

+/**
+ * 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.

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

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 <[email protected]"

(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).

Hi,

Quoted 8 lines · Jon Hunter

From: Seshagiri Holi <[email protected]>

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.

Quoted 8 lines

include/uapi/linux/mmc/ioctl.h +0 -0

index 1f5e68923929..5943e51f22b3 100644
@@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
 };
 #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
 

include/uapi/linux/mmc/ioctl.h struct mmc_ioc_cmd { · line 56

Quoted 10 lines

include/uapi/linux/mmc/ioctl.h +9 -1

-#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.

From fe5849a0d91ebbcfd092c74696f3fa7d7de3a156 Mon Sep 17 00:00:00 2001

Hi Olof,

Quoted 14 lines · Olof Johansson

Hi,

Quoted 8 lines · Jon Hunter

From: Seshagiri Holi <[email protected]>

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.

Quoted 22 lines

Quoted 9 lines

include/uapi/linux/mmc/ioctl.h +0 -0

index 1f5e68923929..5943e51f22b3 100644
@@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
 };
 #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
 

include/uapi/linux/mmc/ioctl.h struct mmc_ioc_cmd { · line 56

Quoted 10 lines

include/uapi/linux/mmc/ioctl.h +9 -1

-#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.

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

[ [email protected]: 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 <[email protected]>

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

drivers/mmc/card/block.c +47 -40

index c742cfd7674e..0423d95ea020 100644
@@ -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;

drivers/mmc/card/block.c +46 -0

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

drivers/mmc/card/block.c +22 -4

+
+       /* 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,

include/uapi/linux/mmc/ioctl.h +16 -2

index 1f5e68923929..593e665177e2 100644
@@ -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

Quoted 15 lines · Jon Hunter

Hi Olof,

Quoted 12 lines · Olof Johansson

Hi,

Quoted 8 lines · Jon Hunter

From: Seshagiri Holi <[email protected]>

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.

Quoted 2 lines

Quoted 2 lines

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.

Quoted 190 lines

Quoted 22 lines

Quoted 9 lines

include/uapi/linux/mmc/ioctl.h +0 -0

index 1f5e68923929..5943e51f22b3 100644
@@ -45,8 +45,23 @@ struct mmc_ioc_cmd {
 };
 #define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr
 

include/uapi/linux/mmc/ioctl.h struct mmc_ioc_cmd { · line 56

Quoted 10 lines

include/uapi/linux/mmc/ioctl.h +9 -1

-#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.

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

[ [email protected]: 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 <[email protected]>

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

drivers/mmc/card/block.c +52 -40

index c742cfd7674e..0423d95ea020 100644
@@ -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)) {

drivers/mmc/card/block.c static int mmc_blk_ioctl_cmd(struct block_device *bdev, · line 628

Quoted 5 lines

drivers/mmc/card/block.c +5 -0

+               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.

drivers/mmc/card/block.c static int mmc_blk_ioctl_cmd(struct block_device *bdev, · line 632

Quoted 4 lines

drivers/mmc/card/block.c +4 -0

+
+       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.

Quoted 53 lines

drivers/mmc/card/block.c +46 -0

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

drivers/mmc/card/block.c cmd_err: · line 697

Quoted 10 lines

drivers/mmc/card/block.c +10 -0

+       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().

drivers/mmc/card/block.c cmd_err: · line 706

Quoted 9 lines

drivers/mmc/card/block.c +9 -0

+       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().

Quoted 38 lines

drivers/mmc/card/block.c +29 -4

+
+       /* 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;
+       }
 }
 

drivers/mmc/card/block.c cmd_err: · line 741

Quoted 1 line

drivers/mmc/card/block.c +0 -0

 #ifdef CONFIG_COMPAT

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

Quoted 19 lines

include/uapi/linux/mmc/ioctl.h +9 -1

index 1f5e68923929..593e665177e2 100644
@@ -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;
+};
 

Quoted 11 lines

include/uapi/linux/mmc/ioctl.h +7 -1

+#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
--

Quoted 2 lines

2.1.4

Kind regards
Uffe

Quoted 28 lines

drivers/mmc/card/block.c +28 -0

+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)) {

drivers/mmc/card/block.c static int mmc_blk_ioctl_cmd(struct block_device *bdev, · line 628

Quoted 5 lines

drivers/mmc/card/block.c +5 -0

+               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.

Hi Ulf,

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

[snip]

Quoted 28 lines

Quoted 28 lines

drivers/mmc/card/block.c +28 -0

+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)) {

Quoted 8 lines

drivers/mmc/card/block.c static int mmc_blk_ioctl_cmd(struct block_device *bdev, · line 628

Quoted 5 lines

drivers/mmc/card/block.c +5 -0

+               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.