Re: [PATCH v7 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 patch introduces a Time-of-Check to Time-of-Use (TOCTOU) race by splitting a single `sink_count` evaluation into two unprotected checks, risking an inconsistent hardware disable sequence.
--

commit 4b86b22d6f1aa41eba1cece8a907cf0b3a12b55c
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

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

This patch refactors the disable sequence by pulling common steps out
of the sink_count condition block to simplify the flow.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -716,27 +716,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) {
[ ... ]
> +	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]
Could this double-evaluation of dp->link->sink_count introduce a race
condition?

The original code checked dp->link->sink_count == 0 exactly once. By
checking it twice, if an asynchronously processed HPD IRQ event modifies
the sink_count between the two checks, could the driver execute a
mismatched hardware disable sequence?

For instance, if sink_count changes from 0 to 1 after the first check,
the driver would perform msm_dp_link_psm_config(), but then take the
else branch and call msm_dp_display_host_phy_exit() below. This would
leave the PHY completely powered down, potentially breaking the ability
to detect future dongle disconnects.

Would it be safer to read sink_count once into a local variable and
evaluate the 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/20260609-dp_mstclean-v7-0-ea04113e8233@oss.qualcomm.com?part=12