should failed calls to device_register() always call put_device()?

should failed calls to device_register() always call put_device()?

[Robert P. J. Day]

  from drivers/base/core.c, we have the fairly unambiguous advice:

* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up the
* reference initialized in this function instead.
*/
int device_register(struct device *dev)
{
        device_initialize(dev);
        return device_add(dev);
}

  and yet, there appears to be driver code that does exactly that,
such as this snippet from drivers/w1/w1_int.c (line 86):

	... snip ...
        err = device_register(&dev->dev);
        if (err) {
                printk(KERN_ERR "Failed to register master device. err=%d\n", err);
                memset(dev, 0, sizeof(struct w1_master));
                kfree(dev);
                dev = NULL;
        }

        return dev;

  isn't the above doing just that -- directly freeing the dev
structure without first calling put_device()?  or am i misreading
something?

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

should failed calls to device_register() always call put_device()?

[Belisko Marek]

Hi Robert,

On Sat, May 28, 2011 at 5:15 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
>
> ?from drivers/base/core.c, we have the fairly unambiguous advice:
>
> * NOTE: _Never_ directly free @dev after calling this function, even
> * if it returned an error! Always use put_device() to give up the
> * reference initialized in this function instead.
> */
> int device_register(struct device *dev)
> {
> ? ? ? ?device_initialize(dev);
> ? ? ? ?return device_add(dev);
> }
>
> ?and yet, there appears to be driver code that does exactly that,
> such as this snippet from drivers/w1/w1_int.c (line 86):
>
> ? ? ? ?... snip ...
> ? ? ? ?err = device_register(&dev->dev);
> ? ? ? ?if (err) {
> ? ? ? ? ? ? ? ?printk(KERN_ERR "Failed to register master device. err=%d\n", err);
> ? ? ? ? ? ? ? ?memset(dev, 0, sizeof(struct w1_master));
> ? ? ? ? ? ? ? ?kfree(dev);
> ? ? ? ? ? ? ? ?dev = NULL;
> ? ? ? ?}
Free is for allocated dev not for struct device so it is OK. IMO thi
snippet should look like:
err = device_register(&dev->dev);
if (err) {
    printk(KERN_ERR "Failed to register master device. err=%d\n", err);
    put_device(&dev->dev);
    memset(dev, 0, sizeof(struct w1_master));
    kfree(dev);
    dev = NULL;
}

>
> ? ? ? ?return dev;
>
> ?isn't the above doing just that -- directly freeing the dev
> structure without first calling put_device()? ?or am i misreading
> something?
>
> rday
>
> --
>
> ========================================================================
> Robert P. J. Day ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? Ottawa, Ontario, CANADA
> ? ? ? ? ? ? ? ? ? ? ? ?http://crashcourse.ca
>
> Twitter: ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? http://twitter.com/rpjday
> LinkedIn: ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? http://ca.linkedin.com/in/rpjday
> ========================================================================
>
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>
regards,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com

should failed calls to device_register() always call put_device()?

[Robert P. J. Day]

On Sat, 28 May 2011, Belisko Marek wrote:

> Hi Robert,
>
> On Sat, May 28, 2011 at 5:15 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> >
> > ?from drivers/base/core.c, we have the fairly unambiguous advice:
> >
> > * NOTE: _Never_ directly free @dev after calling this function, even
> > * if it returned an error! Always use put_device() to give up the
> > * reference initialized in this function instead.
> > */
> > int device_register(struct device *dev)
> > {
> > ? ? ? ?device_initialize(dev);
> > ? ? ? ?return device_add(dev);
> > }
> >
> > ?and yet, there appears to be driver code that does exactly that,
> > such as this snippet from drivers/w1/w1_int.c (line 86):
> >
> > ? ? ? ?... snip ...
> > ? ? ? ?err = device_register(&dev->dev);
> > ? ? ? ?if (err) {
> > ? ? ? ? ? ? ? ?printk(KERN_ERR "Failed to register master device. err=%d\n", err);
> > ? ? ? ? ? ? ? ?memset(dev, 0, sizeof(struct w1_master));
> > ? ? ? ? ? ? ? ?kfree(dev);
> > ? ? ? ? ? ? ? ?dev = NULL;
> > ? ? ? ?}
> Free is for allocated dev not for struct device so it is OK. IMO thi
> snippet should look like:
> err = device_register(&dev->dev);
> if (err) {
>     printk(KERN_ERR "Failed to register master device. err=%d\n", err);
>     put_device(&dev->dev);
>     memset(dev, 0, sizeof(struct w1_master));
>     kfree(dev);
>     dev = NULL;
> }

  i agree that there should be a "put_device(&dev->dev);" statement as
