[PATCH 0/3] cdev: Generic shutdown handling

[PATCH 0/3] cdev: Generic shutdown handling

[Dan Williams]

After reviewing driver submissions with new cdev + ioctl usages one
common stumbling block is coordinating the shutdown of the ioctl path,
or other file operations, at driver ->remove() time. While cdev_del()
guarantees that no new file descriptors will be established, operations
on existing file descriptors can proceed indefinitely.

Given the observation that the kernel spends the resources for a percpu_ref
per request_queue shared with all block_devices on a gendisk, do the
same for all the cdev instances that share the same
cdev_add()-to-cdev_del() lifetime.

With this in place cdev_del() not only guarantees 'no new opens', but it
also guarantees 'no new operations invocations' and 'all threads running
in an operation handler have exited that handler'.

As a proof point of the way driver implementations open-code around this
gap in the api the libnvdimm ioctl path is reworked with a result of:

    4 files changed, 101 insertions(+), 153 deletions(-)

---

Dan Williams (3):
      cdev: Finish the cdev api with queued mode support
      libnvdimm/ida: Switch to non-deprecated ida helpers
      libnvdimm/ioctl: Switch to cdev_register_queued()


 drivers/nvdimm/btt_devs.c       |    6 +
 drivers/nvdimm/bus.c            |  177 +++++++++------------------------------
 drivers/nvdimm/core.c           |   14 ++-
 drivers/nvdimm/dax_devs.c       |    4 -
 drivers/nvdimm/dimm_devs.c      |   53 +++++++++---
 drivers/nvdimm/namespace_devs.c |   14 +--
 drivers/nvdimm/nd-core.h        |   14 ++-
 drivers/nvdimm/pfn_devs.c       |    4 -
 fs/char_dev.c                   |  108 ++++++++++++++++++++++--
 include/linux/cdev.h            |   21 ++++-
 10 files changed, 238 insertions(+), 177 deletions(-)
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH 1/3] cdev: Finish the cdev api with queued mode support

[Dan Williams]

The cdev api aims to preclude many common mistakes of driver developers
with ioctl interface lifetime and lookup. However, one common concern
that is left to driver implementers to handle manually is synchronizing
device shutdown with in-flight calls to file_operations handlers.

Starting with commit:

233ed09d7fda ("chardev: add helper function to register char devs with a struct device")

...common reference count handling scenarios were addressed, but the
shutdown-synchronization problem was only mentioned as something driver
developers need to be aware in the following note:

    NOTE: This guarantees that associated sysfs callbacks are not running
    or runnable, however any cdevs already open will remain and their fops
    will still be callable even after this function returns.

Remove that responsibility from driver developers with the concept of a
'queued' mode for cdevs.

Recall that every 'struct request_queue' in the kernel (one per 'struct
gendisk') has a percpu_ref counter to represent the queue usage count.
One of the useful properties of a percpu_ref, outside of low overhead
multi-CPU reference counting, is the ability to stop new references from
being taken while awaiting outstanding references to be dropped. Add a
percpu_ref to 'struct cdev' to allow cdev_del() to not only assert 'no
future opens', but also 'stop future ops invocations' and 'flush all in
progress ops'.

Similar to how a 'struct request_queue' is shared by all bdevs parented
by a 'gendisk' the new per-cdev percpu_ref is shared by all the
device-minors hosted by a given cdev instance.

The cdev->qactive counter is elevated before calling a driver provided
'struct cdev_operations' handler, and dropped when that handler returns.
With the cdev_queued_ops taking percpu_ref_tryget_live(), then once
cdev_del() has executed percpu_ref_kill() new invocations of driver
operation handlers are blocked, and once all references are dropped
cdev_del() returns.

The fact that driver provided operations are now wrapped by the cdev
core allows for some type-safety to be added to 'struct
cdev_operations'. In particular, it provides @cdev as an argument, and
adds type-safety to @arg. It currently enforces the common case where an
ioctl() takes a pointer as an argument rather than a value. Other ops
like mmap() can be added later.

Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/char_dev.c        |  108 +++++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/cdev.h |   21 +++++++++-
 2 files changed, 122 insertions(+), 7 deletions(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index ba0ded7842a7..c7239fbea0ff 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -367,6 +367,45 @@ void cdev_put(struct cdev *p)
 	}
 }
 
