Messages in this thread | | | Date | Thu, 13 Nov 2008 17:58:54 +0900 | From | Tejun Heo <> | Subject | Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it |
| |
Hello, Greg.
I'm now trying to convert the cdev thing for CUSE and it just ain't feel right.
Greg KH wrote: >> 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. > > Yes, but as you "normally" tie the cdev to the module itself, that > handles the lifetime rules.
That "normally" is from the days when only a single or fixed number of devices are associated with single module or driver.
> 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.
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 to cling to 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 :)
Heh... CUSE did that too. Removing it.
Thanks.
-- tejun
| |