Re: [PATCH v2] usb: mtu3: Convert to platform remove callback returning void

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Sep 14, 2023 at 10:02:51PM +0200, 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.
> 
> The function mtu3_remove() can only return a non-zero value if
> ssusb->dr_mode is neiter USB_DR_MODE_PERIPHERAL nor USB_DR_MODE_HOST nor
> USB_DR_MODE_OTG. In this case however the probe callback doesn't succeed
> and so the remove callback isn't called at all. So the code branch
> resulting in this error path could just be dropped were it not for the
> compiler choking on "enumeration value 'USB_DR_MODE_UNKNOWN' not handled
> in switch [-Werror=switch]". So instead replace this code path by a
> WARN_ON and then mtu3_remove() be converted to return void trivially.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Changes since (implicit) v1 sent with Message-Id:
> 20230709163335.3458886-1-u.kleine-koenig@pengutronix.de:
> 
>  - Keep case USB_DR_MODE_UNKNOWN to cope for the compiler being called
>    with -Werror=switch.
>  - Rebase to a newer tree
> 
> Just to evaluate the options, I tried with a BUG_ON(ssusb->dr_mode ==
> USB_DR_MODE_UNKNOWN) before the switch, but even then gcc insists on the
> case label for this value.
> 
> Best regards
> Uwe
> 
>  drivers/usb/mtu3/mtu3_plat.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/mtu3/mtu3_plat.c b/drivers/usb/mtu3/mtu3_plat.c
> index 6f264b129243..18c6cf9a2d71 100644
> --- a/drivers/usb/mtu3/mtu3_plat.c
> +++ b/drivers/usb/mtu3/mtu3_plat.c
> @@ -451,7 +451,7 @@ static int mtu3_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int mtu3_remove(struct platform_device *pdev)
> +static void mtu3_remove(struct platform_device *pdev)
>  {
>  	struct ssusb_mtk *ssusb = platform_get_drvdata(pdev);
>  
> @@ -469,8 +469,17 @@ static int mtu3_remove(struct platform_device *pdev)
>  		ssusb_gadget_exit(ssusb);
>  		ssusb_host_exit(ssusb);
>  		break;
> -	default:
> -		return -EINVAL;
> +	case USB_DR_MODE_UNKNOWN:
> +		/*
> +		 * This cannot happen because with dr_mode ==
> +		 * USB_DR_MODE_UNKNOWN, .probe() doesn't succeed and so
> +		 * .remove() wouldn't be called at all. However (little
> +		 * surprising) the compiler isn't smart enough to see that, so
> +		 * we explicitly have this case item to not make the compiler
> +		 * wail about an unhandled enumeration value.
> +		 */
> +		WARN_ON(1);

Please don't add new WARN_ON() calls to the kernel, print out a big
error message and return, don't reboot the machine.

thanks,

greg k-h