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 ========================================================================