Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] drm/rockchip: cdn-dp: unguarded cdn_dp_config_video() in cdn_dp_switch_port() - [High] drm/rockchip: cdn-dp: missing PHY switch in cdn_dp_bridge_atomic_enable() - [High] drm/rockchip: cdn-dp: encoder use-after-free on cdn_dp_bind() failure - [High] drm/rockchip: cdn-dp: missing per-bridge check in cdn_dp_bridge_detect() - [High] drm/rockchip: cdn-dp: ignored bridge HPD state in cdn_dp_connected_port() - [Medium] drm/rockchip: cdn-dp: race on dp->active_port in cdn_dp_connected_port() --
commit ecc80bdc2aa1d377f4a7cb763c9aafe0d00f7b9f Author: Chaoyi Chen <[email protected]> drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection This commit adds support for registering multiple DRM bridges and encoders in the Rockchip CDN-DP driver so that users can select between multiple PHY ports using the DRM AUX HPD bridge infrastructure. > diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c > b/drivers/gpu/drm/rockchip/cdn-dp-core.c > index 1c1c1116b5ebe..4067802904250 100644 > --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c > +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c [ ... ] > @@ -191,14 +192,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 && lanes[i]) > return port; > } > + > + 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]; [Severity: Medium] Can dp->active_port race here when cdn_dp_connected_port() is called without holding dp->lock? In cdn_dp_bridge_atomic_disable(), dp->lock is released before calling cdn_dp_connected_port(dp): drivers/gpu/drm/rockchip/cdn-dp-core.c:cdn_dp_bridge_atomic_disable() { ... mutex_unlock(&dp->lock); ... if (!dp->connected && cdn_dp_connected_port(dp)) schedule_work(&dp->event_work); } Concurrently, cdn_dp_pd_event_work(), cdn_dp_bridge_edid_read() -> cdn_dp_switch_port(), or cdn_dp_bridge_atomic_enable() can update dp->active_port under dp->lock, including setting dp->active_port = -1 in cdn_dp_disable_phy(). If dp->active_port is set to -1 right after the dp->active_port >= 0 check, won't lanes[dp->active_port] or dp->port[dp->active_port] read out of bounds at index -1? > + > + /* 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]; > + } > + } > + > return NULL; > } [Severity: High] When dp->next_bridge_valid is true, cdn_dp_bridge_hpd_notify() records the per-port HPD state in dp_bridge->connected, but cdn_dp_connected_port(), cdn_dp_enable(), and cdn_dp_check_sink_connection() never inspect dp->bridge_list[i]->connected. Instead, cdn_dp_connected_port() checks cdn_dp_get_port_lanes(port), which calls phy_get_bus_width(port->phy) when port->extcon is NULL. If the PHY bus width remains non-zero while HPD is deasserted (for example, with a Type-C dock or adapter in DP altmode with no monitor plugged in, or before altmode teardown completes), won't cdn_dp_connected_port() treat the disconnected port as connected? Even if lanes[0] becomes 0 when active port 0 is unplugged while port 1 remains connected (with dp->active == true and dp->active_port == 0), cdn_dp_pd_event_work() sees cdn_dp_connected_port(dp) return port 1 and takes the else if (!cdn_dp_check_sink_connection(dp)) branch: drivers/gpu/drm/rockchip/cdn-dp-core.c:cdn_dp_pd_event_work() { ... if (!cdn_dp_connected_port(dp)) { ... } else if (!dp->active) { ... } else if (!cdn_dp_check_sink_connection(dp)) { DRM_DEV_INFO(dp->dev, "Connected without sink; assert hpd\n"); dp->connected = false; ... } Because cdn_dp_check_sink_connection() checks dp->port[dp->active_port] (the unplugged port 0) and has no !dp_bridge->connected early exit when port->extcon is NULL, won't it poll DPCD for CDN_DPCD_TIMEOUT_MS (5 seconds) and then set dp->connected = false instead of switching to port 1, leaving port 1 unusable when no bridge was enabled in KMS to trigger cdn_dp_bridge_atomic_disable()? [ ... ] > @@ -550,6 +598,50 @@ static bool cdn_dp_check_link_status(struct > cdn_dp_device *dp) > return drm_dp_channel_eq_ok(link_status, min(port->lanes, sink_lanes)); > } > > +static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port > *prev_port, > + struct cdn_dp_port *port) > +{ > + int ret; > + > + ret = cdn_dp_disable_phy(dp, prev_port); > + if (ret) > + goto out; > + ret = cdn_dp_enable_phy(dp, port); > + if (ret) > + goto out; > + > + ret = cdn_dp_get_sink_capability(dp); > + if (ret) { > + cdn_dp_disable_phy(dp, port); > + goto out; > + } > + > + dp->lanes = port->lanes; > + > + 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); > + } > + > +out: > + return ret; > +} [Severity: High] Does cdn_dp_switch_port() always fail with -EINVAL when called from cdn_dp_bridge_edid_read() before the first modeset commit? cdn_dp_bridge_edid_read() only calls cdn_dp_switch_port() when no bridge is currently enabled. Before the first modeset (cdn_dp_bridge_mode_set() and cdn_dp_display_info_update()), dp->mode.clock and dp->video_info.color_depth are both 0. When switching to the new port, cdn_dp_check_link_status(dp) returns false and cdn_dp_switch_port() unconditionally calls cdn_dp_config_video(dp) (unlike cdn_dp_pd_event_work(), which checks mode->clock first). In cdn_dp_config_video(): drivers/gpu/drm/rockchip/cdn-dp-reg.c:cdn_dp_config_video() { ... do { tu_size_reg += 2; symbol = (u64)tu_size_reg * mode->clock * bit_per_pix; do_div(symbol, dp->max_lanes * link_rate * 8); rem = do_div(symbol, 1000); if (tu_size_reg > 64) { ret = -EINVAL; DRM_DEV_ERROR(dp->dev, "tu error, clk:%d, lanes:%d, rate:%d\n", mode->clock, dp->max_lanes, link_rate); goto err_config_video; } } while ((symbol <= 1) || (tu_size_reg - symbol < 4) || (rem > 850) || (rem < 100)); } With mode->clock == 0, symbol is 0 on every iteration until tu_size_reg > 64, returning -EINVAL. In cdn_dp_bridge_edid_read(), that error causes a jump to get_cache, skipping drm_edid_read_custom() and returning NULL when edid_cache is empty. Also, if cdn_dp_enable_phy(dp, port) or cdn_dp_get_sink_capability(dp) fails after cdn_dp_disable_phy(dp, prev_port) has already disabled prev_port, prev_port is not re-enabled. Doesn't that leave all ports with phy_enabled == false and dp->active_port == -1 while dp->active remains true, so subsequent calls to cdn_dp_bridge_edid_read() find prev_port == NULL and attempt to read EDID with all PHYs powered off? [ ... ] > @@ -599,6 +692,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", > @@ -631,6 +727,7 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge > *bridge, struct drm_at > goto out; > } > > + dp_bridge->enabled = true; > out: > mutex_unlock(&dp->lock); > } [Severity: High] How does cdn_dp_bridge_atomic_enable() switch PHYs when dp->active is already true on the other port? When a sink is connected, cdn_dp_pd_event_work() calls cdn_dp_enable(dp), which enables one PHY port and sets dp->active = true. Similarly, probing connectors via cdn_dp_bridge_edid_read() can leave the other port's PHY enabled while dp->active is true. When cdn_dp_bridge_atomic_enable() then runs for dp_bridge, it sets dp->active_port = dp_bridge->id and calls cdn_dp_enable(dp), which returns immediately when dp->active is true: drivers/gpu/drm/rockchip/cdn-dp-core.c:cdn_dp_enable() { ... if (dp->active) return 0; ... } Doesn't this leave the previously enabled PHY powered on and dp->port[dp_bridge->id]->phy_enabled false, causing link training and video output in cdn_dp_bridge_atomic_enable() to run against the wrong PHY? In addition, since each bridge registers a separate drm_encoder with the same possible_crtcs and there is no atomic_check preventing both bridges from being enabled at the same time, what happens if userspace enables both bridges and then disables one in cdn_dp_bridge_atomic_disable(), which calls cdn_dp_disable(dp) and shuts down the entire controller? [ ... ] > @@ -947,10 +1062,12 @@ static void cdn_dp_pd_event_work(struct work_struct > *work) > } > > out: > + notify = last_connected != dp->connected; > 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++) { > + if (notify) > + > drm_kms_helper_connector_hotplug_event(dp->bridge_list[i]->connector); > + } > } [Severity: High] Should cdn_dp_bridge_detect() also check dp_bridge->connected, similar to cdn_dp_bridge_mode_valid()? Currently cdn_dp_bridge_detect() only checks the controller-wide dp->connected flag: drivers/gpu/drm/rockchip/cdn-dp-core.c:cdn_dp_bridge_detect() { struct cdn_dp_device *dp = bridge_to_dp(bridge); enum drm_connector_status status = connector_status_disconnected; mutex_lock(&dp->lock); if (dp->connected) status = connector_status_connected; mutex_unlock(&dp->lock); return status; } Because each bridge sets DRM_BRIDGE_OP_DETECT, drm_bridge_connector_init() selects cdn_dp_bridge as bridge_detect for each connector. When port 0 is connected (dp->connected == true) and port 1 is disconnected (bridge_list[1]->connected == false), cdn_dp_pd_event_work() calls drm_kms_helper_connector_hotplug_event() on all connectors. When drm_bridge_connector_detect_ctx() probes the disconnected port 1 connector: drivers/gpu/drm/display/drm_bridge_connector.c:drm_bridge_connector_detect_ctx() { ... if (detect) { status = detect->funcs->detect(detect, connector); ... drm_bridge_connector_hpd_notify(connector, status); } ... } cdn_dp_bridge_detect() returns connector_status_connected for port 1, and drm_bridge_connector_hpd_notify() immediately calls cdn_dp_bridge_hpd_notify() on bridge_list[1] with connector_status_connected, overwriting bridge_list[1]->connected = true and causing port 1 to attempt an EDID read (switching PHYs if port 0 is not yet enabled). Conversely, when a port first signals HPD connect via drm_bridge_connector_handle_hpd(), cdn_dp_bridge_hpd_notify() sets dp_bridge->connected = true and schedules dp->event_work, while drm_bridge_connector_handle_hpd() immediately triggers connector probing. If drm_bridge_connector_detect_ctx() runs before dp->event_work sets dp->connected = true, won't cdn_dp_bridge_detect() return connector_status_disconnected and overwrite dp_bridge->connected back to false via drm_bridge_connector_hpd_notify()? > > static int cdn_dp_pd_event(struct notifier_block *nb, > @@ -970,28 +1087,16 @@ static int cdn_dp_pd_event(struct notifier_block *nb, > return NOTIFY_DONE; > } > > -static int cdn_dp_bind(struct device *dev, struct device *master, void *data) > +static int cdn_bridge_add(struct device *dev, > + struct drm_bridge *bridge, > + struct drm_bridge *next_bridge, > + struct drm_encoder *encoder) > { > struct cdn_dp_device *dp = dev_get_drvdata(dev); > - struct drm_encoder *encoder; > + struct drm_device *drm_dev = dp->drm_dev; > + struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL; > struct drm_connector *connector; > - struct cdn_dp_port *port; > - struct drm_device *drm_dev = data; > - int ret, i; > - > - ret = cdn_dp_parse_dt(dp); > - if (ret < 0) > - return ret; > - > - dp->drm_dev = drm_dev; > - dp->connected = false; > - dp->active = false; > - dp->active_port = -1; > - dp->fw_loaded = false; > - > - INIT_WORK(&dp->event_work, cdn_dp_pd_event_work); > - > - encoder = &dp->encoder.encoder; > + int ret; > > encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev, > dev->of_node); [ ... ] > @@ -1033,6 +1147,108 @@ static int cdn_dp_bind(struct device *dev, struct > device *master, void *data) > return ret; > } > > + if (last_bridge) { > + fwnode_handle_put(connector->fwnode); > + connector->fwnode = > fwnode_handle_get(of_fwnode_handle(last_bridge->of_node)); > + } > + > + bridge_to_dp_bridge(bridge)->connector = connector; > + > + return 0; > +} [ ... ] > +static int cdn_dp_bind(struct device *dev, struct device *master, void *data) > +{ > + struct cdn_dp_device *dp = dev_get_drvdata(dev); > + struct drm_bridge *bridge, *next_bridge; > + struct drm_encoder *encoder; > + struct cdn_dp_port *port; > + struct drm_device *drm_dev = data; > + struct cdn_dp_bridge *dp_bridge; > + int ret, i; > + > + ret = cdn_dp_parse_dt(dp); > + if (ret < 0) > + return ret; > + > + ret = cdn_dp_parse_next_bridge_dt(dp); > + if (ret) > + goto out; > + > + dp->drm_dev = drm_dev; > + dp->connected = false; > + dp->active = false; > + dp->active_port = -1; > + dp->fw_loaded = false; > + > + for (i = 0; i < dp->bridge_count; i++) { > + dp_bridge = devm_drm_bridge_alloc(dev, struct cdn_dp_bridge, > bridge, > + &cdn_dp_bridge_funcs); > + if (IS_ERR(dp_bridge)) { > + ret = PTR_ERR(dp_bridge); > + goto out; > + } > + dp_bridge->id = i; > + dp_bridge->parent = dp; > + if (!dp->next_bridge_valid) > + dp_bridge->connected = true; > + dp->bridge_list[i] = dp_bridge; > + } > + > + 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); > + > for (i = 0; i < dp->ports; i++) { > port = dp->port[i]; > > @@ -1046,7 +1262,7 @@ static int cdn_dp_bind(struct device *dev, struct > device *master, void *data) > if (ret) { > DRM_DEV_ERROR(dev, > "register EXTCON_DISP_DP notifier err\n"); > - return ret; > + goto out; > } > } > > @@ -1055,15 +1271,31 @@ static int cdn_dp_bind(struct device *dev, struct > device *master, void *data) > schedule_work(&dp->event_work); > > return 0; > + > +out: > + for (i = 0; i < MAX_PHY; i++) > + drm_bridge_clear_and_put(&dp->next_bridge_list[i]); > + > + return ret; > } [Severity: High] Can a failure in cdn_dp_bind() cause a use-after-free on encoder? In this patch, struct rockchip_encoder encoder is embedded inside struct cdn_dp_bridge, which is allocated with devm_drm_bridge_alloc() inside cdn_dp_bind() (under component_bind()'s devres group). In cdn_bridge_add(), drm_encoder_init() adds &dp_bridge->encoder.encoder to drm_dev->mode_config.encoder_list. If a subsequent step in cdn_bridge_add() or cdn_dp_bind() fails (for example, drm_bridge_attach() on next_bridge, drm_bridge_connector_init(), the second iteration of cdn_bridge_add(), or devm_extcon_register_notifier()), the error path at out: does not destroy any already-initialized encoders. When component_bind() then releases its devres group, the devm reference on dp_bridge is dropped, freeing dp_bridge (either immediately if drm_bridge_attach() on dp_bridge->bridge did not run, or during drm_encoder_cleanup() -> drm_bridge_detach() when drm_mode_config_cleanup() unwinds) while drm_encoder_cleanup() still accesses the embedded encoder. Should the error path clean up any initialized encoders (or use drmm_encoder_alloc() / separate the encoder allocation from the bridge container)? > > static void cdn_dp_unbind(struct device *dev, struct device *master, void > *data) [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
