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

[greg@kroah.com (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