[PATCH 0/5] watchdog: Drop platform_driver_probe() and convert to platform remove callback returning void (part II)

[PATCH 0/5] watchdog: Drop platform_driver_probe() and convert to platform remove callback returning void (part II)

[Uwe Kleine-König]

Hello,

there are two different types of patches here that would justify to
different series. But as the patches are not independent I chose to put
them in a single series.

The first two patches drop usage of platform_driver_probe(). This is a
concept that isn't so relevant any more today. I didn't check, but it
saves typically only a few 100k and there are thoughts to deprecate it
to simplify the core. Getting the usage right is not trivial though the
drivers here got it nearly right. The alternative to these patches is to
add __refdata to the driver struct ideally with a comment describing the
need like is e.g. done in commit 5b44abbc39ca ("platform/x86: hp-wmi::
Mark driver struct with __refdata to prevent section mismatch warning").
Note that the warning only happens starting with commit f177cd0c15fc
("modpost: Don't let "driver"s reference .exit.*") that is expected to
be part of v6.7-rc1.

The remaining three patches convert the platform drivers to
.remove_new(), see commit 5c5a7680e67b ("platform: Provide a remove
callback that returns no value") for an extended explanation and the
eventual goal. Some time ago I already did this to watchdog drivers, but
I missed the at91sam9_wdt and txx9wdt driver, because my coccinelle
patch failed to detect these as patch opportunities (because of the
__exit_p annotation). the starfive-wdt driver is new since then.

Best regards
Uwe

Uwe Kleine-König (5):
  watchdog: at91sam9: Stop using module_platform_driver_probe()
  watchdog: txx9: Stop using module_platform_driver_probe()
  watchdog: at91sam9_wdt: Convert to platform remove callback returning
    void
  watchdog: starfive-wdt: Convert to platform remove callback returning
    void
  watchdog: txx9wdt: Convert to platform remove callback returning void

 drivers/watchdog/at91sam9_wdt.c | 12 +++++-------
 drivers/watchdog/starfive-wdt.c |  6 ++----
 drivers/watchdog/txx9wdt.c      | 11 +++++------
 3 files changed, 12 insertions(+), 17 deletions(-)

base-commit: 3ff7a5781ceee3befb9224d29cef6e6a4766c5fe
-- 
2.42.0

[PATCH 1/5] watchdog: at91sam9: Stop using module_platform_driver_probe()

[Uwe Kleine-König]

On today's platforms the benefit of platform_driver_probe() isn't that
relevant any more. It allows to drop some code after booting (or module
loading) for .probe() and discard the .remove() function completely if
the driver is built-in. This typically saves a few 100k.

The downside of platform_driver_probe() is that the driver cannot be
bound and unbound at runtime which is ancient and also slightly
complicates testing. There are also thoughts to deprecate
platform_driver_probe() because it adds some complexity in the driver
core for little gain. Also many drivers don't use it correctly. This
driver for example misses to mark the driver struct with __refdata which
is needed to suppress a (W=1) modpost warning:

	WARNING: modpost: drivers/watchdog/at91sam9_wdt: section mismatch in reference: at91wdt_driver+0x4 (section: .data) -> at91wdt_remove (section: .exit.text)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/at91sam9_wdt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index b111b28acb94..507adb12754d 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -324,7 +324,7 @@ static inline int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 }
 #endif
 
-static int __init at91wdt_probe(struct platform_device *pdev)
+static int at91wdt_probe(struct platform_device *pdev)
 {
 	int err;
 	struct at91wdt *wdt;
@@ -372,7 +372,7 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int __exit at91wdt_remove(struct platform_device *pdev)
+static int at91wdt_remove(struct platform_device *pdev)
 {
 	struct at91wdt *wdt = platform_get_drvdata(pdev);
 	watchdog_unregister_device(&wdt->wdd);
@@ -393,14 +393,14 @@ MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids);
 #endif
 
 static struct platform_driver at91wdt_driver = {
-	.remove		= __exit_p(at91wdt_remove),
+	.probe		= at91wdt_probe,
+	.remove		= at91wdt_remove,
 	.driver		= {
 		.name	= "at91_wdt",
 		.of_match_table = of_match_ptr(at91_wdt_dt_ids),
 	},
 };
-
-module_platform_driver_probe(at91wdt_driver, at91wdt_probe);
+module_platform_driver(at91wdt_driver);
 
 MODULE_AUTHOR("Renaud CERRATO <r.cerrato@til-technologies.fr>");
 MODULE_DESCRIPTION("Watchdog driver for Atmel AT91SAM9x processors");
-- 
2.42.0

Re: [PATCH 1/5] watchdog: at91sam9: Stop using module_platform_driver_probe()

[Guenter Roeck]

On Mon, Nov 06, 2023 at 04:48:09PM +0100, Uwe Kleine-König wrote:
> On today's platforms the benefit of platform_driver_probe() isn't that
> relevant any more. It allows to drop some code after booting (or module
> loading) for .probe() and discard the .remove() function completely if
> the driver is built-in. This typically saves a few 100k.
> 
> The downside of platform_driver_probe() is that the driver cannot be
> bound and unbound at runtime which is ancient and also slightly
> complicates testing. There are also thoughts to deprecate
> platform_driver_probe() because it adds some complexity in the driver
> core for little gain. Also many drivers don't use it correctly. This
> driver for example misses to mark the driver struct with __refdata which
> is needed to suppress a (W=1) modpost warning:
> 
> 	WARNING: modpost: drivers/watchdog/at91sam9_wdt: section mismatch in reference: at91wdt_driver+0x4 (section: .data) -> at91wdt_remove (section: .exit.text)
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 1/5] watchdog: at91sam9: Stop using module_platform_driver_probe()

[Nicolas Ferre]

On 06/11/2023 at 16:48, Uwe Kleine-König wrote:
> On today's platforms the benefit of platform_driver_probe() isn't that
> relevant any more. It allows to drop some code after booting (or module
> loading) for .probe() and discard the .remove() function completely if
> the driver is built-in. This typically saves a few 100k.
> 
> The downside of platform_driver_probe() is that the driver cannot be
> bound and unbound at runtime which is ancient and also slightly
> complicates testing. There are also thoughts to deprecate
> platform_driver_probe() because it adds some complexity in the driver
> core for little gain. Also many drivers don't use it correctly. This
> driver for example misses to mark the driver struct with __refdata which
> is needed to suppress a (W=1) modpost warning:
> 
>          WARNING: modpost: drivers/watchdog/at91sam9_wdt: section mismatch in reference: at91wdt_driver+0x4 (section: .data) -> at91wdt_remove (section: .exit.text)
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

> ---
>   drivers/watchdog/at91sam9_wdt.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

[..]

[PATCH 2/5] watchdog: txx9: Stop using module_platform_driver_probe()

[Uwe Kleine-König]

On today's platforms the benefit of platform_driver_probe() isn't that
relevant any more. It allows to drop some code after booting (or module
loading) for .probe() and discard the .remove() function completely if
the driver is built-in. This typically saves a few 100k.

The downside of platform_driver_probe() is that the driver cannot be
bound and unbound at runtime which is ancient and also slightly
complicates testing. There are also thoughts to deprecate
platform_driver_probe() because it adds some complexity in the driver
core for little gain. Also many drivers don't use it correctly. This
driver for example misses to mark the driver struct with __refdata which
is needed to suppress a (W=1) modpost warning:

	WARNING: modpost: drivers/watchdog/txx9wdt: section mismatch in reference: txx9wdt_driver+0x4 (section: .data) -> txx9wdt_remove (section: .exit.text)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/txx9wdt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/txx9wdt.c b/drivers/watchdog/txx9wdt.c
index 89a54b6645bd..f900c519ed63 100644
--- a/drivers/watchdog/txx9wdt.c
+++ b/drivers/watchdog/txx9wdt.c
@@ -98,7 +98,7 @@ static struct watchdog_device txx9wdt = {
 	.ops = &txx9wdt_ops,
 };
 
-static int __init txx9wdt_probe(struct platform_device *dev)
+static int txx9wdt_probe(struct platform_device *dev)
 {
 	int ret;
 
@@ -145,7 +145,7 @@ static int __init txx9wdt_probe(struct platform_device *dev)
 	return ret;
 }
 
-static int __exit txx9wdt_remove(struct platform_device *dev)
+static int txx9wdt_remove(struct platform_device *dev)
 {
 	watchdog_unregister_device(&txx9wdt);
 	clk_disable_unprepare(txx9_imclk);
@@ -159,14 +159,14 @@ static void txx9wdt_shutdown(struct platform_device *dev)
 }
 
 static struct platform_driver txx9wdt_driver = {
-	.remove = __exit_p(txx9wdt_remove),
+	.probe = txx9wdt_probe,
+	.remove = txx9wdt_remove,
 	.shutdown = txx9wdt_shutdown,
 	.driver = {
 		.name = "txx9wdt",
 	},
 };
-
-module_platform_driver_probe(txx9wdt_driver, txx9wdt_probe);
+module_platform_driver(txx9wdt_driver);
 
 MODULE_DESCRIPTION("TXx9 Watchdog Driver");
 MODULE_LICENSE("GPL");
-- 
2.42.0

Re: [PATCH 2/5] watchdog: txx9: Stop using module_platform_driver_probe()

[Guenter Roeck]

On Mon, Nov 06, 2023 at 04:48:10PM +0100, Uwe Kleine-König wrote:
> On today's platforms the benefit of platform_driver_probe() isn't that
> relevant any more. It allows to drop some code after booting (or module
> loading) for .probe() and discard the .remove() function completely if
> the driver is built-in. This typically saves a few 100k.
> 
> The downside of platform_driver_probe() is that the driver cannot be
> bound and unbound at runtime which is ancient and also slightly
> complicates testing. There are also thoughts to deprecate
> platform_driver_probe() because it adds some complexity in the driver
> core for little gain. Also many drivers don't use it correctly. This
> driver for example misses to mark the driver struct with __refdata which
> is needed to suppress a (W=1) modpost warning:
> 
> 	WARNING: modpost: drivers/watchdog/txx9wdt: section mismatch in reference: txx9wdt_driver+0x4 (section: .data) -> txx9wdt_remove (section: .exit.text)
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH 3/5] watchdog: at91sam9_wdt: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.

