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 <[email protected]> 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/[email protected]?part=5
