Re: [PATCH v2 11/20] drm/msm/dp: Cache eDP link capabilities after successful discovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/dp: TOCTOU race in eDP detect unexpectedly powers off panel
- [High] drm/msm/dp: eDP power sequencing violation in msm_edp_bridge_detect()
--

commit bb9e355f18a717e81f26745b2bb594514c2f43b0
Author: Xilin Wu <sophon@radxa.com>

drm/msm/dp: Cache eDP link capabilities after successful discovery

An eDP panel may be powered off during mode validation. This commit caches
eDP link capabilities after successful discovery to persist them across
disable and suspend, and discovers capabilities from connector detect before
the first modeset.

> 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

[ ... ]

> @@ -971,6 +983,61 @@ void msm_dp_display_set_psr(struct msm_dp *msm_dp_display, bool enter)

[ ... ]

> +enum drm_connector_status msm_edp_bridge_detect(struct drm_bridge *bridge,
> +						struct drm_connector *connector)
> +{

[ ... ]

> +	ret = pm_runtime_resume_and_get(&display->pdev->dev);
> +	if (ret < 0)
> +		goto put_panel;
> +
> +	prepared = panel->prepared;
> +	if (!prepared)
> +		drm_panel_prepare(panel);

[Severity: High]
Does this sequence power up the DP host and PHY before the panel is prepared?

Calling pm_runtime_resume_and_get() triggers msm_dp_pm_runtime_resume(),
which for eDP initializes the DP PHY. Preparing the panel afterwards via
drm_panel_prepare() means the PHY is powered before panel VDD is applied.
Could this drive voltage onto the AUX or main link pins of an unpowered panel
and violate eDP power sequencing requirements?

[Severity: High]
Can a race condition occur here if an asynchronous atomic commit starts
concurrently?

If userspace triggers a non-blocking atomic modeset, it drops locks before
committing. If userspace then immediately issues a GETCONNECTOR ioctl, this
detect function will read prepared == false and call drm_panel_prepare().

The concurrent atomic commit would then execute panel_bridge_pre_enable(),
see that the panel is already prepared, and skip its own preparation.

> +	if (!panel->prepared) {
> +		ret = -EIO;
> +		goto put_pm;
> +	}
> +
> +	scoped_guard(mutex, &dp->plugged_lock) {
> +		ret = drm_dp_read_dpcd_caps(dp->aux, dpcd);
> +		if (!ret) {
> +			dp->link->lttpr_count = msm_dp_display_lttpr_init(dp, dpcd);
> +			ret = msm_dp_panel_read_link_caps(dp->panel, connector);
> +		}
> +	}
> +
> +	if (!prepared)
> +		drm_panel_unprepare(panel);

[Severity: High]
Is it possible for this unprepare to shut down the panel while a concurrent
modeset expects it to remain powered?

Because the atomic commit might have skipped preparing the panel (since it
was already prepared by this function), this call to drm_panel_unprepare()
could power off the panel out from under the active modeset based on the
stale local 'prepared' variable.

> +put_pm:
> +	pm_runtime_put_sync(&display->pdev->dev);

[Severity: High]
Does powering down the panel before the PHY violate the eDP power-off sequence?

Here drm_panel_unprepare() removes VDD before pm_runtime_put_sync() powers
down the DP host and PHY, which reverses the expected power-down order.

> +put_panel:
> +	drm_panel_put(panel);
> +	if (ret < 0)
> +		drm_dbg_dp(dp->drm_dev, "eDP capability discovery failed: %d\n", ret);
> +
> +	/* A failed AUX access does not mean the fixed panel was disconnected. */
> +	return connector_status_connected;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-msm-dp-hdr10-v2-0-1835d4966da3@radxa.com?part=11

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic