diff mbox series

[3/3] libnvdimm/ioctl: Switch to cdev_register_queued()

Message ID 161117154856.2853729.1012816981381380656.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive)
State New, archived
Headers show
Series cdev: Generic shutdown handling | expand

Commit Message

Dan Williams Jan. 20, 2021, 7:39 p.m. UTC 
The ioctl implementation in libnvdimm is a case study in what can be
cleaned up when the cdev core handles synchronizing in-flight ioctls
with device removal. Switch to cdev_register_queued() which allows for
the ugly context lookup and activity tracking implementation to be
dropped, among other cleanups.

Cc: Vishal Verma <vishal.l.verma@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/bus.c       |  175 +++++++++++---------------------------------
 drivers/nvdimm/core.c      |   14 ++--
 drivers/nvdimm/dimm_devs.c |   51 +++++++++++--
 drivers/nvdimm/nd-core.h   |   14 ++--
 4 files changed, 101 insertions(+), 153 deletions(-)

Comments

Greg Kroah-Hartman Jan. 21, 2021, 8:15 a.m. UTC | #1 
On Wed, Jan 20, 2021 at 11:39:08AM -0800, Dan Williams wrote:
> The ioctl implementation in libnvdimm is a case study in what can be
> cleaned up when the cdev core handles synchronizing in-flight ioctls
> with device removal. Switch to cdev_register_queued() which allows for
> the ugly context lookup and activity tracking implementation to be
> dropped, among other cleanups.

I'm confused, the cdev handles the filesystem access from /dev/ which
handles the ioctl.  Any use of a cdev with relationship to a struct
device that might go away is independent, so we really should not tie
these together in any way.

thanks,

greg k-h
Dan Williams Jan. 21, 2021, 5:02 p.m. UTC | #2 
On Thu, Jan 21, 2021 at 12:16 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 20, 2021 at 11:39:08AM -0800, Dan Williams wrote:
> > The ioctl implementation in libnvdimm is a case study in what can be
> > cleaned up when the cdev core handles synchronizing in-flight ioctls
> > with device removal. Switch to cdev_register_queued() which allows for
> > the ugly context lookup and activity tracking implementation to be
> > dropped, among other cleanups.
>
> I'm confused, the cdev handles the filesystem access from /dev/ which
> handles the ioctl.  Any use of a cdev with relationship to a struct
> device that might go away is independent, so we really should not tie
> these together in any way.

Oh, no, there's no object lifetime ties here, that problem was already
solved with Logan's addition of cdev_device_add(). Instead, this about
file_operations liveness. Look at procfs and debugfs that Christoph
pointed out have open coded the same facility. When the driver is
unbound ongoing file operations should be flushed and blocked
currently the cdev api is forcing every driver to open code this
themselves.

This seems a pattern that is asking to be unified as a general
"managed fops" concept.
diff mbox series

Patch

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 0258ec90dce6..4c73abb3bc1d 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -24,8 +24,7 @@ 
 #include "nd.h"
 #include "pfn.h"
 
-int nvdimm_major;
-static int nvdimm_bus_major;
+static dev_t nvdimm_bus_dev_t;
 struct class *nd_class;
 static DEFINE_IDA(nd_ida);
 
@@ -348,10 +347,9 @@  struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
 	nvdimm_bus = kzalloc(sizeof(*nvdimm_bus), GFP_KERNEL);
 	if (!nvdimm_bus)
 		return NULL;
-	INIT_LIST_HEAD(&nvdimm_bus->list);
 	INIT_LIST_HEAD(&nvdimm_bus->mapping_list);
 	init_waitqueue_head(&nvdimm_bus->wait);
-	nvdimm_bus->id = ida_alloc(&nd_ida, GFP_KERNEL);
+	nvdimm_bus->id = ida_alloc_range(&nd_ida, 0, NVDIMM_MAX_BUS, GFP_KERNEL);
 	if (nvdimm_bus->id < 0) {
 		kfree(nvdimm_bus);
 		return NULL;
@@ -408,6 +406,7 @@  static int child_unregister(struct device *dev, void *data)
 			dev_put = true;
 		nvdimm_bus_unlock(dev);
 		cancel_delayed_work_sync(&nvdimm->dwork);
+		cdev_del(&nvdimm->cdev);
 		if (dev_put)
 			put_device(dev);
 	}
@@ -430,14 +429,9 @@  static void free_badrange_list(struct list_head *badrange_list)
 static int nd_bus_remove(struct device *dev)
 {
 	struct nvdimm_bus *nvdimm_bus = to_nvdimm_bus(dev);
+	struct cdev *cdev = &nvdimm_bus->cdev;
 
-	mutex_lock(&nvdimm_bus_list_mutex);
-	list_del_init(&nvdimm_bus->list);
-	mutex_unlock(&nvdimm_bus_list_mutex);
-
-	wait_event(nvdimm_bus->wait,
-			atomic_read(&nvdimm_bus->ioctl_active) == 0);
-
+	cdev_del(cdev);
 	nd_synchronize();
 	device_for_each_child(&nvdimm_bus->dev, NULL, child_unregister);
 
@@ -459,10 +453,6 @@  static int nd_bus_probe(struct device *dev)
 	if (rc)
 		return rc;
 
-	mutex_lock(&nvdimm_bus_list_mutex);
-	list_add_tail(&nvdimm_bus->list, &nvdimm_bus_list);
-	mutex_unlock(&nvdimm_bus_list_mutex);
-
 	/* enable bus provider attributes to look up their local context */
 	dev_set_drvdata(dev, nvdimm_bus->nd_desc);
 
@@ -733,23 +723,50 @@  const struct attribute_group nd_numa_attribute_group = {
 	.is_visible = nd_numa_attr_visible,
 };
 
+static long bus_ioctl(struct cdev *cdev, struct file *file, unsigned int cmd,
+		      void __user *arg)
+{
+	int ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
+
+	return nd_ioctl(file->private_data, NULL, ro, cmd, arg);
+}
+
+static int ndctl_open(struct cdev *cdev, struct file *file)
+{
+	file->private_data = container_of(cdev, struct nvdimm_bus, cdev);
+	return 0;
+}
+
+static const struct cdev_operations nvdimm_bus_ops = {
+	.open = ndctl_open,
+	.ioctl = bus_ioctl,
+};
+
 int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus)
 {
-	dev_t devt = MKDEV(nvdimm_bus_major, nvdimm_bus->id);
+	dev_t devt = MKDEV(MAJOR(nvdimm_bus_dev_t), nvdimm_bus->id);
+	struct cdev *cdev = &nvdimm_bus->cdev;
 	struct device *dev;
+	int rc;
+
+	rc = cdev_register_queued(cdev, devt, 1, &nvdimm_bus_ops);
+	if (rc)
+		return rc;
 
 	dev = device_create(nd_class, &nvdimm_bus->dev, devt, nvdimm_bus,
 			"ndctl%d", nvdimm_bus->id);
 
-	if (IS_ERR(dev))
+	if (IS_ERR(dev)) {
+		cdev_del(cdev);
 		dev_dbg(&nvdimm_bus->dev, "failed to register ndctl%d: %ld\n",
 				nvdimm_bus->id, PTR_ERR(dev));
+	}
 	return PTR_ERR_OR_ZERO(dev);
 }
 
 void nvdimm_bus_destroy_ndctl(struct nvdimm_bus *nvdimm_bus)
 {
-	device_destroy(nd_class, MKDEV(nvdimm_bus_major, nvdimm_bus->id));
+	device_destroy(nd_class, MKDEV(MAJOR(nvdimm_bus_dev_t), nvdimm_bus->id));
 }
 
 static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = {
@@ -1004,14 +1021,13 @@  static int nd_cmd_clear_to_send(struct nvdimm_bus *nvdimm_bus,
 	return 0;
 }
 
-static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
-		int read_only, unsigned int ioctl_cmd, unsigned long arg)
+int nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
+	     int read_only, unsigned int ioctl_cmd, void __user *p)
 {
 	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
 	const struct nd_cmd_desc *desc = NULL;
 	unsigned int cmd = _IOC_NR(ioctl_cmd);
 	struct device *dev = &nvdimm_bus->dev;
-	void __user *p = (void __user *) arg;
 	char *out_env = NULL, *in_env = NULL;
 	const char *cmd_name, *dimm_name;
 	u32 in_len = 0, out_len = 0;
@@ -1188,104 +1204,6 @@  static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	return rc;
 }
 
