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

[Jie Gan <jie.gan@oss.qualcomm.com>]

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
>