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

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