Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The refactoring introduces a WARN_ON_ONCE for reachable states and error paths, potentially causing a kernel panic (if panic_on_warn is enabled). - [Low] The patch missed replacing duplicated pixel clock disablement logic in msm_dp_ctrl_off_link_stream with the new helper, and the helpers have asymmetric signatures. --
--- Patch [7]: [PATCH v9 07/14] drm/msm/dp: move the pixel clock control to its own API --- commit c980fe633e423ad69174b99f07ed497b0b4f6cf2 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 f343bd6c2b81a..068d4f8b673f6 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -2176,6 +2176,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] Is it safe to replace the debug log with a WARN_ON_ONCE() here? If an atomic commit enables the display, and msm_dp_ctrl_on_stream() succeeds in enabling clocks but fails later (e.g., msm_dp_ctrl_wait4video_ready() times out), it can return an error without disabling the pixel clock. When the caller msm_dp_display_enable() propagates the error, dp->power_on remains false. The teardown path msm_dp_display_disable() will then return early because power_on is false, leaving the clock enabled. If userspace attempts to enable the display again, msm_dp_ctrl_on_pixel_clk() would be called with stream_clks_on == true, triggering this WARN_ON_ONCE(). This could also be triggered if an external DP sink sends a PHY test pattern request before the display is enabled, via msm_dp_ctrl_process_phy_test_request(). Since this state is reachable from hardware errors or untrusted external DP sinks, could this cause a denial of service if panic_on_warn is enabled? > + > + 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) > +{ > + struct msm_dp_ctrl_private *ctrl; > + > + ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, > msm_dp_ctrl); > + > + if (ctrl->stream_clks_on) { > + clk_disable_unprepare(ctrl->pixel_clk); > + ctrl->stream_clks_on = false; > + } > +} [Severity: Low] Is this API signature asymmetry intentional? msm_dp_ctrl_on_pixel_clk() takes a struct msm_dp_ctrl_private *, but this function takes a struct msm_dp_ctrl *. Also, it looks like msm_dp_ctrl_off_link_stream() in this same file was missed in the refactoring and still contains the duplicated pixel clock disablement logic. Should it be updated to use this new helper? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
