Re: [PATCH] platform/x86: simatic-ipc: fix platform device leak on registration failure

[Markus Elfring <Markus.Elfring@web.de>]

>> …
>>> +++ b/drivers/platform/x86/siemens/simatic-ipc.c
>> …
>>> @@ -118,8 +119,11 @@ static int register_platform_devices(u32 station_id)
>>>                       platform_device_register_data(NULL, pdevname,
>>>                               PLATFORM_DEVID_NONE, &platform_data,
>>>                               sizeof(struct simatic_ipc_platform));
>>> -             if (IS_ERR(ipc_batt_platform_device))
>>> -                     return PTR_ERR(ipc_batt_platform_device);
>>> +             if (IS_ERR(ipc_batt_platform_device)) {
>>> +                     ret = PTR_ERR(ipc_batt_platform_device);
>>> +                     ipc_batt_platform_device = NULL;
>>> +                     goto err_unregister;
>>> +             }
>> …
>>
>> How do you think about to avoid duplicate checks and variable resets
>> in such a function implementation?
…
> Would it make sense to share the cleanup path and rely on
> platform_device_unregister() handling NULL/ERR_PTR, like this?

It probably depends on corresponding case distinctions.


> +static void unregister_platform_devices(void)
> +{
> +        platform_device_unregister(ipc_wdt_platform_device);
> +        platform_device_unregister(ipc_led_platform_device);
> +        platform_device_unregister(ipc_batt_platform_device);
> +}

* Would you like to avoid the passing of any error pointers here?

* How do you think about the relevance for variable resets?


> +
>  static int register_platform_devices(u32 station_id)
>  {
…
>         return 0;
> +
> +err_unregister:
> +       unregister_platform_devices();
> +       return ret;
>  }…

Regards,
Markus