NVMe: Add a character device for each nvme device

Registers a character device for the nvme module and creates character files as /dev/nvmeN for each nvme device probed, where N is the device instance. The character devices support nvme admin ioctl commands so that nvme devices without namespaces can be managed.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/block/nvme.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 54 insertions(+), 1 deletions(-)

drivers/block/nvme.c +54 -1

index 7bcd882..8a16ac8 100644
@@ -20,6 +20,7 @@
 #include <linux/bio.h>
 #include <linux/bitops.h>
 #include <linux/blkdev.h>
+#include <linux/cdev.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/fs.h>

@@ -45,6 +46,7 @@
 #define SQ_SIZE(depth)		(depth * sizeof(struct nvme_command))
 #define CQ_SIZE(depth)		(depth * sizeof(struct nvme_completion))
 #define NVME_MINORS 64
+#define NVME_MAX_DEVS 1024
 #define NVME_IO_TIMEOUT	(5 * HZ)
 #define ADMIN_TIMEOUT	(60 * HZ)
 

@@ -54,9 +56,13 @@ module_param(nvme_major, int, 0);
 static int use_threaded_interrupts;
 module_param(use_threaded_interrupts, int, 0);
 
+static int nvme_char_major;
+module_param(nvme_char_major, int, 0);
+
 static DEFINE_SPINLOCK(dev_list_lock);
 static LIST_HEAD(dev_list);
 static struct task_struct *nvme_thread;
