Hi all,
First some background which is not necessary to understand the rest of this
post: I encountered the below problem while looking into some videodev.c
reference counting patches in combination with converting videodev.c from
register_chrdev, to register_chrdev_region + cdev. I wanted to put links to
relevant posts here, but for some reason the video4linux-list archive is
private, I'll kick someone about that.
Now the problem, struct cdev contains a kobj for reference counting, which gets
initialized either in cdev_alloc (for dynamically allocated cdev structs) or in
cdev_init (for structs part of a larger struct). Thus setting the reference
count to 1. When the device underlying the cdev is removed (usb disconnect for
example) cdev_del gets called which does a kobject_put on the kobj, lowering
the ref count to 0 causing it to be released.
However releasing the cdev struct, while some apps still has an open fd
refering to the character device is not a good idea, so chrdev_open does a
kobj_get on the cdev's kobj.
And here is the problem when the fd refering to the character device gets
closed, no-one does a kobj_put. chrdev_open replace the file's f_op pointer
with the device driver fops, so the only fops release which will get called is
that of the device driver, cdev_put (which will call kobj_put on the kobj) is
exported, so device driver release methods could and I guess should call
cdev_put, but under drivers/char there is not a single driver calling cdev_put !!
So unless I'm missing something the kojb release callback never gets called.
I see 2 solutions here:
1) Fix all cdev users to call cdev_put on their cdev in their fops release
method, adding a release method to do this where necessary, but this seems
rather error prone
2) Add a private fops struct to cdev, which gets filled with the device drivers
fops, except for release, which will point to a chrdev_release function
which call cdev_put after calling the device driver fops release method if
present, to me this seems the preferable solution.
###
While on this topic in case of an usb device whose driver exports an chardev to
userspace, the device can be disconnected while the chardev is still open.
Currently usb-chardev drivers need to do their own reference counting in their
open / release fops to make sure their device structure stays around until the
last user has closed the device.
If the reference counting in cdev is fixed, this would almost be an exact
duplicate of the ref counting done in the device driver, thus I would like to
propose to add a release function ptr to the cdev structs which if not NULL
gets called from the cdev kobj release handler, then then device driver no
longer has to duplicate the ref counting.
This esp seems to make sense in cases where the device driver uses cdev_init,
as then the cdev structure could currently be freed by the device driver (in
case of hot unplug) without it knowing for sure that there are no more users of
the cdev structure. For example even when the device driver does its own ref
counting in the open / release fops, there could still be some users in the
form of open cdev sysfs files.
Regards,
Hans
p.s.
Please keep me in the CC, I'm not subscribed to the kernel mailinglist.
Hans de Goede wrote:
<snip>
> And here is the problem when the fd refering to the character device
> gets closed, no-one does a kobj_put. chrdev_open replace the file's f_op
> pointer with the device driver fops, so the only fops release which will
> get called is that of the device driver, cdev_put (which will call
> kobj_put on the kobj) is exported, so device driver release methods
> could and I guess should call cdev_put, but under drivers/char there is
> not a single driver calling cdev_put !!
>
> So unless I'm missing something the kojb release callback never gets
> called.
>
Never mind, I just found out that cdev_put gets called from __fput() in
fs/file_table.c, thats somewhat convoluted if I may say so, I think atleast a
comment in char_dev.c explaining this would be in order.
So that only leaves this part of my mail:
> ###
>
> While on this topic in case of an usb device whose driver exports an
> chardev to userspace, the device can be disconnected while the chardev
> is still open. Currently usb-chardev drivers need to do their own
> reference counting in their open / release fops to make sure their
> device structure stays around until the last user has closed the device.
>
> The reference counting in cdev is almost an
> exact duplicate of the ref counting done in the device driver, thus I
> would like to propose to add a release function ptr to the cdev struct
> which if not NULL gets called from the cdev kobj release handler, then
> then device driver no longer has to duplicate the ref counting.
>
> This esp seems to make sense in cases where the device driver uses
> cdev_init, as then the cdev structure could currently be freed by the
> device driver (in case of hot unplug) without it knowing for sure that
> there are no more users of the cdev structure. For example even when the
> device driver does its own ref counting in the open / release fops,
> there could still be some users in the form of open cdev sysfs files.
>
I would still very much like to see this release callback get added, if there
are no objections I'll do a patch for this.
Regards,
Hans
p.s.
Please keep me in the CC, I'm not subscribed to the kernel mailinglist.