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