Re: [PATCH v4 13/14] drm/bridge: analogix_dp: Handle HPD notification from downstream bridge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/bridge: analogix_dp: race on dp->connection_notified in analogix_dp_bridge_notify()
--

commit e243ae0110d3bc583be1aa034e4af8955ad61137
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Handle HPD notification from downstream bridge

This commit handles hotplug notifications from a downstream bridge that
supports HPD events (DRM_BRIDGE_OP_HPD) when no downstream bridge provides
detection (DRM_BRIDGE_OP_DETECT). It caches the status passed to the
bridge's .hpd_notify callback and uses it in analogix_dp_get_plug_in_status()
while muting and skipping native HPD initialization.

Link: https://lore.kernel.org/all/5609825.iZASKD2KPV@diego/

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index df898f3142549..90245d55d1d2c 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c

[ ... ]

> @@ -1260,6 +1276,14 @@ static void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
>  		DRM_ERROR("Failed to enable psr (%d)\n", ret);
>  }
>  
> +static void analogix_dp_bridge_notify(struct drm_bridge *bridge, struct drm_connector *connector,
> +				      enum drm_connector_status status)
> +{
> +	struct analogix_dp_device *dp = to_dp(bridge);
> +
> +	dp->connection_notified = (status == connector_status_connected);
> +}

[Severity: Medium]
Can a concurrent connector probe overwrite a downstream HPD notification here
and cause the hotplug transition to be lost?

When dp->hpd_notify_mode is enabled, drm_bridge_connector_init() sets
bridge_connector->bridge_detect to the Analogix DP bridge and
bridge_connector->bridge_hpd to the downstream bridge.

In the connector detection path (for example via drm_mode_getconnector(),
drm_client_modeset_probe(), or status_store()),
drm_bridge_connector_detect_ctx() holds dev->mode_config.mutex, calls
detect->funcs->detect() which reads dp->connection_notified via:

  analogix_dp_bridge_detect()
    -> analogix_dp_detect_hpd()
      -> analogix_dp_get_plug_in_status()

and then feeds that return value back into 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);
}

Meanwhile, when a downstream HPD event arrives via drm_bridge_hpd_notify(),
drm_bridge_connector_handle_hpd() releases dev->mode_config.mutex before
calling drm_bridge_connector_hpd_notify():

drivers/gpu/drm/display/drm_bridge_connector.c:drm_bridge_connector_handle_hpd() {
	...
	mutex_lock(&dev->mode_config.mutex);
	connector->status = status;
	mutex_unlock(&dev->mode_config.mutex);

	drm_bridge_connector_hpd_notify(connector, status);

	drm_kms_helper_connector_hotplug_event(connector);
}

If these two paths interleave:

1. drm_bridge_connector_handle_hpd() updates connector->status and unlocks
   dev->mode_config.mutex.
2. A concurrent probe enters drm_bridge_connector_detect_ctx() under
   dev->mode_config.mutex and calls analogix_dp_bridge_detect(), reading the
   stale dp->connection_notified value.
3. drm_bridge_connector_handle_hpd() calls drm_bridge_connector_hpd_notify()
   -> analogix_dp_bridge_notify(), setting dp->connection_notified to the new
   status, and then waits on dev->mode_config.mutex inside
   drm_kms_helper_connector_hotplug_event().
4. drm_bridge_connector_detect_ctx() finishes and calls
   drm_bridge_connector_hpd_notify() -> analogix_dp_bridge_notify() with the
   stale status it read in step 2, clobbering dp->connection_notified.

When drm_kms_helper_connector_hotplug_event() (or userspace handling the
hotplug event) subsequently probes the connector,
analogix_dp_get_plug_in_status() will see the stale dp->connection_notified
value and the hotplug state change is lost.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019085836.3065008-1-damon.ding@rock-chips.com?part=13