+static struct class *nvme_char_cl;
 
 /*
  * Represents an NVM Express device.  Each nvme_dev is a PCI function.

@@ -1222,6 +1228,35 @@ static const struct block_device_operations nvme_fops = {
 	.compat_ioctl	= nvme_ioctl,
 };
 
+static long nvme_char_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
+{
+	struct nvme_dev *dev;
+	int instance = iminor(f->f_dentry->d_inode);
+
+	spin_lock(&dev_list_lock);
+	list_for_each_entry(dev, &dev_list, node) {
+		if (dev->instance == instance)
+			break;
+	}
+	spin_unlock(&dev_list_lock);
+
+	if (&dev->node == &dev_list)
+		return -ENOTTY;
+	
+	switch (cmd) {
+	case NVME_IOCTL_ADMIN_CMD:
+		return nvme_user_admin_cmd(dev, (void __user *)arg);
+	default:
+		return -ENOTTY;
+	}
+}
+
+static const struct file_operations nvme_char_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= nvme_char_ioctl,
+	.compat_ioctl	= nvme_char_ioctl,
+};
+
 static void nvme_timeout_ios(struct nvme_queue *nvmeq)
 {
 	int depth = nvmeq->q_depth - 1;

@@ -1632,6 +1667,8 @@ static int __devinit nvme_probe(struct pci_dev *pdev,
 	if (result)
 		goto delete;
 
+	device_create(nvme_char_cl, NULL, MKDEV(nvme_char_major, dev->instance),
+						NULL, "nvme%d", dev->instance);
 	return 0;
 
  delete:

@@ -1660,6 +1697,7 @@ static void __devexit nvme_remove(struct pci_dev *pdev)
 {
 	struct nvme_dev *dev = pci_get_drvdata(pdev);
 	nvme_dev_remove(dev);
+	device_destroy(nvme_char_cl, MKDEV(nvme_char_major, dev->instance));
 	pci_disable_msix(pdev);
 	iounmap(dev->bar);
 	nvme_release_instance(dev);

@@ -1721,11 +1759,24 @@ static int __init nvme_init(void)
 	else if (result > 0)
 	    nvme_major = result;
 
+	result = __register_chrdev(nvme_char_major, 0, NVME_MAX_DEVS, "nvme",
+							&nvme_char_fops);
+	if (result < 0)
+		goto unregister_blkdev;
+	else if (result > 0)
+		nvme_char_major = result;
+	nvme_char_cl = class_create(THIS_MODULE, "nvme");
+	if (!nvme_char_cl)
+		goto unregister_chrdev;
 	result = pci_register_driver(&nvme_driver);
 	if (result)
-		goto unregister_blkdev;
+		goto destroy_class;
 	return 0;
 
+ destroy_class:
+	class_destroy(nvme_char_cl);
+ unregister_chrdev:
+	__unregister_chrdev(nvme_char_major, 0, NVME_MAX_DEVS, "nvme");
  unregister_blkdev:
 	unregister_blkdev(nvme_major, "nvme");
  kill_kthread:

@@ -1736,6 +1787,8 @@ static int __init nvme_init(void)
 static void __exit nvme_exit(void)
 {
 	pci_unregister_driver(&nvme_driver);
+	class_destroy(nvme_char_cl);
+	__unregister_chrdev(nvme_char_major, 0, NVME_MAX_DEVS, "nvme");
 	unregister_blkdev(nvme_major, "nvme");
 	kthread_stop(nvme_thread);
 }

Signature · 7 lines

1.7.0.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

Quoted 4 lines · Keith Busch

Registers a character device for the nvme module and creates character files as /dev/nvmeN for each nvme device probed, where N is the device instance. The character devices support nvme admin ioctl commands so that nvme devices without namespaces can be managed.

I don't see a problem here, but I'm no expert at sysfs / character devices. Alan, Greg, anyone else see any problems with how this character device is created / destroyed?

drivers/block/nvme.c static int __devinit nvme_probe(struct pci_dev *pdev, · line 1671

Quoted 2 lines

drivers/block/nvme.c +2 -0

+	device_create(nvme_char_cl, NULL, MKDEV(nvme_char_major, dev->instance),
+						NULL, "nvme%d", dev->instance);

Quoted 3 lines · Matthew Wilcox

I don't see a problem here, but I'm no expert at sysfs / character devices. Alan, Greg, anyone else see any problems with how this character device is created / destroyed?

Yes, see below:

Quoted 2 lines

drivers/block/nvme.c static int __devinit nvme_probe(struct pci_dev *pdev, · line 1671

Quoted 2 lines

drivers/block/nvme.c +2 -0

+	device_create(nvme_char_cl, NULL, MKDEV(nvme_char_major, dev->instance),
+						NULL, "nvme%d", dev->instance);

You just created a device at the "root" of sysfs, which is wrong, especially when you do have a parent device here. Please use it.

Also, why are you creating your own class? Can't this just be a misc device? And if you want to create your own class, please don't, use a bus, as that is what is really happening here, right? We are trying to move away from using 'struct class' wherever possible (one of these days we'll just remove it...)

Quoted 6 lines · Greg KH

Quoted 3 lines · Matthew Wilcox

I don't see a problem here, but I'm no expert at sysfs / character devices. Alan, Greg, anyone else see any problems with how this character device is created / destroyed?

Yes, see below:

Thanks!

Quoted 5 lines

Quoted 2 lines

drivers/block/nvme.c static int __devinit nvme_probe(struct pci_dev *pdev, · line 1671

Quoted 2 lines

drivers/block/nvme.c +2 -0

+	device_create(nvme_char_cl, NULL, MKDEV(nvme_char_major, dev->instance),
+						NULL, "nvme%d", dev->instance);

You just created a device at the "root" of sysfs, which is wrong, especially when you do have a parent device here. Please use it.

OK, that makes sense; this device should be the child of the pci_dev that it belongs to.

Quoted 5 lines

Also, why are you creating your own class? Can't this just be a misc device? And if you want to create your own class, please don't, use a bus, as that is what is really happening here, right? We are trying to move away from using 'struct class' wherever possible (one of these days we'll just remove it...)

What we're trying to achieve here is to create one character device per NVMe controller that gets plugged in. Each NVMe controller is-a PCI function. The reason we're trying to do this is so that we can send commands to the NVMe controller, even when there is no storage present (eg a drive is shipped from the factory with no configured storage).

So we have no particular desire to create a new struct class, or struct bus. If we can create a misc device per PCIe function that's bound to our driver, that's great! Can you recommend a driver that does this already?

Quoted 12 lines

Quoted 5 lines

Also, why are you creating your own class? Can't this just be a misc device? And if you want to create your own class, please don't, use a bus, as that is what is really happening here, right? We are trying to move away from using 'struct class' wherever possible (one of these days we'll just remove it...)

What we're trying to achieve here is to create one character device per NVMe controller that gets plugged in. Each NVMe controller is-a PCI function. The reason we're trying to do this is so that we can send commands to the NVMe controller, even when there is no storage present (eg a drive is shipped from the factory with no configured storage).

Quoted 3 lines

So we have no particular desire to create a new struct class, or struct bus. If we can create a misc device per PCIe function that's bound to our driver, that's great! Can you recommend a driver that does this already?

I don't think there is one, but it shouldn't be that hard to just create a 'struct misdevice' for each one of the devices you want to create, would it?

But, as you really are a "specific type", a bus_type might be overkill, so the original use of device_create() should be fine. Just be sure to fix the parent pointer issue, and you should be fine, right?

Quoted 4 lines · Keith Busch

Registers a character device for the nvme module and creates character files as /dev/nvmeN for each nvme device probed, where N is the device instance. The character devices support nvme admin ioctl commands so that nvme devices without namespaces can be managed.

I don't see a problem here, but I'm no expert at sysfs / character devices. Alan, Greg, anyone else see any problems with how this character device is created / destroyed?

Quoted 6 lines · Matthew Wilcox

Quoted 4 lines · Keith Busch

Registers a character device for the nvme module and creates character files as /dev/nvmeN for each nvme device probed, where N is the device instance. The character devices support nvme admin ioctl commands so that nvme devices without namespaces can be managed.

Quoted 3 lines

I don't see a problem here, but I'm no expert at sysfs / character devices. Alan, Greg, anyone else see any problems with how this character device is created / destroyed?

This seems like something normally done via a control device that is addressible via bsg.

This is -not- a NAK, but maybe the storage folks have a different preference for an admin-command path.

Jeff

Maybe we should design such a mechanism, but maybe we shouldn't ... as we find common things to do, we tend to move those to sysfs, not ioctls, and the kinds of commands that are being sent here are essentially vendor-specific NVMe commands; it's not clear they'd fit neatly into a generic mechanism.

drivers/block/nvme.c +1 -0

@@ -1222,6 +1228,35 @@ static const struct block_device_operations nvme_fops = {
 	.compat_ioctl	= nvme_ioctl,
 };
 
+static long nvme_char_ioctl(struct file *f, unsigned int cmd, unsigned long arg)

drivers/block/nvme.c +10 -0

+	if (&dev->node == &dev_list)
+		return -ENOTTY;
+	
+	switch (cmd) {
+	case NVME_IOCTL_ADMIN_CMD:
+		return nvme_user_admin_cmd(dev, (void __user *)arg);
+	default:
+		return -ENOTTY;
+	}
+}