From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection
Date: Mon, 25 May 2026 21:00:53 +1000
Message-ID: <review-patch5-20260521032854.103-6-kernel@airkyi.com>
In-Reply-To: <20260521032854.103-6-kernel@airkyi.com>
References: <20260521032854.103-1-kernel@airkyi.com> <20260521032854.103-6-kernel@airkyi.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the largest and most complex patch in the series.

**Bug: `prev_port` used uninitialized**
In `cdn_dp_bridge_edid_read()`:
```c
struct cdn_dp_port *prev_port;
...
if (dp->bridge_count > 1 && !port->phy_enabled) {
    for (i =3D 0; i < dp->bridge_count; i++) {
        if (dp->bridge_list[i] !=3D dp_bridge && dp->bridge_list[i]->enable=
d)
            goto get_cache;
        if (dp->port[i]->phy_enabled)
            prev_port =3D dp->port[i];
    }

    /* Switch to current port */
    if (prev_port) {
        ret =3D cdn_dp_switch_port(dp, prev_port, port);
```
`prev_port` is never initialized to `NULL`. If no port has `phy_enabled` se=
t, `prev_port` contains stack garbage, and the `if (prev_port)` check passe=
s with a random non-zero value, leading to a crash in `cdn_dp_switch_port()=
`. **Fix: initialize `prev_port =3D NULL`.**

**Issue: `cdn_dp_switch_port` sets `dp->active =3D true` from EDID context**
```c
static int cdn_dp_switch_port(struct cdn_dp_device *dp, ...)
{
    if (dp->active)
        return 0;
    ...
    dp->active =3D true;
    dp->lanes =3D port->lanes;
```
This function is called from `cdn_dp_bridge_edid_read()`, which is a detect=
/probe path. Setting `dp->active =3D true` during EDID read means the subse=
quent `cdn_dp_enable()` call from `cdn_dp_bridge_atomic_enable()` will retu=
rn early (`if (dp->active) return 0`), potentially skipping important initi=
alization that normally happens during `cdn_dp_enable()` (firmware init, cl=
ock enable). This seems like it could leave the driver in an inconsistent s=
tate.

**Issue: `cdn_dp_connected_port` logic change breaks non-bridge path**
```c
for (i =3D 0; i < dp->ports; i++) {
    port =3D dp->port[i];
    lanes[i] =3D cdn_dp_get_port_lanes(port);
    if (!dp->next_bridge_valid)
        return port;
}
```
In the old code, the function returned the first port with non-zero lanes. =
Now with `!dp->next_bridge_valid`, it returns the first port **unconditiona=
lly** (even if `lanes[i]` is 0). This is a regression for the extcon path =
=E2=80=94 a port with zero lanes would now be returned as "connected". Shou=
ld be:
```c
    if (!dp->next_bridge_valid && lanes[i])
        return port;
```

**Issue: `bridge_count` vs `ports` inconsistency**
The `next_bridge_valid` path in `cdn_dp_connected_port()` iterates with `dp=
->bridge_count`:
```c
for (i =3D 0; i < dp->bridge_count; i++) {
    if (lanes[i])
        return dp->port[i];
}
```
But `lanes[]` was populated over `dp->ports` iterations. If `bridge_count !=
=3D ports`, some `lanes[]` entries could be uninitialized or we could acces=
s `dp->port[i]` out of bounds.

**Issue: `cdn_dp_bridge_detect` not updated for multi-bridge**
The `cdn_dp_bridge_detect()` function still only checks `dp->connected`, bu=
t the per-bridge `dp_bridge->connected` flag is only checked in `cdn_dp_bri=
dge_mode_valid()`. The detect callback should probably also consider the pe=
r-bridge connected state, otherwise all bridges report the same connection =
status.

**Design: EDID caching**
The EDID caching design (`edid_cache[MAX_PHY]`) is reasonable for the port-=
switching scenario. The `drm_edid_dup()`/`drm_edid_free()` calls appear cor=
rectly paired.

**Good: `devm_drm_bridge_alloc` usage**
Moving from a single bridge embedded in `cdn_dp_device` to dynamically allo=
cated `cdn_dp_bridge` structs is the right structural change.

**Cleanup needed in unbind:**
```c
for (i =3D 0; i < dp->bridge_count; i++) {
    encoder =3D &dp->bridge_list[i]->encoder.encoder;
    encoder->funcs->destroy(encoder);
    drm_bridge_put(dp->next_bridge_list[i]);
}
```
`drm_bridge_put(dp->next_bridge_list[i])` could be called with NULL if `nex=
t_bridge_list[i]` wasn't populated (e.g., extcon path where `bridge_count =
=3D 1` but `next_bridge_valid =3D false`). Should check for NULL before cal=
ling `drm_bridge_put()`.

---

**Summary:** The series has the right architectural direction but patch 5 h=
as several correctness issues that need fixing before merge, most criticall=
y the uninitialized `prev_port` and the `cdn_dp_connected_port` logic regre=
ssion. Patch 4 also has a real bug in the probe filter condition.

---
Generated by Claude Code Patch Reviewer