[PATCH 0/5] driver core: Clean up dev_get/set_drvdata

[PATCH 0/5] driver core: Clean up dev_get/set_drvdata

[Jean Delvare]

This patch set is a proposal to revert most of commit b4028437. This
would solve a memory leak and also allow to simplify and ultimately
inline again functions dev_get_drvdata and dev_set_drvdata for smaller
footprint and improved performance.

[PATCH 1/5] driver core: Move driver_data back to struct device
[PATCH 2/5] driver core: dev_set_drvdata can no longer fail
[PATCH 3/5] driver core: dev_set_drvdata returns void
[PATCH 4/5] driver core: dev_get_drvdata: Don't check for NULL dev
[PATCH 5/5] driver core: Inline dev_set/get_drvdata

-- 
Jean Delvare
SUSE L3 Support

[PATCH 1/5] driver core: Move driver_data back to struct device

[Jean Delvare]

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 a partial revert of commit b4028437.

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

--- linux-3.15-rc1.orig/drivers/base/base.h	2014-04-14 09:28:47.122857147 +0200
+++ linux-3.15-rc1/drivers/base/base.h	2014-04-14 09:42:50.026552544 +0200
@@ -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.15-rc1.orig/drivers/base/dd.c	2014-04-14 09:28:47.122857147 +0200
+++ linux-3.15-rc1/drivers/base/dd.c	2014-04-14 12:52:52.957743343 +0200
@@ -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.15-rc1.orig/include/linux/device.h	2014-04-14 09:42:31.772119674 +0200
+++ linux-3.15-rc1/include/linux/device.h	2014-04-14 12:53:10.284166917 +0200
@@ -679,6 +679,7 @@ 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.
  * @power:	For device power management.
  * 		See Documentation/power/devices.txt for details.
  * @pm_domain:	Provide callbacks that are executed during system suspend,
@@ -740,6 +741,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;
 

-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/5] driver core: dev_set_drvdata can no longer fail

[Jean Delvare]

So there is no point in checking its return value, which will soon
disappear.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/iommu/exynos-iommu.c |    7 +------
 drivers/vfio/vfio.c          |    8 +-------
 2 files changed, 2 insertions(+), 13 deletions(-)

--- linux-3.15-rc0.orig/drivers/iommu/exynos-iommu.c	2014-04-08 16:23:59.159885163 +0200
+++ linux-3.15-rc0/drivers/iommu/exynos-iommu.c	2014-04-08 16:25:47.491199974 +0200
@@ -542,12 +542,7 @@ static int exynos_sysmmu_probe(struct pl
 		goto err_alloc;
 	}
 
-	ret = dev_set_drvdata(dev, data);
-	if (ret) {
-		dev_dbg(dev, "Unabled to initialize driver data\n");
-		goto err_init;
-	}
-
+	dev_set_drvdata(dev, data);
 	data->nsfrs = pdev->num_resources / 2;
 	data->sfrbases = kmalloc(sizeof(*data->sfrbases) * data->nsfrs,
 								GFP_KERNEL);
--- linux-3.15-rc0.orig/drivers/vfio/vfio.c	2014-04-08 16:23:59.159885163 +0200
+++ linux-3.15-rc0/drivers/vfio/vfio.c	2014-04-08 16:28:14.690345108 +0200
@@ -349,7 +349,6 @@ struct vfio_device *vfio_group_create_de
 					     void *device_data)
 {
 	struct vfio_device *device;
-	int ret;
 
 	device = kzalloc(sizeof(*device), GFP_KERNEL);
 	if (!device)
@@ -360,12 +359,7 @@ struct vfio_device *vfio_group_create_de
 	device->group = group;
 	device->ops = ops;
 	device->device_data = device_data;
-
-	ret = dev_set_drvdata(dev, device);
-	if (ret) {
-		kfree(device);
-		return ERR_PTR(ret);
-	}
+	dev_set_drvdata(dev, device);
 
 	/* No need to get group_lock, caller has group reference */
 	vfio_group_get(group);

-- 
Jean Delvare
SUSE L3 Support

[PATCH 3/5] driver core: dev_set_drvdata returns void

[Jean Delvare]

dev_set_drvdata can no longer fail, so it could return void.

All callers have hopefully been updated to no longer check for the
return value.

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

--- linux-3.15-rc1.orig/drivers/base/dd.c	2014-04-14 12:52:52.957743343 +0200
+++ linux-3.15-rc1/drivers/base/dd.c	2014-04-14 12:54:21.520907390 +0200
@@ -583,9 +583,8 @@ void *dev_get_drvdata(const struct devic
 }
 EXPORT_SYMBOL(dev_get_drvdata);
 
-int dev_set_drvdata(struct device *dev, void *data)
+void dev_set_drvdata(struct device *dev, void *data)
 {
 	dev->driver_data = data;
-	return 0;
 }
 EXPORT_SYMBOL(dev_set_drvdata);
--- linux-3.15-rc1.orig/include/linux/device.h	2014-04-14 12:53:10.284166917 +0200
+++ linux-3.15-rc1/include/linux/device.h	2014-04-14 12:54:18.863842502 +0200
@@ -917,7 +917,7 @@ extern const char *device_get_devnode(st
 				      umode_t *mode, kuid_t *uid, kgid_t *gid,
 				      const char **tmp);
 extern void *dev_get_drvdata(const struct device *dev);
-extern int dev_set_drvdata(struct device *dev, void *data);
+extern void dev_set_drvdata(struct device *dev, void *data);
 
 static inline bool device_supports_offline(struct device *dev)
 {

-- 
Jean Delvare
SUSE L3 Support

[PATCH 4/5] driver core: dev_get_drvdata: Don't check for NULL dev

[Jean Delvare]

There is no point in calling dev_get_drvdata without a valid device.
So checking for dev == NULL is pointless. If such a check is ever
needed - which I doubt - the driver should do it before calling
dev_get_drvdata.

We were returning NULL if dev was NULL, which the caller certainly did
not expect anyway, so that was only delaying the crash if the caller
is not paying attention.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/dd.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--- linux-3.15-rc0.orig/drivers/base/dd.c	2014-04-09 16:16:01.631698736 +0200
+++ linux-3.15-rc0/drivers/base/dd.c	2014-04-09 16:55:09.293001255 +0200
@@ -577,9 +577,7 @@ void driver_detach(struct device_driver
  */
 void *dev_get_drvdata(const struct device *dev)
 {
-	if (dev)
-		return dev->driver_data;
-	return NULL;
+	return dev->driver_data;
 }
 EXPORT_SYMBOL(dev_get_drvdata);
 

-- 
Jean Delvare
SUSE L3 Support

[PATCH 5/5] driver core: Inline dev_set/get_drvdata

[Jean Delvare]

dev_set_drvdata and dev_get_drvdata are now simple enough again that
we can inline them as they used to be before commit b40284378.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/dd.c      |   16 ----------------
 include/linux/device.h |   12 ++++++++++--
 2 files changed, 10 insertions(+), 18 deletions(-)

--- linux-3.15-rc1.orig/drivers/base/dd.c	2014-04-14 12:54:23.994967807 +0200
+++ linux-3.15-rc1/drivers/base/dd.c	2014-04-14 12:54:24.491979944 +0200
@@ -570,19 +570,3 @@ void driver_detach(struct device_driver
 		put_device(dev);
 	}
 }
-
-/*
- * These exports can't be _GPL due to .h files using this within them, and it
- * might break something that was previously working...
- */
-void *dev_get_drvdata(const struct device *dev)
-{
-	return dev->driver_data;
-}
-EXPORT_SYMBOL(dev_get_drvdata);
-
-void dev_set_drvdata(struct device *dev, void *data)
-{
-	dev->driver_data = data;
-}
-EXPORT_SYMBOL(dev_set_drvdata);
--- linux-3.15-rc1.orig/include/linux/device.h	2014-04-14 12:54:18.863842502 +0200
+++ linux-3.15-rc1/include/linux/device.h	2014-04-14 12:54:24.492979969 +0200
@@ -832,6 +832,16 @@ static inline void set_dev_node(struct d
 }
 #endif
 
+static inline void *dev_get_drvdata(const struct device *dev)
+{
+	return dev->driver_data;
+}
+
+static inline void dev_set_drvdata(struct device *dev, void *data)
+{
+	dev->driver_data = data;
+}
+
 static inline struct pm_subsys_data *dev_to_psd(struct device *dev)
 {
 	return dev ? dev->power.subsys_data : NULL;
@@ -916,8 +926,6 @@ extern int device_move(struct device *de
 extern const char *device_get_devnode(struct device *dev,
 				      umode_t *mode, kuid_t *uid, kgid_t *gid,
 				      const char **tmp);
-extern void *dev_get_drvdata(const struct device *dev);
-extern void dev_set_drvdata(struct device *dev, void *data);
 
 static inline bool device_supports_offline(struct device *dev)
 {

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/5] driver core: Clean up dev_get/set_drvdata

[Greg KH]

On Mon, Apr 14, 2014 at 12:48:02PM +0200, Jean Delvare wrote:
> This patch set is a proposal to revert most of commit b4028437. This
> would solve a memory leak and also allow to simplify and ultimately
> inline again functions dev_get_drvdata and dev_set_drvdata for smaller
> footprint and improved performance.
> 
> [PATCH 1/5] driver core: Move driver_data back to struct device
> [PATCH 2/5] driver core: dev_set_drvdata can no longer fail
> [PATCH 3/5] driver core: dev_set_drvdata returns void
> [PATCH 4/5] driver core: dev_get_drvdata: Don't check for NULL dev
> [PATCH 5/5] driver core: Inline dev_set/get_drvdata

Thanks, I'll queue this up later this week.

greg k-h

Re: [PATCH 0/5] driver core: Clean up dev_get/set_drvdata

[Greg KH]

On Mon, Apr 14, 2014 at 12:48:02PM +0200, Jean Delvare wrote:
> This patch set is a proposal to revert most of commit b4028437. This
> would solve a memory leak and also allow to simplify and ultimately
> inline again functions dev_get_drvdata and dev_set_drvdata for smaller
> footprint and improved performance.
> 
> [PATCH 1/5] driver core: Move driver_data back to struct device
> [PATCH 2/5] driver core: dev_set_drvdata can no longer fail
> [PATCH 3/5] driver core: dev_set_drvdata returns void
> [PATCH 4/5] driver core: dev_get_drvdata: Don't check for NULL dev
> [PATCH 5/5] driver core: Inline dev_set/get_drvdata
> 

Sorry for the long delay, these are finally all now applied to my tree.

greg k-h

Re: [PATCH 0/5] driver core: Clean up dev_get/set_drvdata

[Jean Delvare]

On Tue, 27 May 2014 13:50:36 -0700, Greg KH wrote:
> On Mon, Apr 14, 2014 at 12:48:02PM +0200, Jean Delvare wrote:
> > This patch set is a proposal to revert most of commit b4028437. This
> > would solve a memory leak and also allow to simplify and ultimately
> > inline again functions dev_get_drvdata and dev_set_drvdata for smaller
> > footprint and improved performance.
> > 
> > [PATCH 1/5] driver core: Move driver_data back to struct device
> > [PATCH 2/5] driver core: dev_set_drvdata can no longer fail
> > [PATCH 3/5] driver core: dev_set_drvdata returns void
> > [PATCH 4/5] driver core: dev_get_drvdata: Don't check for NULL dev
> > [PATCH 5/5] driver core: Inline dev_set/get_drvdata
> > 
> 
> Sorry for the long delay, these are finally all now applied to my tree.

Cool, thanks!

-- 
Jean Delvare
SUSE L3 Support