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? [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/[email protected]?part=7
