What to do on cdev_add failure

Hi all,

I am currently working on the i2c-dev driver, which has just been converted to the non-ancestral cdev API. As I am cleaning up the driver, I would like to switch from static cdev initialization (cdev_init) to dynamic allocation (cdev_alloc.)

While I was looking at other drivers to figure out how to deal with error cases, I found that different drivers do different things if cdev_add fails after cdev_alloc was called successfully. I guess some of them are right, others are wrong, and I'd like to know which is which ;-)

* char/virtio_console.c, s390/char/tape_class.c, s390/char/vmur.c, infiniband/.../qib_file_ops.c, fuse/cuse.c, scsi/sg.c and scsi/st.g are calling cdev_del(cdev).
* v4l2-core/v4l2-dev.c is calling kfree(cdev).
* s390/char/vmlogrdr.c, uio/uio.c, tty/ty_io.c and __register_chrdev() are calling kobject_put(&cdev->kobj). The former explicitly says "no cdev_del here!" in a comment.

My gut feeling is that kobject_put(&cdev->kobj) is correct, even though it feels strange to have to use a low-level function to clean-up after a higher level API call.

If cdev_del(cdev) is also correct (and as I read the code it could be, iff calling kobj_unmap() is a no-op if kobj_map() failed - is it the case?), then it should be clearly documented as such, as it is counter-intuitive (to me, at least.)

Anyone wants to comment on this?

On top of this, another thing looks strange to me. cdev_add() only gets the parent kobj on success. However the release methods (cdev_default_release and cdev_dynamic_release) will put the parent kobj unconditionally. So it looks to me that we are over-putting the parent whenever cdev_add() fails. OTOH I can't see where the parent is set. If it is NULL then all these get and put are no-ops to start with and it no longer matters. But why would we be doing that?

Then again, what do I know about kobj black magic...

Quoted 40 lines · Jean Delvare

Hi all,

I am currently working on the i2c-dev driver, which has just been converted to the non-ancestral cdev API. As I am cleaning up the driver, I would like to switch from static cdev initialization (cdev_init) to dynamic allocation (cdev_alloc.)

While I was looking at other drivers to figure out how to deal with error cases, I found that different drivers do different things if cdev_add fails after cdev_alloc was called successfully. I guess some of them are right, others are wrong, and I'd like to know which is which ;-)

* char/virtio_console.c, s390/char/tape_class.c, s390/char/vmur.c, infiniband/.../qib_file_ops.c, fuse/cuse.c, scsi/sg.c and scsi/st.g are calling cdev_del(cdev).
* v4l2-core/v4l2-dev.c is calling kfree(cdev).
* s390/char/vmlogrdr.c, uio/uio.c, tty/ty_io.c and __register_chrdev() are calling kobject_put(&cdev->kobj). The former explicitly says "no cdev_del here!" in a comment.

My gut feeling is that kobject_put(&cdev->kobj) is correct, even though it feels strange to have to use a low-level function to clean-up after a higher level API call.

If cdev_del(cdev) is also correct (and as I read the code it could be, iff calling kobj_unmap() is a no-op if kobj_map() failed - is it the case?), then it should be clearly documented as such, as it is counter-intuitive (to me, at least.)

Anyone wants to comment on this?

On top of this, another thing looks strange to me. cdev_add() only gets the parent kobj on success. However the release methods (cdev_default_release and cdev_dynamic_release) will put the parent kobj unconditionally. So it looks to me that we are over-putting the parent whenever cdev_add() fails. OTOH I can't see where the parent is set. If it is NULL then all these get and put are no-ops to start with and it no longer matters. But why would we be doing that?

Quoted 1 line

Then again, what do I know about kobj black magic...

It's worse than you think, the kobject in a cdev is not a "real" kobject. Well, it's kind of real, but it's only there to be used for the kmap logic. I have a 10+ year old TODO item here that says "remove kobj from cdev" that I really should get to one of these days.

Anyone that touches the kobj outside of the cdev core code is probably wrong, it's "funny" that both uio and tty do that, the maintainer of that code must be lazy... :)

Let me look into what the "correct" thing to do here is, I used to know it, need some time to refresh my memory...

And the cdev interface has what, 4 different ways it can be used? Another of my TODO items is to fix it all up to only use it one way, or maybe just 2 as it does have the ability to make driver code pretty small if you use it in unique ways...