+static long cdev_queued_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct inode *inode = file_inode(file);
+	struct cdev *cdev = inode->i_cdev;
+	long rc;
+
+	if (!percpu_ref_tryget_live(&cdev->qactive))
+		return -ENXIO;
+
+	rc = cdev->qops->ioctl(cdev, file, cmd, (void __user *) arg);
+
+	percpu_ref_put(&cdev->qactive);
+
+	return rc;
+}
+
+static int cdev_queued_open(struct inode *inode, struct file *file)
+{
+	struct cdev *cdev = inode->i_cdev;
+	int rc;
+
+	if (!percpu_ref_tryget_live(&cdev->qactive))
+		return -ENXIO;
+
+	rc = cdev->qops->open(cdev, file);
+
+	percpu_ref_put(&cdev->qactive);
+
+	return rc;
+}
+
+static const struct file_operations cdev_queued_fops = {
+	.owner = THIS_MODULE,
+	.open = cdev_queued_open,
+	.unlocked_ioctl = cdev_queued_ioctl,
+	.compat_ioctl = compat_ptr_ioctl,
+	.llseek = noop_llseek,
+};
+
 /*
  * Called every time a character special file is opened
  */
@@ -405,7 +444,10 @@ static int chrdev_open(struct inode *inode, struct file *filp)
 		return ret;
 
 	ret = -ENXIO;
-	fops = fops_get(p->ops);
+	if (p->qregistered)
+		fops = &cdev_queued_fops;
+	else
+		fops = fops_get(p->ops);
 	if (!fops)
 		goto out_cdev_put;
 
@@ -582,7 +624,7 @@ static void cdev_unmap(dev_t dev, unsigned count)
 
 /**
  * cdev_del() - remove a cdev from the system
- * @p: the cdev structure to be removed
+ * @cdev: the cdev structure to be removed
  *
  * cdev_del() removes @p from the system, possibly freeing the structure
  * itself.
@@ -590,14 +632,22 @@ static void cdev_unmap(dev_t dev, unsigned count)
  * NOTE: This guarantees that cdev device will no longer be able to be
  * opened, however any cdevs already open will remain and their fops will
  * still be callable even after cdev_del returns.
+ *
+ * That is unless the cdev was initialized in queued mode. In queued
+ * mode new invocations of the fops are blocked and in-flight calls to
+ * the fops are drained before this returns.
  */
-void cdev_del(struct cdev *p)
+void cdev_del(struct cdev *cdev)
 {
-	cdev_unmap(p->dev, p->count);
-	kobject_put(&p->kobj);
+	cdev_unmap(cdev->dev, cdev->count);
+	kobject_put(&cdev->kobj);
+	if (!cdev->qregistered)
+		return;
+	percpu_ref_kill(&cdev->qactive);
+	wait_for_completion(&cdev->qdead);
+	percpu_ref_exit(&cdev->qactive);
 }
 
-
 static void cdev_default_release(struct kobject *kobj)
 {
 	struct cdev *p = container_of(kobj, struct cdev, kobj);
@@ -656,6 +706,52 @@ void cdev_init(struct cdev *cdev, const struct file_operations *fops)
 	cdev->ops = fops;
 }
 