you show above.  however, i still don't see how this can be just a
stylistic improvement as you seem to suggest.  based on the warning
from the kernel source file, it would seem that you *must* do a
put_device() in that situation -- it's not optional.

rday

p.s.  i would also never do a memset() to zero, followed by a kfree(),
when a kzfree() is so much more concise.

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

should failed calls to device_register() always call put_device()?

[Belisko Marek]

On Sat, May 28, 2011 at 6:29 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> On Sat, 28 May 2011, Belisko Marek wrote:
>
>> Hi Robert,
>>
>> On Sat, May 28, 2011 at 5:15 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
>> >
>> > ?from drivers/base/core.c, we have the fairly unambiguous advice:
>> >
>> > * NOTE: _Never_ directly free @dev after calling this function, even
>> > * if it returned an error! Always use put_device() to give up the
>> > * reference initialized in this function instead.
>> > */
>> > int device_register(struct device *dev)
>> > {
>> > ? ? ? ?device_initialize(dev);
>> > ? ? ? ?return device_add(dev);
>> > }
>> >
>> > ?and yet, there appears to be driver code that does exactly that,
>> > such as this snippet from drivers/w1/w1_int.c (line 86):
>> >
>> > ? ? ? ?... snip ...
>> > ? ? ? ?err = device_register(&dev->dev);
>> > ? ? ? ?if (err) {
>> > ? ? ? ? ? ? ? ?printk(KERN_ERR "Failed to register master device. err=%d\n", err);
>> > ? ? ? ? ? ? ? ?memset(dev, 0, sizeof(struct w1_master));
>> > ? ? ? ? ? ? ? ?kfree(dev);
>> > ? ? ? ? ? ? ? ?dev = NULL;
>> > ? ? ? ?}
>> Free is for allocated dev not for struct device so it is OK. IMO thi
>> snippet should look like:
>> err = device_register(&dev->dev);
>> if (err) {
>> ? ? printk(KERN_ERR "Failed to register master device. err=%d\n", err);
>> ? ? put_device(&dev->dev);
>> ? ? memset(dev, 0, sizeof(struct w1_master));
>> ? ? kfree(dev);
>> ? ? dev = NULL;
>> }
>
> ?i agree that there should be a "put_device(&dev->dev);" statement as
> you show above. ?however, i still don't see how this can be just a
> stylistic improvement as you seem to suggest. ?based on the warning
> from the kernel source file, it would seem that you *must* do a
> put_device() in that situation -- it's not optional.
Sure you're right. You can send a patch to fix this problem. Good catch.
>
> rday
>
> p.s. ?i would also never do a memset() to zero, followed by a kfree(),
> when a kzfree() is so much more concise.
>
> --
>
> ========================================================================
> Robert P. J. Day ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? Ottawa, Ontario, CANADA
> ? ? ? ? ? ? ? ? ? ? ? ?http://crashcourse.ca
>
> Twitter: ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? http://twitter.com/rpjday
> LinkedIn: ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? http://ca.linkedin.com/in/rpjday
> ========================================================================

regards,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com

should failed calls to device_register() always call put_device()?

[Robert P. J. Day]

On Sat, 28 May 2011, Belisko Marek wrote:

> On Sat, May 28, 2011 at 6:29 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> >
> > ?i agree that there should be a "put_device(&dev->dev);" statement
> > as you show above. ?however, i still don't see how this can be
> > just a stylistic improvement as you seem to suggest. ?based on the
> > warning from the kernel source file, it would seem that you *must*
> > do a put_device() in that situation -- it's not optional.