-enum nd_ioctl_mode {
-	BUS_IOCTL,
-	DIMM_IOCTL,
-};
-
-static int match_dimm(struct device *dev, void *data)
-{
-	long id = (long) data;
-
-	if (is_nvdimm(dev)) {
-		struct nvdimm *nvdimm = to_nvdimm(dev);
-
-		return nvdimm->id == id;
-	}
-
-	return 0;
-}
-
-static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
-		enum nd_ioctl_mode mode)
-
-{
-	struct nvdimm_bus *nvdimm_bus, *found = NULL;
-	long id = (long) file->private_data;
-	struct nvdimm *nvdimm = NULL;
-	int rc, ro;
-
-	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
-	mutex_lock(&nvdimm_bus_list_mutex);
-	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
-		if (mode == DIMM_IOCTL) {
-			struct device *dev;
-
-			dev = device_find_child(&nvdimm_bus->dev,
-					file->private_data, match_dimm);
-			if (!dev)
-				continue;
-			nvdimm = to_nvdimm(dev);
-			found = nvdimm_bus;
-		} else if (nvdimm_bus->id == id) {
-			found = nvdimm_bus;
-		}
-
-		if (found) {
-			atomic_inc(&nvdimm_bus->ioctl_active);
-			break;
-		}
-	}
-	mutex_unlock(&nvdimm_bus_list_mutex);
-
-	if (!found)
-		return -ENXIO;
-
-	nvdimm_bus = found;
-	rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
-
-	if (nvdimm)
-		put_device(&nvdimm->dev);
-	if (atomic_dec_and_test(&nvdimm_bus->ioctl_active))
-		wake_up(&nvdimm_bus->wait);
-
-	return rc;
-}
-
-static long bus_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	return nd_ioctl(file, cmd, arg, BUS_IOCTL);
-}
-
-static long dimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	return nd_ioctl(file, cmd, arg, DIMM_IOCTL);
-}
-
-static int nd_open(struct inode *inode, struct file *file)
-{
-	long minor = iminor(inode);
-
-	file->private_data = (void *) minor;
-	return 0;
-}
-
-static const struct file_operations nvdimm_bus_fops = {
-	.owner = THIS_MODULE,
-	.open = nd_open,
-	.unlocked_ioctl = bus_ioctl,
-	.compat_ioctl = compat_ptr_ioctl,
-	.llseek = noop_llseek,
-};
-
-static const struct file_operations nvdimm_fops = {
-	.owner = THIS_MODULE,
-	.open = nd_open,
-	.unlocked_ioctl = dimm_ioctl,
-	.compat_ioctl = compat_ptr_ioctl,
-	.llseek = noop_llseek,
-};
-
 int __init nvdimm_bus_init(void)
 {
 	int rc;
@@ -1294,15 +1212,9 @@  int __init nvdimm_bus_init(void)
 	if (rc)
 		return rc;
 
-	rc = register_chrdev(0, "ndctl", &nvdimm_bus_fops);
-	if (rc < 0)
-		goto err_bus_chrdev;
-	nvdimm_bus_major = rc;
-
-	rc = register_chrdev(0, "dimmctl", &nvdimm_fops);
+	rc = alloc_chrdev_region(&nvdimm_bus_dev_t, 0, NVDIMM_MAX_BUS, "ndctl");
 	if (rc < 0)
-		goto err_dimm_chrdev;
-	nvdimm_major = rc;
+		goto err_chrdev;
 
 	nd_class = class_create(THIS_MODULE, "nd");
 	if (IS_ERR(nd_class)) {
@@ -1319,10 +1231,8 @@  int __init nvdimm_bus_init(void)
  err_nd_bus:
 	class_destroy(nd_class);
  err_class:
-	unregister_chrdev(nvdimm_major, "dimmctl");
- err_dimm_chrdev:
-	unregister_chrdev(nvdimm_bus_major, "ndctl");
- err_bus_chrdev:
+	unregister_chrdev_region(nvdimm_bus_dev_t, NVDIMM_MAX_BUS);
+ err_chrdev:
 	bus_unregister(&nvdimm_bus_type);
 
 	return rc;
@@ -1332,8 +1242,7 @@  void nvdimm_bus_exit(void)
 {
 	driver_unregister(&nd_bus_driver.drv);
 	class_destroy(nd_class);
-	unregister_chrdev(nvdimm_bus_major, "ndctl");
-	unregister_chrdev(nvdimm_major, "dimmctl");
+	unregister_chrdev_region(nvdimm_bus_dev_t, NVDIMM_MAX_BUS);
 	bus_unregister(&nvdimm_bus_type);
 	ida_destroy(&nd_ida);
 }
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index 7de592d7eff4..017ae313a4bb 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -16,9 +16,6 @@ 
 #include "nd-core.h"
 #include "nd.h"
 
-LIST_HEAD(nvdimm_bus_list);
-DEFINE_MUTEX(nvdimm_bus_list_mutex);
-
 void nvdimm_bus_lock(struct device *dev)
 {
 	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
@@ -584,6 +581,9 @@  static __init int libnvdimm_init(void)
 	rc = nvdimm_bus_init();
 	if (rc)
 		return rc;
+	rc = nvdimm_devs_init();
+	if (rc)
+		goto err_dimm_devs;
 	rc = nvdimm_init();
 	if (rc)
 		goto err_dimm;
@@ -594,18 +594,20 @@  static __init int libnvdimm_init(void)
 	nd_label_init();
 
 	return 0;
- err_region:
+err_region:
 	nvdimm_exit();
- err_dimm:
+err_dimm:
+	nvdimm_devs_exit();
+err_dimm_devs:
 	nvdimm_bus_exit();
 	return rc;
 }
 
 static __exit void libnvdimm_exit(void)
 {
-	WARN_ON(!list_empty(&nvdimm_bus_list));
 	nd_region_exit();
 	nvdimm_exit();
+	nvdimm_devs_exit();
 	nvdimm_bus_exit();
 	nvdimm_devs_exit();
 }
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 3dec809ef20a..e2c3a58f712d 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -16,6 +16,7 @@ 
 #include "pmem.h"
 #include "nd.h"
 
+static dev_t nvdimm_dev_t;
 static DEFINE_IDA(dimm_ida);
 
 static bool noblk;
@@ -579,6 +580,26 @@  bool is_nvdimm(struct device *dev)
 	return dev->type == &nvdimm_device_type;
 }
 
+static long dimm_ioctl(struct cdev *cdev, struct file *file, unsigned int cmd,
+		       void __user *arg)
+{
+	struct nvdimm *nvdimm = file->private_data;
+	int ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
+
+	return nd_ioctl(nvdimm_to_bus(nvdimm), nvdimm, ro, cmd, arg);
+}
+
+static int nmem_open(struct cdev *cdev, struct file *file)
+{
+	file->private_data = container_of(cdev, struct nvdimm, cdev);
+	return 0;
+}
+
+static const struct cdev_operations nvdimm_ops = {
+	.open = nmem_open,
+	.ioctl = dimm_ioctl,
+};
+
 struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
 		void *provider_data, const struct attribute_group **groups,
 		unsigned long flags, unsigned long cmd_mask, int num_flush,
@@ -587,16 +608,15 @@  struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
 		const struct nvdimm_fw_ops *fw_ops)
 {
 	struct nvdimm *nvdimm = kzalloc(sizeof(*nvdimm), GFP_KERNEL);
-	struct device *dev;
+	struct cdev *cdev = &nvdimm->cdev;
+	struct device *dev = &nvdimm->dev;
 
 	if (!nvdimm)
 		return NULL;
 
-	nvdimm->id = ida_alloc(&dimm_ida, GFP_KERNEL);
-	if (nvdimm->id < 0) {
-		kfree(nvdimm);
-		return NULL;
-	}
+	nvdimm->id = ida_alloc_range(&dimm_ida, 0, NVDIMM_MAX_NVDIMM, GFP_KERNEL);
+	if (nvdimm->id < 0)
+		goto err_id;
 
 	nvdimm->dimm_id = dimm_id;
 	nvdimm->provider_data = provider_data;
@@ -607,16 +627,18 @@  struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
 	nvdimm->num_flush = num_flush;
 	nvdimm->flush_wpq = flush_wpq;
 	atomic_set(&nvdimm->busy, 0);
-	dev = &nvdimm->dev;
 	dev_set_name(dev, "nmem%d", nvdimm->id);
 	dev->parent = &nvdimm_bus->dev;
 	dev->type = &nvdimm_device_type;
-	dev->devt = MKDEV(nvdimm_major, nvdimm->id);
+	dev->devt = MKDEV(MAJOR(nvdimm_dev_t), nvdimm->id);
 	dev->groups = groups;
 	nvdimm->sec.ops = sec_ops;
 	nvdimm->fw_ops = fw_ops;
 	nvdimm->sec.overwrite_tmo = 0;
 	INIT_DELAYED_WORK(&nvdimm->dwork, nvdimm_security_overwrite_query);
+	if (cdev_register_queued(cdev, dev->devt, 1, &nvdimm_ops) != 0)
+		goto err_cdev;
+
 	/*
 	 * Security state must be initialized before device_add() for
 	 * attribute visibility.
@@ -627,6 +649,11 @@  struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
 	nd_device_register(dev);
 
 	return nvdimm;
+err_cdev:
+	ida_free(&dimm_ida, nvdimm->id);
+err_id:
+	kfree(nvdimm);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(__nvdimm_create);
 
@@ -1007,7 +1034,13 @@  int nvdimm_bus_check_dimm_count(struct nvdimm_bus *nvdimm_bus, int dimm_count)
 }
 EXPORT_SYMBOL_GPL(nvdimm_bus_check_dimm_count);
 
-void __exit nvdimm_devs_exit(void)
+int __init nvdimm_devs_init(void)
+{
+	return alloc_chrdev_region(&nvdimm_dev_t, 0, NVDIMM_MAX_NVDIMM, "nmem");
+}
+
+void nvdimm_devs_exit(void)
 {
 	ida_destroy(&dimm_ida);
+	unregister_chrdev_region(nvdimm_dev_t, NVDIMM_MAX_NVDIMM);
 }
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 564faa36a3ca..cf32e0cad7a3 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -8,30 +8,31 @@ 
 #include <linux/device.h>
 #include <linux/sizes.h>
 #include <linux/mutex.h>
+#include <linux/cdev.h>
 #include <linux/nd.h>
 #include "nd.h"
 
-extern struct list_head nvdimm_bus_list;
-extern struct mutex nvdimm_bus_list_mutex;
 extern int nvdimm_major;
 extern struct workqueue_struct *nvdimm_wq;
 
+#define NVDIMM_MAX_BUS 64
 struct nvdimm_bus {
 	struct nvdimm_bus_descriptor *nd_desc;
 	wait_queue_head_t wait;
-	struct list_head list;
+	struct cdev cdev;
 	struct device dev;
 	int id, probe_active;
-	atomic_t ioctl_active;
 	struct list_head mapping_list;
 	struct mutex reconfig_mutex;
 	struct badrange badrange;
 };
 
+#define NVDIMM_MAX_NVDIMM 256
 struct nvdimm {
 	unsigned long flags;
 	void *provider_data;
 	unsigned long cmd_mask;
+	struct cdev cdev;
 	struct device dev;
 	atomic_t busy;
 	int id, num_flush;
@@ -48,6 +49,9 @@  struct nvdimm {
 	const struct nvdimm_fw_ops *fw_ops;
 };
 
+int nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
+	     int read_only, unsigned int ioctl_cmd, void __user *p);
+
 static inline unsigned long nvdimm_security_flags(
 		struct nvdimm *nvdimm, enum nvdimm_passphrase_type ptype)
 {
@@ -98,7 +102,6 @@  struct blk_alloc_info {
 	resource_size_t available, busy;
 	struct resource *res;
 };
-
 bool is_nvdimm(struct device *dev);
 bool is_nd_pmem(struct device *dev);
 bool is_nd_volatile(struct device *dev);
@@ -114,6 +117,7 @@  static inline bool is_memory(struct device *dev)
 struct nvdimm_bus *walk_to_nvdimm_bus(struct device *nd_dev);
 int __init nvdimm_bus_init(void);
 void nvdimm_bus_exit(void);
+int __init nvdimm_devs_init(void);
 void nvdimm_devs_exit(void);
 struct nd_region;
 void nd_region_advance_seeds(struct nd_region *nd_region, struct device *dev);