Re: [PATCH v2 2/3] drm/msm/a5xx: Hold a CPU latency request while the GPU is powered

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/a5xx: premature pm_qos_req removal in a5xx_destroy() before cleanup
--

commit fc8618fd147137559c55740f177ada72c5eca286
Author: Fabricio Akio <fabricioakio@gmail.com>

drm/msm/a5xx: Hold a CPU latency request while the GPU is powered

This patch holds a CPU latency QoS request while A508, A509, and A512 GPUs
are powered on so the SoC does not enter system-level power collapse while
GPU work is active. The request is added on GPU resume and removed on GPU
suspend or destruction.

> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 0aff8252a00f9..1d4993f8cb7c4 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1060,6 +1060,10 @@ static void a5xx_destroy(struct msm_gpu *gpu)
>  		drm_gem_object_put(a5xx_gpu->shadow_bo);
>  	}
>  
> +	/* Still held if suspending the GPU failed before it was destroyed */
> +	if (cpu_latency_qos_request_active(&a5xx_gpu->pm_qos_req))
> +		cpu_latency_qos_remove_request(&a5xx_gpu->pm_qos_req);
> +
>  	adreno_gpu_cleanup(adreno_gpu);
>  	kfree(a5xx_gpu);
>  }

[Severity: High]
Should the removal of a5xx_gpu->pm_qos_req in a5xx_destroy() happen after
adreno_gpu_cleanup(adreno_gpu) rather than before it?

In adreno_unbind(), if adreno_system_suspend() fails (for example, timing
out with -EBUSY because a GPU submission is hung or still active, or if
pm_runtime_force_suspend() fails), adreno_system_suspend() restarts the
scheduler and returns with runtime PM still enabled:

drivers/gpu/drm/msm/adreno/adreno_device.c:adreno_system_suspend() {
	remaining = wait_event_timeout(gpu->retire_event,
				       gpu->active_submits == 0,
				       msecs_to_jiffies(1000));
	if (remaining == 0) {
		dev_err(dev, "Timeout waiting for GPU to suspend\n");
		ret = -EBUSY;
		goto out;
	}

	ret = pm_runtime_force_suspend(dev);
out:
	if (ret)
		resume_scheduler(gpu);
}

adreno_unbind() then proceeds directly into gpu->funcs->destroy(gpu) ->
a5xx_destroy() while runtime PM and gpu->worker are still running.

Runtime PM is only disabled and gpu->worker is only stopped inside
adreno_gpu_cleanup() -> msm_gpu_cleanup():

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_gpu_cleanup() {
	...
	pm_runtime_dont_use_autosuspend(&gpu->pdev->dev);

	if (priv && pm_runtime_enabled(&priv->gpu_pdev->dev))
		pm_runtime_disable(&priv->gpu_pdev->dev);

	msm_gpu_cleanup(&adreno_gpu->base);
}

Meanwhile, recover_worker on gpu->worker runs a5xx_recover() ->
adreno_recover(), which directly calls gpu->funcs->pm_suspend(gpu) and
gpu->funcs->pm_resume(gpu).

If cpu_latency_qos_remove_request(&a5xx_gpu->pm_qos_req) runs before
adreno_gpu_cleanup(adreno_gpu), can recover_worker or runtime PM execute
a5xx_pm_suspend() or a5xx_pm_resume() concurrently with or after the
removal, re-adding &a5xx_gpu->pm_qos_req to cpu_latency_constraints right
before kfree(a5xx_gpu) frees it?

[ ... ]

>
> base-commit: 2efb7a11f1aa4d2dc285fe2680cca22cb9f72f64

Boris
在进行深入审查时，我发现了一个潜在的释放后使用（Use-After-Free）问题，已在上面的内联评论中详细说明。

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-a5xx-gpu-pm-v2-0-457aafbb28da@gmail.com?part=2