[PATCH] x86: mcheck: call put_device on device_register failure

[PATCH] x86: mcheck: call put_device on device_register failure

[Levente Kurusa]

This patch adds a call to put_device() when the device_register()
call has failed. This is required so that the last reference to the
device is given up.

Signed-off-by: Levente Kurusa <levex@linux.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index b3218cd..a389c1d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2272,8 +2272,10 @@ static int mce_device_create(unsigned int cpu)
 	dev->release = &mce_device_release;

 	err = device_register(dev);
-	if (err)
+	if (err) {
+		put_device(dev);
 		return err;
+	}

 	for (i = 0; mce_device_attrs[i]; i++) {
 		err = device_create_file(dev, mce_device_attrs[i]);
-- 
1.8.1.2

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Borislav Petkov]

On Fri, Nov 29, 2013 at 09:28:48PM +0100, Levente Kurusa wrote:
> This patch adds a call to put_device() when the device_register()
> call has failed. This is required so that the last reference to the
> device is given up.

I'd assume this is not something you're actually hitting but have caught
this by code staring...?

> Signed-off-by: Levente Kurusa <levex@linux.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index b3218cd..a389c1d 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -2272,8 +2272,10 @@ static int mce_device_create(unsigned int cpu)
>  	dev->release = &mce_device_release;
> 
>  	err = device_register(dev);
> -	if (err)
> +	if (err) {
> +		put_device(dev);
>  		return err;
> +	}

If we're going to do this, I'd also like to see you add another label
after device_unregister(dev) which also kfrees dev because apparently
we're not doing that either.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Levente Kurusa]

2013-11-29 21:56, Borislav Petkov:
> On Fri, Nov 29, 2013 at 09:28:48PM +0100, Levente Kurusa wrote:
>> This patch adds a call to put_device() when the device_register()
>> call has failed. This is required so that the last reference to the
>> device is given up.
> 
> I'd assume this is not something you're actually hitting but have caught
> this by code staring...?
> 

Yup, I have been staring at it.

> If we're going to do this, I'd also like to see you add another label
> after device_unregister(dev) which also kfrees dev because apparently
> we're not doing that either.
> 

No, if the call to put_device gives up the last reference to the device,
then device_release gets called which in turn frees the memory associated with it.
In this case, mce_device_release() will get called, which is just a simple kfree call.

-- 
Regards,
Levente Kurusa

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Borislav Petkov]

On Sat, Nov 30, 2013 at 08:30:33AM +0100, Levente Kurusa wrote:
> No, if the call to put_device gives up the last reference to the
> device, then device_release gets called which in turn frees the memory
> associated with it. In this case, mce_device_release() will get
> called, which is just a simple kfree call.

Aah, that's that delayed freeing the driver core does, right. Now you
made me go and look into detail:

device_unregister
|->put_device
  |->kobject_put
     |->kref_put(&kobj->kref, kobject_release)
	|->kref_sub(kref, 1, release)
	   |->release
	   |->kobject_release
	      |->kobject_cleanup
	         |->t->release
		 |->device_release
		    |->mce_device_release


Ok, I see it now. :-) :-)

Thanks, I'll take your patch as-is.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Levente Kurusa]

2013-11-30 12:12, Borislav Petkov:
> On Sat, Nov 30, 2013 at 08:30:33AM +0100, Levente Kurusa wrote:
>> No, if the call to put_device gives up the last reference to the
>> device, then device_release gets called which in turn frees the memory
>> associated with it. In this case, mce_device_release() will get
>> called, which is just a simple kfree call.
> 
> Aah, that's that delayed freeing the driver core does, right. Now you
> made me go and look into detail:
> 
> device_unregister
> |->put_device
>   |->kobject_put
>      |->kref_put(&kobj->kref, kobject_release)
> 	|->kref_sub(kref, 1, release)
> 	   |->release
> 	   |->kobject_release
> 	      |->kobject_cleanup
> 	         |->t->release
> 		 |->device_release
> 		    |->mce_device_release
> 
Now this tree makes me wonder if there are devices where
the author forgot to set a device_release or when the put_device
is not called. I will take a look into this.
> 
> Ok, I see it now. :-) :-)
> 
> Thanks, I'll take your patch as-is.
> 
Awesome, thanks! :-)


-- 
Regards,
Levente Kurusa

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Borislav Petkov]

On Sat, Nov 30, 2013 at 12:25:44PM +0100, Levente Kurusa wrote:
> Now this tree makes me wonder if there are devices where the author
> forgot to set a device_release or when the put_device is not called. I
> will take a look into this.

kobject_cleanup() warns about !t->release already.

If you want to fix actual issues and not waste time with potential
issues which have never actually triggered, try building a couple of
randconfigs and look at the output :-)

Also, we have "make W=" which gives you even more :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Levente Kurusa]

2013-11-30 12:32, Borislav Petkov:
> On Sat, Nov 30, 2013 at 12:25:44PM +0100, Levente Kurusa wrote:
>> Now this tree makes me wonder if there are devices where the author
>> forgot to set a device_release or when the put_device is not called. I
>> will take a look into this.
> 
> kobject_cleanup() warns about !t->release already.

Yes, I saw that as well. By that I meant that by doing some identifier searches
for device_register and then checking whether they call put_device and have device_release
registered. Also, I wonder if it would be beneficial to have a generic device_release? Most
of the drivers I quickly swept through only call kfree(). Wouldn't a generic one save
some space?

> 
> If you want to fix actual issues and not waste time with potential
> issues which have never actually triggered, try building a couple of
> randconfigs and look at the output :-)

Yes, I do that daily usually, but most of the time I only get some uninitialized warnings. :-)

> 
> Also, we have "make W=" which gives you even more :-)

What does that do? Never heard of it yet.

-- 
Regards,
Levente Kurusa

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Borislav Petkov]

On Sat, Nov 30, 2013 at 12:44:59PM +0100, Levente Kurusa wrote:
> Yes, I saw that as well. By that I meant that by doing some identifier
> searches for device_register and then checking whether they call
> put_device and have device_release registered. Also, I wonder if it
> would be beneficial to have a generic device_release? Most of the
> drivers I quickly swept through only call kfree(). Wouldn't a generic
> one save some space?

Again, I wouldn't waste my time with hypothetical issues which never
happen - there are many other, real issues which would rather need
attention than what-if ones.

About saving space, that could be worth a try. If you can actually do
that and show numbers to back it up, I'm sure people will have a look.
And if you can't show any space savings, you'll still have learned stuff
along the way.

But don't ask me about whether it makes sense to have a generic
device_release - I'm no driver core and am not even trying. You could
try to answer that question yourself, btw. :)

> Yes, I do that daily usually, but most of the time I only get some
> uninitialized warnings. :-)

You can always try to understand why such warnings get issued and maybe
even fix them if they're legit and the compiler is right. Also, look
through git log for examples how others have fixed such warnings.

For example, sometimes changing code flow instead of simply shutting
up the variable is much better. But in order to do that, you'd need to
understand the code first and try to change it so that it doesn't break
and the warning disappears. This is a very good way, IMO, to get to
really understand what the code does and learn from others.

Another good exercise is trying to boot those random kernels with kvm -
that can catch a bunch of issues too.

The save-space experiment you can also quickly test with kvm. By now you
probably are catching my drift: testing kernels with kvm is awesome! :-)

> What does that do? Never heard of it yet.

Well, you can have a look: scripts/Makefile.build

:-)

Good luck!

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Levente Kurusa]

2013-11-30 13:08 keltezéssel, Borislav Petkov írta:
> On Sat, Nov 30, 2013 at 12:44:59PM +0100, Levente Kurusa wrote:
>> Yes, I saw that as well. By that I meant that by doing some identifier
>> searches for device_register and then checking whether they call
>> put_device and have device_release registered. Also, I wonder if it
>> would be beneficial to have a generic device_release? Most of the
>> drivers I quickly swept through only call kfree(). Wouldn't a generic
>> one save some space?
> 
> Again, I wouldn't waste my time with hypothetical issues which never
> happen - there are many other, real issues which would rather need
> attention than what-if ones.
> 
> About saving space, that could be worth a try. If you can actually do
> that and show numbers to back it up, I'm sure people will have a look.
> And if you can't show any space savings, you'll still have learned stuff
> along the way.
> 
> But don't ask me about whether it makes sense to have a generic
> device_release - I'm no driver core and am not even trying. You could
> try to answer that question yourself, btw. :)

