Re: [PATCH 11/41] rtc: ds1685: Convert to platform remove callback returning void

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/8/2023 11:47, Uwe Kleine-König wrote:
Hello Joshua,

On Wed, Mar 08, 2023 at 11:20:34AM -0500, Joshua Kinard wrote:
On 3/7/2023 03:11, Uwe Kleine-König wrote:
On Mon, Mar 06, 2023 at 09:09:03PM -0500, Joshua Kinard wrote:
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)?

A bit of statistic: Based on v6.3-rc1 I have 2286 patches like the ones
from this series that (mostly) convert drivers that today already return
zero unconditionally. Then there is my todo-list of ~100 additional
drivers that don't return 0 that need manual inspection and fixing.

So we're talking about 2300+ drivers in all subsystems here. To get a
bisectable series that does the complete conversion, we need:

	2300 patches to convert drivers to .remove_new()
	the above patch
	2300 patches to convert drivers back to the new .remove()

Last time I sent a series with ~640 patches (for a similar conversion
for i2c drivers) people were unlucky already and I got tons of bounces.
Please consider the address list for the cover letter. While most
patches are trivial this would require a massive coordination.

So no, this isn't a sensible suggestion. I'll continue to send out
conversions to .remove_new() per subsystem and once most of them are
converted, the above patch will be sent with the remainder of the
unapplied patches.

I was actually thinking more along the lines of doing one patch series per
subsystem, so for RTC, ~160 patches, one per rtc-*.c driver making the
change to have .remove() be void, plus a patch to adjust the struct
platform_driver rtc definition itself.

The problem is that all subsystems use the same struct platform_driver.
So I cannot change it for rtc and then in the next development cycle for
(say) iio and i2c.


Hah, you're right! Sorry, I was thinking more classic OOP and thought that the rtc base API had a derived struct from platform_driver that allowed for a more precise patch series to be applied. I know some subsystems do have this, but not rtc, so yes, your approach does make more sense now.


That way you don't have to have that
interval period where drivers are carrying around the .remove_new() method,
cause I've seen instances in the past where such large-scale changes could
not be completed in a single kernel development cycle.

Different maintainers seem to have different preferences, but most
prefer one patch per driver. And I don't plan to complete the conversion
in a single development cycle.

But it sounds like you've already got a plan worked out, so your call on how
you want to handle this.  Thanks for the additional clarification!

Acked-By: Joshua Kinard <kumba@xxxxxxxxxx>

Thanks
Uwe


--
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




[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux