[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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[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 <jdel...@suse.de>
Cc: Greg Kroah-Hartman <gre...@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]

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 <jdel...@suse.de>
Cc: Greg Kroah-Hartman <gre...@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]

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 <jdel...@suse.de>
Cc: Greg Kroah-Hartman <gre...@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]

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

Signed-off-by: Jean Delvare <jdel...@suse.de>
Cc: Greg Kroah-Hartman <gre...@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]

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 <jdel...@suse.de>
Cc: Greg Kroah-Hartman <gre...@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;

[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

[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

[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