> Sure you're right. You can send a patch to fix this problem. Good
> catch.

  i didn't want to submit anything until i verified what correct code
should look like.  and it's not like that's the only example -- others
are trivially easy to find, like this in
drivers/media/video/bt8xx/bttv-gpio.c (line 97):

  err = device_register(&sub->dev);
  if (0 != err) {
           kfree(sub);
           return err;
  }

that would seem to be incorrect as well, no?

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

should failed calls to device_register() always call put_device()?

[Belisko Marek]

On Sat, May 28, 2011 at 9:43 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> On Sat, 28 May 2011, Belisko Marek wrote:
>
>> On Sat, May 28, 2011 at 6:29 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
>> >
>> > ?i agree that there should be a "put_device(&dev->dev);" statement
>> > as you show above. ?however, i still don't see how this can be
>> > just a stylistic improvement as you seem to suggest. ?based on the
>> > warning from the kernel source file, it would seem that you *must*
>> > do a put_device() in that situation -- it's not optional.
>
>> Sure you're right. You can send a patch to fix this problem. Good
>> catch.
>
> ?i didn't want to submit anything until i verified what correct code
> should look like. ?and it's not like that's the only example -- others
> are trivially easy to find, like this in
> drivers/media/video/bt8xx/bttv-gpio.c (line 97):
>
> ?err = device_register(&sub->dev);
> ?if (0 != err) {
> ? ? ? ? ? kfree(sub);
> ? ? ? ? ? return err;
> ?}
I'm little bit confused. If you look at device_add which is part of
device_register if
something in device_add failed it will always call put_device (in any
error case) so why
then is necessary to call it again when device_register return error?
As you said on some
places put_device is used on some not. Assume device_register in
normal conditions
don't fail so this code is not called anyway or?

>
> that would seem to be incorrect as well, no?
>
> rday
>
> --
>
> ========================================================================
> Robert P. J. Day ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? Ottawa, Ontario, CANADA
> ? ? ? ? ? ? ? ? ? ? ? ?http://crashcourse.ca
>
> Twitter: ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? http://twitter.com/rpjday
> LinkedIn: ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? http://ca.linkedin.com/in/rpjday
> ========================================================================

regards,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com

should failed calls to device_register() always call put_device()?

[Greg KH]

On Sat, May 28, 2011 at 10:22:20PM +0200, Belisko Marek wrote:
> On Sat, May 28, 2011 at 9:43 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> > On Sat, 28 May 2011, Belisko Marek wrote:
> >
> >> On Sat, May 28, 2011 at 6:29 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> >> >
> >> > ?i agree that there should be a "put_device(&dev->dev);" statement
> >> > as you show above. ?however, i still don't see how this can be
> >> > just a stylistic improvement as you seem to suggest. ?based on the
> >> > warning from the kernel source file, it would seem that you *must*
> >> > do a put_device() in that situation -- it's not optional.
> >
> >> Sure you're right. You can send a patch to fix this problem. Good
> >> catch.
> >
> > ?i didn't want to submit anything until i verified what correct code
> > should look like. ?and it's not like that's the only example -- others
> > are trivially easy to find, like this in
> > drivers/media/video/bt8xx/bttv-gpio.c (line 97):
> >
> > ?err = device_register(&sub->dev);
> > ?if (0 != err) {
> > ? ? ? ? ? kfree(sub);
> > ? ? ? ? ? return err;
> > ?}
> I'm little bit confused. If you look at device_add which is part of
> device_register if something in device_add failed it will always call
> put_device (in any error case) so why then is necessary to call it
> again when device_register return error?

Because there still is a "dangling" reference on the device.

You wouldn't want the device to be "gone" without you really knowing
about it, right?  If the device_register fails, you might need to do
something with the larger "device" structure before doing the last
put_device() to clean up the memory (like unwind other things you set up
before calling device_register.)

Trust me, dig through the driver core and kobject model, it's tricky to
follow, but it's there.  Or at least it was there the last time I did
this, that is why I documented it so that no one would have to do that
again and they could just easily follow the rules.

Hope this helps,

greg k-h

should failed calls to device_register() always call put_device()?

[Robert P. J. Day]

