Re: [PATCH 11/41] rtc: ds1685: Convert to platform remove callback returning void - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Joshua Kinard <kumba@gentoo.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	linux-rtc@vger.kernel.org, kernel@pengutronix.de
Subject: Re: [PATCH 11/41] rtc: ds1685: Convert to platform remove callback returning void
Date: Mon, 6 Mar 2023 22:22:18 +0100	[thread overview]
Message-ID: <20230306212218.qzer65c74rb7d6yy@pengutronix.de> (raw)
In-Reply-To: <9e2df07f-92d3-966a-5092-22572e102253@gentoo.org>

[-- Attachment #1: Type: text/plain, Size: 4426 bytes --]

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@pengutronix.de>
> > ---
> >   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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
