Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/msm/dp: null pointer dereference in stream-aware link accessors - [High] drm/msm/dp: hardware register misrouting for MST video enable - [High] drm/msm/dp: resource mapping corruption due to hardcoded DT indices --
commit 55281b71e2076f4bad3f57eba868f53bb2700307 Author: Yongxing Mou <[email protected]> drm/msm/dp: add stream-aware link register accessors DisplayPort MST uses multiple stream-specific register spaces. Streams 0 and 1 share the primary link register block with different register offsets, while streams 2 and 3 use dedicated MST link register blocks. Add stream-aware register access helpers that translate stream-specific register offsets and route accesses to the appropriate register space based on the stream id. > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c > b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index c0f5226dc383b..e1614bc49a7ca 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c [ ... ] > @@ -172,6 +174,49 @@ static inline void msm_dp_write_link(struct > msm_dp_ctrl_private *ctrl, > writel(data, ctrl->link_base + offset); > } > > +static inline u32 msm_dp_read_stream_link(struct msm_dp_ctrl_private *ctrl, > + enum msm_dp_stream_id stream_id, u32 > offset) > +{ > + offset = msm_dp_stream_reg(stream_id, offset); > + switch (stream_id) { > + case DP_STREAM_0: > + case DP_STREAM_1: > + return readl_relaxed(ctrl->link_base + offset); > + case DP_STREAM_2: > + return readl_relaxed(ctrl->mst2link_base + offset); [Severity: High] Can this regression lead to a NULL pointer dereference? If a platform has 3 or 4 pixel clocks but omits mst2link in the device tree, msm_dp_ioremap_optional() returns NULL for mst2link_base. If a connected MST sink or userspace modeset then triggers stream 2 initialization, this direct read access without validation will panic the kernel. > + case DP_STREAM_3: > + return readl_relaxed(ctrl->mst3link_base + offset); > + default: > + DRM_ERROR("error stream_id\n"); > + return 0; > + } > +} > + > +static inline void msm_dp_write_stream_link(struct msm_dp_ctrl_private *ctrl, > + enum msm_dp_stream_id stream_id, > u32 offset, u32 data) > +{ > + /* > + * To make sure link reg writes happens before any other operation, > + * this function uses writel() instread of writel_relaxed() > + */ > + offset = msm_dp_stream_reg(stream_id, offset); > + switch (stream_id) { > + case DP_STREAM_0: > + case DP_STREAM_1: > + writel(data, ctrl->link_base + offset); > + break; > + case DP_STREAM_2: > + writel(data, ctrl->mst2link_base + offset); [Severity: High] Does this code risk a write to a NULL pointer? Similar to the read accessor above, if mst2link_base fails to map or is omitted from the device tree, this direct write access will dereference a NULL pointer. [Severity: High] Is this a regression where MST streams 2 and 3 fail to enable video because of a missed conversion to these new accessors? Looking at msm_dp_ctrl_on_stream() in dp_ctrl.c: msm_dp_ctrl_setup_tr_unit(ctrl, panel); msm_dp_write_link(ctrl, REG_DP_STATE_CTRL, DP_STATE_CTRL_SEND_VIDEO); ret = msm_dp_ctrl_wait4video_ready(ctrl); The code still unconditionally calls the non-stream-aware msm_dp_write_link() which forces writes to ctrl->link_base (Stream 0/1's block) instead of using msm_dp_write_stream_link(). This sends the video enable command for streams 2 and 3 to the wrong hardware register, causing silent corruption of stream 0's state. > + break; > + case DP_STREAM_3: > + writel(data, ctrl->mst3link_base + offset); > + break; > + default: > + DRM_ERROR("error stream_id\n"); > + break; > + } > +} [ ... ] > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index d8b28a5626535..bb5e35e11b4d4 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c [ ... ] > @@ -1208,6 +1222,14 @@ static int msm_dp_display_get_io(struct > msm_dp_display_private *display) > break; > } > > + display->mst2link_base = msm_dp_ioremap_optional(pdev, 7, > &display->mst2link_len); [Severity: High] Can this hardcoded index cause a resource mapping regression? Device tree 'reg' arrays are contiguous. If a platform has only 3 pixel bases (p0, p1, p2), they occupy indices 3, 4, and 5, making the mst2link resource occupy index 6. Fetching index 7 assumes there are exactly 4 pixel bases preceding it, which may return NULL or map an unrelated memory region during driver probe. > + if (IS_ERR(display->mst2link_base)) > + return PTR_ERR(display->mst2link_base); > + > + display->mst3link_base = msm_dp_ioremap_optional(pdev, 8, > &display->mst3link_len); > + if (IS_ERR(display->mst3link_base)) > + return PTR_ERR(display->mst3link_base); > + > return 0; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
