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 > > > > > > --- > > > > > > 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 Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |