Re: [PATCH 4/4] drm/msm: Inhibit IFPC for state snapshot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 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()
- [High] drm/msm: premature GMU_OOB_PERFCOUNTER_SET before hw_init() in msm_gpu_open()
- [Medium] drm/msm/a8xx: hw_init() still checks msm_gpu_sysprof_no_perfcntr_zap()
--

commit 468ea56ed27402560d02d77cd1b25b3b9b6a8ecd
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Inhibit IFPC for state snapshot

This commit adds an ifpc_disable guard in msm_gpu_open() while capturing a
GPU state snapshot via debugfs so that IFPC remains inhibited while GPU
registers are read.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index e1b872576c7b1..14b588aeeb562 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -81,6 +81,8 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
>  		return -ENOMEM;
>  
>  	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
> +		guard(ifpc_disable)(gpu);

[Severity: High]
Could acquiring guard(ifpc_disable)(gpu) here send GMU_OOB_PERFCOUNTER_SET
to the GMU before msm_gpu_hw_init() completes the GMU_OOB_GPU_SET boot
handshake?

When msm_gpu_open() is called while the GPU is runtime-suspended,
scoped_guard(pm_runtime_active, &gpu->pdev->dev) resumes the device via:

  pm_runtime_get_sync()
    -> adreno_runtime_resume()
      -> a6xx_gmu_pm_resume()

which sets gpu->needs_hw_init = true and boots the GMU without performing
the GMU_OOB_GPU_SET handshake (deferred until msm_gpu_hw_init()). Once
adreno_runtime_resume() returns, &gpu->pdev->dev is marked RPM_ACTIVE.

Next, guard(ifpc_disable)(gpu) runs before gpu->lock is acquired and before
msm_gpu_hw_init(gpu) is called:

  msm_gpu_disable_ifpc_get(gpu)
    -> a6xx_gmu_ifpc_update(gpu)

In drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update():

	/* 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);

Because &gpu->pdev->dev is already RPM_ACTIVE and gpu->needs_hw_init is not
checked, a6xx_gmu_ifpc_update() sets GMU_STATUS_OOB_PERF_SET in gmu->status
and sends GMU_OOB_PERFCOUNTER_SET to the GMU before hw_init() performs the
GMU_OOB_GPU_SET (and GMU_OOB_BOOT_SLUMBER) handshake.

As noted in commit d34b6919798c ("drm/msm/a6xx: Correct OOB usage"), sending
another OOB request such as GMU_OOB_PERFCOUNTER_SET during the GMU resume
sequence before GMU_OOB_GPU_SET is set and cleared confuses the internal
power state of the GMU firmware. In addition, because
GMU_STATUS_OOB_PERF_SET is already set in gmu->status prior to hw_init(),
the subsequent a6xx_gmu_ifpc_update() call in hw_init() sees
test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) as already set and
does not re-assert GMU_OOB_PERFCOUNTER_SET after GMU_OOB_GPU_SET is cleared.
And if scoped_cond_guard(mutex_intr, ..., &gpu->lock) fails with -EINTR,
msm_gpu_hw_init(gpu) does not run at all after GMU_OOB_PERFCOUNTER_SET was
already set and cleared on the uninitialized GMU.

[Severity: High]
Is there also a TOCTOU race in a6xx_gmu_ifpc_update() when sampling
gpu->ifpc_disable outside gmu->lock?

Because msm_gpu_open() acquires and releases guard(ifpc_disable)(gpu)
outside gpu->lock, msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put()
can run concurrently across msm_gpu_open(), sample_worker() in
drivers/gpu/drm/msm/msm_perfcntr.c, and msm_context_set_sysprof() in
drivers/gpu/drm/msm/msm_submitqueue.c without a shared lock.

In drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update(), force_on is
evaluated before pm_runtime_get_if_active() and before mutex_lock(&gmu->lock):

void a6xx_gmu_ifpc_update(struct msm_gpu *gpu)
{
	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);

If Thread A calls msm_gpu_disable_ifpc_put(gpu), decrements
gpu->ifpc_disable from 2 to 1, and reads force_on = false before acquiring
gmu->lock, Thread B (such as msm_gpu_open()) can concurrently call
msm_gpu_disable_ifpc_get(gpu), increment gpu->ifpc_disable from 1 to 2, read
force_on = true, acquire gmu->lock first, see GMU_STATUS_OOB_PERF_SET
already set, unlock gmu->lock, and begin reading GX registers in
gpu_state_get(). Thread A then acquires gmu->lock with its stale
force_on == false and clears GMU_STATUS_OOB_PERF_SET and
GMU_OOB_PERFCOUNTER_SET while Thread B holds an active ifpc_disable vote
(or conversely, a stale force_on == true can leave IFPC permanently
disabled after the last voter drops its reference).

Should force_on be evaluated after acquiring gmu->lock?

> +
>  		scoped_cond_guard (mutex_intr, return PTR_ERR(scope.lock), &gpu->lock) {
>  			msm_gpu_hw_init(gpu);

[Severity: High]
Won't calling msm_gpu_hw_init(gpu) when gpu->needs_hw_init is true
self-deadlock on a6xx_gpu->gmu.lock on GMU-based A6xx and A7xx platforms?

During msm_gpu_hw_init(gpu) (whether called from msm_gpu_open(),
adreno_load_gpu(), msm_gpu_submit(), sel_worker(), or a6xx_recover()),
&gpu->pdev->dev is runtime-active (RPM_ACTIVE). On A6xx and A7xx,
msm_gpu_hw_init() calls a6xx_hw_init() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c:

static int a6xx_hw_init(struct msm_gpu *gpu)
{
	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
	int ret;

	mutex_lock(&a6xx_gpu->gmu.lock);
	ret = hw_init(gpu);
	mutex_unlock(&a6xx_gpu->gmu.lock);

At the end of hw_init(gpu) in drivers/gpu/drm/msm/adreno/a6xx_gpu.c,
a6xx_gmu_ifpc_update(gpu) is called unconditionally while
a6xx_gpu->gmu.lock is held (including when an earlier step jumps to the
out: label with a non-zero ret):

out:
	...
	a6xx_gmu_ifpc_update(gpu);

	return ret;
}

Inside drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update(),
pm_runtime_get_if_active(&gpu->pdev->dev) returns 1 because the device is
RPM_ACTIVE, and mutex_lock(&gmu->lock) attempts to acquire the same
a6xx_gpu->gmu.lock already held by a6xx_hw_init() while gpu->lock is held
and gpu->irq is disabled.

[Severity: Medium]
On A8xx GPUs, was drivers/gpu/drm/msm/adreno/a8xx_gpu.c:hw_init() missed
when converting IFPC inhibition from msm_gpu_sysprof_no_ifpc() to
gpu->ifpc_disable?

In drivers/gpu/drm/msm/adreno/a6xx_gpu.c, a8xx_gpu_funcs sets:

		.progress = a8xx_progress,
		.ifpc_update = a6xx_gmu_ifpc_update,
		.perfcntr_configure = a6xx_perfcntr_configure,

which manages GMU_OOB_PERFCOUNTER_SET and GMU_STATUS_OOB_PERF_SET based on
refcount_read(&gpu->ifpc_disable) > 1. However, in
drivers/gpu/drm/msm/adreno/a8xx_gpu.c:hw_init(), the code still checks
msm_gpu_sysprof_no_perfcntr_zap(gpu) instead of gpu->ifpc_disable:

	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);
	}

Doesn't this cause three mismatches on A8xx when msm_gpu_hw_init() runs:
first, if a global perfcntr stream is active (gpu->perfcntrs->stream != NULL)
while gpu->ifpc_disable == 1, hw_init() sets GMU_OOB_PERFCOUNTER_SET and
GMU_STATUS_OOB_PERF_SET even though no continuous IFPC disable vote is held;
second, if gpu->ifpc_disable > 1 (such as from guard(ifpc_disable)(gpu) in
msm_gpu_open()) while neither sysprof_active > 1 nor perfcntrs->stream is
set, hw_init() will not assert GMU_OOB_PERFCOUNTER_SET after clearing
GMU_OOB_GPU_SET; and third, if GMU_STATUS_OOB_PERF_SET was already set by
a6xx_gmu_ifpc_update() before a8xx_hw_init(), hw_init() calls
a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) a second time without
checking GMU_STATUS_OOB_PERF_SET?

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