Messages in this thread Patch in this message | | | Subject | [PATCH 3/3] libnvdimm/ioctl: Switch to cdev_register_queued() | From | Dan Williams <> | Date | Wed, 20 Jan 2021 11:39:08 -0800 |
| |
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(-)
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);
| |