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