Re: [PATCH v8 12/15] drm/msm/dp: simplify link and clock disable sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The refactored code introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition by evaluating `dp->link->sink_count == 0` twice.
--

--- Patch [12]: [PATCH v8 12/15] drm/msm/dp: simplify link and clock disable sequence ---
commit 84b41be14edf75da081c987f79e9f547234b2511
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: simplify link and clock disable sequence

This commit moves common disable steps out of the sink_count check to
make the flow easier to follow. No functional change is intended.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 1ecd700a474fe..d1a3a6f6c303d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -718,27 +718,19 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp)
>  
>  	msm_dp_panel_disable_vsc_sdp(dp->panel);
>  
> -	/* dongle is still connected but sinks are disconnected */
> -	if (dp->link->sink_count == 0) {
> -		/*
> -		 * irq_hpd with sink_count = 0
> -		 * hdmi unplugged out of dongle
> -		 */
> +	msm_dp_ctrl_off_pixel_clk(dp->ctrl);
>  
> -		/* set dongle to D3 (power off) mode */
> +	/* dongle is still connected but sinks are disconnected */
> +	if (dp->link->sink_count == 0)
>  		msm_dp_link_psm_config(dp->link, &dp->panel->link_info, true);
> -		msm_dp_ctrl_off_pixel_clk(dp->ctrl);
> -		msm_dp_ctrl_off_link(dp->ctrl, dp->panel);
> +
> +	msm_dp_ctrl_off_link(dp->ctrl, dp->panel);
> +
> +	if (dp->link->sink_count == 0)

[Severity: High]
Can checking dp->link->sink_count twice in this function introduce a race
condition?

If a hotplug or unplug event occurs exactly between these two checks, the
IRQ thread could dynamically update dp->link->sink_count via
msm_dp_irq_hpd_handle() -> msm_dp_link_process_request() while
msm_dp_display_disable() is executing.

Since there are no locks serializing the IRQ thread update against this
atomic disable sequence, could this result in an inconsistent hardware state?

For example, the dongle might be put into D3 power-off mode during the first
check, but if sink_count changes before the second check, the host PHY
might be exited instead of re-initialized.

The original code evaluated sink_count once and used an if/else block,
which guaranteed a mutually exclusive and consistent teardown sequence.
Would it be safer to cache the value of sink_count in a local variable
prior to the first check and use that local variable for both conditions?

> +		/* re-init the PHY so that we can listen to Dongle disconnect */
>  		msm_dp_ctrl_reinit_phy(dp->ctrl);
> -	} else {
> -		/*
> -		 * unplugged interrupt
> -		 * dongle unplugged out of DUT
> -		 */
> -		msm_dp_ctrl_off_pixel_clk(dp->ctrl);
> -		msm_dp_ctrl_off_link(dp->ctrl, dp->panel);
> +	else
>  		msm_dp_display_host_phy_exit(dp);
> -	}
>  
>  	msm_dp_display->power_on = false;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-dp_mstclean-v8-0-bea261bf4e7d@oss.qualcomm.com?part=12