Re: [PATCH v7 17/35] drm/msm/dp: add msm_dp_display_get_panel() to initialize DP panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/dp: null-deref in msm_dp_write_pn() due to optional pixel_base
- [Medium] drm/msm/dp: devm_kzalloc leak and state desync in msm_dp_display_get_panel()
--

commit 06b6470d2aa1e63fd1b8da09b3340d3557e87642
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

drm/msm/dp: add msm_dp_display_get_panel() to initialize DP panel
    
Add an API msm_dp_display_get_panel() to initialize and return a DP
panel to be used by DP MST module. Since some of the fields of
DP panel are private, dp_display module needs to initialize these
parts and return the panel back.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 487c76e0f990d..1d1fb45024c6a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -529,6 +529,33 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
>  	return rc;
>  }
>  
> +struct msm_dp_panel *msm_dp_display_get_panel(struct msm_dp *msm_dp_display,
> +					      enum msm_dp_stream_id stream_id)
> +{
> +	struct msm_dp_display_private *dp;
> +	struct msm_dp_panel *dp_panel;
> +
> +	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
> +
> +	if (stream_id >= msm_dp_ctrl_get_stream_cnt(dp->ctrl)) {
> +		DRM_ERROR("invalid stream_id %d\n", stream_id);
> +		return NULL;
> +	}
> +
> +	dp_panel = msm_dp_panel_get(&dp->msm_dp_display.pdev->dev, dp->aux, dp->link,
> +				    dp->link_base, dp->mst2link_base, dp->mst3link_base,
> +				    dp->pixel_base[stream_id]);

[Severity: High]
Can this cause a NULL pointer dereference later in msm_dp_write_pn()?

If a device tree provides the clocks but not the optional pixel_base memory
regions, dp->pixel_base[stream_id] will be NULL here. The bounds check above
verifies against msm_dp_ctrl_get_stream_cnt() (the number of available clocks),
not the number of available memory regions.

When the stream is later enabled, msm_dp_panel_timing_cfg() calls
msm_dp_write_pn(), which blindly dereferences panel->pixel_base.

[Severity: Medium]
Does this unconditionally allocate a new panel and risk a memory leak?

Since msm_dp_panel_get() uses devm_kzalloc() tied to the DP device, if the KMS
component is unbound and rebound, this will leak memory since it lacks a freeing
mechanism for dynamic binds.

Furthermore, for DP_STREAM_0, does this create a duplicate panel alongside
dp->panel? The newly allocated panels bypass early initialization steps (such
as msm_dp_ctrl_reset() and msm_dp_panel_read_link_caps()), which leaves
critical fields empty and can cause features like msm_dp_panel_update_sdp()
to malfunction.

> +
> +	if (IS_ERR(dp_panel)) {
> +		DRM_ERROR("failed to initialize panel\n");
> +		return NULL;
> +	}
> +
> +	dp_panel->stream_id = stream_id;
> +
> +	return dp_panel;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=17