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

["Chunfeng Yun (云春峰)" <Chunfeng.Yun@mediatek.com>]

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@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);
> +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
>