Re: [PATCH v7 7/7] drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing atomic_check to prevent concurrent enablement of mutually exclusive DP bridges.
- [High] Hardware registers are accessed while DP clocks are disabled, leading to a synchronous external abort and kernel panic.
- [High] The driver fails to propagate internal hotplug state changes to userspace because its bridge no longer receives the HPD callback.
- [High] Video pipeline configuration is inappropriately executed during EDID read, leading to inevitable failure on secondary ports.
- [Medium] The connected status of bridges is updated and read concurrently without holding the necessary synchronization locks.
--

commit a45588df990bb11f402ba98f56052bec1405db96
Author: Chaoyi Chen <chaoyi.chen@rock-chips.com>

drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection

This commit adds support for multiple bridges to the Rockchip CDN-DP driver,
allowing dynamic output port selection. It registers separate encoders and
bridges for each PHY port and parses the device tree to describe connections.
Port switching is handled dynamically during EDID reads or when enabling the
DP block.

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index 74e1f5422a6e2..999309b39b34c 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c

[ ... ]

> @@ -254,12 +268,45 @@ static const struct drm_edid *
>  cdn_dp_bridge_edid_read(struct drm_bridge *bridge, struct drm_connector *connector)
>  {
>  	struct cdn_dp_device *dp = bridge_to_dp(bridge);
> -	const struct drm_edid *drm_edid;
> +	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
> +	struct cdn_dp_port *port = dp->port[dp_bridge->id];
> +	struct cdn_dp_port *prev_port = NULL;
> +	const struct drm_edid *drm_edid = NULL;
> +	int i, ret;
>  
>  	mutex_lock(&dp->lock);
> +
> +	/* More than one port is available */
> +	if (dp->bridge_count > 1 && !port->phy_enabled) {
> +		for (i = 0; i < dp->bridge_count; i++) {
> +			/* Another port already enable */
> +			if (dp->bridge_list[i] != dp_bridge && dp->bridge_list[i]->enabled)
> +				goto get_cache;
> +			/* Find already enabled port */
> +			if (dp->port[i]->phy_enabled)
> +				prev_port = dp->port[i];
> +		}
> +
> +		/* Switch to current port */
> +		if (prev_port) {
> +			ret = cdn_dp_switch_port(dp, prev_port, port);

[Severity: High]
Will this video pipeline configuration fail during an EDID read?

When reading the EDID on a secondary port, this switches the port and
ultimately calls cdn_dp_config_video() via cdn_dp_switch_port(). Since this
happens during connector probing, no atomic mode has been committed yet,
meaning dp->mode.clock is either 0 or holds a stale value.

This will cause the timing calculation in cdn_dp_config_video() to return
-EINVAL. The switch then fails, and this function falls back to returning the
cache (which is NULL on the first plug), breaking secondary port detection.

> +			if (ret)
> +				goto get_cache;
> +		}
> +	}
> +
>  	drm_edid = drm_edid_read_custom(connector, cdn_dp_get_edid_block, dp);

[Severity: High]
Is there a risk of a synchronous external abort when accessing the DP hardware
registers here?

Hotplug events from downstream bridges trigger cdn_dp_bridge_hpd_notify(),
which schedules dp->event_work to enable the DP clocks and hardware via
cdn_dp_enable(). At the same time, the DRM core processes the hotplug event
and schedules output_poll_work, which initiates an EDID read here.

If the EDID read happens before the worker thread has enabled the clocks, it
will bypass the port switch logic (since prev_port is NULL) and directly
access the APB registers (like MAILBOX_EMPTY_ADDR) inside
cdn_dp_get_edid_block(). Accessing these registers while pclk and core_clk
are disabled causes a kernel panic.

> +	/* replace edid cache */
> +	if (dp->edid_cache[dp_bridge->id])
> +		drm_edid_free(dp->edid_cache[dp_bridge->id]);
> +	dp->edid_cache[dp_bridge->id] = drm_edid_dup(drm_edid);
> +
>  	mutex_unlock(&dp->lock);
> +	return drm_edid;

[ ... ]

> @@ -572,6 +664,7 @@ static void cdn_dp_display_info_update(struct cdn_dp_device *dp,
>  static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_atomic_commit *state)
>  {
>  	struct cdn_dp_device *dp = bridge_to_dp(bridge);
> +	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
>  	struct drm_connector *connector;
>  	int ret, val;
>  
> @@ -600,6 +693,9 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
>  
>  	mutex_lock(&dp->lock);
>  
> +	if (dp->next_bridge_valid)
> +		dp->active_port = dp_bridge->id;
> +
>  	ret = cdn_dp_enable(dp);
>  	if (ret) {
>  		DRM_DEV_ERROR(dp->dev, "Failed to enable bridge %d\n",
> @@ -632,6 +728,7 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
>  		goto out;
>  	}
>  
> +	dp_bridge->enabled = true;

[Severity: High]
What prevents userspace from enabling multiple CRTCs simultaneously on this
single DP hardware block?

The driver registers multiple independent DRM bridges and encoders that share
a single DP controller. Without a custom atomic_check to enforce mutual
exclusion, the DRM core allows userspace to assign and enable multiple CRTCs
to these encoders.

If multiple bridges are enabled concurrently, cdn_dp_bridge_atomic_enable()
is called for both. The second call will see dp->active == true, skip the
PHY enablement, and then proceed to call cdn_dp_train_link() and
cdn_dp_config_video() here, which overrides the shared hardware registers
and corrupts the active display.

>  out:
>  	mutex_unlock(&dp->lock);
>  }

[ ... ]

> @@ -828,6 +927,17 @@ static int cdn_dp_audio_mute_stream(struct drm_bridge *bridge,
>  	return ret;
>  }
>  
> +static void cdn_dp_bridge_hpd_notify(struct drm_bridge *bridge,
> +				     struct drm_connector *connector,
> +				     enum drm_connector_status status)
> +{
> +	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
> +	struct cdn_dp_device *dp = bridge_to_dp(bridge);
> +
> +	dp->bridge_list[dp_bridge->id]->connected = status == connector_status_connected;
> +	schedule_work(&dp->event_work);
> +}

[Severity: Medium]
Is it safe to modify this connected state without acquiring dp->lock?

The boolean state dp->bridge_list[dp_bridge->id]->connected is written here
directly, but cdn_dp_pd_event_work() reads this exact flag immediately after
explicitly dropping dp->lock. This data race could cause the worker to
process stale state if a concurrent HPD notification occurs.

> +
>  static const struct drm_bridge_funcs cdn_dp_bridge_funcs = {
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,

[ ... ]

> @@ -945,9 +1057,12 @@ static void cdn_dp_pd_event_work(struct work_struct *work)
>  
>  out:
>  	mutex_unlock(&dp->lock);
> -	drm_bridge_hpd_notify(&dp->bridge,
> -			      dp->connected ? connector_status_connected
> -					    : connector_status_disconnected);
> +	for (i = 0; i < dp->bridge_count; i++) {
> +		connected = dp->connected && dp->bridge_list[i]->connected;
> +		drm_bridge_hpd_notify(&dp->bridge_list[i]->bridge,
> +				      connected ? connector_status_connected
> +						: connector_status_disconnected);
> +	}
>  }

[Severity: High]
Will this HPD notification actually reach userspace?

The DRM bridge connector (drm_bridge_connector_init) only registers the
hpd_cb on the last bridge in the chain that supports DRM_BRIDGE_OP_HPD.
Because the downstream AUX HPD bridge supports HPD and is added to the chain,
it becomes the designated HPD bridge and takes the hpd_cb.

As a result, this bridge's hpd_cb is never set, making this call to
drm_bridge_hpd_notify() a silent no-op. If link training fails, userspace
is never notified that the connection has dropped, permanently breaking the
display state machine.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828084737.565-1-kernel@airkyi.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy