Freeing of dev->p

Freeing of dev->p

[Jean Delvare]

Hi Greg, hi all,

A memory leak has been reported to me:
http://marc.info/?l=linux-i2c&m=138779165123331&w=2

The leak is in i801_probe, caused by an early call to
i2c_set_adapdata() which in turn calls dev_set_drvdata() which
allocates some memory in device_private_init(). That memory is only
freed by the driver core when the i2c_adapter class device is removed.
But if the parent (PCI) device probing itself fails for whatever
reason, the class device never gets to be created, so it's never
removed, thus the memory is never freed.

It is not possible to move the call to i2c_set_adapdata() until after
the class device is created, because the data pointed is needed very
early after (almost during) i2c adapter creation. So I could make the
leak less likely to happen but I can't fix it completely.

I am wondering how this can be solved, and this brought three questions:

1* What is the rationale for allocating dev->p dynamically? It is
allocated as soon as the device is created (in device_add), so as far
as I can see every device will need the allocation. Including struct
device_private in struct device would not cost more memory, plus it
would reduce memory fragmentation. So is this a lifetime issue? Or
something else I can't think of?

2* What is the rationale for making void *driver_data part of struct
device_private and not struct device itself?

3* If the current implementation is considered correct, does it mean
that dev_set_drvdata() should never be used for class devices?

Thanks,
-- 
Jean Delvare

Re: Freeing of dev->p

[Greg Kroah-Hartman]

On Wed, Jan 08, 2014 at 04:40:58PM +0100, Jean Delvare wrote:
> Hi Greg, hi all,
> 
> A memory leak has been reported to me:
> http://marc.info/?l=linux-i2c&m=138779165123331&w=2
> 
> The leak is in i801_probe, caused by an early call to
> i2c_set_adapdata() which in turn calls dev_set_drvdata() which
> allocates some memory in device_private_init(). That memory is only
> freed by the driver core when the i2c_adapter class device is removed.
> But if the parent (PCI) device probing itself fails for whatever
> reason, the class device never gets to be created, so it's never
> removed, thus the memory is never freed.
> 
> It is not possible to move the call to i2c_set_adapdata() until after
> the class device is created, because the data pointed is needed very
> early after (almost during) i2c adapter creation. So I could make the
> leak less likely to happen but I can't fix it completely.

I don't understand, what exactly is leaking?  You have control over your
own structures, so can't you clean things up on the error path if you
know something went wrong?

> I am wondering how this can be solved, and this brought three questions:
> 
> 1* What is the rationale for allocating dev->p dynamically? It is
> allocated as soon as the device is created (in device_add), so as far
> as I can see every device will need the allocation. Including struct
> device_private in struct device would not cost more memory, plus it
> would reduce memory fragmentation. So is this a lifetime issue? Or
> something else I can't think of?

I did it to keep things that non-driver-core code should not be
touching.  Previously, lots of people messed around with the device
private fields that could confuse the driver core.  Ideally, I'd like to
be able to move the kobject itself into this structure, to allow devices
to handle static initialization better, but that never happened, unlike
other driver core structures.

> 2* What is the rationale for making void *driver_data part of struct
> device_private and not struct device itself?

To "hide" information / details that non-driver core code should not
care about or touch.

> 3* If the current implementation is considered correct, does it mean
> that dev_set_drvdata() should never be used for class devices?

No, it should be fine, I don't understand the issue with your usage that
is causing the problem, care to explain it better, or point me at some
code?

thanks,

greg k-h

Re: Freeing of dev->p

[Jean Delvare]

On Wed, 8 Jan 2014 08:56:28 -0800, Greg Kroah-Hartman wrote:
> On Wed, Jan 08, 2014 at 04:40:58PM +0100, Jean Delvare wrote:
> > Hi Greg, hi all,
> > 
> > A memory leak has been reported to me:
> > http://marc.info/?l=linux-i2c&m=138779165123331&w=2
> > 
> > The leak is in i801_probe, caused by an early call to
> > i2c_set_adapdata() which in turn calls dev_set_drvdata() which
> > allocates some memory in device_private_init(). That memory is only
> > freed by the driver core when the i2c_adapter class device is removed.
> > But if the parent (PCI) device probing itself fails for whatever
> > reason, the class device never gets to be created, so it's never
> > removed, thus the memory is never freed.
> > 
> > It is not possible to move the call to i2c_set_adapdata() until after
> > the class device is created, because the data pointed is needed very
> > early after (almost during) i2c adapter creation. So I could make the
> > leak less likely to happen but I can't fix it completely.
> 
> I don't understand, what exactly is leaking?  You have control over your
> own structures, so can't you clean things up on the error path if you
> know something went wrong?

