On Wed, Mar 12, 2025 at 03:39:41PM +0800, Chung, ChiaHsuan (Tom) wrote:
> The original code will check the drm_new_conn_state if it's valid in here
>
> 10712 if (IS_ERR(drm_new_conn_state)) {
>
That's checking for error pointers. It's irrelevant. The warning is
about NULL pointers. It should probably be a NULL check. I have
written a blog that might be related?
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
> 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?
Calling PTR_ERR_OR_ZERO() doesn't make sense when we know that
drm_new_conn_state is an error pointer.
regards,
dan carpenter
>
> 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