On Sun, 29 May 2011, Greg KH wrote:

> Trust me, dig through the driver core and kobject model, it's tricky
> to follow, but it's there.  Or at least it was there the last time I
> did this, that is why I documented it so that no one would have to
> do that again and they could just easily follow the rules.

  i'm currently digging through all of that so i'm interested in
somehow summarizing what is proper code for registering a device and
what to do if it fails, so let me report what i've discovered so far
and greg is free to point out where i'm wrong. :-)

  from drivers/base/core.c, we have the following admonition regarding
calling device_register():

 ...
 * NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up the
 * reference initialized in this function instead.
 */

that suggests that, if one calls device_register() and it fails, one
should *always* (probably immediately) call put_device() or something
equivalent to give up that reference.  some examples right out of the
source tree under drivers/ that seem to be following the rules
(located via a trivial application of grep):

base/isa.c:147:         error = device_register(&isa_dev->dev);
base/isa.c-148-         if (error) {
base/isa.c-149-                 put_device(&isa_dev->dev);
base/isa.c-150-                 break;
base/isa.c-151-         }

media/video/pvrusb2/pvrusb2-sysfs.c:655:        ret = device_register(class_dev);
media/video/pvrusb2/pvrusb2-sysfs.c-656-        if (ret) {
media/video/pvrusb2/pvrusb2-sysfs.c-657-                pvr2_trace(PVR2_TRACE_ERROR_LEGS,
media/video/pvrusb2/pvrusb2-sysfs.c:658:                           "device_register failed");
media/video/pvrusb2/pvrusb2-sysfs.c-659-                put_device(class_dev);
media/video/pvrusb2/pvrusb2-sysfs.c-660-                return;
media/video/pvrusb2/pvrusb2-sysfs.c-661-        }

  and there's lots more of that.  so if one calls device_register()
and it fails, the proper approach would be that one can print some
debugging information, but one *must* thereupon call put_device() to
return the reference corresponding to the failed register call.  so
far, so good?

  it also seems fine to, after calling put_device(), call kfree() to
free the enclosing struct, as in:

memstick/core/memstick.c:467:                   if (device_register(&card->dev)) {
memstick/core/memstick.c-468-                           put_device(&card->dev);
memstick/core/memstick.c-469-                           kfree(host->card);
memstick/core/memstick.c-470-                           host->card = NULL;
memstick/core/memstick.c-471-                   }

  the above looks OK since, one you call put_device(), you're free to
do what you want with that enclosing space, correct?  or no?

  what is apparently *not* OK is to either call kfree() *before*
calling put_device(), or to call kfree() and nothing else upon a
failed device_register() call.  some apparently broken code from the
current drivers/ directory:

firewire/core-device.c:687:             if (device_register(&unit->device) < 0)
firewire/core-device.c-688-                     goto skip_unit;
firewire/core-device.c-689-
firewire/core-device.c-690-             continue;
firewire/core-device.c-691-
firewire/core-device.c-692-     skip_unit:
firewire/core-device.c-693-             kfree(unit);
firewire/core-device.c-694-     }
firewire/core-device.c-695-}

firmware/dmi-id.c:230:  ret = device_register(dmi_dev);
firmware/dmi-id.c-231-  if (ret)
firmware/dmi-id.c-232-          goto fail_free_dmi_dev;
firmware/dmi-id.c-233-
firmware/dmi-id.c-234-  return 0;
firmware/dmi-id.c-235-
firmware/dmi-id.c-236-fail_free_dmi_dev:
firmware/dmi-id.c-237-  kfree(dmi_dev);

mca/mca-bus.c:156:      if (device_register(&mca_bus->dev)) {
mca/mca-bus.c-157-              kfree(mca_bus);
mca/mca-bus.c-158-              return NULL;
mca/mca-bus.c-159-      }

media/video/bt8xx/bttv-gpio.c:97:       err = device_register(&sub->dev);
media/video/bt8xx/bttv-gpio.c-98-       if (0 != err) {
media/video/bt8xx/bttv-gpio.c-99-               kfree(sub);
media/video/bt8xx/bttv-gpio.c-100-              return err;
media/video/bt8xx/bttv-gpio.c-101-      }

