Re: [PATCH V2] nvme: enable char device per namespace

A bunch of nitpicks (mostly naming as usual, sorry..):

+static int __nvme_ns_ioctl(struct gendisk *disk, unsigned int cmd,
+			   unsigned long arg)
 {

What about nvme_disk_ioctl instead as that is what it operates on?

+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);
+}

No need for the cast.

Also can we keep all the char device methods together close to the
struct file_operations declaration?  I just prefer to keep the code
a little grouped.

nvme_ns_open and nvme_ns_release?

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

No need for the local ns variable in both cases.

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

Do we need the ->private_data assignment at all?  I think the ioctl
handler could just grab it directly from i_cdev.

+	sprintf(cdisk_name, "nvme%dn%dc", ctrl->instance, ns->head->instance);

And the most important naming decision is this.  I have two issues with
naming still:

 - we aready use the c for controller in the hidden disk naming.  Although
   that is in a different position, but I think this not super intuitive.
 - this is missing multipath support entirely, so once we want to add
   multipath support we'll run into issues.  So maybe use something
   based off the hidden node naming?  E.g.:

	sprintf(disk_name, "nvme-generic-%dc%dn%d", ctrl->subsys->instance,
		ctrl->instance, ns->head->instance);

+	/* When the device does not support any of the features required by the
+	 * kernel (or viceversa), hide the block device. We can still rely on
+	 * the namespace char device for submitting IOCTLs
+	 */

Normal kernel comment style is the opening

	/*

on its own line.

 	if (nvme_update_ns_info(ns, id))
-		goto out_put_disk;
+		disk->flags |= GENHD_FL_HIDDEN;

I don't think we can do this based on all the error returns.  I think
we'll have to move the flags manipulation into nvme_update_ns_info to
also cover the revalidate case.