mfd: intel-lpss: fix iDMA device leak on probe failure

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

Posted by Guangshuo Li 3 days, 8 hours ago

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.

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@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 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

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

Posted by Mika Westerberg 3 days, 7 hours ago

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@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  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

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

Posted by Guangshuo Li 2 days, 10 hours ago

Hi Mika,

Thanks for the suggestion.

On Mon, 21 Sept 2026 at 19:05, Mika Westerberg
<mika.westerberg@linux.intel.com> 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@vger.kernel.org
> > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> > ---
> >  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

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

Posted by Andy Shevchenko 2 days, 6 hours ago

On Tue, Sep 22, 2026 at 03:59:32PM +0800, Guangshuo Li wrote:
> On Mon, 21 Sept 2026 at 19:05, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Mon, Sep 21, 2026 at 06:40:58PM +0800, Guangshuo Li wrote:

> > 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.

> 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?

Won't work properly. The rule of thumb, it should be no 'goto' after devm_*() call.

> Would this be the preferred change?

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Mika Westerberg 2 days, 6 hours ago

On Tue, Sep 22, 2026 at 03:29:24PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 22, 2026 at 03:59:32PM +0800, Guangshuo Li wrote:
> > On Mon, 21 Sept 2026 at 19:05, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Mon, Sep 21, 2026 at 06:40:58PM +0800, Guangshuo Li wrote:
> 
> > > 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.
> 
> > 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?
> 
> Won't work properly. The rule of thumb, it should be no 'goto' after devm_*() call.

hmm, isn't the unwind done in driver core and has nothing to do with goto?
Maybe things have changed since I last time looked? ;-)

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

Posted by Guangshuo Li 6 hours ago

Thanks Andy and Mika.

On Tue, 22 Sept 2026 at 20:33, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Tue, Sep 22, 2026 at 03:29:24PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 22, 2026 at 03:59:32PM +0800, Guangshuo Li wrote:
> > > On Mon, 21 Sept 2026 at 19:05, Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:
> > > > On Mon, Sep 21, 2026 at 06:40:58PM +0800, Guangshuo Li wrote:
> >
> > > > 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.
> >
> > > 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?
> >
> > Won't work properly. The rule of thumb, it should be no 'goto' after devm_*() call.
>
> hmm, isn't the unwind done in driver core and has nothing to do with goto?
> Maybe things have changed since I last time looked? ;-)

devm_mfd_add_devices() would remove the children when the parent
device's devres is released after probe returns an error. However, the
current error path tears down the debugfs, LTR, clock, and IDA state
before returning, while intel_lpss_remove() removes the MFD children
before those resources.

Given that ordering, would keeping mfd_add_devices() and adding
mfd_remove_devices() at err_remove_ltr, as in the original patch, be
preferable here?

Thanks,
Guangshuo

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

Posted by Mika Westerberg 5 hours ago

On Thu, Sep 24, 2026 at 08:00:29PM +0800, Guangshuo Li wrote:
> Thanks Andy and Mika.
> 
> On Tue, 22 Sept 2026 at 20:33, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Tue, Sep 22, 2026 at 03:29:24PM +0300, Andy Shevchenko wrote:
> > > On Tue, Sep 22, 2026 at 03:59:32PM +0800, Guangshuo Li wrote:
> > > > On Mon, 21 Sept 2026 at 19:05, Mika Westerberg
> > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > On Mon, Sep 21, 2026 at 06:40:58PM +0800, Guangshuo Li wrote:
> > >
> > > > > 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.
> > >
> > > > 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?
> > >
> > > Won't work properly. The rule of thumb, it should be no 'goto' after devm_*() call.
> >
> > hmm, isn't the unwind done in driver core and has nothing to do with goto?
> > Maybe things have changed since I last time looked? ;-)
> 
> devm_mfd_add_devices() would remove the children when the parent
> device's devres is released after probe returns an error. However, the
> current error path tears down the debugfs, LTR, clock, and IDA state
> before returning, while intel_lpss_remove() removes the MFD children
> before those resources.
> 
> Given that ordering, would keeping mfd_add_devices() and adding
> mfd_remove_devices() at err_remove_ltr, as in the original patch, be
> preferable here?

Yes I agree,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>