+static void cdev_queued_release(struct percpu_ref *ref)
+{
+	struct cdev *cdev = container_of(ref, struct cdev, qactive);
+
+	complete(&cdev->qdead);
+}
+
+/**
+ * cdev_register_queued() - register a cdev structure with queued ops
+ * @cdev: the structure to init and add
+ * @owner: host module for the device + ops
+ * @dev: the first device number for which this device is responsible
+ * @count: the number of consecutive minor numbers corresponding to this
+ *         device
+ * @ops: the cdev_operations for this device
+ *
+ * In addition to base cdev_init() allocate and initialize a reference
+ * counter to track in-flight operations. With cdev_register_queued()
+ * cdev_del() guarantees no in-flight operations in addition to no new
+ * opens.
+ */
+__must_check int __cdev_register_queued(struct cdev *cdev, struct module *owner,
+					dev_t dev, unsigned count,
+					const struct cdev_operations *qops)
+{
+	int rc;
+
+	if (!qops->ioctl || !owner)
+		return -EINVAL;
+
+	cdev_init(cdev, NULL);
+	cdev->qops = qops;
+	cdev->owner = owner;
+	cdev->qregistered = true;
+	init_completion(&cdev->qdead);
+	rc = percpu_ref_init(&cdev->qactive, cdev_queued_release, 0, GFP_KERNEL);
+	if (rc)
+		return rc;
+
+	rc = cdev_add(cdev, dev, count);
+	if (rc)
+		percpu_ref_exit(&cdev->qactive);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(__cdev_register_queued);
+
 static struct kobject *base_probe(dev_t dev, int *part, void *data)
 {
 	if (request_module("char-major-%d-%d", MAJOR(dev), MINOR(dev)) > 0)
diff --git a/include/linux/cdev.h b/include/linux/cdev.h
index 0e8cd6293deb..5ef1bfb3b495 100644
--- a/include/linux/cdev.h
+++ b/include/linux/cdev.h
@@ -2,6 +2,7 @@
 #ifndef _LINUX_CDEV_H
 #define _LINUX_CDEV_H
 
+#include <linux/percpu-refcount.h>
 #include <linux/kobject.h>
 #include <linux/kdev_t.h>
 #include <linux/list.h>
@@ -10,17 +11,35 @@
 struct file_operations;
 struct inode;
 struct module;
+struct cdev;
+
+struct cdev_operations {
+	int (*open)(struct cdev *cdev, struct file *);
+	long (*ioctl)(struct cdev *cdev, struct file *file, unsigned int cmd,
+		      void __user *arg);
+};
 
 struct cdev {
 	struct kobject kobj;
 	struct module *owner;
-	const struct file_operations *ops;
+	union {
+		const struct file_operations *ops;
+		const struct cdev_operations *qops;
+	};
 	struct list_head list;
 	dev_t dev;
 	unsigned int count;
+	struct percpu_ref qactive;
+	struct completion qdead;
+	bool qregistered;
 } __randomize_layout;
 
 void cdev_init(struct cdev *, const struct file_operations *);
+#define cdev_register_queued(cdev, dev, count, ops)                            \
+	__cdev_register_queued(cdev, THIS_MODULE, dev, count, ops)
+__must_check int __cdev_register_queued(struct cdev *cdev, struct module *owner,
+					dev_t dev, unsigned count,
+					const struct cdev_operations *qops);
 
 struct cdev *cdev_alloc(void);
 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH 1/3] cdev: Finish the cdev api with queued mode support

[Christoph Hellwig]

The subject doesn't make any sense to me.

But thn again queued sound really weird.  You just have a managed
API with a refcount and synchronization, right?

procfs and debugfs already support these kind of managed ops, kinda sad
to duplicate this concept yet another time.

> +static long cdev_queued_ioctl(struct file *file, unsigned int cmd, unsigned long arg)

Overly long line.

> +__must_check int __cdev_register_queued(struct cdev *cdev, struct module *owner,
> +					dev_t dev, unsigned count,
> +					const struct cdev_operations *qops)
> +{
> +	int rc;
> +
> +	if (!qops->ioctl || !owner)
> +		return -EINVAL;

Why is the ioctl method mandatory?
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH 1/3] cdev: Finish the cdev api with queued mode support

[Dan Williams]

On Wed, Jan 20, 2021 at 11:46 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> The subject doesn't make any sense to me.
>
> But thn again queued sound really weird.  You just have a managed
> API with a refcount and synchronization, right?

Correct.

"queue" was in reference to the way q_usage_count behaves, but yes,
just refcount + synchronization is all this is.

>
> procfs and debugfs already support these kind of managed ops, kinda sad
> to duplicate this concept yet another time.

Oh, I didn't realize there were managed ops there, I'll go take a look
and see if cdev can adopt that scheme.

> > +static long cdev_queued_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>
> Overly long line.
>
> > +__must_check int __cdev_register_queued(struct cdev *cdev, struct module *owner,
> > +                                     dev_t dev, unsigned count,
> > +                                     const struct cdev_operations *qops)
> > +{
> > +     int rc;
> > +
> > +     if (!qops->ioctl || !owner)
> > +             return -EINVAL;
>
> Why is the ioctl method mandatory?

Yeah, that can drop. It was there more to ask the question about
whether cdev should be mandating ioctls with pointer arguments and
taking the need to specify the compat fallback away from a driver
responsibility.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH 1/3] cdev: Finish the cdev api with queued mode support

[Dan Williams]

On Wed, Jan 20, 2021 at 12:20 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Wed, Jan 20, 2021 at 11:46 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > The subject doesn't make any sense to me.
> >
> > But thn again queued sound really weird.  You just have a managed
> > API with a refcount and synchronization, right?
>
> Correct.
>
> "queue" was in reference to the way q_usage_count behaves, but yes,
> just refcount + synchronization is all this is.
>
> >
> > procfs and debugfs already support these kind of managed ops, kinda sad
> > to duplicate this concept yet another time.
>
> Oh, I didn't realize there were managed ops there, I'll go take a look
> and see if cdev can adopt that scheme.

So in both cases they are leveraging the fact that they are the
filesystems that allocated the inode and will stash the private
reference counting somewhere relative to the container_of the
vfs_inode. I don't immediately see how that scheme can be implied to
special device files that can come from an inode on any filesystem.

I do see how debugfs and procfs could be unified, but I don't think
this percpu_ref for cdev is compatible.

Other ideas?
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH 1/3] cdev: Finish the cdev api with queued mode support

