Re: [PATCH] platform: Provide a remove callback that returns no value

[Greg Kroah-Hartman]

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@xxxxxxxxxxxxxx>
> ---
>  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