[PATCH 04/14] memstick: core: fix device_register() error handling

[PATCH 04/14] memstick: core: fix device_register() error handling

[Vasiliy Kulikov]

If device_register() fails then call put_device().
See comment to device_register.

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 compile tested.

 drivers/memstick/core/memstick.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index c00fe82..4303b7e 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
 		if (!host->card) {
 			host->card = card;
 			if (device_register(&card->dev)) {
+				put_device(&card->dev);
 				kfree(host->card);
 				host->card = NULL;
 			}
-- 
1.7.0.4

[PATCH 04/14] memstick: core: fix device_register() error handling

[Vasiliy Kulikov]

If device_register() fails then call put_device().
See comment to device_register.

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 compile tested.

 drivers/memstick/core/memstick.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index c00fe82..4303b7e 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
 		if (!host->card) {
 			host->card = card;
 			if (device_register(&card->dev)) {
+				put_device(&card->dev);
 				kfree(host->card);
 				host->card = NULL;
 			}
-- 
1.7.0.4

Re: [PATCH 04/14] memstick: core: fix device_register() error

[Andrew Morton]

On Sun, 19 Sep 2010 16:54:49 +0400
Vasiliy Kulikov <segooon@gmail.com> wrote:

> If device_register() fails then call put_device().
> See comment to device_register.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  compile tested.
> 
>  drivers/memstick/core/memstick.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> index c00fe82..4303b7e 100644
> --- a/drivers/memstick/core/memstick.c
> +++ b/drivers/memstick/core/memstick.c
> @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
>  		if (!host->card) {
>  			host->card = card;
>  			if (device_register(&card->dev)) {
> +				put_device(&card->dev);
>  				kfree(host->card);
>  				host->card = NULL;
>  			}

A failed device_register() takes a bogus ref on the not-registered
device?  It's no surprise that people are getting this wrong.  

The principle of least surprise says: fix device_register()!

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Andrew Morton]

On Sun, 19 Sep 2010 16:54:49 +0400
Vasiliy Kulikov <segooon@gmail.com> wrote:

> If device_register() fails then call put_device().
> See comment to device_register.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  compile tested.
> 
>  drivers/memstick/core/memstick.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> index c00fe82..4303b7e 100644
> --- a/drivers/memstick/core/memstick.c
> +++ b/drivers/memstick/core/memstick.c
> @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
>  		if (!host->card) {
>  			host->card = card;
>  			if (device_register(&card->dev)) {
> +				put_device(&card->dev);
>  				kfree(host->card);
>  				host->card = NULL;
>  			}

A failed device_register() takes a bogus ref on the not-registered
device?  It's no surprise that people are getting this wrong.  

The principle of least surprise says: fix device_register()!

Re: [PATCH 04/14] memstick: core: fix device_register() error

[Greg KH]

On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote:
> On Sun, 19 Sep 2010 16:54:49 +0400
> Vasiliy Kulikov <segooon@gmail.com> wrote:
> 
> > If device_register() fails then call put_device().
> > See comment to device_register.
> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
> >  compile tested.
> > 
> >  drivers/memstick/core/memstick.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> > index c00fe82..4303b7e 100644
> > --- a/drivers/memstick/core/memstick.c
> > +++ b/drivers/memstick/core/memstick.c
> > @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
> >  		if (!host->card) {
> >  			host->card = card;
> >  			if (device_register(&card->dev)) {
> > +				put_device(&card->dev);
> >  				kfree(host->card);
> >  				host->card = NULL;
> >  			}
> 
> A failed device_register() takes a bogus ref on the not-registered
> device?  It's no surprise that people are getting this wrong.  
> 
> The principle of least surprise says: fix device_register()!

One might think that, but it's a bit more difficult.

How does device_register know it should destroy the device if it fails?

Here's how it works:
 - device_register is just a wrapper around device_initialize() and
   device_add()
     - device_initialize() can't do anything wrong, so it's safe, BUT,
       at this point in time, the reference for the device is
       incremented, so any caller must now drop the reference and
       properly free stuff.
     - device_add() does a lot.

Hm, I guess, because we "know" in device_register() that we must drop
something if device_add() fails, then I guess it's not being consistant
with it's own calls...

So, something as simple as this?


diff --git a/drivers/base/core.c b/drivers/base/core.c
index d1b2c9a..4ba8599 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1084,14 +1084,16 @@ name_error:
  * have a clearly defined need to use and refcount the device
  * before it is added to the hierarchy.
  *
- * 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)
 {
+	int retval;
+
 	device_initialize(dev);
-	return device_add(dev);
+	retval = device_add(dev);
+	if (retval)
+		put_device(dev);
+	return retval;
 }
 
 /**
     	


Kay, what am I missing here, why can't we just do this?  Hm, the
side-affect might be that if device_register() fails, NO ONE had better
touch that device again, as it might have just been freed from the
system.  I wonder if that will cause problems...

thanks,

greg k-h

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Greg KH]

On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote:
> On Sun, 19 Sep 2010 16:54:49 +0400
> Vasiliy Kulikov <segooon@gmail.com> wrote:
> 
> > If device_register() fails then call put_device().
> > See comment to device_register.
> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
> >  compile tested.
> > 
> >  drivers/memstick/core/memstick.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> > index c00fe82..4303b7e 100644
> > --- a/drivers/memstick/core/memstick.c
> > +++ b/drivers/memstick/core/memstick.c
> > @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
> >  		if (!host->card) {
> >  			host->card = card;
> >  			if (device_register(&card->dev)) {
> > +				put_device(&card->dev);
> >  				kfree(host->card);
> >  				host->card = NULL;
> >  			}
> 
> A failed device_register() takes a bogus ref on the not-registered
> device?  It's no surprise that people are getting this wrong.  
> 
> The principle of least surprise says: fix device_register()!

One might think that, but it's a bit more difficult.

How does device_register know it should destroy the device if it fails?

Here's how it works:
 - device_register is just a wrapper around device_initialize() and
   device_add()
     - device_initialize() can't do anything wrong, so it's safe, BUT,
       at this point in time, the reference for the device is
       incremented, so any caller must now drop the reference and
       properly free stuff.
     - device_add() does a lot.

Hm, I guess, because we "know" in device_register() that we must drop
something if device_add() fails, then I guess it's not being consistant
with it's own calls...

So, something as simple as this?


diff --git a/drivers/base/core.c b/drivers/base/core.c
index d1b2c9a..4ba8599 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1084,14 +1084,16 @@ name_error:
  * have a clearly defined need to use and refcount the device
  * before it is added to the hierarchy.
  *
- * 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)
 {
+	int retval;
+
 	device_initialize(dev);
-	return device_add(dev);
+	retval = device_add(dev);
+	if (retval)
+		put_device(dev);
+	return retval;
 }
 
 /**
     	


Kay, what am I missing here, why can't we just do this?  Hm, the
side-affect might be that if device_register() fails, NO ONE had better
touch that device again, as it might have just been freed from the
system.  I wonder if that will cause problems...

thanks,

greg k-h

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Kay Sievers]

On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:

>  int device_register(struct device *dev)
>  {
> +       int retval;
> +
>        device_initialize(dev);
> -       return device_add(dev);
> +       retval = device_add(dev);
> +       if (retval)
> +               put_device(dev);
> +       return retval;
>  }

> Kay, what am I missing here, why can't we just do this?  Hm, the
> side-affect might be that if device_register() fails, NO ONE had better
> touch that device again, as it might have just been freed from the
> system.  I wonder if that will cause problems...

That looks right, besides that there might be callers already doing
this. Which needs to be checked.

I never liked this pretty useless "convenience API", which just wraps
two simple functions and the first one can never fail anyway.

We better remove that device_register() stuff entirely in the long
run, it's not doing any good. At the kobject level we killed the same
stuff already long ago.

Kay

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Kay Sievers]

On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:

>  int device_register(struct device *dev)
>  {
> +       int retval;
> +
>        device_initialize(dev);
> -       return device_add(dev);
> +       retval = device_add(dev);
> +       if (retval)
> +               put_device(dev);
> +       return retval;
>  }

> Kay, what am I missing here, why can't we just do this?  Hm, the
> side-affect might be that if device_register() fails, NO ONE had better
> touch that device again, as it might have just been freed from the
> system.  I wonder if that will cause problems...

That looks right, besides that there might be callers already doing
this. Which needs to be checked.

I never liked this pretty useless "convenience API", which just wraps
two simple functions and the first one can never fail anyway.

We better remove that device_register() stuff entirely in the long
run, it's not doing any good. At the kobject level we killed the same
stuff already long ago.

Kay

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Boaz Harrosh]

On 09/22/2010 10:53 AM, Kay Sievers wrote:
> On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> 
>>  int device_register(struct device *dev)
>>  {
>> +       int retval;
>> +
>>        device_initialize(dev);
>> -       return device_add(dev);
>> +       retval = device_add(dev);
>> +       if (retval)
>> +               put_device(dev);
>> +       return retval;
>>  }
> 
>> Kay, what am I missing here, why can't we just do this?  Hm, the
>> side-affect might be that if device_register() fails, NO ONE had better
>> touch that device again, as it might have just been freed from the
>> system.  I wonder if that will cause problems...
> 
> That looks right, besides that there might be callers already doing
> this. Which needs to be checked.
> 
> I never liked this pretty useless "convenience API", which just wraps
> two simple functions and the first one can never fail anyway.
> 
> We better remove that device_register() stuff entirely in the long
> run, it's not doing any good. At the kobject level we killed the same
> stuff already long ago.
> 

That would be fine, and ping me when you do it, I'll help with my
driver. But don't forget to let us have a way to embed a device inside
a bigger structure.

For meanwhile Please check the patch James sent to add_device that cleans
up the allocation of the kobj.name member. (And the comment made there)

> Kay

Thanks
Boaz

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Boaz Harrosh]

On 09/22/2010 10:53 AM, Kay Sievers wrote:
> On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> 
>>  int device_register(struct device *dev)
>>  {
>> +       int retval;
>> +
>>        device_initialize(dev);
>> -       return device_add(dev);
>> +       retval = device_add(dev);
>> +       if (retval)
>> +               put_device(dev);
>> +       return retval;
>>  }
> 
>> Kay, what am I missing here, why can't we just do this?  Hm, the
>> side-affect might be that if device_register() fails, NO ONE had better
>> touch that device again, as it might have just been freed from the
>> system.  I wonder if that will cause problems...
> 
> That looks right, besides that there might be callers already doing
> this. Which needs to be checked.
> 
> I never liked this pretty useless "convenience API", which just wraps
> two simple functions and the first one can never fail anyway.
> 
> We better remove that device_register() stuff entirely in the long
> run, it's not doing any good. At the kobject level we killed the same
> stuff already long ago.
> 

That would be fine, and ping me when you do it, I'll help with my
driver. But don't forget to let us have a way to embed a device inside
a bigger structure.

For meanwhile Please check the patch James sent to add_device that cleans
up the allocation of the kobj.name member. (And the comment made there)

> Kay

Thanks
Boaz

Re: [PATCH 04/14] memstick: core: fix device_register() error

[Greg KH]

On Wed, Sep 22, 2010 at 12:02:30PM +0200, Boaz Harrosh wrote:
> On 09/22/2010 10:53 AM, Kay Sievers wrote:
> > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> > 
> >>  int device_register(struct device *dev)
> >>  {
> >> +       int retval;
> >> +
> >>        device_initialize(dev);
> >> -       return device_add(dev);
> >> +       retval = device_add(dev);
> >> +       if (retval)
> >> +               put_device(dev);
> >> +       return retval;
> >>  }
> > 
> >> Kay, what am I missing here, why can't we just do this?  Hm, the
> >> side-affect might be that if device_register() fails, NO ONE had better
> >> touch that device again, as it might have just been freed from the
> >> system.  I wonder if that will cause problems...
> > 
> > That looks right, besides that there might be callers already doing
> > this. Which needs to be checked.
> > 
> > I never liked this pretty useless "convenience API", which just wraps
> > two simple functions and the first one can never fail anyway.
> > 
> > We better remove that device_register() stuff entirely in the long
> > run, it's not doing any good. At the kobject level we killed the same
> > stuff already long ago.
> > 
> 
> That would be fine, and ping me when you do it, I'll help with my
> driver. But don't forget to let us have a way to embed a device inside
> a bigger structure.

That's what you should be doing anyway, so it shouldn't be a problem.

> For meanwhile Please check the patch James sent to add_device that cleans
> up the allocation of the kobj.name member. (And the comment made there)

See my other email why that isn't a good idea.

thanks,

greg k-h

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Greg KH]

On Wed, Sep 22, 2010 at 12:02:30PM +0200, Boaz Harrosh wrote:
> On 09/22/2010 10:53 AM, Kay Sievers wrote:
> > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> > 
> >>  int device_register(struct device *dev)
> >>  {
> >> +       int retval;
> >> +
> >>        device_initialize(dev);
> >> -       return device_add(dev);
> >> +       retval = device_add(dev);
> >> +       if (retval)
> >> +               put_device(dev);
> >> +       return retval;
> >>  }
> > 
> >> Kay, what am I missing here, why can't we just do this?  Hm, the
> >> side-affect might be that if device_register() fails, NO ONE had better
> >> touch that device again, as it might have just been freed from the
> >> system.  I wonder if that will cause problems...
> > 
> > That looks right, besides that there might be callers already doing
> > this. Which needs to be checked.
> > 
> > I never liked this pretty useless "convenience API", which just wraps
> > two simple functions and the first one can never fail anyway.
> > 
> > We better remove that device_register() stuff entirely in the long
> > run, it's not doing any good. At the kobject level we killed the same
> > stuff already long ago.
> > 
> 
> That would be fine, and ping me when you do it, I'll help with my
> driver. But don't forget to let us have a way to embed a device inside
> a bigger structure.

That's what you should be doing anyway, so it shouldn't be a problem.

> For meanwhile Please check the patch James sent to add_device that cleans
> up the allocation of the kobj.name member. (And the comment made there)

See my other email why that isn't a good idea.

thanks,

greg k-h

Re: [PATCH 04/14] memstick: core: fix device_register() error

[James Bottomley]

On Wed, 2010-09-22 at 08:47 -0700, Greg KH wrote:
> On Wed, Sep 22, 2010 at 12:02:30PM +0200, Boaz Harrosh wrote:
> > On 09/22/2010 10:53 AM, Kay Sievers wrote:
> > > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> > > 
> > >>  int device_register(struct device *dev)
> > >>  {
> > >> +       int retval;
> > >> +
> > >>        device_initialize(dev);
> > >> -       return device_add(dev);
> > >> +       retval = device_add(dev);
> > >> +       if (retval)
> > >> +               put_device(dev);
> > >> +       return retval;
> > >>  }
> > > 
> > >> Kay, what am I missing here, why can't we just do this?  Hm, the
> > >> side-affect might be that if device_register() fails, NO ONE had better
> > >> touch that device again, as it might have just been freed from the
> > >> system.  I wonder if that will cause problems...
> > > 
> > > That looks right, besides that there might be callers already doing
> > > this. Which needs to be checked.
> > > 
> > > I never liked this pretty useless "convenience API", which just wraps
> > > two simple functions and the first one can never fail anyway.
> > > 
> > > We better remove that device_register() stuff entirely in the long
> > > run, it's not doing any good. At the kobject level we killed the same
> > > stuff already long ago.
> > > 
> > 
> > That would be fine, and ping me when you do it, I'll help with my
> > driver. But don't forget to let us have a way to embed a device inside
> > a bigger structure.
> 
> That's what you should be doing anyway, so it shouldn't be a problem.
> 
> > For meanwhile Please check the patch James sent to add_device that cleans
> > up the allocation of the kobj.name member. (And the comment made there)
> 
> See my other email why that isn't a good idea.

What other email? ... neither you nor Kay replied after either Boaz's
patch or the corrected one were posted.

James

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[James Bottomley]

On Wed, 2010-09-22 at 08:47 -0700, Greg KH wrote:
> On Wed, Sep 22, 2010 at 12:02:30PM +0200, Boaz Harrosh wrote:
> > On 09/22/2010 10:53 AM, Kay Sievers wrote:
> > > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> > > 
> > >>  int device_register(struct device *dev)
> > >>  {
> > >> +       int retval;
> > >> +
> > >>        device_initialize(dev);
> > >> -       return device_add(dev);
> > >> +       retval = device_add(dev);
> > >> +       if (retval)
> > >> +               put_device(dev);
> > >> +       return retval;
> > >>  }
> > > 
> > >> Kay, what am I missing here, why can't we just do this?  Hm, the
> > >> side-affect might be that if device_register() fails, NO ONE had better
> > >> touch that device again, as it might have just been freed from the
> > >> system.  I wonder if that will cause problems...
> > > 
> > > That looks right, besides that there might be callers already doing
> > > this. Which needs to be checked.
> > > 
> > > I never liked this pretty useless "convenience API", which just wraps
> > > two simple functions and the first one can never fail anyway.
> > > 
> > > We better remove that device_register() stuff entirely in the long
> > > run, it's not doing any good. At the kobject level we killed the same
> > > stuff already long ago.
> > > 
> > 
> > That would be fine, and ping me when you do it, I'll help with my
> > driver. But don't forget to let us have a way to embed a device inside
> > a bigger structure.
> 
> That's what you should be doing anyway, so it shouldn't be a problem.
> 
> > For meanwhile Please check the patch James sent to add_device that cleans
> > up the allocation of the kobj.name member. (And the comment made there)
> 
> See my other email why that isn't a good idea.

What other email? ... neither you nor Kay replied after either Boaz's
patch or the corrected one were posted.

James

Re: [PATCH 04/14] memstick: core: fix device_register() error

[Greg KH]

On Wed, Sep 22, 2010 at 10:56:36AM -0500, James Bottomley wrote:
> On Wed, 2010-09-22 at 08:47 -0700, Greg KH wrote:
> > On Wed, Sep 22, 2010 at 12:02:30PM +0200, Boaz Harrosh wrote:
> > > On 09/22/2010 10:53 AM, Kay Sievers wrote:
> > > > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> > > > 
> > > >>  int device_register(struct device *dev)
> > > >>  {
> > > >> +       int retval;
> > > >> +
> > > >>        device_initialize(dev);
> > > >> -       return device_add(dev);
> > > >> +       retval = device_add(dev);
> > > >> +       if (retval)
> > > >> +               put_device(dev);
> > > >> +       return retval;
> > > >>  }
> > > > 
> > > >> Kay, what am I missing here, why can't we just do this?  Hm, the
> > > >> side-affect might be that if device_register() fails, NO ONE had better
> > > >> touch that device again, as it might have just been freed from the
> > > >> system.  I wonder if that will cause problems...
> > > > 
> > > > That looks right, besides that there might be callers already doing
> > > > this. Which needs to be checked.
> > > > 
> > > > I never liked this pretty useless "convenience API", which just wraps
> > > > two simple functions and the first one can never fail anyway.
> > > > 
> > > > We better remove that device_register() stuff entirely in the long
> > > > run, it's not doing any good. At the kobject level we killed the same
> > > > stuff already long ago.
> > > > 
> > > 
> > > That would be fine, and ping me when you do it, I'll help with my
> > > driver. But don't forget to let us have a way to embed a device inside
> > > a bigger structure.
> > 
> > That's what you should be doing anyway, so it shouldn't be a problem.
> > 
> > > For meanwhile Please check the patch James sent to add_device that cleans
> > > up the allocation of the kobj.name member. (And the comment made there)
> > 
> > See my other email why that isn't a good idea.
> 
> What other email? ... neither you nor Kay replied after either Boaz's
> patch or the corrected one were posted.

See my other email a minute ago in response to this thread.

thanks,

greg k-h

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Greg KH]

On Wed, Sep 22, 2010 at 10:56:36AM -0500, James Bottomley wrote:
> On Wed, 2010-09-22 at 08:47 -0700, Greg KH wrote:
> > On Wed, Sep 22, 2010 at 12:02:30PM +0200, Boaz Harrosh wrote:
> > > On 09/22/2010 10:53 AM, Kay Sievers wrote:
> > > > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> > > > 
> > > >>  int device_register(struct device *dev)
> > > >>  {
> > > >> +       int retval;
> > > >> +
> > > >>        device_initialize(dev);
> > > >> -       return device_add(dev);
> > > >> +       retval = device_add(dev);
> > > >> +       if (retval)
> > > >> +               put_device(dev);
> > > >> +       return retval;
> > > >>  }
> > > > 
> > > >> Kay, what am I missing here, why can't we just do this?  Hm, the
> > > >> side-affect might be that if device_register() fails, NO ONE had better
> > > >> touch that device again, as it might have just been freed from the
> > > >> system.  I wonder if that will cause problems...
> > > > 
> > > > That looks right, besides that there might be callers already doing
> > > > this. Which needs to be checked.
> > > > 
> > > > I never liked this pretty useless "convenience API", which just wraps
> > > > two simple functions and the first one can never fail anyway.
> > > > 
> > > > We better remove that device_register() stuff entirely in the long
> > > > run, it's not doing any good. At the kobject level we killed the same
> > > > stuff already long ago.
> > > > 
> > > 
> > > That would be fine, and ping me when you do it, I'll help with my
> > > driver. But don't forget to let us have a way to embed a device inside
> > > a bigger structure.
> > 
> > That's what you should be doing anyway, so it shouldn't be a problem.
> > 
> > > For meanwhile Please check the patch James sent to add_device that cleans
> > > up the allocation of the kobj.name member. (And the comment made there)
> > 
> > See my other email why that isn't a good idea.
> 
> What other email? ... neither you nor Kay replied after either Boaz's
> patch or the corrected one were posted.

See my other email a minute ago in response to this thread.

thanks,

greg k-h

Re: [PATCH 04/14] memstick: core: fix device_register() error

[James Bottomley]

On Wed, 2010-09-22 at 09:20 -0700, Greg KH wrote:
> > What other email? ... neither you nor Kay replied after either Boaz's
> > patch or the corrected one were posted.
> 
> See my other email a minute ago in response to this thread.

Can we start again?  You've been replying to a patch to do a
put_device() in device_register() which everyone agrees looks wrong.

The proposal to fix all of this is to free the kobject name and NULL it
out in device_add().  That fixes all the places you and kay broke when
you added the variable length name stuff.

James

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[James Bottomley]

On Wed, 2010-09-22 at 09:20 -0700, Greg KH wrote:
> > What other email? ... neither you nor Kay replied after either Boaz's
> > patch or the corrected one were posted.
> 
> See my other email a minute ago in response to this thread.

Can we start again?  You've been replying to a patch to do a
put_device() in device_register() which everyone agrees looks wrong.

The proposal to fix all of this is to free the kobject name and NULL it
out in device_add().  That fixes all the places you and kay broke when
you added the variable length name stuff.

James

Re: [PATCH 04/14] memstick: core: fix device_register() error

[Greg KH]

On Wed, Sep 22, 2010 at 10:53:21AM +0200, Kay Sievers wrote:
> On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> 
> >  int device_register(struct device *dev)
> >  {
> > +       int retval;
> > +
> >        device_initialize(dev);
> > -       return device_add(dev);
> > +       retval = device_add(dev);
> > +       if (retval)
> > +               put_device(dev);
> > +       return retval;
> >  }
> 
> > Kay, what am I missing here, why can't we just do this?  Hm, the
> > side-affect might be that if device_register() fails, NO ONE had better
> > touch that device again, as it might have just been freed from the
> > system.  I wonder if that will cause problems...
> 
> That looks right, besides that there might be callers already doing
> this. Which needs to be checked.

Yes, it would be.  I'll go through the tree.

> I never liked this pretty useless "convenience API", which just wraps
> two simple functions and the first one can never fail anyway.

Agreed.

> We better remove that device_register() stuff entirely in the long
> run, it's not doing any good. At the kobject level we killed the same
> stuff already long ago.

It's one of the things on my "to change" list.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Greg KH]

On Wed, Sep 22, 2010 at 10:53:21AM +0200, Kay Sievers wrote:
> On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> 
> >  int device_register(struct device *dev)
> >  {
> > +       int retval;
> > +
> >        device_initialize(dev);
> > -       return device_add(dev);
> > +       retval = device_add(dev);
> > +       if (retval)
> > +               put_device(dev);
> > +       return retval;
> >  }
> 
> > Kay, what am I missing here, why can't we just do this?  Hm, the
> > side-affect might be that if device_register() fails, NO ONE had better
> > touch that device again, as it might have just been freed from the
> > system.  I wonder if that will cause problems...
> 
> That looks right, besides that there might be callers already doing
> this. Which needs to be checked.

Yes, it would be.  I'll go through the tree.

> I never liked this pretty useless "convenience API", which just wraps
> two simple functions and the first one can never fail anyway.

Agreed.

> We better remove that device_register() stuff entirely in the long
> run, it's not doing any good. At the kobject level we killed the same
> stuff already long ago.

It's one of the things on my "to change" list.

thanks,

greg k-h

Re: [PATCH 04/14] memstick: core: fix device_register() error

[Vasiliy Kulikov]

On Wed, Sep 22, 2010 at 08:50 -0700, Greg KH wrote:
> On Wed, Sep 22, 2010 at 10:53:21AM +0200, Kay Sievers wrote:
> > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> > 
> > >  int device_register(struct device *dev)
> > >  {
> > > +       int retval;
> > > +
> > >        device_initialize(dev);
> > > -       return device_add(dev);
> > > +       retval = device_add(dev);
> > > +       if (retval)
> > > +               put_device(dev);
> > > +       return retval;
> > >  }
> > 
> > > Kay, what am I missing here, why can't we just do this?  Hm, the
> > > side-affect might be that if device_register() fails, NO ONE had better
> > > touch that device again, as it might have just been freed from the
> > > system.  I wonder if that will cause problems...
> > 
> > That looks right, besides that there might be callers already doing
> > this. Which needs to be checked.
> 
> Yes, it would be.  I'll go through the tree.

As I see with this in-device_register patch we should check for 2 things:

1) nobody should call put_device() because of failed device_register().

2) dev has to be already got, other words its ref counter should not be zero.

Correct?

-- 
Vasiliy
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Vasiliy Kulikov]

On Wed, Sep 22, 2010 at 08:50 -0700, Greg KH wrote:
> On Wed, Sep 22, 2010 at 10:53:21AM +0200, Kay Sievers wrote:
> > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote:
> > 
> > >  int device_register(struct device *dev)
> > >  {
> > > +       int retval;
> > > +
> > >        device_initialize(dev);
> > > -       return device_add(dev);
> > > +       retval = device_add(dev);
> > > +       if (retval)
> > > +               put_device(dev);
> > > +       return retval;
> > >  }
> > 
> > > Kay, what am I missing here, why can't we just do this?  Hm, the
> > > side-affect might be that if device_register() fails, NO ONE had better
> > > touch that device again, as it might have just been freed from the
> > > system.  I wonder if that will cause problems...
> > 
> > That looks right, besides that there might be callers already doing
> > this. Which needs to be checked.
> 
> Yes, it would be.  I'll go through the tree.

As I see with this in-device_register patch we should check for 2 things:

1) nobody should call put_device() because of failed device_register().

2) dev has to be already got, other words its ref counter should not be zero.

Correct?

-- 
Vasiliy

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Boaz Harrosh]

On 09/22/2010 12:49 AM, Greg KH wrote:
> On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote:
>> On Sun, 19 Sep 2010 16:54:49 +0400
>> Vasiliy Kulikov <segooon@gmail.com> wrote:
>>
>>> If device_register() fails then call put_device().
>>> See comment to device_register.
>>>
>>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
>>> ---
>>>  compile tested.
>>>
>>>  drivers/memstick/core/memstick.c |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>>> index c00fe82..4303b7e 100644
>>> --- a/drivers/memstick/core/memstick.c
>>> +++ b/drivers/memstick/core/memstick.c
>>> @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
>>>  		if (!host->card) {
>>>  			host->card = card;
>>>  			if (device_register(&card->dev)) {
>>> +				put_device(&card->dev);
>>>  				kfree(host->card);
>>>  				host->card = NULL;
>>>  			}
>>
>> A failed device_register() takes a bogus ref on the not-registered
>> device?  It's no surprise that people are getting this wrong.  
>>
>> The principle of least surprise says: fix device_register()!
> 
> One might think that, but it's a bit more difficult.
> 
> How does device_register know it should destroy the device if it fails?
> 
> Here's how it works:
>  - device_register is just a wrapper around device_initialize() and
>    device_add()
>      - device_initialize() can't do anything wrong, so it's safe, BUT,
>        at this point in time, the reference for the device is
>        incremented, so any caller must now drop the reference and
>        properly free stuff.
>      - device_add() does a lot.
> 
> Hm, I guess, because we "know" in device_register() that we must drop
> something if device_add() fails, then I guess it's not being consistant
> with it's own calls...
> 
> So, something as simple as this?
> 
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d1b2c9a..4ba8599 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1084,14 +1084,16 @@ name_error:
>   * have a clearly defined need to use and refcount the device
>   * before it is added to the hierarchy.
>   *
> - * 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)
>  {
> +	int retval;
> +
>  	device_initialize(dev);
> -	return device_add(dev);
> +	retval = device_add(dev);
> +	if (retval)
> +		put_device(dev);

This is all theoretically sound. But it will crash my device driver
and a dozen more like mine.

Because the passed *dev is an embedded structure inside a bigger
driver-specific structure. (Hence the call to device_register).

The kref inside the *dev is used to also govern the lifetime of
the outer struct. With a registered release and the use of a
container_of.

Now up until now, and was fine before the set_name changes.
If device_register failed the build-up function would just
deallocate the outer struct together with the inner device.
If you call put_device(dev) here then the release function
will be called. Which is a double free. And also the release
function is coded to expect a fully setup device, though
calling device_unregister, and all the other resources contained
in the outer struct.

So in short it would work but not with current code, and not
with out a major restructuring.

What about the patch James sent to just free the name string
and be done with it?

> +	return retval;
>  }
>  
>  /**
>      	
> 
> 
> Kay, what am I missing here, why can't we just do this?  Hm, the
> side-affect might be that if device_register() fails, NO ONE had better
> touch that device again, as it might have just been freed from the
> system.  I wonder if that will cause problems...
> 

Exactly. Lets please let us recap the scope here. device_register
is specifically for Embedded devices where the life time rules are
"delicate" to say the least. All we have is the name string memory
leak. Can we fix that and not force a major Kernel re-write.

> thanks,
> 
> greg k-h

Thanks
Boaz

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Boaz Harrosh]

On 09/22/2010 12:49 AM, Greg KH wrote:
> On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote:
>> On Sun, 19 Sep 2010 16:54:49 +0400
>> Vasiliy Kulikov <segooon@gmail.com> wrote:
>>
>>> If device_register() fails then call put_device().
>>> See comment to device_register.
>>>
>>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
>>> ---
>>>  compile tested.
>>>
>>>  drivers/memstick/core/memstick.c |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>>> index c00fe82..4303b7e 100644
>>> --- a/drivers/memstick/core/memstick.c
>>> +++ b/drivers/memstick/core/memstick.c
>>> @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
>>>  		if (!host->card) {
>>>  			host->card = card;
>>>  			if (device_register(&card->dev)) {
>>> +				put_device(&card->dev);
>>>  				kfree(host->card);
>>>  				host->card = NULL;
>>>  			}
>>
>> A failed device_register() takes a bogus ref on the not-registered
>> device?  It's no surprise that people are getting this wrong.  
>>
>> The principle of least surprise says: fix device_register()!
> 
> One might think that, but it's a bit more difficult.
> 
> How does device_register know it should destroy the device if it fails?
> 
> Here's how it works:
>  - device_register is just a wrapper around device_initialize() and
>    device_add()
>      - device_initialize() can't do anything wrong, so it's safe, BUT,
>        at this point in time, the reference for the device is
>        incremented, so any caller must now drop the reference and
>        properly free stuff.
>      - device_add() does a lot.
> 
> Hm, I guess, because we "know" in device_register() that we must drop
> something if device_add() fails, then I guess it's not being consistant
> with it's own calls...
> 
> So, something as simple as this?
> 
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d1b2c9a..4ba8599 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1084,14 +1084,16 @@ name_error:
>   * have a clearly defined need to use and refcount the device
>   * before it is added to the hierarchy.
>   *
> - * 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)
>  {
> +	int retval;
> +
>  	device_initialize(dev);
> -	return device_add(dev);
> +	retval = device_add(dev);
> +	if (retval)
> +		put_device(dev);

This is all theoretically sound. But it will crash my device driver
and a dozen more like mine.

Because the passed *dev is an embedded structure inside a bigger
driver-specific structure. (Hence the call to device_register).

The kref inside the *dev is used to also govern the lifetime of
the outer struct. With a registered release and the use of a
container_of.

Now up until now, and was fine before the set_name changes.
If device_register failed the build-up function would just
deallocate the outer struct together with the inner device.
If you call put_device(dev) here then the release function
will be called. Which is a double free. And also the release
function is coded to expect a fully setup device, though
calling device_unregister, and all the other resources contained
in the outer struct.

So in short it would work but not with current code, and not
with out a major restructuring.

What about the patch James sent to just free the name string
and be done with it?

> +	return retval;
>  }
>  
>  /**
>      	
> 
> 
> Kay, what am I missing here, why can't we just do this?  Hm, the
> side-affect might be that if device_register() fails, NO ONE had better
> touch that device again, as it might have just been freed from the
> system.  I wonder if that will cause problems...
> 

Exactly. Lets please let us recap the scope here. device_register
is specifically for Embedded devices where the life time rules are
"delicate" to say the least. All we have is the name string memory
leak. Can we fix that and not force a major Kernel re-write.

> thanks,
> 
> greg k-h

Thanks
Boaz

Re: [PATCH 04/14] memstick: core: fix device_register() error

[Greg KH]

On Wed, Sep 22, 2010 at 11:58:48AM +0200, Boaz Harrosh wrote:
> On 09/22/2010 12:49 AM, Greg KH wrote:
> > On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote:
> >> On Sun, 19 Sep 2010 16:54:49 +0400
> >> Vasiliy Kulikov <segooon@gmail.com> wrote:
> >>
> >>> If device_register() fails then call put_device().
> >>> See comment to device_register.
> >>>
> >>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> >>> ---
> >>>  compile tested.
> >>>
> >>>  drivers/memstick/core/memstick.c |    1 +
> >>>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> >>> index c00fe82..4303b7e 100644
> >>> --- a/drivers/memstick/core/memstick.c
> >>> +++ b/drivers/memstick/core/memstick.c
> >>> @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
> >>>  		if (!host->card) {
> >>>  			host->card = card;
> >>>  			if (device_register(&card->dev)) {
> >>> +				put_device(&card->dev);
> >>>  				kfree(host->card);
> >>>  				host->card = NULL;
> >>>  			}
> >>
> >> A failed device_register() takes a bogus ref on the not-registered
> >> device?  It's no surprise that people are getting this wrong.  
> >>
> >> The principle of least surprise says: fix device_register()!
> > 
> > One might think that, but it's a bit more difficult.
> > 
> > How does device_register know it should destroy the device if it fails?
> > 
> > Here's how it works:
> >  - device_register is just a wrapper around device_initialize() and
> >    device_add()
> >      - device_initialize() can't do anything wrong, so it's safe, BUT,
> >        at this point in time, the reference for the device is
> >        incremented, so any caller must now drop the reference and
> >        properly free stuff.
> >      - device_add() does a lot.
> > 
> > Hm, I guess, because we "know" in device_register() that we must drop
> > something if device_add() fails, then I guess it's not being consistant
> > with it's own calls...
> > 
> > So, something as simple as this?
> > 
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d1b2c9a..4ba8599 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1084,14 +1084,16 @@ name_error:
> >   * have a clearly defined need to use and refcount the device
> >   * before it is added to the hierarchy.
> >   *
> > - * 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)
> >  {
> > +	int retval;
> > +
> >  	device_initialize(dev);
> > -	return device_add(dev);
> > +	retval = device_add(dev);
> > +	if (retval)
> > +		put_device(dev);
> 
> This is all theoretically sound. But it will crash my device driver
> and a dozen more like mine.

Then they are broken :)

> Because the passed *dev is an embedded structure inside a bigger
> driver-specific structure. (Hence the call to device_register).

So, you would like to keep the way the code currently works in that if
device_register() fails, you are responsible for calling put_device() on
your own when finished?

> The kref inside the *dev is used to also govern the lifetime of
> the outer struct. With a registered release and the use of a
> container_of.

That's as it should be.

> Now up until now, and was fine before the set_name changes.
> If device_register failed the build-up function would just
> deallocate the outer struct together with the inner device.
> If you call put_device(dev) here then the release function
> will be called. Which is a double free. And also the release
> function is coded to expect a fully setup device, though
> calling device_unregister, and all the other resources contained
> in the outer struct.

Well, you don't do the double free then :)

> So in short it would work but not with current code, and not
> with out a major restructuring.

Do you have an example of a driver that would break so I can look at it?

> What about the patch James sent to just free the name string
> and be done with it?

Don't poke into the kobject structure from the driver core, that's not
nice and not allowed.

> > +	return retval;
> >  }
> >  
> >  /**
> >      	
> > 
> > 
> > Kay, what am I missing here, why can't we just do this?  Hm, the
> > side-affect might be that if device_register() fails, NO ONE had better
> > touch that device again, as it might have just been freed from the
> > system.  I wonder if that will cause problems...
> > 
> 
> Exactly. Lets please let us recap the scope here. device_register
> is specifically for Embedded devices where the life time rules are
> "delicate" to say the least. All we have is the name string memory
> leak. Can we fix that and not force a major Kernel re-write.

device_register was never written for embedded people at all, so don't
think that.

And why are your lifetime rules any more "delicate" than anyone elses?
Is it because the code is perhaps broken?  :)

thanks,

greg k-h

Re: [PATCH 04/14] memstick: core: fix device_register() error handling

[Greg KH]

On Wed, Sep 22, 2010 at 11:58:48AM +0200, Boaz Harrosh wrote:
> On 09/22/2010 12:49 AM, Greg KH wrote:
> > On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote:
> >> On Sun, 19 Sep 2010 16:54:49 +0400
> >> Vasiliy Kulikov <segooon@gmail.com> wrote:
> >>
> >>> If device_register() fails then call put_device().
> >>> See comment to device_register.
> >>>
> >>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> >>> ---
> >>>  compile tested.
> >>>
> >>>  drivers/memstick/core/memstick.c |    1 +
> >>>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> >>> index c00fe82..4303b7e 100644
> >>> --- a/drivers/memstick/core/memstick.c
> >>> +++ b/drivers/memstick/core/memstick.c
> >>> @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work)
> >>>  		if (!host->card) {
> >>>  			host->card = card;
> >>>  			if (device_register(&card->dev)) {
> >>> +				put_device(&card->dev);
> >>>  				kfree(host->card);
> >>>  				host->card = NULL;
> >>>  			}
> >>
> >> A failed device_register() takes a bogus ref on the not-registered
> >> device?  It's no surprise that people are getting this wrong.  
> >>
> >> The principle of least surprise says: fix device_register()!
> > 
> > One might think that, but it's a bit more difficult.
> > 
> > How does device_register know it should destroy the device if it fails?
> > 
> > Here's how it works:
> >  - device_register is just a wrapper around device_initialize() and
> >    device_add()
> >      - device_initialize() can't do anything wrong, so it's safe, BUT,
> >        at this point in time, the reference for the device is
> >        incremented, so any caller must now drop the reference and
> >        properly free stuff.
> >      - device_add() does a lot.
> > 
> > Hm, I guess, because we "know" in device_register() that we must drop
> > something if device_add() fails, then I guess it's not being consistant
> > with it's own calls...
> > 
> > So, something as simple as this?
> > 
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d1b2c9a..4ba8599 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1084,14 +1084,16 @@ name_error:
> >   * have a clearly defined need to use and refcount the device
> >   * before it is added to the hierarchy.
> >   *
> > - * 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)
> >  {
> > +	int retval;
> > +
> >  	device_initialize(dev);
> > -	return device_add(dev);
> > +	retval = device_add(dev);
> > +	if (retval)
> > +		put_device(dev);
> 
> This is all theoretically sound. But it will crash my device driver
> and a dozen more like mine.

Then they are broken :)

> Because the passed *dev is an embedded structure inside a bigger
> driver-specific structure. (Hence the call to device_register).

So, you would like to keep the way the code currently works in that if
device_register() fails, you are responsible for calling put_device() on
your own when finished?

> The kref inside the *dev is used to also govern the lifetime of
> the outer struct. With a registered release and the use of a
> container_of.

That's as it should be.

> Now up until now, and was fine before the set_name changes.
> If device_register failed the build-up function would just
> deallocate the outer struct together with the inner device.
> If you call put_device(dev) here then the release function
> will be called. Which is a double free. And also the release
> function is coded to expect a fully setup device, though
> calling device_unregister, and all the other resources contained
> in the outer struct.

Well, you don't do the double free then :)

> So in short it would work but not with current code, and not
> with out a major restructuring.

Do you have an example of a driver that would break so I can look at it?

> What about the patch James sent to just free the name string
> and be done with it?

Don't poke into the kobject structure from the driver core, that's not
nice and not allowed.

> > +	return retval;
> >  }
> >  
> >  /**
> >      	
> > 
> > 
> > Kay, what am I missing here, why can't we just do this?  Hm, the
> > side-affect might be that if device_register() fails, NO ONE had better
> > touch that device again, as it might have just been freed from the
> > system.  I wonder if that will cause problems...
> > 
> 
> Exactly. Lets please let us recap the scope here. device_register
> is specifically for Embedded devices where the life time rules are
> "delicate" to say the least. All we have is the name string memory
> leak. Can we fix that and not force a major Kernel re-write.

device_register was never written for embedded people at all, so don't
think that.

And why are your lifetime rules any more "delicate" than anyone elses?
Is it because the code is perhaps broken?  :)

thanks,

greg k-h