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