The original code will check the drm_new_conn_state if it's valid in here
10712 if (IS_ERR(drm_new_conn_state)) {
After that the drm_new_conn_state does not touch by anyone before
call the
--> 10751 ret = fill_hdr_info_packet(drm_new_conn_state,
I think it should be no issue in this case.
We call the PTR_ERR_OR_ZERO() just because we need to get the error code and return to the caller.
10713 ret = PTR_ERR_OR_ZERO(drm_new_conn_state);
Maybe it's just a false warning?
Tom
On 3/12/2025 10:34 AM, Srinivasan
Shanmugam wrote:
Added checks for NULL values after retrieving drm_new_conn_state and
drm_old_conn_state to prevent dereferencing NULL pointers.
Fixes the below:
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:10751 dm_update_crtc_state()
warn: 'drm_new_conn_state' can also be NULL
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c
10672 static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
10673 struct drm_atomic_state *state,
10674 struct drm_crtc *crtc,
10675 struct drm_crtc_state *old_crtc_state,
10676 struct drm_crtc_state *new_crtc_state,
10677 bool enable,
10678 bool *lock_and_validation_needed)
10679 {
10680 struct dm_atomic_state *dm_state = NULL;
10681 struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;
10682 struct dc_stream_state *new_stream;
10683 int ret = 0;
10684
...
10703
10704 /* TODO This hack should go away */
10705 if (connector && enable) {
10706 /* Make sure fake sink is created in plug-in scenario */
10707 drm_new_conn_state = drm_atomic_get_new_connector_state(state,
10708 connector);
drm_atomic_get_new_connector_state() can't return error pointers, only NULL.
10709 drm_old_conn_state = drm_atomic_get_old_connector_state(state,
10710 connector);
10711
10712 if (IS_ERR(drm_new_conn_state)) {
^^^^^^^^^^^^^^^^^^
10713 ret = PTR_ERR_OR_ZERO(drm_new_conn_state);
Calling PTR_ERR_OR_ZERO() doesn't make sense. It can't be success.
10714 goto fail;
10715 }
10716
...
10748
10749 dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
10750
--> 10751 ret = fill_hdr_info_packet(drm_new_conn_state,
^^^^^^^^^^^^^^^^^^ Unchecked dereference
10752 &new_stream->hdr_static_metadata);
10753 if (ret)
10754 goto fail;
10755
Cc: Harry Wentland <harry.wentland@xxxxxxx>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
Cc: Tom Chung <chiahsuan.chung@xxxxxxx>
Cc: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>
Cc: Roman Li <roman.li@xxxxxxx>
Cc: Alex Hung <alex.hung@xxxxxxx>
Cc: Aurabindo Pillai <aurabindo.pillai@xxxxxxx>
Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 +++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
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 1b92930119cc..e3df11662fff 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10727,11 +10727,20 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
drm_old_conn_state = drm_atomic_get_old_connector_state(state,
connector);
- if (IS_ERR(drm_new_conn_state)) {
- ret = PTR_ERR_OR_ZERO(drm_new_conn_state);
- goto fail;
+ /* Check if drm_new_conn_state is valid */
+ if (drm_new_conn_state) {
+ dm_new_conn_state = to_dm_connector_state(drm_new_conn_state);
+
+ /* Attempt to fill HDR info packet */
+ ret = fill_hdr_info_packet(drm_new_conn_state,
+ &new_stream->hdr_static_metadata);
+ if (ret)
+ goto fail;
}
+ if (drm_old_conn_state)
+ dm_old_conn_state = to_dm_connector_state(drm_old_conn_state);
+
dm_new_conn_state = to_dm_connector_state(drm_new_conn_state);
dm_old_conn_state = to_dm_connector_state(drm_old_conn_state);
@@ -10766,11 +10775,6 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
- ret = fill_hdr_info_packet(drm_new_conn_state,
- &new_stream->hdr_static_metadata);
- if (ret)
- goto fail;
-
/*
* If we already removed the old stream from the context
* (and set the new stream to NULL) then we can't reuse
