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@xxxxxxxxxxxxxx>
> ---
> Changes since (implicit) v1 sent with Message-Id:
> 20230709163335.3458886-1-u.kleine-koenig@xxxxxxxxxxxxxx:
>
> - 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
