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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, May 28, 2011 at 6:29 PM, Robert P. J. Day <rpjday@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@xxxxxxxxxxxxxxxxx
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies



[Index of Archives]     [Newbies FAQ]     [Linux Kernel Mentors]     [Linux Kernel Development]     [IETF Annouce]     [Git]     [Networking]     [Security]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux SCSI]     [Linux ACPI]
  Powered by Linux