[PATCH v3 5/5] drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Fri Jul 17 00:38:18 PDT 2026
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 at 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
More information about the linux-phy
mailing list