Re: [PATCH 03/13] drm/msm/dp: Track PHY power ownership

[sashiko-bot@kernel.org]

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 <sophon@radxa.com>

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/20260930-msm-dp-hdr10-v1-0-dbbf8b2b42bc@radxa.com?part=3