[Logan Gunthorpe]

On 2021-01-20 12:38 p.m., Dan Williams wrote:
> ...common reference count handling scenarios were addressed, but the
> shutdown-synchronization problem was only mentioned as something driver
> developers need to be aware in the following note:
> 
>     NOTE: This guarantees that associated sysfs callbacks are not running
>     or runnable, however any cdevs already open will remain and their fops
>     will still be callable even after this function returns.
> 
> Remove that responsibility from driver developers with the concept of a
> 'queued' mode for cdevs.

I find the queued name confusing. What's being queued?

> +static const struct file_operations cdev_queued_fops = {
> +	.owner = THIS_MODULE,
> +	.open = cdev_queued_open,
> +	.unlocked_ioctl = cdev_queued_ioctl,
> +	.compat_ioctl = compat_ptr_ioctl,
> +	.llseek = noop_llseek,
> +};

Why do we only protect these fops? I'd find it a bit confusing to have
ioctl protected from use after del, but not write/read/etc.

Logan
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH 1/3] cdev: Finish the cdev api with queued mode support

[Dan Williams]

On Wed, Jan 20, 2021 at 11:51 AM Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
>
>
> On 2021-01-20 12:38 p.m., Dan Williams wrote:
> > ...common reference count handling scenarios were addressed, but the
> > shutdown-synchronization problem was only mentioned as something driver
> > developers need to be aware in the following note:
> >
> >     NOTE: This guarantees that associated sysfs callbacks are not running
> >     or runnable, however any cdevs already open will remain and their fops
> >     will still be callable even after this function returns.
> >
> > Remove that responsibility from driver developers with the concept of a
> > 'queued' mode for cdevs.
>
> I find the queued name confusing. What's being queued?

Yeah, as I mentioned to Christoph, a bit too much inspiration from
q_usage_count. Perhaps "managed" makes more sense.

>
> > +static const struct file_operations cdev_queued_fops = {
> > +     .owner = THIS_MODULE,
> > +     .open = cdev_queued_open,
> > +     .unlocked_ioctl = cdev_queued_ioctl,
> > +     .compat_ioctl = compat_ptr_ioctl,
> > +     .llseek = noop_llseek,
> > +};
>
> Why do we only protect these fops? I'd find it a bit confusing to have
> ioctl protected from use after del, but not write/read/etc.

More ops can certainly be added over time, I didn't want to go do the
work to wrap all file_operations before getting consensus on the idea
that the cdev core should provide managed ops at all.

The other question I'm posing with cdev_operations is whether the cdev
core should take away some of the flexibility from end drivers in
favor of adding more type safety. For example, mandate that all ioctls
take a pointer argument not an integer argument? The question of
whether wrapping cdev file_operations around a new cdev_operations is
a good idea can be deferred after finalizing a mechanism for managed
cdev file_operations.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH 1/3] cdev: Finish the cdev api with queued mode support

[Greg KH]

