Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221404 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands Ulf Hansson <ulf.hansson@linaro.org> - 2015-09-09 15:00 +0200
    Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands Jon Hunter <jonathanh@nvidia.com> - 2015-09-09 16:00 +0200
    Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands Jon Hunter <jonathanh@nvidia.com> - 2015-09-10 10:50 +0200
      Re: [RFC PATCH] mmc: block: Add new ioctl to send combo commands Ulf Hansson <ulf.hansson@linaro.org> - 2015-09-10 12:20 +0200

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221479

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222025

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222081

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web