Re: [REVIEW] NVM Express driver

[Greg KH <greg@kroah.com>]

On Thu, Mar 03, 2011 at 03:47:49PM -0500, Matthew Wilcox wrote:
> 
> Hi everyone,
> 
> 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
> 
> Jens has already offered to pull my nvme tree into his block tree, so
> that's the route I intend to take into Linus' tree.  Comments appreciated,
> however in a masterpiece of bad timing I am going to be travelling for
> the next week, so please bear with me if I don't respond quickly.
> 
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index 63ffd78..f8159ba 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -148,6 +148,7 @@ Code  Seq#(hex)	Include File		Comments
>  'M'	01-03	drivers/scsi/megaraid/megaraid_sas.h
>  'M'	00-0F	drivers/video/fsl-diu-fb.h	conflict!
>  'N'	00-1F	drivers/usb/scanner.h
> +'N'	40-7F	drivers/block/nvme.c

I hate to ask this, but why do you have ioctls for this?  At first
glance, a number of the ioctls you have should just be sysfs files to
export the information.  What am I misunderstanding here?

> +static int nvme_download_firmware(struct nvme_ns *ns,
> +						struct nvme_dlfw __user *udlfw)
> +{
> +	struct nvme_dev *dev = ns->dev;
> +	struct nvme_dlfw dlfw;
> +	struct nvme_command c;
> +	int nents, status;
> +	struct scatterlist *sg;
> +	struct nvme_prps *prps;
> +
> +	if (copy_from_user(&dlfw, udlfw, sizeof(dlfw)))
> +		return -EFAULT;
> +	if (dlfw.length >= (1 << 30))
> +		return -EINVAL;
> +
> +	nents = nvme_map_user_pages(dev, 1, dlfw.addr, dlfw.length * 4, &sg);
> +	if (nents < 0)
> +		return nents;
> +
> +	memset(&c, 0, sizeof(c));
> +	c.dlfw.opcode = nvme_admin_download_fw;
> +	c.dlfw.numd = cpu_to_le32(dlfw.length);
> +	c.dlfw.offset = cpu_to_le32(dlfw.offset);
> +	prps = nvme_setup_prps(dev, &c.common, sg, dlfw.length * 4);
> +
> +	status = nvme_submit_admin_cmd(dev, &c, NULL);
> +	nvme_unmap_user_pages(dev, 0, dlfw.addr, dlfw.length * 4, sg, nents);
> +	nvme_free_prps(dev, prps);
> +	return status;
> +}

Shouldn't you be using the build-in firmware kernel interface instead of
rolling your own in an ioctl?

thanks,

greg k-h