Re: [PATCH 1/4] drm/msm: don't wait for a fault devcoredump during GPU recovery

[sashiko-bot@kernel.org]

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 <nicoangelodelacruz@gmail.com>

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/CACu1E7EApr6Ne3XxFv4cUa2JuwNW2kfo1c5wSsAUN78iWyQ2sg@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/20261101175553.53746-1-nicoangelodelacruz@gmail.com?part=1