Re: [REVIEW] NVM Express driver

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 03 March 2011 21:47:49 Matthew Wilcox wrote:

> You may have seen the recent announcement of the NVM Express spec;
> here's the driver.  I've put a git tree up on kernel.org with
> all the history that's fit to be released (earlier versions
> were full of Intel codenames and are not suited to public
> consumption).  If you'd rather review the history, it's at
> http://git.kernel.org/?p=linux/kernel/git/willy/nvme.git;a=summary

Looks really nice overall, I only have a few comments about
the ioctl interface. As Greg already commented, it would be
good to reduce the number of ioctl commands to a minimum.

> +static int nvme_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
> +                                                       unsigned long arg)
> +{
> +       struct nvme_ns *ns = bdev->bd_disk->private_data;
> +
> +       switch (cmd) {
> +       case NVME_IOCTL_IDENTIFY_NS:
> +               return nvme_identify(ns, arg, 0);
> +       case NVME_IOCTL_IDENTIFY_CTRL:
> +               return nvme_identify(ns, arg, 1);
> +       case NVME_IOCTL_GET_RANGE_TYPE:
> +               return nvme_get_range_type(ns, arg);
> +       case NVME_IOCTL_SUBMIT_IO:
> +               return nvme_submit_io(ns, (void __user *)arg);
> +       case NVME_IOCTL_DOWNLOAD_FW:
> +               return nvme_download_firmware(ns, (void __user *)arg);
> +       case NVME_IOCTL_ACTIVATE_FW:
> +               return nvme_activate_firmware(ns, arg);
> +       default:
> +               return -ENOTTY;
> +       }
> +}

The argument is a pointer for all of these, as far as I can tell,
so you could do the typecast here once instead of doing it for
two of them.

> +static const struct block_device_operations nvme_fops = {
> +       .owner          = THIS_MODULE,
> +       .ioctl          = nvme_ioctl,
> +};

This is missing a .compat_ioctl pointer to work with 32 bit user
space. The data structures all seem compatible, so that should
be a trivial change.

> +#define NVME_IOCTL_IDENTIFY_NS _IOW('N', 0x40, struct nvme_id_ns)
> +#define NVME_IOCTL_IDENTIFY_CTRL _IOW('N', 0x41, struct nvme_id_ctrl)
> +#define NVME_IOCTL_GET_RANGE_TYPE _IOW('N', 0x42, struct nvme_lba_range_type)
> +#define NVME_IOCTL_SUBMIT_IO   _IOWR('N', 0x43, struct nvme_rw_command)
> +#define NVME_IOCTL_DOWNLOAD_FW _IOR('N', 0x44, struct nvme_dlfw)
> +#define NVME_IOCTL_ACTIVATE_FW _IO('N', 0x45)

I believe you have confused read and write here. IIRC, _IOW means
copy_from_user and _IOR means copy_to_user.

The code for NVME_IOCTL_SUBMIT_IO takes a nvme_user_io argument,
not nvme_rw_command.

You might want to have an upper bound on the amount of user data
that one can pass to SUBMIT_IO and others.

	Arnd