Re: [PATCH v8 11/15] drm/msm/dp: split dp_ctrl_off() into stream and link parts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Hardware power-down sequence violation leading to potential DP controller underflows.

Pre-existing issues:
- [High] TOCTOU race condition and lockless access to hardware clock state.
--

--- Patch [11]: [PATCH v8 11/15] drm/msm/dp: split dp_ctrl_off() into stream and link parts ---
commit b7bbbd3dcfb3d3e5bbed08959f3a0e241b42ce06
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

drm/msm/dp: split dp_ctrl_off() into stream and link parts
    
Split dp_ctrl_off() into stream and link parts so that for MST
cases we can control the link and pixel parts separately.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 207643fa73063..fc3fbc40b118d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
[ ... ]
> @@ -2241,7 +2241,8 @@ static int msm_dp_ctrl_process_phy_test_request(struct msm_dp_ctrl_private *ctrl
>  	 * running. Add the global reset just before disabling the
>  	 * link clocks and core clocks.
>  	 */
> -	msm_dp_ctrl_off(&ctrl->msm_dp_ctrl, panel);
> +	msm_dp_ctrl_off_pixel_clk(&ctrl->msm_dp_ctrl);
> +	msm_dp_ctrl_off_link(&ctrl->msm_dp_ctrl, panel);

[Severity: High]
Does this reverse the hardware power-down sequence?

In the original msm_dp_ctrl_off() implementation, the pixel stream clock was
disabled after msm_dp_ctrl_mainlink_disable() and msm_dp_ctrl_reset() had
stopped the hardware.

By calling msm_dp_ctrl_off_pixel_clk() before msm_dp_ctrl_off_link() here,
could we be killing the pixel stream while the DP mainlink is still enabled
and attempting to transport video data? This seems like it could starve the DP
controller's stream FIFO and lead to underflows or hardware lockups.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 8694f366623d8..1ecd700a474fe 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -727,15 +727,16 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp)
>  
>  		/* set dongle to D3 (power off) mode */
>  		msm_dp_link_psm_config(dp->link, &dp->panel->link_info, true);
> -		msm_dp_ctrl_off(dp->ctrl, dp->panel);
> -		/* re-init the PHY so that we can listen to Dongle disconnect */
> +		msm_dp_ctrl_off_pixel_clk(dp->ctrl);
> +		msm_dp_ctrl_off_link(dp->ctrl, dp->panel);

[Severity: High]
This is a pre-existing issue, but could this concurrent execution lead to a
race condition resulting in unbalanced clock disables?

The KMS atomic disable path in msm_dp_display_disable() runs in process
context holding DRM modeset locks. However, the HPD notification path
triggering msm_dp_ctrl_process_phy_test_request() is executed without
modeset locks.

Since msm_dp_ctrl_off_pixel_clk() accesses ctrl->stream_clks_on without
synchronization:

drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_off_pixel_clk() {
    if (ctrl->stream_clks_on) {
        clk_disable_unprepare(ctrl->pixel_clk);
        ctrl->stream_clks_on = false;
    }
}

Would an untrusted external DP sink sending concurrent PHY test requests
during a KMS atomic commit cause both paths to evaluate the flag as true?
This might result in double execution of clk_disable_unprepare() and
corruption of the clock reference counters.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-dp_mstclean-v8-0-bea261bf4e7d@oss.qualcomm.com?part=11