If cdev_add() fails there is no good reason to call cdev_del().
Signed-off-by: Rolf Eike Beer <[email protected]>
---
commit 7d8c15d5af591c6039a4bba6ab20e4952ed11c1f
tree 8849441bf17c6c8145f09123a320c34572746c7c
parent 10e3d9d489c71aaf7ff86c81178ae5aeefe1ad6f
author Rolf Eike Beer <[email protected]> Thu, 21 Sep 2006 09:42:49 +0200
committer Rolf Eike Beer <[email protected]> Thu, 21 Sep 2006 09:42:49 +0200
drivers/rtc/rtc-dev.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index 61a5825..062c0ab 100644
--- a/drivers/rtc/rtc-dev.c
+++ b/drivers/rtc/rtc-dev.c
@@ -406,7 +406,6 @@ #endif
rtc->char_dev.owner = rtc->owner;
if (cdev_add(&rtc->char_dev, MKDEV(MAJOR(rtc_devt), rtc->id), 1)) {
- cdev_del(&rtc->char_dev);
dev_err(class_dev->dev,
"failed to add char device %d:%d\n",
MAJOR(rtc_devt), rtc->id);
On Thu, 21 Sep 2006 09:46:06 +0200
Rolf Eike Beer <[email protected]> wrote:
> If cdev_add() fails there is no good reason to call cdev_del().
>
> Signed-off-by: Rolf Eike Beer <[email protected]>
>
> rtc->char_dev.owner = rtc->owner;
>
> if (cdev_add(&rtc->char_dev, MKDEV(MAJOR(rtc_devt), rtc->id), 1)) {
> - cdev_del(&rtc->char_dev);
> dev_err(class_dev->dev,
> "failed to add char device %d:%d\n",
> MAJOR(rtc_devt), rtc->id);
I'm not sure.. this is drivers/char/raw.c:
cdev_init(&raw_cdev, &raw_fops);
if (cdev_add(&raw_cdev, dev, MAX_RAW_MINORS)) {
kobject_put(&raw_cdev.kobj);
unregister_chrdev_region(dev, MAX_RAW_MINORS);
goto error;
}
in case of failure, it does a kobject_put.
tha same call is done by cdev_del.
other drivers have implemented different error paths.
which one is correct?
--
Best regards,
Alessandro Zummo,
Tower Technologies - Turin, Italy
http://www.towertech.it
Alessandro Zummo wrote:
> On Thu, 21 Sep 2006 09:46:06 +0200
>
> Rolf Eike Beer <[email protected]> wrote:
> > If cdev_add() fails there is no good reason to call cdev_del().
> >
> > Signed-off-by: Rolf Eike Beer <[email protected]>
> >
> > rtc->char_dev.owner = rtc->owner;
> >
> > if (cdev_add(&rtc->char_dev, MKDEV(MAJOR(rtc_devt), rtc->id), 1)) {
> > - cdev_del(&rtc->char_dev);
> > dev_err(class_dev->dev,
> > "failed to add char device %d:%d\n",
> > MAJOR(rtc_devt), rtc->id);
>
> I'm not sure.. this is drivers/char/raw.c:
>
>
> cdev_init(&raw_cdev, &raw_fops);
> if (cdev_add(&raw_cdev, dev, MAX_RAW_MINORS)) {
> kobject_put(&raw_cdev.kobj);
> unregister_chrdev_region(dev, MAX_RAW_MINORS);
> goto error;
> }
>
> in case of failure, it does a kobject_put.
> tha same call is done by cdev_del.
This is unneeded here as it's embedded into struct rtc_device. Jonathan?
> other drivers have implemented different error paths.
> which one is correct?
Probably half of them are wrong, ugly or both. I think this interface is not
very intuitive at all. This two calls needed to set up an embedded cdev are
IMHO the best way to keep people confused. At least the (possible) need to
call cdev_del() on failed cdev_add() is just weird.
Eike
On 9/21/06, Rolf Eike Beer <[email protected]> wrote:
> Alessandro Zummo wrote:
> > On Thu, 21 Sep 2006 09:46:06 +0200
> >
> > Rolf Eike Beer <[email protected]> wrote:
> > > If cdev_add() fails there is no good reason to call cdev_del().
> > >
> > > Signed-off-by: Rolf Eike Beer <[email protected]>
> > >
> > > rtc->char_dev.owner = rtc->owner;
> > >
> > > if (cdev_add(&rtc->char_dev, MKDEV(MAJOR(rtc_devt), rtc->id), 1)) {
> > > - cdev_del(&rtc->char_dev);
> > > dev_err(class_dev->dev,
> > > "failed to add char device %d:%d\n",
> > > MAJOR(rtc_devt), rtc->id);
> >
> > I'm not sure.. this is drivers/char/raw.c:
> >
> >
> > cdev_init(&raw_cdev, &raw_fops);
> > if (cdev_add(&raw_cdev, dev, MAX_RAW_MINORS)) {
> > kobject_put(&raw_cdev.kobj);
> > unregister_chrdev_region(dev, MAX_RAW_MINORS);
> > goto error;
> > }
> >
> > in case of failure, it does a kobject_put.
> > tha same call is done by cdev_del.
But cdev_del also tries to do kobj_unmap before doing kobject_put. If
cdev_add fails the object is not added to the map so we shoult not try
to unmap it (although it does not hurt in the current implementation).
>
> This is unneeded here as it's embedded into struct rtc_device. Jonathan?
>
cdev distingueshes between stattically and synamically allocated
objects and so it is safe to do kobject_put/cdev_del even on cdevs
embedded into other structures.
> > other drivers have implemented different error paths.
> > which one is correct?
>
raw.c seems to be DTRT.
> Probably half of them are wrong, ugly or both. I think this interface is not
> very intuitive at all. This two calls needed to set up an embedded cdev are
> IMHO the best way to keep people confused. At least the (possible) need to
> call cdev_del() on failed cdev_add() is just weird.
>
The rule is simple - after cdev_init/cdev_alloc call kobject_put.
After successful cdev_add call cdev_del.
--
Dmitry