Re: [PATCH] drm/amd/display: fix dc_lock leak on GPU reset error paths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing error handling for `dc_state_create_copy()` allocation failure leads to a NULL pointer dereference in `dm_resume()`.
--

--- Patch [1]: [PATCH] drm/amd/display: fix dc_lock leak on GPU reset error paths ---
commit 052121f5a12c1a8a145a6c7fd55be37159a21de9
Author: Linkai Gong <gonglinkai@kylinos.cn>

drm/amd/display: fix dc_lock leak on GPU reset error paths

On GPU reset, dm_suspend() takes dc_lock and leaves it for dm_resume()
to drop. This commit releases the cached DC state and unlocks the lock
before returning an error if amdgpu_dm_commit_zero_streams() or
dm_dmub_hw_init() fails.

Cc: stable@vger.kernel.org

> 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 567bf04979ee5..2fe934036e363 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1589,6 +1589,9 @@ static int dm_suspend(struct amdgpu_ip_block *ip_block)
>  		dm->cached_dc_state = dc_state_create_copy(dm->dc->current_state);
>  
>  		if (dm->cached_dc_state)
>  			dm_gpureset_toggle_interrupts(adev, dm->cached_dc_state, false);
>  
>  		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);
> +			dc_state_release(dm->cached_dc_state);
> +			dm->cached_dc_state = NULL;
> +			mutex_unlock(&dm->dc_lock);
>  			return -EINVAL;
>  		}

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the error handling
when dc_state_create_copy() fails here in dm_suspend().

If memory allocation fails, dm->cached_dc_state becomes NULL. The code
correctly skips dm_gpureset_toggle_interrupts() but continues execution
and ultimately returns success (0) without aborting the suspend operation.

Since dm_suspend() returns success, dm_resume() will be called later on
the same GPU reset path.

[ ... ]

> @@ -1884,6 +1887,9 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)
>  	if (amdgpu_in_reset(adev)) {
>  		dc_state = dm->cached_dc_state;
[ ... ]
>  		link_enc_cfg_copy(adev->dm.dc->current_state, dc_state);
>  
>  		r = dm_dmub_hw_init(adev);
>  		if (r) {
>  			drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
> +			dc_state_release(dm->cached_dc_state);
> +			dm->cached_dc_state = NULL;
> +			mutex_unlock(&dm->dc_lock);
>  			return r;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does this lead to a NULL
pointer dereference when calling link_enc_cfg_copy()?

In dm_resume(), dc_state is initialized to dm->cached_dc_state, which would
be NULL if the earlier allocation failed. It is then unconditionally passed
to link_enc_cfg_copy(), which appears to dereference it without checking.

Should dm_suspend() return an error when dc_state_create_copy() fails,
preventing the system from continuing the GPU reset process with a missing
DC state and crashing here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819054742.3143856-1-gonglinkai@kylinos.cn?part=1