Re: [PATCH 3/3] drm/msm/dp: validate modes against the trained link, not the sink's claim

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/dp: stale link training parameters artificially restrict modes after hotplug
- [Medium] drm/msm/dp: data race and TOCTOU in mode validation reading link_params
--

commit 69ad059161eff4e2e43af0ffa4e037a24d8b918c
Author: Jean-Francois Bobier <jean-francois.bobier@laposte.net>

drm/msm/dp: validate modes against the trained link, not the sink's claim

This commit updates msm_dp_display_mode_valid() to validate modes against
the trained link parameters rather than the sink's advertised capabilities.
This prevents advertising modes that the link cannot carry after a training
fallback.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 6abe30bdff120..d1e111c6a5fb8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -793,10 +795,42 @@ enum drm_mode_status msm_dp_display_mode_valid(struct msm_dp *dp,
>  			mode_bpp, link_pclk_khz);
>  
>  	mode_rate_khz = link_pclk_khz * mode_bpp;
> -	supported_rate_khz = link_info->num_lanes * link_info->rate * 8;
>  
> -	if (mode_rate_khz > supported_rate_khz)
> +	/*
> +	 * link_info is what the sink advertises in DPCD, which is not
> +	 * necessarily what we managed to train. msm_dp_ctrl_on_link() walks
> +	 * the rate down HBR3 -> HBR2 -> HBR -> RBR and then halves the lane
> +	 * count until training succeeds, so a cable with two unusable lanes
> +	 * ends up carrying half the bandwidth the sink claims.
> +	 *
> +	 * Sizing modes against the sink's maximum then advertises modes the
> +	 * link cannot carry: training "succeeds" at the reduced parameters,
> +	 * the mode does not fit in them, and the sink shows a black screen
> +	 * with nothing in the log to say why.
> +	 *
> +	 * link->link_params holds the trained values, and is reset to the
> +	 * sink's maximum at the top of every msm_dp_ctrl_on_link(), so taking
> +	 * the lower of the two is accurate after a fallback and a no-op
> +	 * before the first training.
> +	 */
> +	num_lanes = link_info->num_lanes;
> +	rate = link_info->rate;
> +
> +	trained = &msm_dp_display->link->link_params;
> +	if (trained->num_lanes && trained->num_lanes < num_lanes)
> +		num_lanes = trained->num_lanes;
> +	if (trained->rate && trained->rate < rate)
> +		rate = trained->rate;

[Severity: High]
Will this improperly restrict modes for new monitors after a hotplug?

When a monitor is unplugged, msm_dp_hpd_unplug_handle() and
msm_dp_link_reset_data() do not clear or invalidate link_params.

If the previous monitor fell back to a lower lane count or rate, does this
mean the new monitor's modes will be evaluated against the old monitor's
degraded parameters?

[Severity: Medium]
Is it safe to read trained->num_lanes and trained->rate locklessly here?

During an asynchronous non-blocking modeset, msm_dp_ctrl_on_link() can
modify these values concurrently:

drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_on_link() {
    ...
		ctrl->link->link_params.rate = rate;
		ctrl->link->link_params.num_lanes = panel->link_info.num_lanes;
    ...
}

Since userspace can concurrently read the EDID and trigger mode validation,
could the lack of locking or READ_ONCE() here lead to a data race or a TOCTOU
issue where the condition check and subsequent assignment see different
values?

> +
> +	supported_rate_khz = num_lanes * rate * 8;
> +
> +	if (mode_rate_khz > supported_rate_khz) {
> +		drm_dbg_dp(dp->drm_dev,
> +			   "mode %s rejected: needs %u kHz, link carries %u kHz (%u lanes @ %u)\n",
> +			   mode->name, mode_rate_khz, supported_rate_khz,
> +			   num_lanes, rate);
>  		return MODE_BAD;
> +	}
>  
>  	return MODE_OK;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005131432.22840-1-jean-francois.bobier@laposte.net?part=3