Okay, I will, thanks for the tips! :)
> 
>> Yes, I do that daily usually, but most of the time I only get some
>> uninitialized warnings. :-)
> 
> You can always try to understand why such warnings get issued and maybe
> even fix them if they're legit and the compiler is right. Also, look
> through git log for examples how others have fixed such warnings.
Yes, but there are some fake one where for example it doesn't recognize the
pci_read_config_byte call as something which could write to its args. So most
of them are fake warnings.

> 
> For example, sometimes changing code flow instead of simply shutting
> up the variable is much better. But in order to do that, you'd need to
> understand the code first and try to change it so that it doesn't break
> and the warning disappears. This is a very good way, IMO, to get to
> really understand what the code does and learn from others.
> 
> Another good exercise is trying to boot those random kernels with kvm -
> that can catch a bunch of issues too.
> 
> The save-space experiment you can also quickly test with kvm. By now you
> probably are catching my drift: testing kernels with kvm is awesome! :-)
I will try kvm, didn't use that before. :p

> 
>> What does that do? Never heard of it yet.
> 
> Well, you can have a look: scripts/Makefile.build

Ahh, now I see. Just didn't know where to look.

> 
> :-)
> 
> Good luck!
Thank you and for your time! :)

-- 
Regards,
Levente Kurusa

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

On Sat, Nov 30, 2013 at 12:12:14PM +0100, Borislav Petkov wrote:
> Date:	Sat, 30 Nov 2013 12:12:14 +0100
> From: Borislav Petkov <bp@alien8.de>
> To: Levente Kurusa <levex@linux.com>
> Cc: Ingo Molnar <mingo@kernel.org>, Thomas Gleixner <tglx@linutronix.de>,
>  Tony Luck <tony.luck@intel.com>, "H. Peter Anvin" <hpa@zytor.com>,
>  x86@kernel.org, EDAC <linux-edac@vger.kernel.org>, LKML
>  <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Sat, Nov 30, 2013 at 08:30:33AM +0100, Levente Kurusa wrote:
> > No, if the call to put_device gives up the last reference to the
> > device, then device_release gets called which in turn frees the memory
> > associated with it. In this case, mce_device_release() will get
> > called, which is just a simple kfree call.
> 
> Aah, that's that delayed freeing the driver core does, right. Now you
> made me go and look into detail:
> 
> device_unregister
> |->put_device
>   |->kobject_put
>      |->kref_put(&kobj->kref, kobject_release)
> 	|->kref_sub(kref, 1, release)
> 	   |->release
> 	   |->kobject_release
> 	      |->kobject_cleanup
> 	         |->t->release
> 		 |->device_release
> 		    |->mce_device_release
> 
> 
> Ok, I see it now. :-) :-)
> 
> Thanks, I'll take your patch as-is.
> 

I have some concerns about it. if device_register is failed, it will
backtraces all kinds of conditions automatically, including put_device
definately. So do we really need an extra put_device when it returns
failure?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Borislav Petkov]

Can you please fix your

Mail-Followup-To:

header? It is impossible to reply to your emails without fiddling with
the To: and Cc: by hand which gets very annoying over time.

On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
> I have some concerns about it. if device_register is failed, it will
> backtraces all kinds of conditions automatically, including put_device
> definately. So do we really need an extra put_device when it returns
> failure?

Do you mean the "done:" label in device_add() which does put_device()
and which gets called by device_register()?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
> Date: Tue, 3 Dec 2013 18:01:50 +0100
> From: Borislav Petkov <bp@alien8.de>
> To: "Chen, Gong" <gong.chen@linux.intel.com>
> Cc: Levente Kurusa <levex@linux.com>, Ingo Molnar <mingo@kernel.org>,
>  Thomas Gleixner <tglx@linutronix.de>, Tony Luck <tony.luck@intel.com>, "H.
>  Peter Anvin" <hpa@zytor.com>, x86@kernel.org, EDAC
>  <linux-edac@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> Can you please fix your
> 
> Mail-Followup-To:
> 
> header? It is impossible to reply to your emails without fiddling with
> the To: and Cc: by hand which gets very annoying over time.

I add some configs in my muttrc. Hope it works.

> 
> On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
> > I have some concerns about it. if device_register is failed, it will
> > backtraces all kinds of conditions automatically, including put_device
> > definately. So do we really need an extra put_device when it returns
> > failure?
> 
> Do you mean the "done:" label in device_add() which does put_device()
> and which gets called by device_register()?
> 

