Re: [PATCH v5 5/6] drm/msm/debugfs: Use cleanup.h for gpu state dump

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm: runtime PM ref leak on early return in sel_worker()
- [Low] drm/msm/debugfs: stray revision note after Signed-off-by in commit message
--

commit 0021c5b2e7827c592eea3552f7dbac0b157fff35
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/debugfs: Use cleanup.h for gpu state dump

This series reworks IFPC and performance counter handling around GPU hardware
initialization and runtime PM, and updates msm_gpu_open() in debugfs to use
scope-based cleanup helpers.

>     drm/msm/debugfs: Use cleanup.h for gpu state dump
>     
>     Use scoped_guard/etc to simplify the error paths.
>     
>     Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>
>     Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
>     
>     Reword, we are no longer reordering locking/runpm

[Severity: Low]
This isn't a bug, but it looks like a patch revision note was left after the
Signed-off-by line in the commit message. Should that trailing line be
removed?

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index 7fa2e858bd082..e6f4f9b098842 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -199,10 +199,17 @@ sel_worker(struct work_struct *w)
>  		return;
>  
>  	scoped_guard (mutex, &gpu->lock) {
> +		/*
> +		 * If gpu still needs hw_init, bail.  We'll be rescheduled
> +		 * after hw_init, and there is no point to collect counters
> +		 * beforehand
> +		 */
> +		if (gpu->needs_hw_init)
> +			return;

[Severity: High]
Does this early return in sel_worker() leak the runtime PM usage reference on
&gpu->pdev->dev?

Earlier in sel_worker(), a runtime PM reference is acquired manually before
entering the scoped_guard(mutex, &gpu->lock) block, and released after the
block finishes:

drivers/gpu/drm/msm/msm_perfcntr.c:sel_worker() {
	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
		return;

	scoped_guard (mutex, &gpu->lock) {
		if (gpu->needs_hw_init)
			return;
		...
	}

	pm_runtime_put_autosuspend(&gpu->pdev->dev);
}

When gpu->needs_hw_init is true while the device is runtime-active (for
example, if msm_ioctl_perfcntr_config() queues sel_work after
adreno_runtime_resume() sets gpu->needs_hw_init = true and before
msm_gpu_hw_init() clears it), returning from inside the scoped_guard block
unlocks gpu->lock via cleanup but skips pm_runtime_put_autosuspend() at the
end of sel_worker(), preventing the GPU from runtime-suspending again.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104175931.28475-1-robin.clark@oss.qualcomm.com?part=5