Re: [PATCH v7 07/35] drm/msm/dp: add stream-aware link register accessors

[sashiko-bot@kernel.org]

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 <yongxing.mou@oss.qualcomm.com>

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/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=7