Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] msm/dp: TOCTOU race in PHY power ownership tracking causes power and clock leak --
commit c9182f8e77cc8634e408d0bae0ac2751df04598b Author: Xilin Wu <[email protected]> drm/msm/dp: Track PHY power ownership This commit introduces PHY power ownership tracking to prevent underflow during repeated cleanup operations. It adds boolean flags to track PHY power and link clock states and uses them to skip redundant enables or disables. > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c > b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 59070f399e2d3..f005421630c63 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c [ ... ] > @@ -1813,6 +1814,9 @@ static void msm_dp_ctrl_link_clk_disable(struct > msm_dp_ctrl *msm_dp_ctrl) > > ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, > msm_dp_ctrl); > > + if (!ctrl->link_clks_on) > + return; > + [Severity: High] Could this lockless check introduce a clock reference leak when executing concurrently? If a DP sink device sends an HPD interrupt with a DP_TEST_LINK_PHY_TEST_PATTERN request at the same time as a userspace modeset, msm_dp_ctrl_link_clk_enable() might run concurrently from two threads (one holding plugged_lock, the other holding drm_modeset_lock). Both could enable the clock and set the flag. Later, the first disable call un-sets the flag, causing subsequent calls to return here early and leak the extra clock reference. > clk_bulk_disable_unprepare(ctrl->num_link_clks, ctrl->link_clks); > > ctrl->link_clks_on = false; > @@ -1824,6 +1828,23 @@ static void msm_dp_ctrl_link_clk_disable(struct > msm_dp_ctrl *msm_dp_ctrl) > str_on_off(ctrl->core_clks_on)); > } > > +static int msm_dp_ctrl_phy_power_off(struct msm_dp_ctrl_private *ctrl) > +{ > + int ret; > + > + if (!ctrl->phy_powered) > + return 0; > + [Severity: High] Similarly, does this skip release when the flag was unset by a racing thread? If phy_power_on() was called twice due to the same race, the second power-off call will skip releasing the reference here, permanently leaking the PHY power reference. > + ret = phy_power_off(ctrl->phy); > + if (ret) { > + DRM_ERROR("Failed to power off PHY: %d\n", ret); > + return ret; > + } > + > + ctrl->phy_powered = false; > + return 0; > +} > + > static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private > *ctrl, > struct msm_dp_panel *panel) > { > @@ -1836,7 +1857,11 @@ static int msm_dp_ctrl_enable_mainlink_clocks(struct > msm_dp_ctrl_private *ctrl, > ctrl->phy_opts.dp.ssc = drm_dp_max_downspread(dpcd); > > phy_configure(phy, &ctrl->phy_opts); > - phy_power_on(phy); > + if (!ctrl->phy_powered) { [Severity: High] Does this flag need lock protection? The phy_powered state is checked and modified without locking here. When msm_dp_irq_hpd_handle() and msm_dp_display_atomic_enable() race, both threads could see !ctrl->phy_powered as true, calling phy_power_on() twice and incrementing the PHY core's power_count twice. > + ret = phy_power_on(phy); > + if (!ret) > + ctrl->phy_powered = true; > + } > > dev_pm_opp_set_rate(ctrl->dev, ctrl->link->link_params.rate * 1000); > ret = msm_dp_ctrl_link_clk_enable(&ctrl->msm_dp_ctrl); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