I do, but I can only care for memory I allocated myself, not the memory
allocated by the driver core.

> > I am wondering how this can be solved, and this brought three questions:
> > 
> > 1* What is the rationale for allocating dev->p dynamically? It is
> > allocated as soon as the device is created (in device_add), so as far
> > as I can see every device will need the allocation. Including struct
> > device_private in struct device would not cost more memory, plus it
> > would reduce memory fragmentation. So is this a lifetime issue? Or
> > something else I can't think of?
> 
> I did it to keep things that non-driver-core code should not be
> touching.  Previously, lots of people messed around with the device
> private fields that could confuse the driver core.  Ideally, I'd like to
> be able to move the kobject itself into this structure, to allow devices
> to handle static initialization better, but that never happened, unlike
> other driver core structures.
> 
> > 2* What is the rationale for making void *driver_data part of struct
> > device_private and not struct device itself?
> 
> To "hide" information / details that non-driver core code should not
> care about or touch.

This is a respectable goal, but I'm unsure it's worth the price...

> > 3* If the current implementation is considered correct, does it mean
> > that dev_set_drvdata() should never be used for class devices?
> 
> No, it should be fine, I don't understand the issue with your usage that
> is causing the problem, care to explain it better, or point me at some
> code?

Sorry I wasn't clear. Let me guide you step by step with the real-world
example that caused me to look into this. It all starts in
drivers/i2c/busses/i2c-i801.c, function i801_probe(). This is a driver
for a PCI device implementing an SMBus controller (aka i2c_adapter.)

static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
	int err;
	struct i801_priv *priv;

	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
	if (!priv)
		return -ENOMEM;

	i2c_set_adapdata(&priv->adapter, priv);

So we allocate private data for the i2c_adapter we're about to create.
i2c_set_adapdata() is a wrapper around dev_set_drvdata() for class
devices of type i2c_adapter. If everything goes well, we would end up
doing:

	err = i2c_add_adapter(&priv->adapter);

and everything would be fine. However, there are a number of things
that can go wrong in between, for example if an ACPI resource conflict
is detected:

	err = acpi_check_resource_conflict(&dev->resource[SMBBAR]);
	if (err) {
		err = -ENODEV;
		goto exit;
	}

	(...)
exit:
	kfree(priv);
	return err;
}

As you can see, we properly free the memory we allocated ourselves for
the i2c_adapter's data on the error path. You'd think we're alright
then, but we're not. The problem is that i2c_set_adapdata() indirectly
allocated some memory:

static inline void i2c_set_adapdata(struct i2c_adapter *dev, void *data)
{
	dev_set_drvdata(&dev->dev, data);
}

int dev_set_drvdata(struct device *dev, void *data)
{
	int error;

	if (!dev->p) {
		error = device_private_init(dev);
		if (error)
			return error;
	}
	dev->p->driver_data = data;
	return 0;
}

int device_private_init(struct device *dev)
{
	dev->p = kzalloc(sizeof(*dev->p), GFP_KERNEL);
	(...)
}

In the success case, it's fine because the memory gets freed at device
release time in:

static void device_release(struct kobject *kobj)
{
	struct device *dev = kobj_to_dev(kobj);
	struct device_private *p = dev->p;

	(...)
	kfree(p);
}

However in the error case, device_release() will never get called, so
dev->p is leaked. This is what I am worried about.

I hope I was clear enough this time :) If not I'll try harder.

I consider allocating memory in dev_set_drvdata() very misleading, I
don't think we should keep doing that.

Thanks,
-- 
Jean Delvare

Re: Freeing of dev->p

[Greg Kroah-Hartman]

On Wed, Jan 08, 2014 at 09:33:30PM +0100, Jean Delvare wrote:
> On Wed, 8 Jan 2014 08:56:28 -0800, Greg Kroah-Hartman wrote:
> > On Wed, Jan 08, 2014 at 04:40:58PM +0100, Jean Delvare wrote:
> > > Hi Greg, hi all,
> > > 
> > > A memory leak has been reported to me:
> > > http://marc.info/?l=linux-i2c&m=138779165123331&w=2
> > > 
> > > The leak is in i801_probe, caused by an early call to
> > > i2c_set_adapdata() which in turn calls dev_set_drvdata() which
> > > allocates some memory in device_private_init(). That memory is only
> > > freed by the driver core when the i2c_adapter class device is removed.
> > > But if the parent (PCI) device probing itself fails for whatever
> > > reason, the class device never gets to be created, so it's never
> > > removed, thus the memory is never freed.
> > > 
> > > It is not possible to move the call to i2c_set_adapdata() until after
> > > the class device is created, because the data pointed is needed very
> > > early after (almost during) i2c adapter creation. So I could make the
> > > leak less likely to happen but I can't fix it completely.
> > 
> > I don't understand, what exactly is leaking?  You have control over your
> > own structures, so can't you clean things up on the error path if you
> > know something went wrong?
> 
> I do, but I can only care for memory I allocated myself, not the memory
> allocated by the driver core.
> 
> > > I am wondering how this can be solved, and this brought three questions:
> > > 
> > > 1* What is the rationale for allocating dev->p dynamically? It is
> > > allocated as soon as the device is created (in device_add), so as far
> > > as I can see every device will need the allocation. Including struct
> > > device_private in struct device would not cost more memory, plus it
> > > would reduce memory fragmentation. So is this a lifetime issue? Or
> > > something else I can't think of?
> > 
> > I did it to keep things that non-driver-core code should not be
> > touching.  Previously, lots of people messed around with the device
> > private fields that could confuse the driver core.  Ideally, I'd like to
> > be able to move the kobject itself into this structure, to allow devices
> > to handle static initialization better, but that never happened, unlike
> > other driver core structures.
> > 
> > > 2* What is the rationale for making void *driver_data part of struct
> > > device_private and not struct device itself?
> > 
> > To "hide" information / details that non-driver core code should not
> > care about or touch.
> 
> This is a respectable goal, but I'm unsure it's worth the price...
> 
> > > 3* If the current implementation is considered correct, does it mean
> > > that dev_set_drvdata() should never be used for class devices?
> > 
> > No, it should be fine, I don't understand the issue with your usage that
> > is causing the problem, care to explain it better, or point me at some
> > code?
> 
> Sorry I wasn't clear. Let me guide you step by step with the real-world
> example that caused me to look into this. It all starts in
> drivers/i2c/busses/i2c-i801.c, function i801_probe(). This is a driver
> for a PCI device implementing an SMBus controller (aka i2c_adapter.)
> 
> static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> {
> 	int err;
> 	struct i801_priv *priv;
> 
> 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> 	if (!priv)
> 		return -ENOMEM;
> 
> 	i2c_set_adapdata(&priv->adapter, priv);
> 
> So we allocate private data for the i2c_adapter we're about to create.
> i2c_set_adapdata() is a wrapper around dev_set_drvdata() for class
> devices of type i2c_adapter. If everything goes well, we would end up
> doing:
> 
> 	err = i2c_add_adapter(&priv->adapter);
> 
> and everything would be fine. However, there are a number of things
> that can go wrong in between, for example if an ACPI resource conflict
> is detected:
> 
> 	err = acpi_check_resource_conflict(&dev->resource[SMBBAR]);
> 	if (err) {
> 		err = -ENODEV;
> 		goto exit;
> 	}
> 
> 	(...)
> exit:
> 	kfree(priv);
> 	return err;
> }
> 
> As you can see, we properly free the memory we allocated ourselves for
> the i2c_adapter's data on the error path. You'd think we're alright
> then, but we're not. The problem is that i2c_set_adapdata() indirectly
> allocated some memory:
> 
> static inline void i2c_set_adapdata(struct i2c_adapter *dev, void *data)
> {
> 	dev_set_drvdata(&dev->dev, data);
> }
> 
> int dev_set_drvdata(struct device *dev, void *data)
> {
> 	int error;
> 
> 	if (!dev->p) {
> 		error = device_private_init(dev);
> 		if (error)
> 			return error;
> 	}
> 	dev->p->driver_data = data;
> 	return 0;
> }
> 
> int device_private_init(struct device *dev)
> {
> 	dev->p = kzalloc(sizeof(*dev->p), GFP_KERNEL);
> 	(...)
> }
> 
> In the success case, it's fine because the memory gets freed at device
> release time in:
> 
> static void device_release(struct kobject *kobj)
> {
> 	struct device *dev = kobj_to_dev(kobj);
> 	struct device_private *p = dev->p;
> 
> 	(...)
> 	kfree(p);
> }
> 
> However in the error case, device_release() will never get called, so
> dev->p is leaked. This is what I am worried about.
> 
> I hope I was clear enough this time :) If not I'll try harder.

