What to do on cdev_add failure

What to do on cdev_add failure

[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?

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

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: What to do on cdev_add failure

[Greg KH]

On Wed, Jul 13, 2016 at 03:46:14PM +0200, Jean Delvare wrote:
> 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...

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...

thanks,

greg k-h

Re: What to do on cdev_add failure

[Jean Delvare]

Hi Greg,

On Thu, 14 Jul 2016 15:47:35 +0900, Greg KH wrote:
> On Wed, Jul 13, 2016 at 03:46:14PM +0200, Jean Delvare wrote:
> > 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...
> 
> 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.

I did figure that out actually.

> 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... :)

Or just as confused as myself.

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

Did you find?

> 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...

-- 
Jean Delvare
SUSE L3 Support