Messages in this thread | | | Date | Fri, 9 Dec 2022 16:21:30 +0100 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH] platform: Provide a remove callback that returns no value |
| |
On Fri, Dec 09, 2022 at 04:09:14PM +0100, Uwe Kleine-König wrote: > struct platform_driver::remove returning an integer made driver authors > expect that returning an error code was proper error handling. However > the driver core ignores the error and continues to remove the device > because there is nothing the core could do anyhow and reentering the > remove callback again is only calling for trouble. > > So this is an source for errors typically yielding resource leaks in the > error path. > > As there are too many platform drivers to neatly convert them all to > return void in a single go, do it in several steps after this patch: > > a) Convert all drivers to implement .remove_new() returning void instead > of .remove() returning int; > b) Change struct platform_driver::remove() to return void and so make > it identical to .remove_new(); > c) Change all drivers back to .remove() now with the better prototype;
Change c) seems like it will be just as much work as a), right?
> d) drop struct platform_driver::remove_new().
> > While this touches all drivers eventually twice, steps a) and c) can be > done one driver after another and so reduces coordination efforts > immensely and simplifies review. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/base/platform.c | 4 +++- > include/linux/platform_device.h | 11 +++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 968f3d71eeab..a4938d1c8fe1 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -1416,7 +1416,9 @@ 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) { > + if (drv->remove_new) { > + drv->remove_new(dev); > + } else if (drv->remove) { > int ret = drv->remove(dev); > > if (ret) > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index b0d5a253156e..b845fd83f429 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -207,7 +207,18 @@ extern void platform_device_put(struct platform_device *pdev); > > struct platform_driver { > int (*probe)(struct platform_device *); > + > + /* > + * Traditionally the remove callback returned an int which however is > + * 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. > + */ > int (*remove)(struct platform_device *); > + void (*remove_new)(struct platform_device *); > +
Who is going to do the work of the conversion to this new prototype? I'll be glad to take this, but I don't want to see a half-finished conversion happen and us stuck with a "new" and "old" call, as that would just be a mess.
thanks,
greg k-h
| |