Re: [PATCH 4/4] nvme: enable char device per namespace

[Minwoo Im <minwoo.im.dev@gmail.com>]

Hello,

On 20-12-01 13:56:10, javier@javigon.com wrote:
> From: Javier González <javier.gonz@samsung.com>
> 
> Create a char device per NVMe namespace. This char device is always
> initialized, independently of whether thedeatures implemented by the
> device are supported by the kernel. User-space can therefore always
> issue IOCTLs to the NVMe driver using this char device.
> 
> The char device is presented as /dev/nvmeXcYnZ to follow the hidden
> block device. This naming also aligns with nvme-cli filters, so the char
> device should be usable without tool changes.
> 
> Signed-off-by: Javier González <javier.gonz@samsung.com>
> ---
>  drivers/nvme/host/core.c | 144 +++++++++++++++++++++++++++++++++++----
>  drivers/nvme/host/nvme.h |   3 +
>  2 files changed, 132 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2c23ea6dc296..9c4acf2725f3 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -86,7 +86,9 @@ static DEFINE_MUTEX(nvme_subsystems_lock);
>  
>  static DEFINE_IDA(nvme_instance_ida);
>  static dev_t nvme_ctrl_base_chr_devt;
> +static dev_t nvme_ns_base_chr_devt;
>  static struct class *nvme_class;
> +static struct class *nvme_ns_class;
>  static struct class *nvme_subsys_class;
>  
>  static void nvme_put_subsystem(struct nvme_subsystem *subsys);
> @@ -497,6 +499,7 @@ static void nvme_free_ns(struct kref *kref)
>  	if (ns->ndev)
>  		nvme_nvm_unregister(ns);
>  
> +	cdev_device_del(&ns->cdev, &ns->cdev_device);
>  	put_disk(ns->disk);
>  	nvme_put_ns_head(ns->head);
>  	nvme_put_ctrl(ns->ctrl);
> @@ -1696,15 +1699,15 @@ static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
>  	return ret;
>  }
>  
> -static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
> -		unsigned int cmd, unsigned long arg)
> +static int __nvme_ns_ioctl(struct gendisk *disk, unsigned int cmd,
> +			   unsigned long arg)
>  {
>  	struct nvme_ns_head *head = NULL;
>  	void __user *argp = (void __user *)arg;
>  	struct nvme_ns *ns;
>  	int srcu_idx, ret;
>  
> -	ns = nvme_get_ns_from_disk(bdev->bd_disk, &head, &srcu_idx);
> +	ns = nvme_get_ns_from_disk(disk, &head, &srcu_idx);
>  	if (unlikely(!ns))
>  		return -EWOULDBLOCK;
>  
> @@ -1741,6 +1744,18 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>  	return ret;
>  }
>  
> +static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
> +		      unsigned int cmd, unsigned long arg)
> +{
> +	return __nvme_ns_ioctl(bdev->bd_disk, cmd, arg);
> +}
> +
> +static long nvme_cdev_ioctl(struct file *file, unsigned int cmd,
> +			    unsigned long arg)
> +{
> +	return __nvme_ns_ioctl((struct gendisk *)file->private_data, cmd, arg);
> +}
> +
>  #ifdef CONFIG_COMPAT
>  struct nvme_user_io32 {
>  	__u8	opcode;
> @@ -1782,10 +1797,8 @@ static int nvme_compat_ioctl(struct block_device *bdev, fmode_t mode,
>  #define nvme_compat_ioctl	NULL
>  #endif /* CONFIG_COMPAT */
>  
> -static int nvme_open(struct block_device *bdev, fmode_t mode)
> +static int __nvme_open(struct nvme_ns *ns)
>  {
> -	struct nvme_ns *ns = bdev->bd_disk->private_data;
> -
>  #ifdef CONFIG_NVME_MULTIPATH
>  	/* should never be called due to GENHD_FL_HIDDEN */
>  	if (WARN_ON_ONCE(ns->head->disk))
> @@ -1804,12 +1817,24 @@ static int nvme_open(struct block_device *bdev, fmode_t mode)
>  	return -ENXIO;
>  }
>  
> +static void __nvme_release(struct nvme_ns *ns)
> +{
> +	module_put(ns->ctrl->ops->module);
> +	nvme_put_ns(ns);
> +}
> +
> +static int nvme_open(struct block_device *bdev, fmode_t mode)
> +{
> +	struct nvme_ns *ns = bdev->bd_disk->private_data;
> +
> +	return __nvme_open(ns);
> +}
> +
>  static void nvme_release(struct gendisk *disk, fmode_t mode)
>  {
>  	struct nvme_ns *ns = disk->private_data;
>  
> -	module_put(ns->ctrl->ops->module);
> -	nvme_put_ns(ns);
> +	__nvme_release(ns);
>  }
>  
>  static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> @@ -1821,6 +1846,26 @@ static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>  	return 0;
>  }
>  
> +static int nvme_cdev_open(struct inode *inode, struct file *file)
> +{
> +	struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
> +	int ret;
> +
> +	ret = __nvme_open(ns);
> +	if (!ret)
> +		file->private_data = ns->disk;
> +
> +	return ret;
> +}
> +
> +static int nvme_cdev_release(struct inode *inode, struct file *file)
> +{
> +	struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
> +
> +	__nvme_release(ns);
> +	return 0;
> +}
> +
>  #ifdef CONFIG_BLK_DEV_INTEGRITY
>  static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type,
>  				u32 max_integrity_segments)
> @@ -2303,6 +2348,14 @@ static const struct block_device_operations nvme_bdev_ops = {
>  	.pr_ops		= &nvme_pr_ops,
>  };
>  
> +static const struct file_operations nvme_cdev_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= nvme_cdev_open,
> +	.release	= nvme_cdev_release,
> +	.unlocked_ioctl	= nvme_cdev_ioctl,
> +	.compat_ioctl	= compat_ptr_ioctl,
> +};
> +
>  #ifdef CONFIG_NVME_MULTIPATH
>  static int nvme_ns_head_open(struct block_device *bdev, fmode_t mode)
>  {
> @@ -3301,6 +3354,9 @@ static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
>  {
>  	struct gendisk *disk = dev_to_disk(dev);
>  
> +	if (dev->class == nvme_ns_class)
> +		return ((struct nvme_ns *)dev_get_drvdata(dev))->head;

I think it would be better if it can have inline function
nvme_get_ns_from_cdev() just like nvme_get_ns_from_dev().

> +
>  	if (disk->fops == &nvme_bdev_ops)
>  		return nvme_get_ns_from_dev(dev)->head;
>  	else
> @@ -3390,7 +3446,7 @@ static struct attribute *nvme_ns_id_attrs[] = {
>  };
>  
>  static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
> -		struct attribute *a, int n)
> +	       struct attribute *a, int n)

Unrelated changes for this patch.

>  {
>  	struct device *dev = container_of(kobj, struct device, kobj);
>  	struct nvme_ns_ids *ids = &dev_to_ns_head(dev)->ids;
> @@ -3432,6 +3488,11 @@ const struct attribute_group *nvme_ns_id_attr_groups[] = {
>  	NULL,
>  };
>  
> +const struct attribute_group *nvme_ns_char_id_attr_groups[] = {
> +	&nvme_ns_id_attr_group,
> +	NULL,
> +};
> +
>  #define nvme_show_str_function(field)						\
>  static ssize_t  field##_show(struct device *dev,				\
>  			    struct device_attribute *attr, char *buf)		\
> @@ -3824,6 +3885,36 @@ struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>  }
>  EXPORT_SYMBOL_NS_GPL(nvme_find_get_ns, NVME_TARGET_PASSTHRU);
>  
> +static int nvme_alloc_chardev_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns)
> +{
> +	char cdisk_name[DISK_NAME_LEN];
> +	int ret = 0;

Unnecessary initialization for local variable.

> +
> +	device_initialize(&ns->cdev_device);
> +	ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt),
> +				     ns->head->instance);
> +	ns->cdev_device.class = nvme_ns_class;
> +	ns->cdev_device.parent = ctrl->device;
> +	ns->cdev_device.groups = nvme_ns_char_id_attr_groups;
> +	dev_set_drvdata(&ns->cdev_device, ns);
> +
> +	sprintf(cdisk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
> +			ctrl->instance, ns->head->instance);

In multi-path, private namespaces for a head are not in /dev, so I don't
think this will hurt private namespaces (e.g., nvme0c0n1), But it looks
like it will make a little bit confusions between chardev and hidden blkdev.

I don't against to update nvme-cli things also even naming conventions are
going to become different than nvmeXcYnZ.