Re: [PATCH] NVMe: Add a character device for each nvme device

Re: [PATCH] NVMe: Add a character device for each nvme device

[Matthew Wilcox]

On Fri, Jul 27, 2012 at 10:44:18AM -0600, Keith Busch wrote:
> 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?

> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/block/nvme.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 54 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/nvme.c b/drivers/block/nvme.c
> index 7bcd882..8a16ac8 100644
> --- a/drivers/block/nvme.c
> +++ b/drivers/block/nvme.c
> @@ -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);
>  }
> -- 
> 1.7.0.4
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://merlin.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] NVMe: Add a character device for each nvme device

[Greg KH]

On Fri, Jul 27, 2012 at 02:12:12PM -0400, Matthew Wilcox wrote:
> On Fri, Jul 27, 2012 at 10:44:18AM -0600, Keith Busch wrote:
> > 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?

Yes, see below:

> > +	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...)

thanks,

greg k-h

Re: [PATCH] NVMe: Add a character device for each nvme device

[Matthew Wilcox]

On Fri, Jul 27, 2012 at 11:25:46AM -0700, Greg KH wrote:
> On Fri, Jul 27, 2012 at 02:12:12PM -0400, Matthew Wilcox wrote:
> > 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!

> > > +	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.

> 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?

Re: [PATCH] NVMe: Add a character device for each nvme device

[Greg KH]

On Fri, Jul 27, 2012 at 03:08:21PM -0400, Matthew Wilcox wrote:
> On Fri, Jul 27, 2012 at 11:25:46AM -0700, Greg KH wrote:
> > On Fri, Jul 27, 2012 at 02:12:12PM -0400, Matthew Wilcox wrote:
> > > 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!
> 
> > > > +	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.
> 
> > 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?

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?

greg k-h

Re: [PATCH] NVMe: Add a character device for each nvme device

[Matthew Wilcox]

On Fri, Jul 27, 2012 at 12:21:00PM -0700, Greg KH wrote:
> > > 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?
> 
> 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?

Works for me.  I don't think we're going to have any software that
depends on it being a class or a bus, so it's easy to change later.
All we really care about is /dev/nvmeN being created.

Re: [PATCH] NVMe: Add a character device for each nvme device

[Jeff Garzik]

On 07/27/2012 02:12 PM, Matthew Wilcox wrote:
> On Fri, Jul 27, 2012 at 10:44:18AM -0600, Keith Busch wrote:
>> 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?

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

Re: [PATCH] NVMe: Add a character device for each nvme device

[Matthew Wilcox]

On Fri, Jul 27, 2012 at 03:28:25PM -0400, Jeff Garzik wrote:
> On 07/27/2012 02:12 PM, Matthew Wilcox wrote:
> >On Fri, Jul 27, 2012 at 10:44:18AM -0600, Keith Busch wrote:
> >>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?
> 
> This seems like something normally done via a control device that is
> addressible via bsg.

I'm not convinced about that.  bsg requires a request_queue, and we
don't have one in the absence of any storage.  There doesn't even seem
to be a standard way of sending commands to SCSI hosts, let alone block
device controllers.

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.

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

Re: [PATCH] NVMe: Add a character device for each nvme device

[Jeff Garzik]

On 07/27/2012 04:26 PM, Matthew Wilcox wrote:
> 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.

You're delivering arbitrary packets to the device from userspace, and it 
is returning arbitrary packets to userspace.

This is a familiar pattern...  It is quite analagous to "send 
vendor-specific commands from userspace to a drive"

	Jeff