Re: [PATCH v8 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 1 potential issue(s) to consider:
- [High] drm/msm/dp: msm_dp_hpd_plug_handle() skips HPD init when dp->plugged is set
--

commit 4113312f878c771b378b82c05f3076f2ed79cfc7
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, avoiding extra runtime PM references.

> 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]
Could this early return cause msm_dp_hpd_plug_handle() to skip
msm_dp_display_process_hpd_high() completely when detection runs before HPD
plug handling?

When connector detection runs while dp->plugged is false and a DisplayPort
sink is connected (for example at boot via
drm_helper_probe_single_connector_modes() -> drm_bridge_connector_detect_ctx(),
on system resume, or via DRM_IOCTL_MODE_GETCONNECTOR),
drm_bridge_connector_detect_ctx() calls detect->funcs->detect() immediately
before drm_bridge_connector_hpd_notify():

drivers/gpu/drm/display/drm_bridge_connector.c:drm_bridge_connector_detect_ctx() {
	if (detect) {
		status = detect->funcs->detect(detect, connector);
		...
		drm_bridge_connector_hpd_notify(connector, status, DRM_CONNECTOR_NO_EXTRA_STATUS);
	}
}

In msm_dp_bridge_detect(), priv->plugged is set to true after reading DPCD
into local stack buffers, 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 drm_bridge_connector_hpd_notify() then invokes msm_dp_bridge_hpd_notify()
-> msm_dp_hpd_plug_handle(), dp->plugged is already true, so
msm_dp_hpd_plug_handle() returns 0 without calling
msm_dp_display_process_hpd_high().

Similarly, if msm_dp_hpd_plug_handle() runs first and
msm_dp_display_process_hpd_high() fails (for example returning -ENOTCONN in
msm_dp_panel_read_link_caps() when a branch device is connected with
sink_count == 0, or on a transient AUX timeout), msm_dp_hpd_plug_handle()
still sets dp->plugged = true unconditionally:

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

	return ret;
}

Wouldn't both paths leave dp->panel->link_info, the connector EDID, LTTPR,
and dp->link->sink_count uninitialized, causing msm_dp_display_get_modes()
to return 0 modes, msm_dp_display_mode_valid() to reject modes with
MODE_BAD, and msm_dp_display_prepare_link() to fail with -ENOTCONN?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-hpd-irq-events-v8-0-6ee949e7a384@oss.qualcomm.com?part=6

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