pci/pcie/portdrv_core.c:331:    retval = device_register(device);
pci/pcie/portdrv_core.c-332-    if (retval)
pci/pcie/portdrv_core.c-333-            kfree(pcie);
pci/pcie/portdrv_core.c-334-    else
pci/pcie/portdrv_core.c-335-            get_device(device);
pci/pcie/portdrv_core.c-336-    return retval;
pci/pcie/portdrv_core.c-337-}

target/loopback/tcm_loop.c:515: ret = device_register(&tl_hba->dev);
target/loopback/tcm_loop.c-516- if (ret) {
target/loopback/tcm_loop.c:517:         printk(KERN_ERR "device_register() failed for"
target/loopback/tcm_loop.c-518-                         " tl_hba->dev: %d\n", ret);
target/loopback/tcm_loop.c-519-         return -ENODEV;
target/loopback/tcm_loop.c-520- }
target/loopback/tcm_loop.c-521-
target/loopback/tcm_loop.c-522- return 0;
target/loopback/tcm_loop.c-523-}

thermal/thermal_sys.c:1097:     result = device_register(&tz->device);
thermal/thermal_sys.c-1098-     if (result) {
thermal/thermal_sys.c-1099-             release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
thermal/thermal_sys.c-1100-             kfree(tz);
thermal/thermal_sys.c-1101-             return ERR_PTR(result);
thermal/thermal_sys.c-1102-     }

video/backlight/lcd.c:215:      rc = device_register(&new_ld->dev);
video/backlight/lcd.c-216-      if (rc) {
video/backlight/lcd.c-217-              kfree(new_ld);
video/backlight/lcd.c-218-              return ERR_PTR(rc);
video/backlight/lcd.c-219-      }


  so as you can see, there's numerous examples of failed calls to
device_register() that never call put_device() and simply bail or call
kfree().  do these examples represent bad code?  because it's not hard
to find lots of examples of it.

  what have i misunderstood here?

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

should failed calls to device_register() always call put_device()?

[Greg KH]

On Sun, May 29, 2011 at 07:21:10AM -0400, Robert P. J. Day wrote:
> On Sun, 29 May 2011, Greg KH wrote:
> 
> > Trust me, dig through the driver core and kobject model, it's tricky
> > to follow, but it's there.  Or at least it was there the last time I
> > did this, that is why I documented it so that no one would have to
> > do that again and they could just easily follow the rules.
> 
>   i'm currently digging through all of that so i'm interested in
> somehow summarizing what is proper code for registering a device and
> what to do if it fails, so let me report what i've discovered so far
> and greg is free to point out where i'm wrong. :-)
> 
>   from drivers/base/core.c, we have the following admonition regarding
> calling device_register():
> 
>  ...
>  * NOTE: _Never_ directly free @dev after calling this function, even
>  * if it returned an error! Always use put_device() to give up the
>  * reference initialized in this function instead.
>  */
> 
> that suggests that, if one calls device_register() and it fails, one
> should *always* (probably immediately) call put_device() or something
> equivalent to give up that reference.  some examples right out of the
> source tree under drivers/ that seem to be following the rules
> (located via a trivial application of grep):
> 
> base/isa.c:147:         error = device_register(&isa_dev->dev);
> base/isa.c-148-         if (error) {
> base/isa.c-149-                 put_device(&isa_dev->dev);
> base/isa.c-150-                 break;
> base/isa.c-151-         }
> 
> media/video/pvrusb2/pvrusb2-sysfs.c:655:        ret = device_register(class_dev);
> media/video/pvrusb2/pvrusb2-sysfs.c-656-        if (ret) {
> media/video/pvrusb2/pvrusb2-sysfs.c-657-                pvr2_trace(PVR2_TRACE_ERROR_LEGS,
> media/video/pvrusb2/pvrusb2-sysfs.c:658:                           "device_register failed");
> media/video/pvrusb2/pvrusb2-sysfs.c-659-                put_device(class_dev);
> media/video/pvrusb2/pvrusb2-sysfs.c-660-                return;
> media/video/pvrusb2/pvrusb2-sysfs.c-661-        }
> 
>   and there's lots more of that.  so if one calls device_register()
> and it fails, the proper approach would be that one can print some
> debugging information, but one *must* thereupon call put_device() to
> return the reference corresponding to the failed register call.  so
> far, so good?

