On Thu, 2023-09-14 at 22:02 +0200, Uwe Kleine-König wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 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);
> +break;
How about changing as below:
defualt:
break;
> }
>
> ssusb_rscs_exit(ssusb);
> @@ -478,8 +487,6 @@ static int mtu3_remove(struct platform_device
> *pdev)
> pm_runtime_disable(&pdev->dev);
> pm_runtime_put_noidle(&pdev->dev);
> pm_runtime_set_suspended(&pdev->dev);
> -
> -return 0;
> }
>
> static int resume_ip_and_ports(struct ssusb_mtk *ssusb, pm_message_t
> msg)
> @@ -615,7 +622,7 @@ MODULE_DEVICE_TABLE(of, mtu3_of_match);
>
> static struct platform_driver mtu3_driver = {
> .probe = mtu3_probe,
> -.remove = mtu3_remove,
> +.remove_new = mtu3_remove,
> .driver = {
> .name = MTU3_DRIVER_NAME,
> .pm = DEV_PM_OPS,
>
> base-commit: 98897dc735cf6635f0966f76eb0108354168fb15
> --
> 2.40.1
>
