[PATCH RESEND 0/4] platform: fix accidently erasing .owner

[PATCH RESEND 0/4] platform: fix accidently erasing .owner

[Wolfram Sang]

Since platform_register_driver() was converted to set the .owner of a driver,
there was a codepath on which the owner would have been erased. This series
fixes that and adds a warning to prevent such cases go unnoticed in the future.
For consistency, a semantic patch is also added which reports when .owner gets
unnecissarily set. More details in the patch descriptions. Thanks go to Russell
King for pointing out this problem and to Julia Lawall for help with the
semantic patch.

All these patches have been tested on a custom board.

This resend was rebased to rc2. It produced identical patches. IMO this should
go into v3.18 since it is a bugfix.

Please apply. The cleanup series for removing .owner in the kernel tree will
be resend later, too, as a seperate pull request.

Wolfram Sang (4):
  core: platform: add warning if driver has no owner
  core: platform: let platform_driver_probe initialize module owner
  core: platform: let platform_create_bundle initialize module owner
  coccinelle: api: add spatch to prevent unnecessary .owner

 drivers/base/driver.c                              |   3 +
 drivers/base/platform.c                            |  22 +++--
 include/linux/platform_device.h                    |  12 ++-
 scripts/coccinelle/api/platform_no_drv_owner.cocci | 106 +++++++++++++++++++++
 4 files changed, 129 insertions(+), 14 deletions(-)
 create mode 100644 scripts/coccinelle/api/platform_no_drv_owner.cocci

-- 
2.0.0

[PATCH RESEND 1/4] core: platform: add warning if driver has no owner

[Wolfram Sang]

