[RTC] Remove superfluous call to call to cdev_del()

[RTC] Remove superfluous call to call to cdev_del()

[Rolf Eike Beer]

If cdev_add() fails there is no good reason to call cdev_del().

Signed-off-by: Rolf Eike Beer <eike-kernel@sf-tec.de>

---
commit 7d8c15d5af591c6039a4bba6ab20e4952ed11c1f
tree 8849441bf17c6c8145f09123a320c34572746c7c
parent 10e3d9d489c71aaf7ff86c81178ae5aeefe1ad6f
author Rolf Eike Beer <eike-kernel@sf-tec.de> Thu, 21 Sep 2006 09:42:49 +0200
committer Rolf Eike Beer <eike-kernel@sf-tec.de> 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);

Re: [RTC] Remove superfluous call to call to cdev_del()

[Alessandro Zummo]

On Thu, 21 Sep 2006 09:46:06 +0200
Rolf Eike Beer <eike-kernel@sf-tec.de> wrote:

> If cdev_add() fails there is no good reason to call cdev_del().
> 
> Signed-off-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
>
>  	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

Re: [RTC] Remove superfluous call to call to cdev_del()

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]

Alessandro Zummo wrote:
> On Thu, 21 Sep 2006 09:46:06 +0200
>
> Rolf Eike Beer <eike-kernel@sf-tec.de> wrote:
> > If cdev_add() fails there is no good reason to call cdev_del().
> >
> > Signed-off-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
> >
> >  	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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RTC] Remove superfluous call to call to cdev_del()

[Dmitry Torokhov]

On 9/21/06, Rolf Eike Beer <eike-kernel@sf-tec.de> wrote:
> Alessandro Zummo wrote:
> > On Thu, 21 Sep 2006 09:46:06 +0200
> >
> > Rolf Eike Beer <eike-kernel@sf-tec.de> wrote:
> > > If cdev_add() fails there is no good reason to call cdev_del().
> > >
> > > Signed-off-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
> > >
> > >     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