Re: [PATCH 4/4] drm/msm/dp: continue link training downgrade when cable is still plugged

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Lockless read of dp->plugged in msm_dp_display_prepare_link() causes a data race.

Pre-existing issues:
- [High] The return value of drm_dp_dpcd_read_link_status() is ignored, causing uninitialized stack memory to be used if the DPCD read fails.
--

--- Patch [4]: [PATCH 4/4] drm/msm/dp: continue link training downgrade when cable is still plugged ---
commit ad6d2e949e22a448345ebdb67f36f42020cff884
Author: Saurabh Anand <[email protected]>

drm/msm/dp: continue link training downgrade when cable is still plugged

During a Type-C reconnect the AUX channel may report link-disconnected
transiently while the physical cable is still present. The link training
retry loop in msm_dp_ctrl_on_link() was aborting immediately on any
msm_dp_aux_is_link_connected() failure, preventing the rate/lane downgrade
path from running.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 59070f399e2d3..e8fd8f4c75fec 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -2379,7 +2379,7 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl,
>  			break;
>  		} else if (training_step == DP_TRAINING_1) {
>  			/* link train_1 failed */
> -			if (!msm_dp_aux_is_link_connected(ctrl->aux))
> +			if (!msm_dp_aux_is_link_connected(ctrl->aux) && !msm_dp_ctrl->plugged)
>  				break;
>  
>  			drm_dp_dpcd_read_link_status(ctrl->aux, link_status);

[Severity: High]
This is a pre-existing issue, but by allowing the loop to continue when AUX
is disconnected, is it possible we'll perform a DPCD read that fails, leaving
the link_status stack array uninitialized?

If drm_dp_dpcd_read_link_status() fails (which is likely if the link is
disconnected), the error return value is ignored. Uninitialized stack memory is
then evaluated by msm_dp_ctrl_clock_recovery_any_ok() below to make hardware
configuration decisions.

Does this also affect the DP_TRAINING_2 case below, as well as
msm_dp_ctrl_channel_eq_ok()?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 1f60c78a463dd..93a37a44273f0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -644,6 +644,7 @@ static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
>  	if (!msm_dp_display->power_on) {
>  		msm_dp_display_host_phy_init(dp);
>  		force_link_train = true;
> +		dp->ctrl->plugged = dp->plugged;

[Severity: Medium]
Is it safe to read dp->plugged here without holding the dp->plugged_lock mutex?

This assignment runs in msm_dp_display_prepare_link() during atomic commit,
which could run concurrently with HPD plug/unplug events that modify
dp->plugged.

Could this lead to a data race where a stale value is read?

>  	}
>  
>  	rc = msm_dp_ctrl_on_link(dp->ctrl, dp->panel);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-qcom-dp-typec-reconnect-fixes-v1-0-2825e5bf8a96@oss.qualcomm.com?part=4