should failed calls to device_register() always call put_device()?
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 ========================================================================
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@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
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 ========================================================================
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
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 ========================================================================
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
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
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 ========================================================================
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
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 ========================================================================
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
participants (3)
-
Belisko Marek -
Greg KH -
Robert P. J. Day