Re: [PATCH 2/4] drm/msm: Add cleanup help for disabling IFPC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: self-deadlock on gmu.lock in a6xx_hw_init() via a6xx_gmu_ifpc_update()
- [High] drm/msm: TOCTOU race on gpu->ifpc_disable in a6xx_gmu_ifpc_update()
- [Medium] drm/msm/a8xx: hw_init() checks msm_gpu_sysprof_no_perfcntr_zap() for IFPC OOB
--

commit 755429cf0af1108369a0a2ba9d370fe222b14717
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Add cleanup help for disabling IFPC

This commit adds a DEFINE_GUARD(ifpc_disable, ...) helper for
msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put(), and converts
sample_worker() in msm_perfcntr.c to use guard(ifpc_disable)(gpu).

> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 0334a0f604ac1..e7912d2180a51 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -399,6 +399,10 @@ msm_gpu_disable_ifpc_put(struct msm_gpu *gpu)
>  		gpu->funcs->ifpc_update(gpu);
>  }
>  
> +DEFINE_GUARD(ifpc_disable, struct msm_gpu *,
> +	     msm_gpu_disable_ifpc_get(_T),
> +	     msm_gpu_disable_ifpc_put(_T))

[Severity: High]
With the IFPC disable rework in this series, does calling
a6xx_gmu_ifpc_update() from hw_init() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c cause a self-deadlock on
a6xx_gpu->gmu.lock during GPU initialization and runtime resume?

In drivers/gpu/drm/msm/adreno/a6xx_gpu.c, a6xx_hw_init() acquires
a6xx_gpu->gmu.lock before calling hw_init():

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_hw_init() {
	mutex_lock(&a6xx_gpu->gmu.lock);
	ret = hw_init(gpu);
	mutex_unlock(&a6xx_gpu->gmu.lock);
}

At the end of hw_init() (under the out: label), for all non-gmu_wrapper A6xx
and A7xx GPUs, it now calls a6xx_gmu_ifpc_update(gpu):

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() {
    ...
out:
	if (adreno_has_gmu_wrapper(adreno_gpu))
		return ret;
    ...
	a6xx_gmu_ifpc_update(gpu);

	return ret;
}

When msm_gpu_hw_init() is invoked by callers such as adreno_load_gpu(),
msm_gpu_submit(), a6xx_recover(), sel_worker(), or msm_gpu_open(), runtime PM
is active on &gpu->pdev->dev, so pm_runtime_get_if_active() succeeds in
a6xx_gmu_ifpc_update() and the thread attempts to acquire gmu->lock a second
time:

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

	mutex_lock(&gmu->lock);
    ...
}

Also, because out: is reached when adreno_hw_init() or
a7xx_cp_init()/a6xx_cp_init() fails with ret != 0, should
a6xx_gmu_ifpc_update() be skipped on error paths and propagate any failure
from a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET)?

[Severity: Medium]
Similarly, in drivers/gpu/drm/msm/adreno/a8xx_gpu.c, hw_init() still checks
msm_gpu_sysprof_no_perfcntr_zap(gpu) rather than gpu->ifpc_disable when
programming GMU_OOB_PERFCOUNTER_SET:

drivers/gpu/drm/msm/adreno/a8xx_gpu.c:hw_init() {
    ...
	if (!ret && msm_gpu_sysprof_no_perfcntr_zap(gpu)) {
		ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
		if (!ret)
			set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status);
	}
}

Since a8xx_gpu_funcs in drivers/gpu/drm/msm/adreno/a6xx_gpu.c sets
.ifpc_update = a6xx_gmu_ifpc_update, which manages GMU_OOB_PERFCOUNTER_SET
and GMU_STATUS_OOB_PERF_SET based on refcount_read(&gpu->ifpc_disable) > 1,
won't A8xx hw_init() leave GMU_OOB_PERFCOUNTER_SET out of sync with
gpu->ifpc_disable whenever a perfcntr stream is open (with ifpc_disable == 1)
or when another voter holds an ifpc_disable reference without sysprof or a
perfcntr stream active?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index bf73752dac3de..0d33f0eae35d7 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -259,7 +259,7 @@ sample_worker(struct kthread_work *work)
>  	}
>  
>  	/* Inhibit IFPC while accessing registers: */
> -	msm_gpu_disable_ifpc_get(gpu);
> +	guard(ifpc_disable)(gpu);

[Severity: High]
Can concurrent callers of msm_gpu_disable_ifpc_get() and
msm_gpu_disable_ifpc_put() race in a6xx_gmu_ifpc_update() and clear
GMU_OOB_PERFCOUNTER_SET while sample_worker() is inside this
guard(ifpc_disable)(gpu) critical section?

In drivers/gpu/drm/msm/msm_gpu.h, msm_gpu_disable_ifpc_get() and
msm_gpu_disable_ifpc_put() update gpu->ifpc_disable without holding
gmu->lock:

drivers/gpu/drm/msm/msm_gpu.h:msm_gpu_disable_ifpc_get() {
	refcount_inc(&gpu->ifpc_disable);
	if (gpu->funcs->ifpc_update)
		gpu->funcs->ifpc_update(gpu);
}

And in drivers/gpu/drm/msm/adreno/a6xx_gmu.c, a6xx_gmu_ifpc_update() samples
gpu->ifpc_disable into force_on before acquiring gmu->lock:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update() {
	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
	bool force_on = refcount_read(&gpu->ifpc_disable) > 1;

	/* Nothing to do if GPU is suspended. We will handle this during GMU resume */
	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
		return;

	mutex_lock(&gmu->lock);

	if (force_on && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
		a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	else if (!force_on && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);

	mutex_unlock(&gmu->lock);
    ...
}

If Thread A (for example in msm_context_set_sysprof()) calls
msm_gpu_disable_ifpc_put(gpu), decrements gpu->ifpc_disable from 2 to 1, and
reads force_on = false before taking gmu->lock, Thread B in sample_worker()
can enter guard(ifpc_disable)(gpu), increment gpu->ifpc_disable from 1 to 2,
read force_on = true, and acquire gmu->lock first.

Thread B sees GMU_STATUS_OOB_PERF_SET already set, unlocks gmu->lock, and
proceeds to flush and read GX performance counter registers in
sample_worker(). Thread A then acquires gmu->lock with its stale
force_on = false value and calls a6xx_gmu_clear_oob(gmu,
GMU_OOB_PERFCOUNTER_SET) while gpu->ifpc_disable is 2 and sample_worker() is
actively reading GPU registers. The reverse ordering can likewise leave
GMU_OOB_PERFCOUNTER_SET stuck set after all voters drop their references.

Should force_on be evaluated after acquiring gmu->lock?

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