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