Re: [PATCH] ASoC: Intel: SST: fix platform device leak on probe failure

[Guangshuo Li]

Hi Czarek,

Thanks for the clarification.

On Wed, 23 Sept 2026 at 16:15, Cezary Rojewski
<cezary.rojewski@xxxxxxxxx> wrote:
>
> On 9/22/2026 10:17 AM, Guangshuo Li wrote:
> > Hi Czarek,
> >
> > Thanks for the review.
>
> >>> @@ -361,15 +362,21 @@ static int sst_acpi_probe(struct platform_device *pdev)
> >>>
> >>>       ret = sst_platform_get_resources(ctx);
> >>>       if (ret)
> >>> -             return ret;
> >>> +             goto err_unregister_mdev;
> >>>
> >>>       ret = sst_context_init(ctx);
> >>>       if (ret < 0)
> >>> -             return ret;
> >>> +             goto err_unregister_mdev;
> >>>
> >>>       sst_configure_runtime_pm(ctx);
> >>>       platform_set_drvdata(pdev, ctx);
> >>>       return ret;
> >>> +
> >>> +err_unregister_mdev:
> >>> +     platform_device_unregister(mdev);
> >>> +err_unregister_plat_dev:
> >>> +     platform_device_unregister(plat_dev);
> >>> +     return ret;
> >>>  }
> >>
> >> Looks like sst_acpi_remove() does not unregister the devices either.
> >> That could be fixed by enlisting devm_add_action_or_reset() in
> >> sst_acpi_probe() without altering sst_acpi_remove() at all.
> >>
> >> Would you mind sending a separate patch updating the function so both
> >> the error path and the driver-unload clean up the device objects?
> >>
> >>
> >> Kind regards,
> >> Czarek
> >
> > I'll update the title to:
> >
> > ASoC: Intel: atom: Fix platform device leak on probe failure
> >
> > and trim the commit message as suggested, including the
> > s/machine drivers/machine board/ change. I'll also add your Reviewed-by.
> >
> > For the separate cleanup patch, do you mean something like this?
> >
> > +static void sst_unregister_platform_device(void *data)
> > +{
> > +        platform_device_unregister(data);
> > +}
> > +
> >         plat_dev = platform_device_register_data(...);
> >         if (IS_ERR(plat_dev))
> >                 return PTR_ERR(plat_dev);
> > +
> > +       ret = devm_add_action_or_reset(dev, sst_unregister_platform_device,
> > +                                      plat_dev);
> > +       if (ret)
> > +               return ret;
> >
> >         ...
> >
> >         mdev = platform_device_register_data(...);
> >         if (IS_ERR(mdev))
> >                 return PTR_ERR(mdev);
> > +
> > +       ret = devm_add_action_or_reset(dev, sst_unregister_platform_device,
> > +                                      mdev);
> > +       if (ret)
> > +               return ret;
> >
> > This would let devres clean up both devices on later probe failure and on
> > driver unload, so the explicit unregister error paths from the first patch
> > would no longer be needed after this patch.
> >
> > Would this be what you had in mind? If so, I'll follow Krzysztof's
> > suggestion, organize the related changes into a proper patch series,
> > and send a two-patch v2.
>
> I do not mind either approach. There are two problems here:
>         1) leak on probe() failure
>         2) leak on driver remove()
>
> So, two patches-approach IMHO is perfectly fine. And yes, enlisting
> devm_add_action_or_reset() closes both 1) and 2) in one go.
>
> Kind regards,
> Czarek

I'll use devm_add_action_or_reset() to handle both the probe failure and
driver removal cases, and send the updated change in v2.

Thanks,
Guangshuo