Commit 9447057eaff8 ("platform_device: use a macro instead of
platform_driver_register") introduced a codepath which could result into
drivers having no owner. This went unnoticed for months, so add a
warning in case this happens again somewhere else somewhen.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/base/driver.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 9e29943e56ca..6b10ff3bb410 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -151,6 +151,9 @@ int driver_register(struct device_driver *drv)
 
 	BUG_ON(!drv->bus->p);
 
+	if (!drv->owner)
+		printk(KERN_WARNING "Driver '%s' needs an owner", drv->name);
+
 	if ((drv->bus->probe && drv->probe) ||
 	    (drv->bus->remove && drv->remove) ||
 	    (drv->bus->shutdown && drv->shutdown))
-- 
2.0.0

Re: [PATCH RESEND 1/4] core: platform: add warning if driver has no owner

[Tony Luck]

> +       if (!drv->owner)
> +               printk(KERN_WARNING "Driver '%s' needs an owner", drv->name);

This showed up in next-20141110 and on ia64 I see 44 "needs an owner" messages,
even from drivers like e1000.

Anyone else seeing anything like that? Or is some ia64 initialization path
missing an update?

-Tony

Re: [PATCH RESEND 1/4] core: platform: add warning if driver has no owner

[Greg KH]

On Mon, Nov 10, 2014 at 01:32:08PM -0800, Tony Luck wrote:
> > +       if (!drv->owner)
> > +               printk(KERN_WARNING "Driver '%s' needs an owner", drv->name);
> 
> This showed up in next-20141110 and on ia64 I see 44 "needs an owner" messages,
> even from drivers like e1000.
> 
> Anyone else seeing anything like that? Or is some ia64 initialization path
> missing an update?

Yes, sorry, this patch is now reverted.

greg k-h

[PATCH RESEND 2/4] core: platform: let platform_driver_probe initialize module owner

[Wolfram Sang]

Since commit 9447057eaff8 ("platform_device: use a macro instead of
platform_driver_register"), platform_driver_register() always overwrites
the .owner field of a platform_driver with THIS_MODULE. This breaks
platform_driver_probe() which uses it from within the platform core
instead of the module init. Fix it by using a similar #define construct
to obtain THIS_MODULE and pass it on later.

Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/base/platform.c         | 11 ++++++-----
 include/linux/platform_device.h |  6 ++++--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index b2afc29403f9..c87a63326871 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -580,9 +580,10 @@ void platform_driver_unregister(struct platform_driver *drv)
 EXPORT_SYMBOL_GPL(platform_driver_unregister);
 
 /**
- * platform_driver_probe - register driver for non-hotpluggable device
+ * __platform_driver_probe - register driver for non-hotpluggable device
  * @drv: platform driver structure
  * @probe: the driver probe routine, probably from an __init section
+ * @module: module which will be the owner of the driver
  *
  * Use this instead of platform_driver_register() when you know the device
  * is not hotpluggable and has already been registered, and you want to
@@ -598,8 +599,8 @@ EXPORT_SYMBOL_GPL(platform_driver_unregister);
  * Returns zero if the driver registered and bound to a device, else returns
  * a negative error code and with the driver not registered.
  */
-int __init_or_module platform_driver_probe(struct platform_driver *drv,
-		int (*probe)(struct platform_device *))
+int __init_or_module __platform_driver_probe(struct platform_driver *drv,
+		int (*probe)(struct platform_device *), struct module *module)
 {
 	int retval, code;
 
@@ -614,7 +615,7 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
 
 	/* temporary section violation during probe() */
 	drv->probe = probe;
-	retval = code = platform_driver_register(drv);
+	retval = code = __platform_driver_register(drv, module);
 
 	/*
 	 * Fixup that section violation, being paranoid about code scanning
@@ -633,7 +634,7 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
 		platform_driver_unregister(drv);
 	return retval;
 }
-EXPORT_SYMBOL_GPL(platform_driver_probe);
+EXPORT_SYMBOL_GPL(__platform_driver_probe);
 
 /**
  * platform_create_bundle - register driver and create corresponding device
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 153d303af7eb..c8d95c60da19 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -197,8 +197,10 @@ extern void platform_driver_unregister(struct platform_driver *);
 /* non-hotpluggable platform devices may use this so that probe() and
  * its support may live in __init sections, conserving runtime memory.
  */
-extern int platform_driver_probe(struct platform_driver *driver,
-		int (*probe)(struct platform_device *));
+#define platform_driver_probe(drv, probe) \
+	__platform_driver_probe(drv, probe, THIS_MODULE)
+extern int __platform_driver_probe(struct platform_driver *driver,
+		int (*probe)(struct platform_device *), struct module *module);
 
 static inline void *platform_get_drvdata(const struct platform_device *pdev)
 {
-- 
2.0.0

[PATCH RESEND 3/4] core: platform: let platform_create_bundle initialize module owner

[Wolfram Sang]

Since commit 9447057eaff8 ("platform_device: use a macro instead of
platform_driver_register"), platform_driver_register() always overwrites
the .owner field of a platform_driver with THIS_MODULE. This breaks
platform_create_bundle() which uses it via platform_driver_probe() from
within the platform core instead of the module init. Fix it by using a
similar #define construct to obtain THIS_MODULE and pass it on later.

Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/base/platform.c         | 11 ++++++-----
 include/linux/platform_device.h |  6 ++++--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c87a63326871..cdb6c076c3f7 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -637,24 +637,25 @@ int __init_or_module __platform_driver_probe(struct platform_driver *drv,
 EXPORT_SYMBOL_GPL(__platform_driver_probe);
 
 /**
- * platform_create_bundle - register driver and create corresponding device
+ * __platform_create_bundle - register driver and create corresponding device
  * @driver: platform driver structure
  * @probe: the driver probe routine, probably from an __init section
  * @res: set of resources that needs to be allocated for the device
  * @n_res: number of resources
  * @data: platform specific data for this platform device
  * @size: size of platform specific data
+ * @module: module which will be the owner of the driver
  *
  * Use this in legacy-style modules that probe hardware directly and
  * register a single platform device and corresponding platform driver.
  *
  * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
  */
-struct platform_device * __init_or_module platform_create_bundle(
+struct platform_device * __init_or_module __platform_create_bundle(
 			struct platform_driver *driver,
 			int (*probe)(struct platform_device *),
 			struct resource *res, unsigned int n_res,
-			const void *data, size_t size)
+			const void *data, size_t size, struct module *module)
 {
 	struct platform_device *pdev;
 	int error;
@@ -677,7 +678,7 @@ struct platform_device * __init_or_module platform_create_bundle(
 	if (error)
 		goto err_pdev_put;
 
-	error = platform_driver_probe(driver, probe);
+	error = __platform_driver_probe(driver, probe, module);
 	if (error)
 		goto err_pdev_del;
 
@@ -690,7 +691,7 @@ err_pdev_put:
 err_out:
 	return ERR_PTR(error);
 }
-EXPORT_SYMBOL_GPL(platform_create_bundle);
+EXPORT_SYMBOL_GPL(__platform_create_bundle);
 
 /* modalias support enables more hands-off userspace setup:
  * (a) environment variable lets new-style hotplug events work once system is
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index c8d95c60da19..ae4882ca4a64 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -240,10 +240,12 @@ static void __exit __platform_driver##_exit(void) \
 } \
 module_exit(__platform_driver##_exit);
 
-extern struct platform_device *platform_create_bundle(
+#define platform_create_bundle(driver, probe, res, n_res, data, size) \
+	__platform_create_bundle(driver, probe, res, n_res, data, size, THIS_MODULE)
+extern struct platform_device *__platform_create_bundle(
 	struct platform_driver *driver, int (*probe)(struct platform_device *),
 	struct resource *res, unsigned int n_res,
-	const void *data, size_t size);
+	const void *data, size_t size, struct module *module);
 
 /* early platform driver interface */
 struct early_platform_driver {
-- 
2.0.0

[PATCH RESEND 4/4] coccinelle: api: add spatch to prevent unnecessary .owner

[Wolfram Sang]

There are calls which silently set the owner of a module. This is the
preferred way [1], so avoid setting it manually. Currently, we only care
about platform drivers, but there might be more calls to be added later.

[1] https://lkml.org/lkml/2014/10/12/87

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 scripts/coccinelle/api/platform_no_drv_owner.cocci | 106 +++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100644 scripts/coccinelle/api/platform_no_drv_owner.cocci

diff --git a/scripts/coccinelle/api/platform_no_drv_owner.cocci b/scripts/coccinelle/api/platform_no_drv_owner.cocci
new file mode 100644
index 000000000000..e065b9e714fc
--- /dev/null
+++ b/scripts/coccinelle/api/platform_no_drv_owner.cocci
@@ -0,0 +1,106 @@
+/// Remove .owner field if calls are used which set it automatically
+///
+// Confidence: High
+// Copyright: (C) 2014 Wolfram Sang. GPL v2.
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+@match1@
+declarer name module_platform_driver;
+declarer name module_platform_driver_probe;
+identifier __driver;
+@@
+(
+	module_platform_driver(__driver);
+|
+	module_platform_driver_probe(__driver, ...);
+)
+
+@fix1 depends on match1 && patch && !context && !org && !report@
+identifier match1.__driver;
+@@
+	static struct platform_driver __driver = {
+		.driver = {
+-			.owner = THIS_MODULE,
+		}
+	};
+
+@match2@
+identifier __driver;
+@@
+(
+	platform_driver_register(&__driver)
+|
+	platform_driver_probe(&__driver, ...)
+|
+	platform_create_bundle(&__driver, ...)
+)
+
+@fix2 depends on match2 && patch && !context && !org && !report@
+identifier match2.__driver;
+@@
+	static struct platform_driver __driver = {
+		.driver = {
+-			.owner = THIS_MODULE,
+		}
+	};
+
+// ----------------------------------------------------------------------------
+
+@fix1_context depends on match1 && !patch && (context || org || report)@
+identifier match1.__driver;
+position j0;
+@@
+
+ 	static struct platform_driver __driver = {
+		.driver = {
+*			.owner@j0 = THIS_MODULE,
+		}
+	};
+
+@fix2_context depends on match2 && !patch && (context || org || report)@
+identifier match2.__driver;
+position j0;
+@@
+
+ 	static struct platform_driver __driver = {
+		.driver = {
+*			.owner@j0 = THIS_MODULE,
+		}
+	};
+
+// ----------------------------------------------------------------------------
+
+@script:python fix1_org depends on org@
+j0 << fix1_context.j0;
+@@
+
+msg = "No need to set .owner here. The core will do it."
+coccilib.org.print_todo(j0[0], msg)
+
+@script:python fix2_org depends on org@
+j0 << fix2_context.j0;
+@@
+
+msg = "No need to set .owner here. The core will do it."
+coccilib.org.print_todo(j0[0], msg)
+
+// ----------------------------------------------------------------------------
+
+@script:python fix1_report depends on report@
+j0 << fix1_context.j0;
+@@
+
+msg = "No need to set .owner here. The core will do it."
+coccilib.report.print_report(j0[0], msg)
+
+@script:python fix2_report depends on report@
+j0 << fix2_context.j0;
+@@
+
+msg = "No need to set .owner here. The core will do it."
+coccilib.report.print_report(j0[0], msg)
+
-- 
2.0.0

Re: [PATCH RESEND 4/4] coccinelle: api: add spatch to prevent unnecessary .owner

[Julia Lawall]

Acked-by: Julia Lawall <julia.lawall@lip6.fr>

On Tue, 28 Oct 2014, Wolfram Sang wrote:

> There are calls which silently set the owner of a module. This is the
> preferred way [1], so avoid setting it manually. Currently, we only care
> about platform drivers, but there might be more calls to be added later.
> 
> [1] https://lkml.org/lkml/2014/10/12/87
> 
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>  scripts/coccinelle/api/platform_no_drv_owner.cocci | 106 +++++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 scripts/coccinelle/api/platform_no_drv_owner.cocci
> 
> diff --git a/scripts/coccinelle/api/platform_no_drv_owner.cocci b/scripts/coccinelle/api/platform_no_drv_owner.cocci
> new file mode 100644
> index 000000000000..e065b9e714fc
> --- /dev/null
> +++ b/scripts/coccinelle/api/platform_no_drv_owner.cocci
> @@ -0,0 +1,106 @@
> +/// Remove .owner field if calls are used which set it automatically
> +///
> +// Confidence: High
> +// Copyright: (C) 2014 Wolfram Sang. GPL v2.
> +
> +virtual patch
> +virtual context
> +virtual org
> +virtual report
> +
> +@match1@
> +declarer name module_platform_driver;
> +declarer name module_platform_driver_probe;
> +identifier __driver;
> +@@
> +(
> +	module_platform_driver(__driver);
> +|
> +	module_platform_driver_probe(__driver, ...);
> +)
> +
> +@fix1 depends on match1 && patch && !context && !org && !report@
> +identifier match1.__driver;
> +@@
> +	static struct platform_driver __driver = {
> +		.driver = {
> +-			.owner = THIS_MODULE,
> +		}
> +	};
> +
> +@match2@
> +identifier __driver;
> +@@
> +(
> +	platform_driver_register(&__driver)
> +|
> +	platform_driver_probe(&__driver, ...)
> +|
> +	platform_create_bundle(&__driver, ...)
> +)
> +
> +@fix2 depends on match2 && patch && !context && !org && !report@
> +identifier match2.__driver;
> +@@
> +	static struct platform_driver __driver = {
> +		.driver = {
> +-			.owner = THIS_MODULE,
> +		}
> +	};
> +
> +// ----------------------------------------------------------------------------
> +
> +@fix1_context depends on match1 && !patch && (context || org || report)@
> +identifier match1.__driver;
> +position j0;
> +@@
> +
> + 	static struct platform_driver __driver = {
> +		.driver = {
> +*			.owner@j0 = THIS_MODULE,
> +		}
> +	};
> +
> +@fix2_context depends on match2 && !patch && (context || org || report)@
> +identifier match2.__driver;
> +position j0;
> +@@
> +
> + 	static struct platform_driver __driver = {
> +		.driver = {
> +*			.owner@j0 = THIS_MODULE,
> +		}
> +	};
> +
> +// ----------------------------------------------------------------------------
> +
> +@script:python fix1_org depends on org@
> +j0 << fix1_context.j0;
> +@@
> +
> +msg = "No need to set .owner here. The core will do it."
> +coccilib.org.print_todo(j0[0], msg)
> +
> +@script:python fix2_org depends on org@
> +j0 << fix2_context.j0;
> +@@
> +
> +msg = "No need to set .owner here. The core will do it."
> +coccilib.org.print_todo(j0[0], msg)
> +
> +// ----------------------------------------------------------------------------
> +
> +@script:python fix1_report depends on report@
> +j0 << fix1_context.j0;
> +@@
> +
> +msg = "No need to set .owner here. The core will do it."
> +coccilib.report.print_report(j0[0], msg)
> +
> +@script:python fix2_report depends on report@
> +j0 << fix2_context.j0;
> +@@
> +
> +msg = "No need to set .owner here. The core will do it."
> +coccilib.report.print_report(j0[0], msg)
> +
> -- 
> 2.0.0
> 
>