Almost, you can also do whatever you need to do to unwind other things
that are initialized in the larger structure that you embedded the
struct device in.  So you can do lots of things before calling the final
put_device() on your error path if needed.

>   it also seems fine to, after calling put_device(), call kfree() to
> free the enclosing struct, as in:
> 
> memstick/core/memstick.c:467:                   if (device_register(&card->dev)) {
> memstick/core/memstick.c-468-                           put_device(&card->dev);
> memstick/core/memstick.c-469-                           kfree(host->card);
> memstick/core/memstick.c-470-                           host->card = NULL;
> memstick/core/memstick.c-471-                   }
> 
>   the above looks OK since, one you call put_device(), you're free to
> do what you want with that enclosing space, correct?  or no?

No.  The enclosing space would have already been called, so you just
called kfree twice.  If you enable slab debugging, instant oops.  If you
didn't, oops some random time in the field.

>   what is apparently *not* OK is to either call kfree() *before*
> calling put_device(), or to call kfree() and nothing else upon a
> failed device_register() call.  some apparently broken code from the
> current drivers/ directory:

no, again, never call kfree, you already did that in your release
callback that the final put_device() call called.

> firewire/core-device.c:687:             if (device_register(&unit->device) < 0)
> firewire/core-device.c-688-                     goto skip_unit;
> firewire/core-device.c-689-
> firewire/core-device.c-690-             continue;
> firewire/core-device.c-691-
> firewire/core-device.c-692-     skip_unit:
> firewire/core-device.c-693-             kfree(unit);
> firewire/core-device.c-694-     }
> firewire/core-device.c-695-}

Yup, wrong.

> firmware/dmi-id.c:230:  ret = device_register(dmi_dev);
> firmware/dmi-id.c-231-  if (ret)
> firmware/dmi-id.c-232-          goto fail_free_dmi_dev;
> firmware/dmi-id.c-233-
> firmware/dmi-id.c-234-  return 0;
> firmware/dmi-id.c-235-
> firmware/dmi-id.c-236-fail_free_dmi_dev:
> firmware/dmi-id.c-237-  kfree(dmi_dev);

Wrong.

> mca/mca-bus.c:156:      if (device_register(&mca_bus->dev)) {
> mca/mca-bus.c-157-              kfree(mca_bus);
> mca/mca-bus.c-158-              return NULL;
> mca/mca-bus.c-159-      }

Wrong.

> media/video/bt8xx/bttv-gpio.c:97:       err = device_register(&sub->dev);
> media/video/bt8xx/bttv-gpio.c-98-       if (0 != err) {
> media/video/bt8xx/bttv-gpio.c-99-               kfree(sub);
> media/video/bt8xx/bttv-gpio.c-100-              return err;
> media/video/bt8xx/bttv-gpio.c-101-      }

Wrong.

> pci/pcie/portdrv_core.c:331:    retval = device_register(device);
> pci/pcie/portdrv_core.c-332-    if (retval)
> pci/pcie/portdrv_core.c-333-            kfree(pcie);
> pci/pcie/portdrv_core.c-334-    else
> pci/pcie/portdrv_core.c-335-            get_device(device);
> pci/pcie/portdrv_core.c-336-    return retval;
> pci/pcie/portdrv_core.c-337-}

Wrong.

> target/loopback/tcm_loop.c:515: ret = device_register(&tl_hba->dev);
> target/loopback/tcm_loop.c-516- if (ret) {
> target/loopback/tcm_loop.c:517:         printk(KERN_ERR "device_register() failed for"
> target/loopback/tcm_loop.c-518-                         " tl_hba->dev: %d\n", ret);
> target/loopback/tcm_loop.c-519-         return -ENODEV;
> target/loopback/tcm_loop.c-520- }
> target/loopback/tcm_loop.c-521-
> target/loopback/tcm_loop.c-522- return 0;
> target/loopback/tcm_loop.c-523-}

Really wrong.

