Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/msm: 3-way deadlock on fault_coredump_done with non-recovery GMU waiters - [High] drm/msm: adreno_fault_handler() blocks on gpu->lock across recover_worker() - [High] drm/msm: deadlock on fault_coredump_done with gpu->lock and devfreq --
commit 10d3d28539be32b5770e7379d7853931850318cb Author: Nico Angelo Dela Cruz <[email protected]> drm/msm: don't wait for a fault devcoredump during GPU recovery This commit adds a recovery_in_progress flag to struct msm_gpu, set by recover_worker() while holding gpu->lock, so that adreno_fault_handler() does not start a devcoredump and GMU wait loops in a6xx_gmu_set_oob() and a6xx_hfi_wait_for_msg_interrupt() do not wait on fault_coredump_done during GPU recovery. Link: https://lore.kernel.org/all/cacu1e7eapr6ne3xxfv4cua2juwnw2kfo1c5wssaun78iwyq...@mail.gmail.com/ > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > index 662f09bec9d52..32ed49abedc28 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > @@ -445,12 +445,15 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum > a6xx_gmu_oob_state state) > if (!ret) > break; > > - if (completion_done(&a6xx_gpu->base.fault_coredump_done)) > + if (completion_done(&a6xx_gpu->base.fault_coredump_done) || > + READ_ONCE(a6xx_gpu->base.base.recovery_in_progress)) > break; > > /* We may timeout because the GMU is temporarily wedged from > * pending faults from the GPU and we are taking a devcoredump. > - * Wait until the MMU is resumed and try again. > + * Wait until the MMU is resumed and try again. A fault dump > + * cannot complete while recovery holds gpu->lock, so don't > + * wait for one then. > */ > wait_for_completion(&a6xx_gpu->base.fault_coredump_done); > } while (true); [Severity: High] Can a non-recovery GMU waiter pass the recovery_in_progress check before recover_worker() acquires gpu->lock, leading to a three-way deadlock? In adreno_fault_handler(), fault_coredump_done is reinitialized before msm_gpu_fault_crashstate_capture() acquires gpu->lock: adreno_fault_handler() reinit_completion(&adreno_gpu->fault_coredump_done); msm_gpu_fault_crashstate_capture(gpu, &fault_info) mutex_lock(&gpu->lock); complete_all(&adreno_gpu->fault_coredump_done); If a fault occurs just before recover_worker() acquires gpu->lock, adreno_fault_handler() sees recovery_in_progress as false, resets fault_coredump_done, and blocks on gpu->lock when recover_worker() wins the lock. Meanwhile, a GMU caller that does not hold gpu->lock, such as msm_devfreq_target() -> a6xx_gpu_set_freq() -> a6xx_gmu_set_freq() (holding df->devfreq->lock, df->lock, and a6xx_gpu->gmu.lock) or a6xx_gmu_sysprof_setup() (holding a runtime PM reference and a6xx_gpu->gmu.lock), can time out in a6xx_gmu_set_oob() or a6xx_hfi_wait_for_msg_interrupt(). If it checks recovery_in_progress before recover_worker() sets it, it enters wait_for_completion() on fault_coredump_done. When recover_worker() sets recovery_in_progress to true, it does not wake existing waiters on fault_coredump_done. It then calls a6xx_recover() or a8xx_recover() -> pm_runtime_put_sync() -> a6xx_gmu_pm_suspend(): a6xx_gmu_pm_suspend() msm_devfreq_suspend(gpu); mutex_lock(&a6xx_gpu->gmu.lock); ret = a6xx_gmu_stop(a6xx_gpu); At that point, recover_worker() holds gpu->lock and blocks on df->lock or a6xx_gpu->gmu.lock, the devfreq or sysprof thread holds those locks and waits on fault_coredump_done, and adreno_fault_handler() waits on gpu->lock before it can call complete_all(&adreno_gpu->fault_coredump_done). > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c > b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c > index 6b34e704a70d0..fe4e3572898ab 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c > @@ -122,12 +122,15 @@ static int a6xx_hfi_wait_for_msg_interrupt(struct > a6xx_gmu *gmu, u32 id, u32 seq > if (!ret) > break; > > - if (completion_done(&a6xx_gpu->base.fault_coredump_done)) > + if (completion_done(&a6xx_gpu->base.fault_coredump_done) || > + READ_ONCE(a6xx_gpu->base.base.recovery_in_progress)) > break; > > /* We may timeout because the GMU is temporarily wedged from > * pending faults from the GPU and we are taking a devcoredump. > - * Wait until the MMU is resumed and try again. > + * Wait until the MMU is resumed and try again. A fault dump > + * cannot complete while recovery holds gpu->lock, so don't > + * wait for one then. > */ > wait_for_completion(&a6xx_gpu->base.fault_coredump_done); > } while (true); [Severity: High] Could this also deadlock when callers other than recover_worker() hold gpu->lock while recovery_in_progress is false? First, msm_job_run() -> msm_gpu_submit() holds gpu->lock with recovery_in_progress set to false: msm_gpu_submit() pm_runtime_get_sync(&gpu->pdev->dev); msm_gpu_hw_init(gpu); ... msm_devfreq_active(gpu); If adreno_fault_handler() calls reinit_completion() on fault_coredump_done and blocks on gpu->lock in msm_gpu_fault_crashstate_capture() while msm_gpu_submit() holds gpu->lock, any GMU OOB or HFI timeout during pm_runtime_get_sync(), msm_gpu_hw_init(), or msm_devfreq_active() (or in a devfreq worker holding df->devfreq->lock while msm_devfreq_active() waits for it) will see recovery_in_progress as false and sleep in wait_for_completion(&a6xx_gpu->base.fault_coredump_done) while gpu->lock is held. Second, msm_gpu_fault_crashstate_capture() itself runs under gpu->lock with recovery_in_progress set to false while fault_coredump_done is still incomplete: msm_gpu_fault_crashstate_capture() mutex_lock(&gpu->lock); ... pm_runtime_get_sync(&gpu->pdev->dev); msm_gpu_crashstate_capture(gpu, submit, fault_info, comm, cmd); pm_runtime_put_sync(&gpu->pdev->dev); ... mutex_unlock(&gpu->lock); If the last submit retired in retire_worker and the autosuspend delay elapsed during msm_gpu_crashstate_capture(), pm_runtime_put_sync() will synchronously run adreno_runtime_suspend() -> a6xx_gmu_pm_suspend(). That path can either block in msm_devfreq_suspend() / mutex_lock(&gmu->lock) on a devfreq waiter sleeping on fault_coredump_done, or time out in a6xx_gmu_notify_slumber() and call wait_for_completion() on fault_coredump_done on the fault handler thread itself. > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index dadfe4c694384..138bebf5ef70d 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -289,7 +289,8 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned > long iova, int flags, > struct msm_mmu *mmu = to_msm_vm(gpu->vm)->mmu; > const char *type = "UNKNOWN"; > bool do_devcoredump = info && (info->fsr & ARM_SMMU_FSR_SS) && > - !READ_ONCE(gpu->crashstate); > + !READ_ONCE(gpu->crashstate) && > + !READ_ONCE(gpu->recovery_in_progress); > unsigned long irq_flags; > > /* [Severity: High] What happens if adreno_fault_handler() evaluates do_devcoredump as true just before recover_worker() acquires gpu->lock and sets recovery_in_progress? In that case, msm_gpu_fault_crashstate_capture() blocks on mutex_lock(&gpu->lock) while recover_worker() runs to completion. Because adreno_fault_handler() is invoked synchronously from arm_smmu_context_fault() before ARM_SMMU_CB_FSR and ARM_SMMU_CB_RESUME are written, the SMMU context bank remains stalled throughout recovery while recover_worker() resets the GPU and replays submits. Once recover_worker() finishes, advances cur_ring->memptrs->fence past the faulting submit, replays the remaining submits, and releases gpu->lock, msm_gpu_fault_crashstate_capture() acquires gpu->lock without rechecking whether recovery occurred: msm_gpu_fault_crashstate_capture() mutex_lock(&gpu->lock); submit = find_submit(cur_ring, cur_ring->memptrs->fence + 1); if (submit && submit->fault_dumped) goto resume_smmu; if (submit) { submit->fault_dumped = true; ... } pm_runtime_get_sync(&gpu->pdev->dev); msm_gpu_crashstate_capture(gpu, submit, fault_info, comm, cmd); Wouldn't find_submit(cur_ring, cur_ring->memptrs->fence + 1) now look up the next replayed submit on cur_ring, mark that innocent submit with fault_dumped = true, and (if gpu->crashstate is NULL) run a6xx_gpu_state_get(), which writes REG_A6XX_CP_SQE_CNTL = 3 and halts SQE on the freshly recovered GPU? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
