Hello Joshua,
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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
