Re: [PATCH 7.1.y] drm/amd/display: Fix ISM dc_lock deadlock during suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The `idle_workqueue` is freed without correctly cancelling pending work, leading to a Use-After-Free and workqueue corruption.
- [High] Missing mutex_unlock(&dm->dc_lock) on error paths in the GPU reset path of dm_suspend, leading to a permanent deadlock.
--

commit 147ffd2a09d9a0380b42af11648a4a34c2fd7170
Author: Ray Wu <ray.wu@amd.com>
drm/amd/display: Fix ISM dc_lock deadlock during suspend

This patch fixes an ABBA deadlock during suspend and resume by splitting
the ISM disable path. Workers are first quiesced without dc_lock, and then
the FSM is driven back to FULL_POWER_RUNNING under dc_lock.

Link: https://github.com/CachyOS/linux-cachyos/issues/900

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index f8c13bad4ac23..560ab32989114 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2260,9 +2260,16 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev)
>  		adev->dm.idle_workqueue = NULL;
>  	}
>  

[Severity: Critical]
This is a pre-existing issue, but is it possible for the idle_workqueue
to be freed while work is still pending?

In amdgpu_dm_fini(), flush_work() is only called if idle_workqueue->running 
is true:

if (adev->dm.idle_workqueue->running) {
	adev->dm.idle_workqueue->enable = false;
	flush_work(&adev->dm.idle_workqueue->work);
}
kfree(adev->dm.idle_workqueue);

If schedule_work() was called but the worker hasn't started executing yet,
running remains false.

This means the cleanup logic will skip flush_work() and immediately kfree()
the structure. When the workqueue later processes the queued item, it will
access freed memory, causing a use-after-free.

[ ... ]

> @@ -3290,9 +3297,14 @@ static int dm_suspend(struct amdgpu_ip_block *ip_block)
>  	if (amdgpu_in_reset(adev)) {
>  		enum dc_status res;
>  
> +		/* Quiesce ISM workers before taking dc_lock (workers take
> +		 * dc_lock themselves; syncing under it would deadlock).
> +		 */
> +		amdgpu_dm_ism_disable(dm);
> +
>  		mutex_lock(&dm->dc_lock);
>  
> -		amdgpu_dm_ism_disable(dm);
> +		amdgpu_dm_ism_force_full_power(dm);
>  		dc_allow_idle_optimizations(adev->dm.dc, false);
>  
>  		dm->cached_dc_state = dc_state_create_copy(dm->dc->current_state);

[Severity: High]
This isn't a bug introduced by this patch, but does the error path for 
amdgpu_dm_commit_zero_streams() leave dm->dc_lock permanently locked?

Just below this hunk in dm_suspend(), the function handles a failure
by returning immediately:

res = amdgpu_dm_commit_zero_streams(dm->dc);
if (res != DC_OK) {
	drm_err(adev_to_drm(adev), "Failed to commit zero streams: %d\n", res);
	return -EINVAL;
}

Since mutex_lock(&dm->dc_lock) was acquired right before this, the early 
return skips unlocking the mutex.

This leaves the dc_lock permanently acquired, leading to a system deadlock
on subsequent accesses to the display manager.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628140148.59923-1-admin@ptr1337.dev?part=1