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 <[email protected]> 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/[email protected]?part=4
