All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jean Delvare <jdelvare@suse.de>
To: linux-fsdevel@vger.kernel.org
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Greg KH <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: What to do on cdev_add failure
Date: Wed, 13 Jul 2016 15:46:14 +0200	[thread overview]
Message-ID: <20160713154614.3d2ce2ec@endymion> (raw)

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

             reply	other threads:[~2016-07-13 13:46 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-13 13:46 Jean Delvare [this message]
2016-07-14  6:47 ` What to do on cdev_add failure Greg KH
2016-07-22  9:18   ` Jean Delvare

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160713154614.3d2ce2ec@endymion \
    --to=jdelvare@suse.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.