Re: [PATCH 1/2] coresight: Fix clock refcount imbalance on platform remove

[Jie Gan]

Hi Leo,

On 7/2/2026 8:59 AM, Jie Gan wrote:
> Hi Leo,
>
> On 7/2/2026 12:23 AM, Leo Yan wrote:
> > On Wed, Jul 01, 2026 at 02:05:02PM +0800, Jie Gan wrote:
> >
> > > After probe, pm_runtime_put() allows the device to suspend and the
> > > runtime suspend callback disables the same clocks. During remove the
> > > device is left runtime suspended, so pm_runtime_disable() freezes it
> > > with the clocks already disabled. The devm cleanup that runs afterwards
> > > calls clk_disable_unprepare() a second time, underflowing the clock
> > > enable refcount.
> >
> > Thanks for fixing the issue.
> >
> > The problem is that if the device has already been runtime suspended and
> > its clock has been disabled, afterwards when remove the device, the devm
> > cleanup disables the clock again, resulting in clock count underflow.
> >
> > > diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/ 
> > > drivers/hwtracing/coresight/coresight-funnel.c
> > > index 0abc11f0690c..4c5b94640e6a 100644
> > > --- a/drivers/hwtracing/coresight/coresight-funnel.c
> > > +++ b/drivers/hwtracing/coresight/coresight-funnel.c
> > > @@ -334,6 +334,7 @@ static void funnel_platform_remove(struct 
> > > platform_device *pdev)
> > >           return;
> > >       funnel_remove(&pdev->dev);
> > > +    pm_runtime_get_sync(&pdev->dev);
> > >       pm_runtime_disable(&pdev->dev);
> >
> > Let's use the funnel driver for the discussion. Once we agree on the
> > approach, we can apply the same change to the other CoreSight platform
> > drivers.
> >
> > How about the following teardown?
> >
> >   static void funnel_platform_remove(struct platform_device *pdev)
> >   {
> >          struct funnel_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
> > +       int ret;
> >
> >          if (WARN_ON(!drvdata))
> >                  return;
> >
> > +       ret = pm_runtime_get_sync(&pdev->dev);
> > +       if (ret < 0)
> > +               dev_warn(&pdev->dev, "failed to resume before remove: 
> > %d\n", ret);
> > +
> >          funnel_remove(&pdev->dev);
> > +
> >          pm_runtime_disable(&pdev->dev);
> > +       pm_runtime_set_suspended(&pdev->dev);
> > +       pm_runtime_put_noidle(&pdev->dev);
> >   }
>
> This proposal looks good to me. I forgot to add pm_runtime_put_noidle to 
> drop the useless reference which created by pm_runtime_get_sync.

I dropped the line to check the return value for simplifying.
Here is the final version from my part, can you please help to review again?

 static void funnel_platform_remove(struct platform_device *pdev)
 {
        struct funnel_drvdata *drvdata = dev_get_drvdata(&pdev->dev);

        if (WARN_ON(!drvdata))
                return;

-       funnel_remove(&pdev->dev);
+       /*
+        * Resume the device so its clocks are enabled again, balancing the
+        * clk_disable_unprepare() that devm runs when the driver detaches.
+        * Then mark it suspended and drop the usage count taken here.
+        */
        pm_runtime_get_sync(&pdev->dev);
+       funnel_remove(&pdev->dev);
        pm_runtime_disable(&pdev->dev);
+       pm_runtime_set_suspended(&pdev->dev);
+       pm_runtime_put_noidle(&pdev->dev);
 }

Thanks,
Jie

> Thanks,
> Jie
>
> > The idea is to first resume the device with pm_runtime_get_sync(), then
> > perform the remove (which is safe if they need to access or clean up
> > hardware state), and finally clean up the runtime PM states. I mainly
> > referred to drivers/iio/adc/stm32-adc.c.
> >
> > Thanks,
> > Leo