Not only. I noticed that another put_device under label "Error:".

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Levente Kurusa]

2013-12-04 08:38, Chen, Gong:
> On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
>> Date: Tue, 3 Dec 2013 18:01:50 +0100
>> From: Borislav Petkov <bp@alien8.de>
>> To: "Chen, Gong" <gong.chen@linux.intel.com>
>> Cc: Levente Kurusa <levex@linux.com>, Ingo Molnar <mingo@kernel.org>,
>>  Thomas Gleixner <tglx@linutronix.de>, Tony Luck <tony.luck@intel.com>, "H.
>>  Peter Anvin" <hpa@zytor.com>, x86@kernel.org, EDAC
>>  <linux-edac@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
>> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
>> User-Agent: Mutt/1.5.21 (2010-09-15)
>>
>> Can you please fix your
>>
>> Mail-Followup-To:
>>
>> header? It is impossible to reply to your emails without fiddling with
>> the To: and Cc: by hand which gets very annoying over time.
> 
> I add some configs in my muttrc. Hope it works.
> 
>>
>> On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
>>> I have some concerns about it. if device_register is failed, it will
>>> backtraces all kinds of conditions automatically, including put_device
>>> definately. So do we really need an extra put_device when it returns
>>> failure?
>>
>> Do you mean the "done:" label in device_add() which does put_device()
>> and which gets called by device_register()?
>>
> 
> Not only. I noticed that another put_device under label "Error:".
> 

That label is called when we failed to add the kobject to its parent.
It just puts the parent of the device. I don't think it has anything
to do with us put_device()-ing the actual device too.

-- 
Regards,
Levente Kurusa

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 3568 bytes --]

On Wed, Dec 04, 2013 at 07:39:07PM +0100, Levente Kurusa wrote:
> Date:	Wed, 04 Dec 2013 19:39:07 +0100
> From: Levente Kurusa <levex@linux.com>
> To: Borislav Petkov <bp@alien8.de>, Ingo Molnar <mingo@kernel.org>, Thomas
>  Gleixner <tglx@linutronix.de>, Tony Luck <tony.luck@intel.com>, "H. Peter
>  Anvin" <hpa@zytor.com>, x86@kernel.org, EDAC <linux-edac@vger.kernel.org>,
>  LKML <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101
>  Thunderbird/24.1.0
> 
> 2013-12-04 08:38, Chen, Gong:
> > On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
> >> Date: Tue, 3 Dec 2013 18:01:50 +0100
> >> From: Borislav Petkov <bp@alien8.de>
> >> To: "Chen, Gong" <gong.chen@linux.intel.com>
> >> Cc: Levente Kurusa <levex@linux.com>, Ingo Molnar <mingo@kernel.org>,
> >>  Thomas Gleixner <tglx@linutronix.de>, Tony Luck <tony.luck@intel.com>, "H.
> >>  Peter Anvin" <hpa@zytor.com>, x86@kernel.org, EDAC
> >>  <linux-edac@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
> >> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
> >> User-Agent: Mutt/1.5.21 (2010-09-15)
> >>
> >> Can you please fix your
> >>
> >> Mail-Followup-To:
> >>
> >> header? It is impossible to reply to your emails without fiddling with
> >> the To: and Cc: by hand which gets very annoying over time.
> > 
> > I add some configs in my muttrc. Hope it works.
> > 
> >>
> >> On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
> >>> I have some concerns about it. if device_register is failed, it will
> >>> backtraces all kinds of conditions automatically, including put_device
> >>> definately. So do we really need an extra put_device when it returns
> >>> failure?
> >>
> >> Do you mean the "done:" label in device_add() which does put_device()
> >> and which gets called by device_register()?
> >>
> > 
> > Not only. I noticed that another put_device under label "Error:".
> > 
> 
> That label is called when we failed to add the kobject to its parent.
> It just puts the parent of the device. I don't think it has anything
> to do with us put_device()-ing the actual device too.
> 
OK, you are right. I read some kobject related codes and get:

static inline void kref_init(struct kref *kref)
{
        atomic_set(&kref->refcount, 1);
}

The init refcount is 1, which means even if we meet an error and put_device
in device_add, we still need an extra put_device to make refcount = 0
and then release the dev object.

