Re: [PATCH 2/4] drm/msm/dp: clear force_link_train flag when link training succeeds

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

New issues:
- [Low] Dead store of `force_link_train` introduced by the patch.

Pre-existing issues:
- [High] Failure in `pm_runtime_resume_and_get()` leads to a PM runtime usage count underflow.
- [High] Uninitialized stack memory read in `msm_dp_ctrl_channel_eq_ok()`.
--

--- Patch [2]: [PATCH 2/4] drm/msm/dp: clear force_link_train flag when link training succeeds ---
commit 03a67937ce09f7bc428c5c9c18503745e41c62f6
Author: Saurabh Anand <[email protected]>

drm/msm/dp: clear force_link_train flag when link training succeeds

msm_dp_display_prepare_link() sets force_link_train = true before calling
msm_dp_ctrl_on_link(). On success the flag was never cleared, so
msm_dp_ctrl_prepare_stream_on() would unconditionally trigger a second
link retrain even though the link was already trained. Clear
force_link_train on the success path so that
msm_dp_ctrl_prepare_stream_on() only retrains when the channel EQ check
fails, as intended.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bc646d172abe0..40e19d506a450 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -649,6 +649,8 @@ static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)

[Severity: High]
This isn't a bug introduced by this patch, but does the pm_runtime error
handling cause a usage count underflow?

If pm_runtime_resume_and_get() fails earlier in msm_dp_display_prepare_link(),
it returns an error without acquiring a PM runtime reference:

	rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev);
	if (rc) {
		DRM_ERROR("failed to pm_runtime_resume\n");
		return rc;
	}

Since the DRM core atomic_enable callback returns void, the framework
considers the device enabled and will later call the atomic_post_disable
hook. This routes to msm_dp_display_unprepare(), which unconditionally
drops the reference:

	pm_runtime_put_sync(&msm_dp_display->pdev->dev);

Will this drop a reference that was never acquired and lead to possible
device lockups?

>  		return rc;
>  	}
>  
> +	force_link_train = false;
> +

[Severity: Low]
Does this unconditionally overwrite the previous assignment?

Earlier in msm_dp_display_prepare_link(), force_link_train is set to true:

	if (!msm_dp_display->power_on) {
		msm_dp_display_host_phy_init(dp);
		force_link_train = true;
		dp->ctrl->plugged = dp->plugged;
	}

By setting it to false here unconditionally, does the prior assignment become
dead code? Could this parameter just be removed or passed as false directly?

>  	return msm_dp_ctrl_prepare_stream_on(dp->ctrl, dp->panel, force_link_train);

[Severity: High]
This is a pre-existing issue, but by clearing force_link_train on the
success path here, this patch makes msm_dp_ctrl_prepare_stream_on() evaluate
the channel EQ check. Does this lead to an uninitialized stack memory read?

When force_link_train is false, it eventually calls
msm_dp_ctrl_channel_eq_ok() which passes an uninitialized stack buffer to
drm_dp_dpcd_read_link_status():

	u8 link_status[DP_LINK_STATUS_SIZE];
	...
	drm_dp_dpcd_read_link_status(ctrl->aux, link_status);
	return drm_dp_channel_eq_ok(link_status, num_lanes);

If drm_dp_dpcd_read_link_status() fails (e.g., if the AUX channel fails), it
returns an error without initializing the buffer. Its return value is ignored,
and the uninitialized stack buffer is then read by drm_dp_channel_eq_ok(),
which might cause unpredictable behavior depending on stack garbage.

Can we check the return value of drm_dp_dpcd_read_link_status() before using
the buffer?

>  }

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