On Wed, Jan 20, 2021 at 11:38:57AM -0800, Dan Williams wrote:
> -void cdev_del(struct cdev *p)
> +void cdev_del(struct cdev *cdev)
>  {
> -	cdev_unmap(p->dev, p->count);
> -	kobject_put(&p->kobj);
> +	cdev_unmap(cdev->dev, cdev->count);
> +	kobject_put(&cdev->kobj);

After Christoph's patch series, the kobject in struct cdev does nothing,
so I will be removing it.  So I don't think this patch set is going to
do what you want :(

thanks,

greg k-h
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH 1/3] cdev: Finish the cdev api with queued mode support

[Dan Williams]

On Thu, Jan 21, 2021 at 12:13 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 20, 2021 at 11:38:57AM -0800, Dan Williams wrote:
> > -void cdev_del(struct cdev *p)
> > +void cdev_del(struct cdev *cdev)
> >  {
> > -     cdev_unmap(p->dev, p->count);
> > -     kobject_put(&p->kobj);
> > +     cdev_unmap(cdev->dev, cdev->count);
> > +     kobject_put(&cdev->kobj);
>
> After Christoph's patch series, the kobject in struct cdev does nothing,
> so I will be removing it.  So I don't think this patch set is going to
> do what you want :(

The proposal in this series has nothing to do with kobject lifetime.
Please take another look at the file_operations shutdown coordination
problem and the fact that it's not just cdev that has a use case to
manage file_operations this way. I believe Christoph's only objection,
correct me if I'm wrong, is that this proposal invents a new third way
to do this relative to procfs and debugfs. Perhaps there's a way to
generalize this for all three, and I'm going to put some thought
there, but at this point I think I'm failing to describe the problem
clearly.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH 2/3] libnvdimm/ida: Switch to non-deprecated ida helpers

[Dan Williams]

In preparation for a rework of character device initialization, take the
opportunity to cleanup ida usage.

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/btt_devs.c       |    6 +++---
 drivers/nvdimm/bus.c            |    4 ++--
 drivers/nvdimm/dax_devs.c       |    4 ++--
 drivers/nvdimm/dimm_devs.c      |    4 ++--
 drivers/nvdimm/namespace_devs.c |   14 ++++++--------
 drivers/nvdimm/pfn_devs.c       |    4 ++--
 6 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 05feb97e11ce..e148fd16a47e 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -20,7 +20,7 @@ static void nd_btt_release(struct device *dev)
 
 	dev_dbg(dev, "trace\n");
 	nd_detach_ndns(&nd_btt->dev, &nd_btt->ndns);
-	ida_simple_remove(&nd_region->btt_ida, nd_btt->id);
+	ida_free(&nd_region->btt_ida, nd_btt->id);
 	kfree(nd_btt->uuid);
 	kfree(nd_btt);
 }
@@ -190,7 +190,7 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
 	if (!nd_btt)
 		return NULL;
 
-	nd_btt->id = ida_simple_get(&nd_region->btt_ida, 0, 0, GFP_KERNEL);
+	nd_btt->id = ida_alloc(&nd_region->btt_ida, GFP_KERNEL);
 	if (nd_btt->id < 0)
 		goto out_nd_btt;
 
@@ -215,7 +215,7 @@ static struct device *__nd_btt_create(struct nd_region *nd_region,
 	return dev;
 
 out_put_id:
-	ida_simple_remove(&nd_region->btt_ida, nd_btt->id);
+	ida_free(&nd_region->btt_ida, nd_btt->id);
 
 out_nd_btt:
 	kfree(nd_btt);
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 2304c6183822..0258ec90dce6 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -296,7 +296,7 @@ static void nvdimm_bus_release(struct device *dev)
 	struct nvdimm_bus *nvdimm_bus;
 
 	nvdimm_bus = container_of(dev, struct nvdimm_bus, dev);
-	ida_simple_remove(&nd_ida, nvdimm_bus->id);
+	ida_free(&nd_ida, nvdimm_bus->id);
 	kfree(nvdimm_bus);
 }
 
@@ -351,7 +351,7 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
 	INIT_LIST_HEAD(&nvdimm_bus->list);
 	INIT_LIST_HEAD(&nvdimm_bus->mapping_list);
 	init_waitqueue_head(&nvdimm_bus->wait);
-	nvdimm_bus->id = ida_simple_get(&nd_ida, 0, 0, GFP_KERNEL);
+	nvdimm_bus->id = ida_alloc(&nd_ida, GFP_KERNEL);
 	if (nvdimm_bus->id < 0) {
 		kfree(nvdimm_bus);
 		return NULL;
diff --git a/drivers/nvdimm/dax_devs.c b/drivers/nvdimm/dax_devs.c
index 99965077bac4..374b195ba8d5 100644
--- a/drivers/nvdimm/dax_devs.c
+++ b/drivers/nvdimm/dax_devs.c
@@ -18,7 +18,7 @@ static void nd_dax_release(struct device *dev)
 
 	dev_dbg(dev, "trace\n");
 	nd_detach_ndns(dev, &nd_pfn->ndns);
-	ida_simple_remove(&nd_region->dax_ida, nd_pfn->id);
+	ida_free(&nd_region->dax_ida, nd_pfn->id);
 	kfree(nd_pfn->uuid);
 	kfree(nd_dax);
 }
@@ -55,7 +55,7 @@ static struct nd_dax *nd_dax_alloc(struct nd_region *nd_region)
 		return NULL;
 
 	nd_pfn = &nd_dax->nd_pfn;
-	nd_pfn->id = ida_simple_get(&nd_region->dax_ida, 0, 0, GFP_KERNEL);
+	nd_pfn->id = ida_alloc(&nd_region->dax_ida, GFP_KERNEL);
 	if (nd_pfn->id < 0) {
 		kfree(nd_dax);
 		return NULL;
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index b59032e0859b..3dec809ef20a 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -198,7 +198,7 @@ static void nvdimm_release(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
 
-	ida_simple_remove(&dimm_ida, nvdimm->id);
+	ida_free(&dimm_ida, nvdimm->id);
 	kfree(nvdimm);
 }
 
@@ -592,7 +592,7 @@ struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
 	if (!nvdimm)
 		return NULL;
 
-	nvdimm->id = ida_simple_get(&dimm_ida, 0, 0, GFP_KERNEL);
+	nvdimm->id = ida_alloc(&dimm_ida, GFP_KERNEL);
 	if (nvdimm->id < 0) {
 		kfree(nvdimm);
 		return NULL;
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 6da67f4d641a..c34880310c40 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -26,7 +26,7 @@ static void namespace_pmem_release(struct device *dev)
 	struct nd_region *nd_region = to_nd_region(dev->parent);
 
 	if (nspm->id >= 0)
-		ida_simple_remove(&nd_region->ns_ida, nspm->id);
+		ida_free(&nd_region->ns_ida, nspm->id);
 	kfree(nspm->alt_name);
 	kfree(nspm->uuid);
 	kfree(nspm);
@@ -38,7 +38,7 @@ static void namespace_blk_release(struct device *dev)
 	struct nd_region *nd_region = to_nd_region(dev->parent);
 
 	if (nsblk->id >= 0)
-		ida_simple_remove(&nd_region->ns_ida, nsblk->id);
+		ida_free(&nd_region->ns_ida, nsblk->id);
 	kfree(nsblk->alt_name);
 	kfree(nsblk->uuid);
 	kfree(nsblk->res);
@@ -2114,7 +2114,7 @@ static struct device *nd_namespace_blk_create(struct nd_region *nd_region)
 
 	dev = &nsblk->common.dev;
 	dev->type = &namespace_blk_device_type;
-	nsblk->id = ida_simple_get(&nd_region->ns_ida, 0, 0, GFP_KERNEL);
+	nsblk->id = ida_alloc(&nd_region->ns_ida, GFP_KERNEL);
 	if (nsblk->id < 0) {
 		kfree(nsblk);
 		return NULL;
@@ -2145,7 +2145,7 @@ static struct device *nd_namespace_pmem_create(struct nd_region *nd_region)
 	res->name = dev_name(&nd_region->dev);
 	res->flags = IORESOURCE_MEM;
 
-	nspm->id = ida_simple_get(&nd_region->ns_ida, 0, 0, GFP_KERNEL);
+	nspm->id = ida_alloc(&nd_region->ns_ida, GFP_KERNEL);
 	if (nspm->id < 0) {
 		kfree(nspm);
 		return NULL;
@@ -2633,15 +2633,13 @@ int nd_region_register_namespaces(struct nd_region *nd_region, int *err)
 			struct nd_namespace_blk *nsblk;
 
 			nsblk = to_nd_namespace_blk(dev);
-			id = ida_simple_get(&nd_region->ns_ida, 0, 0,
-					GFP_KERNEL);
+			id = ida_alloc(&nd_region->ns_ida, GFP_KERNEL);
 			nsblk->id = id;
 		} else if (type == ND_DEVICE_NAMESPACE_PMEM) {
 			struct nd_namespace_pmem *nspm;
 
 			nspm = to_nd_namespace_pmem(dev);
-			id = ida_simple_get(&nd_region->ns_ida, 0, 0,
-					GFP_KERNEL);
+			id = ida_alloc(&nd_region->ns_ida, GFP_KERNEL);
 			nspm->id = id;
 		} else
 			id = i;
diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index b499df630d4d..8edfe2d2c77c 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -21,7 +21,7 @@ static void nd_pfn_release(struct device *dev)
 
 	dev_dbg(dev, "trace\n");
 	nd_detach_ndns(&nd_pfn->dev, &nd_pfn->ndns);
-	ida_simple_remove(&nd_region->pfn_ida, nd_pfn->id);
+	ida_free(&nd_region->pfn_ida, nd_pfn->id);
 	kfree(nd_pfn->uuid);
 	kfree(nd_pfn);
 }
@@ -322,7 +322,7 @@ static struct nd_pfn *nd_pfn_alloc(struct nd_region *nd_region)
 	if (!nd_pfn)
 		return NULL;
 
-	nd_pfn->id = ida_simple_get(&nd_region->pfn_ida, 0, 0, GFP_KERNEL);
+	nd_pfn->id = ida_alloc(&nd_region->pfn_ida, GFP_KERNEL);
 	if (nd_pfn->id < 0) {
 		kfree(nd_pfn);
 		return NULL;
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Dan Williams]

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);
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Greg KH]

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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Dan Williams]

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.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH 0/3] cdev: Generic shutdown handling

