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