On 6/9/2026 6:04 PM, [email protected] wrote:
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 <[email protected]>
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?
No, it is okay, printing the call stack can help identify the issue.
[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?
will fix next patch.
