Re: [PATCH] mfd: intel-lpss: fix iDMA device leak on probe failure

[Guangshuo Li]

Hi Mika,

Thanks for the suggestion.

On Mon, 21 Sept 2026 at 19:05, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Mon, Sep 21, 2026 at 06:40:58PM +0800, Guangshuo Li wrote:
> > intel_lpss_probe() may register an iDMA MFD child before registering
> > the main controller cell. If the iDMA child is registered successfully
> > but the subsequent mfd_add_devices() call for the main cell fails, the
> > probe error path returns without removing the already registered iDMA
> > device.
> >
> > Since the normal remove callback is not called after a failed probe,
> > the iDMA platform device remains registered and is never released.
> >
> > Call mfd_remove_devices() on the probe error path to remove any MFD
> > children that were successfully registered before the main cell
> > registration failed.
>
> I think this could use devm_mfd_add_devices() instead.
>
> > The issue was identified by a static analysis tool I developed and
> > confirmed by manual review.
> >
> > Fixes: 4b45efe85263 ("mfd: Add support for Intel Sunrisepoint LPSS devices")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> > ---
> >  drivers/mfd/intel-lpss.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> > index 63d6694f7145..657b1e3fb896 100644
> > --- a/drivers/mfd/intel-lpss.c
> > +++ b/drivers/mfd/intel-lpss.c
> > @@ -455,6 +455,7 @@ int intel_lpss_probe(struct device *dev,
> >       return 0;
> >
> >  err_remove_ltr:
> > +     mfd_remove_devices(dev);
> >       intel_lpss_debugfs_remove(lpss);
> >       intel_lpss_ltr_hide(lpss);
> >       intel_lpss_unregister_clock(lpss);
> > --
> > 2.43.0

Do you mean converting both MFD registrations to devm_mfd_add_devices()
and dropping the explicit mfd_remove_devices() from the remove path, like
this?

        if (intel_lpss_has_idma(lpss)) {
-               ret = mfd_add_devices(dev, lpss->devid, &intel_lpss_idma64_cell,
-                                     1, info->mem, info->irq, NULL);
+               ret = devm_mfd_add_devices(dev, lpss->devid,
+                                          &intel_lpss_idma64_cell, 1,
+                                          info->mem, info->irq, NULL);
                if (ret)
                        dev_warn(dev, "Failed to add %s, fallback to PIO\n",
                                 LPSS_IDMA64_DRIVER_NAME);
        }

-       ret = mfd_add_devices(dev, lpss->devid, lpss->cell,
-                             1, info->mem, info->irq, NULL);
+       ret = devm_mfd_add_devices(dev, lpss->devid, lpss->cell,
+                                  1, info->mem, info->irq, NULL);
        if (ret)
                goto err_remove_ltr;

...

 void intel_lpss_remove(struct device *dev)
 {
        struct intel_lpss *lpss = dev_get_drvdata(dev);

-       mfd_remove_devices(dev);
        intel_lpss_debugfs_remove(lpss);
        intel_lpss_ltr_hide(lpss);
        intel_lpss_unregister_clock(lpss);
        ida_free(&intel_lpss_devid_ida, lpss->devid);
 }

Would this be the preferred change?

Thanks,
Guangshuo