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

From: Dan Williams
Date: Wed Jan 20 2021 - 14:45:41 EST

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@xxxxxxxxx>
Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
---
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);