[Dan Williams]

On Wed, Jan 20, 2021 at 11:38 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> After reviewing driver submissions with new cdev + ioctl usages one
> common stumbling block is coordinating the shutdown of the ioctl path,
> or other file operations, at driver ->remove() time. While cdev_del()
> guarantees that no new file descriptors will be established, operations
> on existing file descriptors can proceed indefinitely.
>
> Given the observation that the kernel spends the resources for a percpu_ref
> per request_queue shared with all block_devices on a gendisk, do the
> same for all the cdev instances that share the same
> cdev_add()-to-cdev_del() lifetime.
>
> With this in place cdev_del() not only guarantees 'no new opens', but it
> also guarantees 'no new operations invocations' and 'all threads running
> in an operation handler have exited that handler'.

Prompted by the reaction I realized that this is pushing an incomplete
story about why this is needed, and the "queued" concept is way off
base. The problem this is trying to solve is situations like this:

long xyz_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
        xyz_ioctl_dev = file->private_data;
        xyz_driver_context = xyz_ioctl_dev->context;
        ...
}

int xyz_probe(struct device *dev)
{
        xyz_driver_context = devm_kzalloc(...);
        ...
        xyz_ioctl_dev = kmalloc(...);
        device_initialize(&xyz_ioctl_dev->dev);
        xyz_ioctl_dev->context = xyz_driver_context;
      ...
        cdev_device_add(&xyz_ioctl_dev->cdev, xyz_ioctl_dev->dev);
}

...where a parent driver allocates context tied to the lifetime of the
parent device driver-bind-lifetime, and that context ends up getting
used in the ioctl path. I interpret Greg's assertion "if you do this
right you don't have this problem" as "don't reference anything with a
lifetime shorter than the xyz_ioctl_dev lifetime in your ioctl
handler". That is true, but it can be awkward to constraint
xyz_driver_context to a sub-device, and it constrains some of the
convenience of devm. So the goal is to have a cdev api that accounts
for all the common lifetimes when devm is in use. So I'm now thinking
of an api like:

    devm_cdev_device_add(struct device *host, struct cdev *cdev,
struct device *dev)

...where @host bounds the lifetime of data used by the cdev
file_operations, and @dev is the typical containing structure for
@cdev. Internally I would refactor the debugfs mechanism for flushing
in-flight file_operations so that is shared by the cdev
implementation. Either adopt the debugfs method for file_operations
syncing, or switch debugfs to percpu_ref (leaning towards the former).

Does this clarify the raised concerns?
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org