Re: [PATCH RFC v3 7/8] drm/analogix/anx7625: Use dp_connector helpers to expose link training state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm: lockdep assertion failure in DP helpers during atomic commit
- [High] drm/analogix: lockdep splat and sleep-in-atomic via dp_connector helpers in atomic callbacks
- [Medium] drm: eDP sink capabilities incorrectly erased on atomic_disable
- [Medium] drm: reading stale DP link parameters in detect callback
--

commit 1548e3d30cfb50d6bc0f342acba2b2ddf8b97bab
Author: Kory Maincent <kory.maincent@bootlin.com>

drm/analogix/anx7625: Use dp_connector helpers to expose link training state

Adopt the DRM core DisplayPort helpers by setting DRM_BRIDGE_OP_DP and
populating the bridge dp_link_caps at probe time. Report the sink
capabilities on hotplug, reset them on disconnect, and report the link
parameters once link training succeeds.

> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index c3af88e14f8b2..2b1235efc9fa4 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c

[ ... ]

> @@ -2580,6 +2606,17 @@ static void anx7625_bridge_atomic_enable(struct drm_bridge *bridge,
>  
>  	anx7625_dp_start(ctx);
>  
> +	/* On DisplayPort case, the caps are set in the detect callback. */
> +	if (bridge->type == DRM_MODE_CONNECTOR_eDP) {
> +		ret = drm_dp_sink_sync_caps(connector, &ctx->aux, NULL);
> +		if (ret)
> +			DRM_DEV_DEBUG_DRIVER(dev,
> +					     "error sink caps synchronisation (%d)\n",
> +					     ret);
> +	}
> +
> +	anx7625_report_link_train(ctx, connector);
> +

[Severity: High]
Does calling drm_dp_sink_sync_caps() and anx7625_report_link_train() here
violate atomic context invariants and locking expectations?

During non-blocking atomic commits, the commit_tail worker executes this
callback asynchronously after the originating thread has dropped all modeset
locks. However, drm_dp_sink_sync_caps() and drm_dp_set_cur_link_params()
(called by anx7625_report_link_train) explicitly assert that
connector->dev->mode_config.connection_mutex is held.

Additionally, atomic_enable callbacks execute in atomic context where sleeping
is forbidden. The DRM DP helpers perform I2C AUX transfers, allocate memory,
and acquire standard mutexes (like connector->dp.mutex), which are sleeping
operations.

>  	conn_state = drm_atomic_get_new_connector_state(state, connector);
>  
>  	if (WARN_ON(!conn_state))

[ ... ]

> @@ -2616,6 +2653,12 @@ static void anx7625_bridge_atomic_disable(struct drm_bridge *bridge,
>  		dev_dbg(dev, "update CP to DESIRE\n");
>  	}
>  
> +	/* On DisplayPort unplug, the caps are already reset in the detect
> +	 * callback.
> +	 */
> +	if (bridge->type == DRM_MODE_CONNECTOR_eDP)
> +		drm_dp_sink_reset_caps(ctx->connector);
> +

[Severity: High]
Similar to atomic_enable, does calling drm_dp_sink_reset_caps() from an
atomic_disable callback trigger lockdep and sleep-in-atomic warnings?

The helper asserts connection_mutex is held, but the lock is dropped before
asynchronous commits reach this callback. It also acquires standard mutexes,
violating the atomic context requirement.

[Severity: Medium]
Is it intentional to clear eDP sink capabilities on display disable?

Unlike hotpluggable DisplayPort monitors where capabilities must be cleared
upon physical unplug, an eDP panel is permanently attached to the system and
its capabilities are static. Erasing the capabilities on atomic_disable
causes sysfs DP capability attributes to read empty or zero whenever the
screen is in a DPMS off state or entering suspend, even though the panel
is still present.

>  	ctx->connector = NULL;
>  	anx7625_dp_stop(ctx);
>  

[ ... ]

> @@ -2634,11 +2677,25 @@ anx7625_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector
>  	struct anx7625_data *ctx = bridge_to_anx7625(bridge);
>  	struct device *dev = ctx->dev;
>  	enum drm_connector_status status;
> +	int ret;
>  
>  	DRM_DEV_DEBUG_DRIVER(dev, "drm bridge detect\n");
>  
>  	status = anx7625_sink_detect(ctx);
>  	anx7625_audio_update_connector_status(ctx, status);
> +
> +	if (status == connector_status_connected) {
> +		ret = drm_dp_sink_sync_caps(connector, &ctx->aux, NULL);
> +		if (ret)
> +			DRM_DEV_DEBUG_DRIVER(dev,
> +					     "error sink caps synchronisation (%d)\n",
> +					     ret);
> +
> +		anx7625_report_link_train(ctx, connector);
> +	} else {
> +		drm_dp_sink_reset_caps(connector);
> +	}
> +

[Severity: Medium]
Will anx7625_report_link_train() read stale hardware registers here?

During the detect phase, the DRM core is merely probing for a monitor
connection. The display pipeline has not been enabled, the sink hasn't been
powered up, and DP link training hasn't taken place yet. Reading the
negotiated link bandwidth and lane count directly from the hardware registers
at this point seems like it would yield uninitialized or stale values from a
previous display session.

These invalid values are then erroneously reported to the DRM core via
drm_dp_set_cur_link_params(), circumventing the logic of reporting them only
after successful link training.

>  	return status;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-feat_link_cap-v3-0-7517f4aba0f3@bootlin.com?part=7