On 3/6/2023 16:22, Uwe Kleine-König wrote:
On Mon, Mar 06, 2023 at 02:43:20PM -0500, Joshua Kinard wrote:
On 3/4/2023 08:29, 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 (mostly) ignored
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.
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@xxxxxxxxxxxxxx>
---
drivers/rtc/rtc-ds1685.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 5db9c737c022..0f707be0eb87 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -1322,7 +1322,7 @@ ds1685_rtc_probe(struct platform_device *pdev)
* ds1685_rtc_remove - removes rtc driver.
* @pdev: pointer to platform_device structure.
*/
-static int
+static void
ds1685_rtc_remove(struct platform_device *pdev)
{
struct ds1685_priv *rtc = platform_get_drvdata(pdev);
@@ -1344,8 +1344,6 @@ ds1685_rtc_remove(struct platform_device *pdev)
rtc->write(rtc, RTC_EXT_CTRL_4A,
(rtc->read(rtc, RTC_EXT_CTRL_4A) &
~(RTC_CTRL_4A_RWK_MASK)));
-
- return 0;
}
/*
@@ -1356,7 +1354,7 @@ static struct platform_driver ds1685_rtc_driver = {
.name = "rtc-ds1685",
},
.probe = ds1685_rtc_probe,
- .remove = ds1685_rtc_remove,
+ .remove_new = ds1685_rtc_remove,
};
module_platform_driver(ds1685_rtc_driver);
/* ----------------------------------------------------------------------- */
Is there a future planned patch that would remove the .remove member
and then rename .remove_new --> .remove?
The eventual plan is to do
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 77510e4f47de..1c65943d6b53 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1420,14 +1420,8 @@ static void platform_remove(struct device *_dev)
struct platform_driver *drv = to_platform_driver(_dev->driver);
struct platform_device *dev = to_platform_device(_dev);
- if (drv->remove_new) {
- drv->remove_new(dev);
- } else if (drv->remove) {
- int ret = drv->remove(dev);
-
- if (ret)
- dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
- }
+ if (drv->remove)
+ drv->remove(dev);
dev_pm_domain_detach(_dev, true);
}
index b845fd83f429..8c5fdaa8645f 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -209,15 +209,16 @@ struct platform_driver {
int (*probe)(struct platform_device *);
/*
- * Traditionally the remove callback returned an int which however is
+ * Traditionally the remove callback returned an int which however was
* ignored by the driver core. This led to wrong expectations by driver
* authors who thought returning an error code was a valid error
- * handling strategy. To convert to a callback returning void, new
- * drivers should implement .remove_new() until the conversion it done
- * that eventually makes .remove() return void.
+ * handling strategy. .remove_new is a hangover from these times which
+ * will be dropped once all drivers are converted to .remove().
*/
- int (*remove)(struct platform_device *);
- void (*remove_new)(struct platform_device *);
+ union {
+ void (*remove)(struct platform_device *);
+ void (*remove_new)(struct platform_device *);
+ };
void (*shutdown)(struct platform_device *);
int (*suspend)(struct platform_device *, pm_message_t state);
and then once all the drivers are converted back to .remove() drop the
union and .remove_new().
Best regards
Uwe
This looks like a pretty simple/minor API change. Why not just do a patch series that makes both the API
change and updates all of the drivers at once (one commit per driver)? It gets it done and out of the way and
you don't have to worry about fixing things later on. I've seen far more intrusive changes done via patch
series before with little resistance.
--
Joshua Kinard
Gentoo/MIPS
kumba@xxxxxxxxxx
rsa6144/5C63F4E3F5C6C943 2015-04-27
177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943
"The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by
moment, lost in that vast, terrible in-between."
--Emperor Turhan, Centauri Republic
