Re: [PATCH v4 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 1 potential issue(s) to consider:
- [Medium] drm/msm/debugfs: msm_gpu_open() takes runtime PM ref before gpu->lock
--

commit 54d158da1032ffdd109205ad3548c04fcccd04a3
Author: Rob Clark <robin.clark@oss.qualcomm.com>

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

This commit converts msm_gpu_open() in drivers/gpu/drm/msm/msm_debugfs.c to
use cleanup.h helpers for show_priv allocation, runtime PM, and gpu->lock.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index d05307bfa1235..9373ac19ee953 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -70,43 +70,36 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
>  	struct drm_device *dev = inode->i_private;
>  	struct msm_drm_private *priv = dev->dev_private;
>  	struct msm_gpu *gpu = priv->gpu;
> -	struct msm_gpu_show_priv *show_priv;
>  	int ret;
>  
>  	if (!gpu || !gpu->funcs->gpu_state_get)
>  		return -ENODEV;
>  
> -	show_priv = kmalloc_obj(*show_priv);
> +	struct msm_gpu_show_priv *show_priv __free(kfree)
> +		= kmalloc_obj(*show_priv);
>  	if (!show_priv)
>  		return -ENOMEM;
>  
> -	ret = mutex_lock_interruptible(&gpu->lock);
> -	if (ret)
> -		goto free_priv;
> -
> -	pm_runtime_get_sync(&gpu->pdev->dev);
> -	msm_gpu_hw_init(gpu);
> -	show_priv->state = gpu->funcs->gpu_state_get(gpu);
> -	pm_runtime_put_sync(&gpu->pdev->dev);
> -
> -	mutex_unlock(&gpu->lock);
> +	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
> +		scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
> +			msm_gpu_hw_init(gpu);
> +			show_priv->state = gpu->funcs->gpu_state_get(gpu);
> +		}
> +	}

[Severity: Medium]
As reported by Konrad Dybcio on v2 [1], does acquiring the runtime PM
reference before gpu->lock in msm_gpu_open() break GPU hang recovery when
debugfs is opened concurrently with recover_worker()?

Previously, msm_gpu_open() acquired gpu->lock before calling
pm_runtime_get_sync(&gpu->pdev->dev). With scoped_guard(pm_runtime_active,
&gpu->pdev->dev) placed outside scoped_cond_guard(mutex_intr, ...,
&gpu->lock), msm_gpu_open() holds an active runtime PM reference while blocked
waiting for gpu->lock.

During hang recovery, recover_worker() holds gpu->lock and calls
gpu->funcs->recover(gpu). Both a6xx_recover() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c and a8xx_recover() in
drivers/gpu/drm/msm/adreno/a8xx_gpu.c rely on dropping the runtime PM usage
count to zero to synchronously suspend the GPU and collapse the CX GDSC:

	/* Drop the rpm refcount from active submits */
	if (active_submits)
		pm_runtime_put(&gpu->pdev->dev);

	/* And the final one from recover worker */
	pm_runtime_put_sync(&gpu->pdev->dev);

	if (!wait_for_completion_timeout(&gmu->pd_gate, msecs_to_jiffies(1000)))
		DRM_DEV_ERROR(&gpu->pdev->dev, "cx gdsc didn't collapse\n");

If msm_gpu_open() is waiting on gpu->lock with a runtime PM reference held,
pm_runtime_put_sync(&gpu->pdev->dev) will not suspend the device,
wait_for_completion_timeout(&gmu->pd_gate, ...) will time out after 1000 ms,
and the subsequent pm_runtime_get_sync() will not run runtime resume to set
gpu->needs_hw_init = true before msm_gpu_hw_init(gpu) is called.

Since the subsequent IFPC changes in the series place guard(ifpc_disable)(gpu)
inside the gpu->lock critical section after msm_gpu_hw_init(gpu), should
scoped_cond_guard(mutex_intr, return -EINTR, &gpu->lock) remain the outer
guard with scoped_guard(pm_runtime_active, &gpu->pdev->dev) nested inside it?

[1] https://lore.kernel.org/r/a4151c32-bac2-40ec-86f3-59d4dbfaae62@oss.qualcomm.com

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