BTW, from the comments of 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. "

Many caller don't follow this logic. For example:
in arch/arm/common/locomo.c
locomo_init_one_child
...
        ret = device_register(&dev->dev);
        if (ret) {
out:
                kfree(dev);
        }
...
 
in arch/parisc/kernel/drivers.c
create_tree_node
...
        if (device_register(&dev->dev)) {
                kfree(dev);
                return NULL;
        }
...

etc.

Maybe we need one more patch to fix them all. :-)
> -- 
> Regards,
> Levente Kurusa
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] x86: mcheck: call put_device on device_register failure

[Levente Kurusa]

2013-12-05 03:57 keltezéssel, Chen, Gong írta:
> On Wed, Dec 04, 2013 at 07:39:07PM +0100, Levente Kurusa wrote:
>> Date:	Wed, 04 Dec 2013 19:39:07 +0100
>> From: Levente Kurusa <levex@linux.com>
>> To: Borislav Petkov <bp@alien8.de>, Ingo Molnar <mingo@kernel.org>, Thomas
>>  Gleixner <tglx@linutronix.de>, Tony Luck <tony.luck@intel.com>, "H. Peter
>>  Anvin" <hpa@zytor.com>, x86@kernel.org, EDAC <linux-edac@vger.kernel.org>,
>>  LKML <linux-kernel@vger.kernel.org>
>> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
>> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101
>>  Thunderbird/24.1.0
>>
>> 2013-12-04 08:38, Chen, Gong:
>>> On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
>>>> Date: Tue, 3 Dec 2013 18:01:50 +0100
>>>> From: Borislav Petkov <bp@alien8.de>
>>>> To: "Chen, Gong" <gong.chen@linux.intel.com>
>>>> Cc: Levente Kurusa <levex@linux.com>, Ingo Molnar <mingo@kernel.org>,
>>>>  Thomas Gleixner <tglx@linutronix.de>, Tony Luck <tony.luck@intel.com>, "H.
>>>>  Peter Anvin" <hpa@zytor.com>, x86@kernel.org, EDAC
>>>>  <linux-edac@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
>>>> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
>>>> User-Agent: Mutt/1.5.21 (2010-09-15)
>>>>
>>>> Can you please fix your
>>>>
>>>> Mail-Followup-To:
>>>>
>>>> header? It is impossible to reply to your emails without fiddling with
>>>> the To: and Cc: by hand which gets very annoying over time.
>>>
>>> I add some configs in my muttrc. Hope it works.
>>>
>>>>
>>>> On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
>>>>> I have some concerns about it. if device_register is failed, it will
>>>>> backtraces all kinds of conditions automatically, including put_device
>>>>> definately. So do we really need an extra put_device when it returns
>>>>> failure?
>>>>
>>>> Do you mean the "done:" label in device_add() which does put_device()
>>>> and which gets called by device_register()?
>>>>
>>>
>>> Not only. I noticed that another put_device under label "Error:".
>>>
>>
>> That label is called when we failed to add the kobject to its parent.
>> It just puts the parent of the device. I don't think it has anything
>> to do with us put_device()-ing the actual device too.
>>
> OK, you are right. I read some kobject related codes and get:
> 
> static inline void kref_init(struct kref *kref)
> {
>         atomic_set(&kref->refcount, 1);
> }
> 
> The init refcount is 1, which means even if we meet an error and put_device
> in device_add, we still need an extra put_device to make refcount = 0
> and then release the dev object.

Exactly. This is why the comment you have found later on. :-)
> 
> BTW, from the comments of 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. "
> 
> Many caller don't follow this logic. For example:
> in arch/arm/common/locomo.c
> locomo_init_one_child
> ...
>         ret = device_register(&dev->dev);
>         if (ret) {
> out:
>                 kfree(dev);

Umm, but it frees dev which is a container_of dev->dev so dev->dev is not
even freed. This is a memleak as well.
>         }
> ...
>  
> in arch/parisc/kernel/drivers.c
> create_tree_node
> ...
>         if (device_register(&dev->dev)) {
>                 kfree(dev);

Same here.
>                 return NULL;
>         }
> ...
> 
> etc.
> 
> Maybe we need one more patch to fix them all. :-)

Yes, I will post a series which fixes this during the weekend when I am not that busy. :-)

-- 
Regards,
Levente Kurusa