Re: [PATCH v6 6/9] drm/msm/dp: don't take extra references on multiple plug events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/dp: skipped HPD high processing in msm_dp_hpd_plug_handle()
- [Medium] drm/msm/dp: runtime PM reference leak in msm_dp_bridge_detect()
--

commit 1182d7f2f2e876c63dd6756de1f85227702b93b4
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dp: don't take extra references on multiple plug events

This commit skips subsequent HPD plug events in msm_dp_hpd_plug_handle() when
dp->plugged is already set so that extra runtime PM references are not taken
before an unplug event arrives.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index fa8942922b533..ce919a26fba28 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -412,6 +412,9 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
>  
>  	guard(mutex)(&dp->plugged_lock);
>  
> +	if (dp->plugged)
> +		return 0;

[Severity: High]
Can returning early here when dp->plugged is true cause
msm_dp_display_process_hpd_high() to be skipped completely when a display is
connected?

In drivers/gpu/drm/display/drm_bridge_connector.c,
drm_bridge_connector_detect_ctx() invokes the bridge detect callback before
notifying HPD:

drm_bridge_connector_detect_ctx()
    detect->funcs->detect() -> msm_dp_bridge_detect()
    drm_bridge_connector_hpd_notify()
        -> msm_dp_bridge_hpd_notify()
        -> msm_dp_hpd_plug_handle()

When msm_dp_bridge_detect() in drivers/gpu/drm/msm/dp/dp_display.c runs
first (for instance at boot or resume when a monitor is already connected, or
during connector probing before the threaded HPD IRQ runs), it reads the DPCD
capabilities into a local buffer and sets priv->plugged = true without
calling msm_dp_display_process_hpd_high():

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_detect() {
    ...
	status = connector_status_connected;
	priv->plugged = true;
    ...
}

When msm_dp_hpd_plug_handle() is then called, it sees dp->plugged == true and
returns 0 immediately.

Similarly, if msm_dp_hpd_plug_handle() runs first and
msm_dp_display_process_hpd_high() returns an error (such as when a branch
device is connected with sink_count == 0 or on a transient AUX error),
msm_dp_hpd_plug_handle() still sets dp->plugged = true at the end of the
function:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_hpd_plug_handle() {
    ...
	ret = msm_dp_display_process_hpd_high(dp);

	drm_dbg_dp(dp->drm_dev, "After, type=%d sink_count=%d\n",
			dp->msm_dp_display.connector_type,
			dp->link->sink_count);

	dp->plugged = true;

	return ret;
}

Because msm_dp_display_process_hpd_high() is never completed in either case,
will dp->panel->link_info, LTTPR state, the connector EDID, and
dp->link->sink_count remain uninitialized?

[Severity: Medium]
How does this single-reference model in msm_dp_hpd_plug_handle() interact
with the runtime PM reference handling in msm_dp_bridge_detect()?

In drivers/gpu/drm/msm/dp/dp_display.c, msm_dp_bridge_detect()
unconditionally acquires a runtime PM reference on &dp->pdev->dev at entry
and only releases it at the end if !priv->plugged:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_detect() {
	guard(mutex)(&priv->plugged_lock);
	ret = pm_runtime_resume_and_get(&dp->pdev->dev);
	if (ret) {
		DRM_ERROR("failed to pm_runtime_resume\n");
		return status;
	}
    ...
	status = connector_status_connected;
	priv->plugged = true;
    ...
end:
	if (!priv->plugged) {
		if (phy_deinit) {
			msm_dp_aux_enable_xfers(priv->aux, false);
			msm_dp_display_host_phy_exit(priv);
		}

		pm_runtime_put_sync(&dp->pdev->dev);
	}

	return status;
}

When a sink is connected and priv->plugged is already true, won't every call
to msm_dp_bridge_detect() leak an extra runtime PM reference on
&dp->pdev->dev because msm_dp_hpd_unplug_handle() only calls
pm_runtime_put_sync(&pdev->dev) once when transitioning dp->plugged from true
to false?

Conversely, if priv->plugged is true (holding a reference from
msm_dp_hpd_plug_handle()) and drm_dp_read_dpcd_caps() or drm_dp_read_desc()
fails inside msm_dp_bridge_detect(), msm_dp_bridge_detect() sets
priv->plugged = false and only drops the reference it took at entry, causing
msm_dp_hpd_unplug_handle() to return early on !dp->plugged without releasing
the reference acquired by msm_dp_hpd_plug_handle().

> +
>  	ret = pm_runtime_resume_and_get(&pdev->dev);
>  	if (ret) {
>  		DRM_ERROR("failed to pm_runtime_resume\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-hpd-irq-events-v6-0-337ea3ddaa62@oss.qualcomm.com?part=6

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