> thermal/thermal_sys.c:1097:     result = device_register(&tz->device);
> thermal/thermal_sys.c-1098-     if (result) {
> thermal/thermal_sys.c-1099-             release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> thermal/thermal_sys.c-1100-             kfree(tz);
> thermal/thermal_sys.c-1101-             return ERR_PTR(result);
> thermal/thermal_sys.c-1102-     }

wrong.

> video/backlight/lcd.c:215:      rc = device_register(&new_ld->dev);
> video/backlight/lcd.c-216-      if (rc) {
> video/backlight/lcd.c-217-              kfree(new_ld);
> video/backlight/lcd.c-218-              return ERR_PTR(rc);
> video/backlight/lcd.c-219-      }

Wrong.

>   so as you can see, there's numerous examples of failed calls to
> device_register() that never call put_device() and simply bail or call
> kfree().  do these examples represent bad code?  because it's not hard
> to find lots of examples of it.

Yes, that's wrong.  I'm sure you can write a spatch rule to catch and
fix these automatically.

>   what have i misunderstood here?

Almost nothing, see above for the exception.

thanks,

greg k-h

should failed calls to device_register() always call put_device()?

[Robert P. J. Day]

On Sun, 29 May 2011, Greg KH wrote:

> On Sun, May 29, 2011 at 07:21:10AM -0400, Robert P. J. Day wrote:

> >   what is apparently *not* OK is to either call kfree() *before*
> > calling put_device(), or to call kfree() and nothing else upon a
> > failed device_register() call.  some apparently broken code from
> > the current drivers/ directory:
>
> no, again, never call kfree, you already did that in your release
> callback that the final put_device() call called.

  ah, that's the part i was missing -- that the enclosing structure
would have been freed already via the kfree() in the release callback.
i'll look at that code later.

> >   so as you can see, there's numerous examples of failed calls to
> > device_register() that never call put_device() and simply bail or
> > call kfree().  do these examples represent bad code?  because it's
> > not hard to find lots of examples of it.
>
> Yes, that's wrong.  I'm sure you can write a spatch rule to catch
> and fix these automatically.

  i was already looking into that.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

should failed calls to device_register() always call put_device()?

[Greg KH]

On Sat, May 28, 2011 at 06:00:10PM +0200, Belisko Marek wrote:
> Hi Robert,
> 
> On Sat, May 28, 2011 at 5:15 PM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> >
> > ?from drivers/base/core.c, we have the fairly unambiguous advice:
> >
> > * NOTE: _Never_ directly free @dev after calling this function, even
> > * if it returned an error! Always use put_device() to give up the
> > * reference initialized in this function instead.
> > */
> > int device_register(struct device *dev)
> > {
> > ? ? ? ?device_initialize(dev);
> > ? ? ? ?return device_add(dev);
> > }
> >
> > ?and yet, there appears to be driver code that does exactly that,
> > such as this snippet from drivers/w1/w1_int.c (line 86):
> >
> > ? ? ? ?... snip ...
> > ? ? ? ?err = device_register(&dev->dev);
> > ? ? ? ?if (err) {
> > ? ? ? ? ? ? ? ?printk(KERN_ERR "Failed to register master device. err=%d\n", err);
> > ? ? ? ? ? ? ? ?memset(dev, 0, sizeof(struct w1_master));
> > ? ? ? ? ? ? ? ?kfree(dev);
> > ? ? ? ? ? ? ? ?dev = NULL;
> > ? ? ? ?}
> Free is for allocated dev not for struct device so it is OK. IMO thi
> snippet should look like:
> err = device_register(&dev->dev);
> if (err) {
>     printk(KERN_ERR "Failed to register master device. err=%d\n", err);
>     put_device(&dev->dev);

Yes, that is correct.

>     memset(dev, 0, sizeof(struct w1_master));
>     kfree(dev);
>     dev = NULL;

Nope, you just accessed memory that was already freed twice, so you
could have oopsed any one of those times.

After the last put_device() happens, you CAN NOT touch the device again,
as it could be gone (might, might not, you don't really know, all you
know is you said you were done with it so you can't access it again.)

Hope this helps,

greg k-h