Add cdev->release() so that cdev can be considered in more involved
object lifetime management.  cdev_alloc() used a separate ktype for
auto-free release().  This patch converts it to use cdev->release() so
that there's no need for separate ktype and cdev_init() can be used
for auto-free variant too.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
This one is also for CUSE.  Oops, forgot lkml.  Resending.  Thanks.

 fs/char_dev.c        |   30 +++++++++++++-----------------
 include/linux/cdev.h |    1 +
 2 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 3cb7cda..3d50199 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -478,26 +478,22 @@ void cdev_del(struct cdev *p)
 }
 
 
-static void cdev_default_release(struct kobject *kobj)
+static void cdev_release(struct kobject *kobj)
 {
 	struct cdev *p = container_of(kobj, struct cdev, kobj);
 	cdev_purge(p);
+	if (p->release)
+		p->release(p);
 }
 
-static void cdev_dynamic_release(struct kobject *kobj)
-{
-	struct cdev *p = container_of(kobj, struct cdev, kobj);
-	cdev_purge(p);
-	kfree(p);
-}
-
-static struct kobj_type ktype_cdev_default = {
-	.release	= cdev_default_release,
+static struct kobj_type cdev_ktype = {
+	.release	= cdev_release,
 };
 
-static struct kobj_type ktype_cdev_dynamic = {
-	.release	= cdev_dynamic_release,
-};
+static void cdev_alloc_release(struct cdev *cdev)
+{
+	kfree(cdev);
+}
 
 /**
  * cdev_alloc() - allocate a cdev structure
@@ -506,10 +502,10 @@ static struct kobj_type ktype_cdev_dynamic = {
  */
 struct cdev *cdev_alloc(void)
 {
-	struct cdev *p = kzalloc(sizeof(struct cdev), GFP_KERNEL);
+	struct cdev *p = kmalloc(sizeof(struct cdev), GFP_KERNEL);
 	if (p) {
-		INIT_LIST_HEAD(&p->list);
-		kobject_init(&p->kobj, &ktype_cdev_dynamic);
+		cdev_init(p, NULL);
+		p->release = cdev_alloc_release;
 	}
 	return p;
 }
@@ -526,7 +522,7 @@ void cdev_init(struct cdev *cdev, const struct file_operations *fops)
 {
 ...
[ message continues ]

Ick, I really don't want struct cdev to be used for lifecycle
management, as it is only for major:minor stuff.  Why do you want to
make this change?

thanks,

greg k-h
--

[ message continues ]

Well, as cdev can be referenced from userspace, ->release is required
for most purposes.  The reason why devices have been getting by without
it is because most chardevs are created on module load and destroyed on
module unload and in the meantime cdev refcount virtually equals module
refcnt, but I'm fairly sure we have cases where cdev can be destroyed
for other reasons then module unloading and it's very likely those cases
are buggy in the current code (backing structure gone bug cdev still
hanging around).

As CUSE can create and destroy devices regardless of module reference
count, it falls in the second category and needs cdev->release() to make
sure the backing structure doesn't go away till cdev is released.

Thanks.

-- 
tejun
--

[ message continues ]

Hm, I thought Al covered that when he created the cdev interface, I

But CUSE should be it's own module, right?  And it would "own" the cdev,
so the module and cdev count should be fine and matching.  The userspace
code could go away but the CUSE code should handle that with a different
reference count.  This is the way that hardware drivers handle the
issue.

thanks,

greg k-h
--

[ message continues ]

Hmmm.... I've never actually audited the code so... it could be that no

The problem is not the device to talk to CUSE (/dev/cuse as in
/dev/fuse), for which module refcount and device refcount match fine.
But the whole point of CUSE is allowing CUSE clients to create arbitrary
character devices, so in addition to /dev/cuse which clients use to talk
to CUSE, CUSE hosts character devices for its clients and they come and
go dynamically and thus requires proper lifetime management.

Thanks.

-- 
tejun
--

[ message continues ]

That's fine, it's just like a USB device that uses a cdev, right?  Or a
PCI device, or any other type of device that can come and go independant
of the cdev or module lifespan.

So, you tie the cdev lifespan to the module lifespan with the
MODULE_OWNER in the file operations.  As the cuse.ko module will own the
cdev, it can handle that reference, and it can not be removed as long as
userspace has the cdev open, right?

It really isn't any different from any other type of removable device
(i.e. any type of device...)

thanks,

greg k-h
--

[ message continues ]

Ah.. right, but taking cdev refcount out of the picture requires adding
'severing' operation on cdev f_ops, which certainly is doable but isn't
it just cleaner to make cdev follow the usual lifetime management rules?
 An object which is referenced counted requires ->release if it's gonna
be used in any non-simplistic way.

Thanks.

-- 
tejun
--

[ message continues ]

Yes, but as you "normally" tie the cdev to the module itself, that
handles the lifetime rules.

Now I really dont' like the current cdev interface, it's a bit too
complex as it needed to support the old-style interfaces without any
build time changes, but I think your change isn't needed as somehow all
the current drivers that support dynamic devices don't need it.

Actually, the kobject in cdev shouldn't be an kobject, it's not used for
registering with sysfs at all, it should just be a kref.  I sweep the
tree for code that sets the name of the cdev every few months as people
don't seem to realize this :)

thanks,

greg k-h
--

[ message continues ]

Hello, Greg.

I'm now trying to convert the cdev thing for CUSE and it just ain't
feel right.


That "normally" is from the days when only a single or fixed number of

Yes, it requires all drivers to have global device table and check
whether the device is still available at ->open.  For most, drivers
usually have certain fixed number of devices which can directly be
indexed with minor.  For CUSE, it gotta be a hash table or a b-tree.
I don't really see any point in not adding ->release.  Time has
changed and everybody is playing with reference counts and ->release
methods.  Plus, cdev_alloc() interface is half-baked anyway (no free
function for cases where cdev_add() fails, drivers call cdev_del() in
those cases risking unregistering other driver's map).  It's perfectly
okay to keep it around for compatibility but there just is no reason

Heh... CUSE did that too.  Removing it.

Thanks.

-- 
tejun
--

[ message continues ]