Re: [PATCH V3] mmc: block: Add new ioctl to send multi commands

[Jon Hunter]

On 21/09/15 10:56, Jon Hunter wrote:
> 
> On 16/09/15 18:54, Gwendal Grignou wrote:
> 
> [snip]
> 
>>> +static int mmc_blk_ioctl_multi_cmd(struct block_device *bdev,
>>> +                                  struct mmc_ioc_multi_cmd __user *user)
>>> +{
>>> +       struct mmc_blk_ioc_data **idata = NULL;
>>> +       struct mmc_ioc_cmd __user *cmds = user->cmds;
>>> +       struct mmc_card *card;
>>> +       struct mmc_blk_data *md;
>>> +       int i, err = -EFAULT;
>>> +       __u64 num_of_cmds;
>>> +
>>> +       /*
>>> +        * 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(&num_of_cmds, &user->num_of_cmds,
>>> +                          sizeof(num_of_cmds)))
>>> +               return -EFAULT;
>>> +
>>> +       if (num_of_cmds > MMC_IOC_MAX_CMDS)
>>> +               return -EINVAL;
>>> +
>>> +       idata = kcalloc(num_of_cmds, sizeof(*idata), GFP_KERNEL);
>>> +       if (!idata)
>>> +               return -ENOMEM;
>>> +
>>> +       for (i = 0; i < num_of_cmds; i++) {
>>> +               idata[i] = mmc_blk_ioctl_copy_from_user(&cmds[i]);
>>> +               if (IS_ERR(idata[i])) {
>>> +                       err = PTR_ERR(idata[i]);
>>> +                       num_of_cmds = i;
>>> +                       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_get_card(card);
>>> +
>>> +       for (i = 0; i < num_of_cmds; i++) {
>>> +               err = __mmc_blk_ioctl_cmd(card, md, idata[i]);
>>> +               if (err) {
>>> +                       mmc_put_card(card);
>>> +                       goto cmd_done;
>> Instead of exiting here, you should first copy to the user the data
>> and response of successful commands, mark the failed command as failed
>> and the remaining ones as "not executed".
>> This way, it will be easier for the user space application to find out
>> where the sequence failed. This especially true if some reverts are
>> needed.
> 
> Yes that sounds like a sensible thing to do. I will incorporate that change.

At first, I thought that may be the response field of the command could
be used to indicate the failed command. However, thinking about this
some more, I am not sure that it seems correct to use this field as this
is really used to carry the MMC response as defined by the MMC
specification.

Should the response field always be non-zero for a successful command?
If this is guaranteed, then may be the best thing to do would be to have
user-space clear the response field to field before submitting the
commands. It would then be easy to detect which command failed and which
were not attempted.

Ulf, what are your thoughts?

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