Re: [PATCH v3 5/5] 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 8 potential issue(s) to consider:

New issues:
- [High] Buffer overflow when parsing device tree endpoints due to missing bounds check.
- [High] Broken hotplug detection for legacy extcon configurations due to unconditional port return.
- [High] NULL pointer dereference and uninitialized memory read if DT endpoints exceed valid PHYs.
- [High] Port switching during EDID read always fails because `cdn_dp_config_video` is called with a zero clock.
- [High] Stale pointers in persistent arrays cause double-free and use-after-free on component re-bind.
- [High] Workqueue `dp->event_work` is scheduled before being initialized, leading to a NULL pointer dereference.
- [Medium] Multiple encoders share a single hardware DP controller but lack an `atomic_check` callback to prevent simultaneous activation.

Pre-existing issues:
- [High] Using `devm_` APIs in the component `bind` callback causes severe resource leaks and active stale notifiers after `unbind`.
--

--- Patch [5]: [PATCH v3 5/5] drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection ---
commit a0d8b7a8a3a01d68ddf971ecfdb92ccb11c60b09
Author: Chaoyi Chen <chaoyi.chen@rock-chips.com>

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

This patch adds support for multiple bridges to allow selecting between
different PHY ports on the RK3399 CDN-DP controller. It registers a separate
encoder and bridge for each PHY port and parses the DT to handle bridge
chain configurations.

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index 74e1f5422a6e2..5e513f6930cb9 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
[ ... ]
> @@ -192,14 +193,27 @@ static int cdn_dp_get_sink_count(struct cdn_dp_device *dp, u8 *sink_count)
>  static struct cdn_dp_port *cdn_dp_connected_port(struct cdn_dp_device *dp)
>  {
>  	struct cdn_dp_port *port;
> -	int i, lanes;
> +	int i, lanes[MAX_PHY];
>  
>  	for (i = 0; i < dp->ports; i++) {
>  		port = dp->port[i];
> -		lanes = cdn_dp_get_port_lanes(port);
> -		if (lanes)
> +		lanes[i] = cdn_dp_get_port_lanes(port);
> +		if (!dp->next_bridge_valid)
>  			return port;

[Severity: High]
Does this break hotplug detection for legacy extcon configurations?

By removing the check for lanes > 0, this will unconditionally return the
first port regardless of actual connection status.

>  	}
> +
> +	if (dp->next_bridge_valid) {
> +		/* If more than one port is available, pick the last active port */
> +		if (dp->active_port >= 0 && lanes[dp->active_port])
> +			return dp->port[dp->active_port];
> +
> +		/* If the last active port is not available, pick an available port in order */
> +		for (i = 0; i < dp->bridge_count; i++) {
> +			if (lanes[i])
> +				return dp->port[i];
> +		}
> +	}

[Severity: High]
Could this lead to an uninitialized memory read and NULL pointer
dereference?

If the device tree provides more endpoints than probed PHYs,
dp->bridge_count will exceed dp->ports. This loop will access
uninitialized stack memory in lanes[i] and potentially return a NULL
dp->port[i], causing a crash later in cdn_dp_bridge_edid_read() when
checking port->phy_enabled.

[ ... ]
> @@ -551,6 +599,50 @@ static bool cdn_dp_check_link_status(struct cdn_dp_device *dp)
> +static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port *prev_port,
> +			      struct cdn_dp_port *port)
> +{
[ ... ]
> +	if (!cdn_dp_check_link_status(dp)) {
> +		dev_info(dp->dev, "Connected with sink; re-train link\n");
> +
> +		ret = cdn_dp_train_link(dp);
> +		if (ret) {
> +			dev_err(dp->dev, "Training link failed: %d\n", ret);
> +			goto out;
> +		}
> +
> +		ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
> +		if (ret) {
> +			dev_err(dp->dev, "Failed to idle video %d\n", ret);
> +			goto out;
> +		}
> +
> +		ret = cdn_dp_config_video(dp);
> +		if (ret)
> +			dev_err(dp->dev, "Failed to configure video: %d\n", ret);
> +	}

[Severity: High]
Will this port switch fail consistently during EDID read operations?

When cdn_dp_bridge_edid_read() calls this function, dp->mode.clock is
typically 0. A zero clock will cause cdn_dp_config_video() to spin until
timeout and return -EINVAL, thus aborting the EDID read.

[ ... ]
> @@ -828,6 +927,17 @@ static int cdn_dp_audio_mute_stream(struct drm_bridge *bridge,
>  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,
>  	.atomic_reset = drm_atomic_helper_bridge_reset,
>  	.detect = cdn_dp_bridge_detect,
>  	.edid_read = cdn_dp_bridge_edid_read,
>  	.atomic_enable = cdn_dp_bridge_atomic_enable,
>  	.atomic_disable = cdn_dp_bridge_atomic_disable,
>  	.mode_valid = cdn_dp_bridge_mode_valid,
>  	.mode_set = cdn_dp_bridge_mode_set,
> +	.hpd_notify = cdn_dp_bridge_hpd_notify,

[Severity: Medium]
Should there be an atomic_check callback here to prevent simultaneous
activation of multiple CRTCs?

Since all registered encoders share the same possible_crtcs mask for a
single underlying DP controller, userspace could potentially enable
multiple displays at once, leading to hardware register corruption.

[ ... ]
> @@ -1030,6 +1142,101 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
> +static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp)
> +{
[ ... ]
> +	for_each_of_graph_port_endpoint(port, dp_ep) {
> +		struct device_node *next_bridge_node __free(device_node) =
> +			of_graph_get_remote_port_parent(dp_ep);
> +
> +		bridge = of_drm_find_bridge(next_bridge_node);
> +		if (!bridge) {
> +			ret = -EPROBE_DEFER;
> +			goto out;
> +		}
> +
> +		dp->next_bridge_valid = true;
> +		dp->next_bridge_list[count] = drm_bridge_get(bridge);
> +		count++;
> +	}

[Severity: High]
Could this code overflow the dp->next_bridge_list[] array?

The count variable is incremented for each endpoint and used as an index,
but it is never checked against MAX_PHY.

[ ... ]
> @@ -1030,6 +1142,101 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
> +	for (i = 0; i < dp->bridge_count; i++) {
> +		dp_bridge = devm_drm_bridge_alloc(dev, struct cdn_dp_bridge, bridge,
> +						    &cdn_dp_bridge_funcs);

[Severity: High]
This is a pre-existing issue, but does using devm_ APIs inside a component
bind callback cause memory leaks and use-after-free crashes?

Component unbind does not release devm_ resources, meaning structures like
dp_bridge and the extcon notifiers remain active after unbind and will
leak on subsequent re-binds.

[ ... ]
> +	for (i = 0; i < dp->bridge_count; i++) {
> +		encoder = &dp->bridge_list[i]->encoder.encoder;
> +		bridge = &dp->bridge_list[i]->bridge;
> +		next_bridge = dp->next_bridge_list[i];
> +		ret = cdn_bridge_add(dev, bridge, next_bridge, encoder);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);

[Severity: High]
Can this sequence cause a NULL pointer dereference?

By registering the bridges via cdn_bridge_add() before calling INIT_WORK,
an early HPD event can trigger cdn_dp_bridge_hpd_notify() and attempt to
schedule the uninitialized dp->event_work queue.

[ ... ]
> +out:
> +	for (i = 0; i < MAX_PHY; i++)
> +		drm_bridge_put(dp->next_bridge_list[i]);
> +
> +	return ret;
>  }
>  
>  static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
>  {
>  	struct cdn_dp_device *dp = dev_get_drvdata(dev);
> -	struct drm_encoder *encoder = &dp->encoder.encoder;
> +	struct drm_encoder *encoder;
> +	int i;
>  
>  	cancel_work_sync(&dp->event_work);
> -	encoder->funcs->destroy(encoder);
> +	for (i = 0; i < dp->bridge_count; i++) {
> +		encoder = &dp->bridge_list[i]->encoder.encoder;
> +		encoder->funcs->destroy(encoder);
> +		drm_bridge_put(dp->next_bridge_list[i]);
> +	}
> +
> +	for (i = 0; i < MAX_PHY; i++)
> +		drm_edid_free(dp->edid_cache[i]);

[Severity: High]
Does this leave dangling pointers in persistent arrays across bind/unbind
cycles?

Because dp is allocated in probe, freeing dp->next_bridge_list and
dp->edid_cache here without setting them to NULL can lead to double-frees
or use-after-free issues if the component is rebound.

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

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