From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Joshua Kinard <kumba@gentoo.org>
Cc: linux-rtc@vger.kernel.org,
Alessandro Zummo <a.zummo@towertech.it>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
kernel@pengutronix.de
Subject: Re: [PATCH 11/41] rtc: ds1685: Convert to platform remove callback returning void
Date: Wed, 8 Mar 2023 17:47:50 +0100 [thread overview]
Message-ID: <20230308164750.x4ozcdxymcqoz73m@pengutronix.de> (raw)
In-Reply-To: <ac8895ab-68cb-1250-199e-4758e44e740a@gentoo.org>
[-- Attachment #1: Type: text/plain, Size: 8004 bytes --]
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@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
> > > >
> > >
> > > 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.
> 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@gentoo.org>
Thanks
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 --]
next prev parent reply other threads:[~2023-03-08 16:48 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-04 13:29 [PATCH 00/41] rtc: Convert to platform remove callback returning void Uwe Kleine-König
2023-03-04 13:29 ` [PATCH 01/41] rtc: 88pm80x: " Uwe Kleine-König
2023-03-04 13:29 ` [PATCH 02/41] rtc: 88pm860x: " Uwe Kleine-König
2023-03-04 13:29 ` [PATCH 03/41] rtc: ab8500: " Uwe Kleine-König
2023-03-07 13:35 ` Linus Walleij
2023-03-04 13:29 ` [PATCH 04/41] rtc: ac100: " Uwe Kleine-König
2023-03-04 13:29 ` [PATCH 05/41] rtc: asm9260: " Uwe Kleine-König
2023-03-04 13:29 ` [PATCH 06/41] rtc: at91sam9: " Uwe Kleine-König
2023-03-06 11:55 ` Claudiu.Beznea
2023-03-04 13:29 ` [PATCH 07/41] rtc: brcmstb-waketimer: " Uwe Kleine-König
2023-03-04 13:29 ` [PATCH 08/41] rtc: cadence: " Uwe Kleine-König
2023-03-04 13:29 ` [PATCH 09/41] rtc: cmos: " Uwe Kleine-König
2023-03-04 13:29 ` [PATCH 10/41] rtc: cros-ec: " Uwe Kleine-König
2023-03-07 5:30 ` Tzung-Bi Shih
2023-03-04 13:29 ` [PATCH 11/41] rtc: ds1685: " Uwe Kleine-König
2023-03-06 19:43 ` Joshua Kinard
2023-03-06 21:22 ` Uwe Kleine-König
2023-03-07 2:09 ` Joshua Kinard
2023-03-07 8:11 ` Uwe Kleine-König
2023-03-08 16:20 ` Joshua Kinard
2023-03-08 16:47 ` Uwe Kleine-König [this message]
2023-03-13 15:08 ` Joshua Kinard
2023-03-04 13:29 ` [PATCH 12/41] rtc: ftrtc010: " Uwe Kleine-König
2023-03-07 13:35 ` Linus Walleij
2023-03-04 13:30 ` [PATCH 13/41] rtc: hid-sensor-time: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 14/41] rtc: lpc24xx: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 15/41] rtc: max77686: " Uwe Kleine-König
2023-03-05 9:50 ` Krzysztof Kozlowski
2023-03-04 13:30 ` [PATCH 16/41] rtc: mc13xxx: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 17/41] rtc: mpc5121: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 18/41] rtc: mpfs: " Uwe Kleine-König
2023-03-04 13:47 ` Conor Dooley
2023-03-04 13:30 ` [PATCH 19/41] rtc: mt7622: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 20/41] rtc: mxc_v2: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 21/41] rtc: omap: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 22/41] rtc: palmas: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 23/41] rtc: pcf50633: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 24/41] rtc: pic32: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 25/41] rtc: pm8xxx: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 26/41] rtc: rc5t583: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 27/41] rtc: rtd119x: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 28/41] rtc: rzn1: " Uwe Kleine-König
2023-03-06 8:12 ` Miquel Raynal
2023-03-04 13:30 ` [PATCH 29/41] rtc: s3c: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 30/41] rtc: sa1100: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 31/41] rtc: spear: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 32/41] rtc: stm32: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 33/41] rtc: stmp3xxx: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 34/41] rtc: sunplus: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 35/41] rtc: tegra: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 36/41] rtc: tps6586x: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 37/41] rtc: twl: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 38/41] rtc: vt8500: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 39/41] rtc: wm8350: " Uwe Kleine-König
2023-03-06 9:37 ` Charles Keepax
2023-03-04 13:30 ` [PATCH 40/41] rtc: xgene: " Uwe Kleine-König
2023-03-04 13:30 ` [PATCH 41/41] rtc: zynqmp: " Uwe Kleine-König
2023-03-14 9:05 ` [PATCH 00/41] rtc: " Naresh Kamboju
2023-03-17 22:09 ` Alexandre Belloni
2023-05-08 3:45 ` patchwork-bot+chrome-platform
2023-05-08 3:56 ` patchwork-bot+chrome-platform
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230308164750.x4ozcdxymcqoz73m@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=kernel@pengutronix.de \
--cc=kumba@gentoo.org \
--cc=linux-rtc@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).