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