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

[Guangshuo Li]

Hi Markus,

Thanks for the suggestion.

On Mon, 21 Sept 2026 at 18:10, Markus Elfring <Markus.Elfring@xxxxxx> wrote:
>
> …
> > +++ 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?
>
> Regards,
> Markus

Would it make sense to share the cleanup path and rely on
platform_device_unregister() handling NULL/ERR_PTR, like this?

+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);
+}
+
 static int register_platform_devices(u32 station_id)
 {
        u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
        u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
        u8 battmode = SIMATIC_IPC_DEVICE_NONE;
        char *pdevname;
+       int ret;
        int i;

        ...

-       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);
+               goto err_unregister;
+       }

        ...

-       if (IS_ERR(ipc_led_platform_device))
-               return PTR_ERR(ipc_led_platform_device);
+       if (IS_ERR(ipc_led_platform_device)) {
+               ret = PTR_ERR(ipc_led_platform_device);
+               goto err_unregister;
+       }

        ...

-       if (IS_ERR(ipc_wdt_platform_device))
-               return PTR_ERR(ipc_wdt_platform_device);
+       if (IS_ERR(ipc_wdt_platform_device)) {
+               ret = PTR_ERR(ipc_wdt_platform_device);
+               goto err_unregister;
+       }

        ...

        return 0;
+
+err_unregister:
+       unregister_platform_devices();
+       return ret;
 }

 ...

 static void __exit simatic_ipc_exit_module(void)
 {
-       platform_device_unregister(ipc_led_platform_device);
-       ipc_led_platform_device = NULL;
-
-       platform_device_unregister(ipc_wdt_platform_device);
-       ipc_wdt_platform_device = NULL;
-       platform_device_unregister(ipc_batt_platform_device);
-       ipc_batt_platform_device = NULL;
+       unregister_platform_devices();
 }

Would this be closer to what you had in mind?

If so, I'll follow Krzysztof's suggestion and organize the related
patches into a proper patch series before sending v2.

Thanks,
Guangshuo