should failed calls to device_register() always call put_device()?
Robert P. J. Day
rpjday at crashcourse.ca
Sun May 29 07:21:10 EDT 2011
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
========================================================================
More information about the Kernelnewbies
mailing list