Re: [PATCH v7 07/15] drm/msm/dp: move the pixel clock control to its own API

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unhandled error return from `msm_dp_ctrl_on_pixel_clk()` causing potential system crash.
- [High] Invalid `WARN_ON_ONCE` escalation leading to potential Denial of Service (DoS) on hardware timeouts.

Pre-existing issues:
- [High] Lockless concurrent access to `ctrl->stream_clks_on` between DRM atomic commit and HPD IRQ threads.
--

commit 4ab86d41ec16eead989a4b3a8579ccd95556770a
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: move the pixel clock control to its own API

Enable/Disable of DP pixel clock happens in multiple code paths
leading to code duplication. Move it into individual helpers so that
the helpers can be called wherever necessary.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 1c2eccec6ec6d..a2c44088e6a68 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -2171,6 +2171,41 @@ static bool msm_dp_ctrl_send_phy_test_pattern(struct msm_dp_ctrl_private *ctrl)
>  	return success;
>  }
>  
> +static int msm_dp_ctrl_on_pixel_clk(struct msm_dp_ctrl_private *ctrl, unsigned long pixel_rate)
> +{
> +	int ret;
> +
> +	ret = clk_set_rate(ctrl->pixel_clk, pixel_rate * 1000);
> +	if (ret) {
> +		DRM_ERROR("Failed to set pixel clock rate. ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	if (WARN_ON_ONCE(ctrl->stream_clks_on))
> +		return 0;

[Severity: High]
Does this WARN_ON_ONCE() introduce a potential system crash with panic_on_warn
enabled?

If msm_dp_ctrl_wait4video_ready() in msm_dp_ctrl_on_stream() times out, the
function returns an error but leaves ctrl->stream_clks_on as true. This causes
the caller, msm_dp_display_enable(), to refuse setting
msm_dp_display->power_on = true.

Later, on teardown, msm_dp_display_disable() bails out early because power_on
is false, skipping msm_dp_ctrl_off_pixel_clk() and leaving
ctrl->stream_clks_on as true.

On the next modeset or hotplug attempt, when msm_dp_ctrl_on_pixel_clk() is
called again, it will trigger this newly added warning. Since external
display hardware timeouts can be triggered by a faulty sink, is it safe to
escalate this from the previous debug print to a warning?

[Severity: High]
This is a pre-existing issue, but does lockless concurrent access to
ctrl->stream_clks_on between DRM atomic commit and HPD IRQ threads risk a NoC
error?

The boolean flag ctrl->stream_clks_on and the pixel clock are accessed and
modified in msm_dp_ctrl_on_pixel_clk() and msm_dp_ctrl_off_pixel_clk()
without any synchronization primitives.

msm_dp_ctrl_on_pixel_clk() can be executed by the DRM atomic commit thread
while the asynchronous HPD IRQ worker thread simultaneously handles a PHY test
request via msm_dp_ctrl_process_phy_test_request(). If Thread 1 evaluates
ctrl->stream_clks_on as true to disable the clock concurrently with Thread 2
accessing DP PHY registers, can it lead to a synchronous external abort when
accessing unclocked DP registers?

> +
> +	ret = clk_prepare_enable(ctrl->pixel_clk);
> +	if (ret) {
> +		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> +		return ret;
> +	}
> +	ctrl->stream_clks_on = true;
> +
> +	return ret;
> +}
> +
> +static void msm_dp_ctrl_off_pixel_clk(struct msm_dp_ctrl *msm_dp_ctrl)
> +{

[ ... ]

> @@ -2196,22 +2231,7 @@ static int msm_dp_ctrl_process_phy_test_request(struct msm_dp_ctrl_private *ctrl
>  	}
>  
>  	pixel_rate = ctrl->panel->msm_dp_mode.drm_mode.clock;
> -	ret = clk_set_rate(ctrl->pixel_clk, pixel_rate * 1000);
> -	if (ret) {
> -		DRM_ERROR("Failed to set pixel clock rate. ret=%d\n", ret);
> -		return ret;
> -	}
> -
> -	if (ctrl->stream_clks_on) {
> -		drm_dbg_dp(ctrl->drm_dev, "pixel clks already enabled\n");
> -	} else {
> -		ret = clk_prepare_enable(ctrl->pixel_clk);
> -		if (ret) {
> -			DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> -			return ret;
> -		}
> -		ctrl->stream_clks_on = true;
> -	}
> +	ret = msm_dp_ctrl_on_pixel_clk(ctrl, pixel_rate);
>  
>  	msm_dp_ctrl_send_phy_test_pattern(ctrl);

[Severity: High]
Does this code ignore the return value of msm_dp_ctrl_on_pixel_clk()?

In msm_dp_ctrl_process_phy_test_request(), ret is assigned from
msm_dp_ctrl_on_pixel_clk(), but it isn't checked before calling
msm_dp_ctrl_send_phy_test_pattern().

If clock enablement fails (e.g., due to an unsupported clock rate requested by
a malicious DP sink), the function unconditionally proceeds to send the PHY
test pattern. Will accessing DP PHY and mainlink hardware registers without
the required clock running trigger a synchronous external abort and panic the
kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-dp_mstclean-v7-0-ea04113e8233@oss.qualcomm.com?part=7