To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/at91sam9_wdt.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 507adb12754d..2c6474cb858b 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -372,15 +372,13 @@ static int at91wdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int at91wdt_remove(struct platform_device *pdev)
+static void at91wdt_remove(struct platform_device *pdev)
 {
 	struct at91wdt *wdt = platform_get_drvdata(pdev);
 	watchdog_unregister_device(&wdt->wdd);
 
 	pr_warn("I quit now, hardware will probably reboot!\n");
 	del_timer(&wdt->timer);
-
-	return 0;
 }
 
 #if defined(CONFIG_OF)
@@ -394,7 +392,7 @@ MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids);
 
 static struct platform_driver at91wdt_driver = {
 	.probe		= at91wdt_probe,
-	.remove		= at91wdt_remove,
+	.remove_new	= at91wdt_remove,
 	.driver		= {
 		.name	= "at91_wdt",
 		.of_match_table = of_match_ptr(at91_wdt_dt_ids),
-- 
2.42.0

Re: [PATCH 3/5] watchdog: at91sam9_wdt: Convert to platform remove callback returning void

[Guenter Roeck]

On Mon, Nov 06, 2023 at 04:48:11PM +0100, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> 
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 3/5] watchdog: at91sam9_wdt: Convert to platform remove callback returning void

[Nicolas Ferre]

On 06/11/2023 at 16:48, Uwe Kleine-König wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> 
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Thanks Uwe!

Best regards,
   Nicolas

> ---
>   drivers/watchdog/at91sam9_wdt.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)

[..]

[PATCH 4/5] watchdog: starfive-wdt: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.

To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/starfive-wdt.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index 5f501b41faf9..16df92837c5d 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -504,7 +504,7 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int starfive_wdt_remove(struct platform_device *pdev)
+static void starfive_wdt_remove(struct platform_device *pdev)
 {
 	struct starfive_wdt *wdt = platform_get_drvdata(pdev);
 
@@ -516,8 +516,6 @@ static int starfive_wdt_remove(struct platform_device *pdev)
 	else
 		/* disable clock without PM */
 		starfive_wdt_disable_clock(wdt);
-
-	return 0;
 }
 
 static void starfive_wdt_shutdown(struct platform_device *pdev)
@@ -587,7 +585,7 @@ MODULE_DEVICE_TABLE(of, starfive_wdt_match);
 
 static struct platform_driver starfive_wdt_driver = {
 	.probe = starfive_wdt_probe,
-	.remove = starfive_wdt_remove,
+	.remove_new = starfive_wdt_remove,
 	.shutdown = starfive_wdt_shutdown,
 	.driver = {
 		.name = "starfive-wdt",
-- 
2.42.0

Re: [PATCH 4/5] watchdog: starfive-wdt: Convert to platform remove callback returning void

[Guenter Roeck]

On Mon, Nov 06, 2023 at 04:48:12PM +0100, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> 
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH 5/5] watchdog: txx9wdt: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.

To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/txx9wdt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/txx9wdt.c b/drivers/watchdog/txx9wdt.c
index f900c519ed63..8d5f67acbff2 100644
--- a/drivers/watchdog/txx9wdt.c
+++ b/drivers/watchdog/txx9wdt.c
@@ -145,12 +145,11 @@ static int txx9wdt_probe(struct platform_device *dev)
 	return ret;
 }
 
-static int txx9wdt_remove(struct platform_device *dev)
+static void txx9wdt_remove(struct platform_device *dev)
 {
 	watchdog_unregister_device(&txx9wdt);
 	clk_disable_unprepare(txx9_imclk);
 	clk_put(txx9_imclk);
-	return 0;
 }
 
 static void txx9wdt_shutdown(struct platform_device *dev)
@@ -160,7 +159,7 @@ static void txx9wdt_shutdown(struct platform_device *dev)
 
 static struct platform_driver txx9wdt_driver = {
 	.probe = txx9wdt_probe,
-	.remove = txx9wdt_remove,
+	.remove_new = txx9wdt_remove,
 	.shutdown = txx9wdt_shutdown,
 	.driver = {
 		.name = "txx9wdt",
-- 
2.42.0

Re: [PATCH 5/5] watchdog: txx9wdt: Convert to platform remove callback returning void

[Guenter Roeck]

On Mon, Nov 06, 2023 at 04:48:13PM +0100, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> 
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>