Nope, that was great, I understand it all now.

> I consider allocating memory in dev_set_drvdata() very misleading, I
> don't think we should keep doing that.

I had to add that later on when it was found that people were setting
private data fields before allocating or initializing the structure,
much like you are doing here.

Which should be fine, except for the big problem you have pointed out.

Hm, let me think about this.  If we move driver_data back out of the
private section of the device, we should be ok, but give me a day or so
to think about it some more...

thanks,

greg k-h

Re: Freeing of dev->p

[Jean Delvare]

Hi Greg,

On Thu, 9 Jan 2014 20:18:53 -0800, Greg Kroah-Hartman wrote:
> On Wed, Jan 08, 2014 at 09:33:30PM +0100, Jean Delvare wrote:
> > I consider allocating memory in dev_set_drvdata() very misleading, I
> > don't think we should keep doing that.
> 
> I had to add that later on when it was found that people were setting
> private data fields before allocating or initializing the structure,
> much like you are doing here.

Yes, I understand the idea.

> Which should be fine, except for the big problem you have pointed out.

Yup :(

> Hm, let me think about this.  If we move driver_data back out of the
> private section of the device, we should be ok, (...)

That's pretty much what I had in mind:

From: Jean Delvare <khali@linux-fr.org>
Subject: driver core: Move driver_data back to struct device

Having to allocate memory as part of dev_set_drvdata() is a problem
because that memory may never get freed if the device itself is not
created. So move driver_data back to struct device.

This is more or less a revert of commit
b4028437876866aba4747a655ede00f892089e14.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/base.h    |    3 ---
 drivers/base/dd.c      |   13 +++----------
 include/linux/device.h |    4 ++++
 3 files changed, 7 insertions(+), 13 deletions(-)

--- linux-3.13-rc7.orig/drivers/base/base.h	2013-11-04 00:41:51.000000000 +0100
+++ linux-3.13-rc7/drivers/base/base.h	2014-01-10 13:04:10.412200948 +0100
@@ -63,8 +63,6 @@ struct driver_private {
  *	binding of drivers which were unable to get all the resources needed by
  *	the device; typically because it depends on another driver getting
  *	probed first.
- * @driver_data - private pointer for driver specific info.  Will turn into a
- * list soon.
  * @device - pointer back to the struct class that this structure is
  * associated with.
  *
@@ -76,7 +74,6 @@ struct device_private {
 	struct klist_node knode_driver;
 	struct klist_node knode_bus;
 	struct list_head deferred_probe;
-	void *driver_data;
 	struct device *device;
 };
 #define to_device_private_parent(obj)	\
--- linux-3.13-rc7.orig/drivers/base/dd.c	2013-11-26 15:44:46.709655132 +0100
+++ linux-3.13-rc7/drivers/base/dd.c	2014-01-10 13:24:08.638450784 +0100
@@ -577,22 +577,15 @@ void driver_detach(struct device_driver
  */
 void *dev_get_drvdata(const struct device *dev)
 {
-	if (dev && dev->p)
-		return dev->p->driver_data;
+	if (dev)
+		return dev->driver_data;
 	return NULL;
 }
 EXPORT_SYMBOL(dev_get_drvdata);
 
 int dev_set_drvdata(struct device *dev, void *data)
 {
-	int error;
-
-	if (!dev->p) {
-		error = device_private_init(dev);
-		if (error)
-			return error;
-	}
-	dev->p->driver_data = data;
+	dev->driver_data = data;
 	return 0;
 }
 EXPORT_SYMBOL(dev_set_drvdata);
--- linux-3.13-rc7.orig/include/linux/device.h	2013-11-26 15:44:48.481695736 +0100
+++ linux-3.13-rc7/include/linux/device.h	2014-01-10 13:04:40.583896319 +0100
@@ -676,6 +676,8 @@ struct acpi_dev_node {
  * 		variants, which GPIO pins act in what additional roles, and so
  * 		on.  This shrinks the "Board Support Packages" (BSPs) and
  * 		minimizes board-specific #ifdefs in drivers.
+ * @driver_data: Private pointer for driver specific info.  Will turn into a
+ *		list soon.
  * @power:	For device power management.
  * 		See Documentation/power/devices.txt for details.
  * @pm_domain:	Provide callbacks that are executed during system suspend,
@@ -737,6 +739,8 @@ struct device {
 					   device */
 	void		*platform_data;	/* Platform specific data, device
 					   core doesn't touch it */
+	void		*driver_data;	/* Driver data, set and get with
+					   dev_set/get_drvdata */
 	struct dev_pm_info	power;
 	struct dev_pm_domain	*pm_domain;
 

For performance I'd even question the point of the dev check in
dev_get_drvdata(), especially when there is no such check in
dev_set_drvdata() which presumably is always called first. Plus
dev_set_drvdata() can no longer fail (something only 3 drivers in the
whole kernel tree were checking for anyway) so it could return void
instead of int. Then I suppose we could inline both functions again, for
performance. Well, put in short, really revering
b4028437876866aba4747a655ede00f892089e14 would be the way to go IMHO.

Really, while I understand your envy to protect driver core internals
from unwanted access, the cost here was simply too high IMHO, both in
terms of getting things right and performance. Some drivers are calling
dev_get_drvdata() directly or indirectly repeatedly at run-time. They
had no reason not to as this used to be so fast, and now it is no
longer an inline function, it has conditionals and a double pointer
indirection...

Plus, I can't think of anything really bad that could result from
accessing driver_data directly, contrary to the other members of struct
device_private.

> but give me a day or so to think about it some more...

Sure, take your time.

-- 
Jean Delvare

Re: Freeing of dev->p

[Greg Kroah-Hartman]

On Fri, Jan 10, 2014 at 03:39:07PM +0100, Jean Delvare wrote:
> + * @driver_data: Private pointer for driver specific info.  Will turn into a
> + *		list soon.

Ah, this comment reminds me of why I originally did this.  I was working
on moving for a way to have multiple drivers bound to the same device,
as people needed that type of thing for something that I can't remember
at the moment.

As it's been years now with no real movement forward on that idea, I
guess it's not going to happen :)

>   * @power:	For device power management.
>   * 		See Documentation/power/devices.txt for details.
>   * @pm_domain:	Provide callbacks that are executed during system suspend,
> @@ -737,6 +739,8 @@ struct device {
>  					   device */
>  	void		*platform_data;	/* Platform specific data, device
>  					   core doesn't touch it */
> +	void		*driver_data;	/* Driver data, set and get with
> +					   dev_set/get_drvdata */
>  	struct dev_pm_info	power;
>  	struct dev_pm_domain	*pm_domain;
>  
> 
> For performance I'd even question the point of the dev check in
> dev_get_drvdata(), especially when there is no such check in
> dev_set_drvdata() which presumably is always called first.

It's nice to not oops if a NULL pointer is passed in :)

> Plus dev_set_drvdata() can no longer fail (something only 3 drivers in
> the whole kernel tree were checking for anyway) so it could return
> void instead of int.

True.

> Then I suppose we could inline both functions
> again, for performance. Well, put in short, really revering
> b4028437876866aba4747a655ede00f892089e14 would be the way to go IMHO.

Almost, the copyright lines should stay :)

> Really, while I understand your envy to protect driver core internals
> from unwanted access, the cost here was simply too high IMHO, both in
> terms of getting things right and performance. Some drivers are calling
> dev_get_drvdata() directly or indirectly repeatedly at run-time. They
> had no reason not to as this used to be so fast, and now it is no
> longer an inline function, it has conditionals and a double pointer
> indirection...
> 
> Plus, I can't think of anything really bad that could result from
> accessing driver_data directly, contrary to the other members of struct
> device_private.

See first response above for why I did this, it wasn't to just make
things "harder" to mess up, I actually had a reason to do it (imagine
that!)

Thanks for the detailed response, I think I'll just revert most of that
patch and see if it's still workable.

greg k-h

Re: Freeing of dev->p

[Jean Delvare]

Hi Greg,

On Fri, 10 Jan 2014 07:24:02 -0800, Greg Kroah-Hartman wrote:
> On Fri, Jan 10, 2014 at 03:39:07PM +0100, Jean Delvare wrote:
> > + * @driver_data: Private pointer for driver specific info.  Will turn into a
> > + *		list soon.
> 
> Ah, this comment reminds me of why I originally did this.  I was working
> on moving for a way to have multiple drivers bound to the same device,
> as people needed that type of thing for something that I can't remember
> at the moment.

Presumably this was to handle multifunction devices. But now we have
the MFD infrastructure which deals with the problem gracefully, so
indeed there no longer is any point in making driver_data a list. The
second part of the comment above can go away.

> As it's been years now with no real movement forward on that idea, I
> guess it's not going to happen :)

Agreed, problem was simply solved differently.

> >   * @power:	For device power management.
> >   * 		See Documentation/power/devices.txt for details.
> >   * @pm_domain:	Provide callbacks that are executed during system suspend,
> > @@ -737,6 +739,8 @@ struct device {
> >  					   device */
> >  	void		*platform_data;	/* Platform specific data, device
> >  					   core doesn't touch it */
> > +	void		*driver_data;	/* Driver data, set and get with
> > +					   dev_set/get_drvdata */
> >  	struct dev_pm_info	power;
> >  	struct dev_pm_domain	*pm_domain;
> >  
> > 
> > For performance I'd even question the point of the dev check in
> > dev_get_drvdata(), especially when there is no such check in
> > dev_set_drvdata() which presumably is always called first.
> 
> It's nice to not oops if a NULL pointer is passed in :)

Or not. Is there any case where passing NULL could be desirable? If not
and passing NULL is always going to be a bug (which I think is the
case) then a oops is the right answer to bogus code. This ensure the
bug will be caught and fixed early. Not oopsing here most certainly
means oopsing at the next line in the driver anyway when it tries to
access the driver data and certainly doesn't expect it to be NULL
either.

> > Plus dev_set_drvdata() can no longer fail (something only 3 drivers in
> > the whole kernel tree were checking for anyway) so it could return
> > void instead of int.
> 
> True.
> 
> > Then I suppose we could inline both functions
> > again, for performance. Well, put in short, really revering
> > b4028437876866aba4747a655ede00f892089e14 would be the way to go IMHO.
> 
> Almost, the copyright lines should stay :)

As you wish :)

> > Really, while I understand your envy to protect driver core internals
> > from unwanted access, the cost here was simply too high IMHO, both in
> > terms of getting things right and performance. Some drivers are calling
> > dev_get_drvdata() directly or indirectly repeatedly at run-time. They
> > had no reason not to as this used to be so fast, and now it is no
> > longer an inline function, it has conditionals and a double pointer
> > indirection...
> > 
> > Plus, I can't think of anything really bad that could result from
> > accessing driver_data directly, contrary to the other members of struct
> > device_private.
> 
> See first response above for why I did this, it wasn't to just make
> things "harder" to mess up, I actually had a reason to do it (imagine
> that!)

Shocking! :D

> Thanks for the detailed response, I think I'll just revert most of that
> patch and see if it's still workable.

Sounds like a good plan, yeah.

Thanks,
-- 
Jean Delvare

Re: Freeing of dev->p

[Jean Delvare]

Hi Greg,

On Fri, 10 Jan 2014 07:24:02 -0800, Greg Kroah-Hartman wrote:
> On Fri, Jan 10, 2014 at 03:39:07PM +0100, Jean Delvare wrote:
> > (...)
> > Then I suppose we could inline both functions
> > again, for performance. Well, put in short, really revering
> > b4028437876866aba4747a655ede00f892089e14 would be the way to go IMHO.
> >
> > Really, while I understand your envy to protect driver core internals
> > from unwanted access, the cost here was simply too high IMHO, both in
> > terms of getting things right and performance. Some drivers are calling
> > dev_get_drvdata() directly or indirectly repeatedly at run-time. They
> > had no reason not to as this used to be so fast, and now it is no
> > longer an inline function, it has conditionals and a double pointer
> > indirection...
> > 
> > Plus, I can't think of anything really bad that could result from
> > accessing driver_data directly, contrary to the other members of struct
> > device_private.
> 
> (...)
> 
> Thanks for the detailed response, I think I'll just revert most of that
> patch and see if it's still workable.

Any news on this?

-- 
Jean Delvare

Re: Freeing of dev->p

[Greg Kroah-Hartman]

On Wed, Jan 22, 2014 at 08:29:11AM +0100, Jean Delvare wrote:
> Hi Greg,
> 
> On Fri, 10 Jan 2014 07:24:02 -0800, Greg Kroah-Hartman wrote:
> > On Fri, Jan 10, 2014 at 03:39:07PM +0100, Jean Delvare wrote:
> > > (...)
> > > Then I suppose we could inline both functions
> > > again, for performance. Well, put in short, really revering
> > > b4028437876866aba4747a655ede00f892089e14 would be the way to go IMHO.
> > >
> > > Really, while I understand your envy to protect driver core internals
> > > from unwanted access, the cost here was simply too high IMHO, both in
> > > terms of getting things right and performance. Some drivers are calling
> > > dev_get_drvdata() directly or indirectly repeatedly at run-time. They
> > > had no reason not to as this used to be so fast, and now it is no
> > > longer an inline function, it has conditionals and a double pointer
> > > indirection...
> > > 
> > > Plus, I can't think of anything really bad that could result from
> > > accessing driver_data directly, contrary to the other members of struct
> > > device_private.
> > 
> > (...)
> > 
> > Thanks for the detailed response, I think I'll just revert most of that
> > patch and see if it's still workable.
> 
> Any news on this?

Stuck with dealing with merge-window issues and conferences, don't
worry, this isn't lost, it's still on my todo list...

thanks for your patience,

greg k-h

Re: Freeing of dev->p

[Grant Likely]

On Wed, 8 Jan 2014 08:56:28 -0800, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 08, 2014 at 04:40:58PM +0100, Jean Delvare wrote:
> > Hi Greg, hi all,
> > 
> > A memory leak has been reported to me:
> > http://marc.info/?l=linux-i2c&m=138779165123331&w=2
> > 
> > The leak is in i801_probe, caused by an early call to
> > i2c_set_adapdata() which in turn calls dev_set_drvdata() which
> > allocates some memory in device_private_init(). That memory is only
> > freed by the driver core when the i2c_adapter class device is removed.
> > But if the parent (PCI) device probing itself fails for whatever
> > reason, the class device never gets to be created, so it's never
> > removed, thus the memory is never freed.
> > 
> > It is not possible to move the call to i2c_set_adapdata() until after
> > the class device is created, because the data pointed is needed very
> > early after (almost during) i2c adapter creation. So I could make the
> > leak less likely to happen but I can't fix it completely.
> 
> I don't understand, what exactly is leaking?  You have control over your
> own structures, so can't you clean things up on the error path if you
> know something went wrong?
> 
> > I am wondering how this can be solved, and this brought three questions:
> > 
> > 1* What is the rationale for allocating dev->p dynamically? It is
> > allocated as soon as the device is created (in device_add), so as far
> > as I can see every device will need the allocation. Including struct
> > device_private in struct device would not cost more memory, plus it
> > would reduce memory fragmentation. So is this a lifetime issue? Or
> > something else I can't think of?
> 
> I did it to keep things that non-driver-core code should not be
> touching.  Previously, lots of people messed around with the device
> private fields that could confuse the driver core.  Ideally, I'd like to
> be able to move the kobject itself into this structure, to allow devices
> to handle static initialization better, but that never happened, unlike
> other driver core structures.

Picking up on an old thread, but speaking generally. I see that Jean has
posted a patch for this sspecific issue...

I do get concerned about this approach of protect fields by putting them
behind an allocated structure that always needs to be there. It
complicates the code and increases the runtime overhead with no benefits
other than catching badly behaved driver writers. Those /should/ be the
sort of failures we can catch at build time.

I like tglx's approach here in include/linux/irqdesc.h:

struct irq_desc {
		...
		unsigned int core_internal_state__do_not_mess_with_it;
		...

We could have a naming convention for private members and do a sparse or
checkpatch test for non-core code touching private data.

g.