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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/msm/dp: pass-by-value plugged in msm_dp_ctrl_on_link()
  breaks unplug abort
- [Medium] drm/msm/dp: uninitialized link_status read in
  msm_dp_ctrl_on_link()
--

commit 03d6b22aa9378458ab94c6193f79ad69b24cfe32
Author: Saurabh Anand <saurabh.anand@oss.qualcomm.com>

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

This commit passes dp->plugged into msm_dp_ctrl_on_link() so the link
training retry loop only aborts when both msm_dp_aux_is_link_connected()
and plugged are false.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 6b9d9a76ba059..f8a6bd7b0d107 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c

[ ... ]

> @@ -2377,7 +2378,7 @@ static bool msm_dp_ctrl_channel_eq_ok(struct msm_dp_ctrl_private *ctrl)
>  }
>  
>  int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl,
> -			struct msm_dp_panel *panel)
> +			struct msm_dp_panel *panel, bool plugged)
>  {
>  	int rc = 0;
>  	struct msm_dp_ctrl_private *ctrl;
> @@ -2427,7 +2428,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) && !plugged)
> +			if (!msm_dp_aux_is_link_connected(ctrl->aux) && !plugged)
>  				break;

[Severity: Medium]
Does passing dp->plugged by value into msm_dp_ctrl_on_link() prevent the
retry loop from ever breaking out on an unplug during link training?

At both callers (msm_dp_display_prepare_link() and
msm_dp_display_handle_irq_hpd()), dp->plugged is true when link training
starts, and msm_dp_display_prepare_link() also reads dp->plugged without
holding dp->plugged_lock.  If the cable is unplugged while the retry loop is
running, msm_dp_hpd_unplug_handle() sets dp->plugged = false, but the local
plugged parameter in msm_dp_ctrl_on_link() stays true for every iteration,
so !plugged is always false and all remaining downshift retries and PHY
power-cycles still run on the disconnected link.

Also, the commit message refers to msm_dp_ctrl->plugged, which does not
exist in struct msm_dp_ctrl or struct msm_dp_ctrl_private.

>  
>  			drm_dp_dpcd_read_link_status(ctrl->aux, link_status);
> @@ -2452,7 +2453,7 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl,
>  			}
>  		} else if (training_step == DP_TRAINING_2) {
>  			/* link train_2 failed */
> -			if (!msm_dp_aux_is_link_connected(ctrl->aux) && !plugged)
> +			if (!msm_dp_aux_is_link_connected(ctrl->aux) && !plugged)
>  				break;
>  
>  			drm_dp_dpcd_read_link_status(ctrl->aux, link_status);

[Severity: Medium]
When continuing past !msm_dp_aux_is_link_connected(ctrl->aux) in the
DP_TRAINING_1 and DP_TRAINING_2 failure paths, can this read uninitialized
stack bytes from link_status[]?

link_status is declared on the stack without initialization in
msm_dp_ctrl_on_link().  When the AUX channel is transiently disconnected or
a concurrent unplug disables AUX transfers, drm_dp_dpcd_read_link_status()
fails without writing to link_status, and its return value is not checked
before msm_dp_ctrl_clock_recovery_any_ok() and drm_dp_clock_recovery_ok()
inspect link_status to choose between rate downshift, lane downshift, and
abort.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111104434.2222182-1-naidu.nama@oss.qualcomm.com?part=3