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/scdc-helper: Add scdc_status debugfs entry
Date: Fri, 05 Jun 2026 06:21:28 +1000
Message-ID: <review-patch2-20260604-scdc-link-health-v5-2-11173b0ac3de@collabora.com>
In-Reply-To: <20260604-scdc-link-health-v5-2-11173b0ac3de@collabora.com>
References: <20260604-scdc-link-health-v5-0-11173b0ac3de@collabora.com> <20260604-scdc-link-health-v5-2-11173b0ac3de@collabora.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 core patch and has a few issues:

**Bug: Off-by-one in SCDC read misses register 0xFF.**

```c
ret =3D drm_scdc_read(ddc, 0, buf, 128);
if (ret)
    return ret;

ret =3D drm_scdc_read(ddc, 127, &buf[127], 128);
```

The first read covers registers `0x00=E2=80=930x7F`. The second reads from =
offset 127 (`0x7F`) for 128 bytes, covering `0x7F=E2=80=930xFE`. Register `=
0xFF` is never read, and byte 127 is read twice (second overwrites first). =
The doc comment says "Reads the entire 256 byte SCDC state" but it only rea=
ds 255 registers. The hex dump then prints all 256 bytes including the unre=
ad byte 255 (which will be stale or zero).

The fix should be:
```c
ret =3D drm_scdc_read(ddc, 128, &buf[128], 128);
```

**Issue: `drm_connector_put()` placed mid-function on success path.**

```c
scdc_print_flag(m, "Low Rate Scrambling Supported", scdc->scrambling.low_ra=
tes);

drm_connector_put(connector);  // <-- refcount dropped here

scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
// ... 10 more lines accessing st-> ...

return 0;

err_conn_put:
    drm_connector_put(connector);
    return ret;
```

The refcount is technically balanced (one get, one put per path), and the c=
ode after the put only accesses `st` (which lives in `priv`, not the connec=
tor), so it's memory-safe. But dropping the connector ref before the functi=
on completes is an unusual pattern. If future changes access `connector` or=
 `scdc` below this point, they'd hit a use-after-potential-free. Consider m=
oving the `put` to just before `return 0`, or restructuring with a single e=
xit path.

**Minor: Typo in kdoc.**

```c
/** @scramling_enabled: true if TMDS scrambling is on */
bool scrambling_enabled;
```

Should be `@scrambling_enabled` (missing 'b').

**Minor: `drm_scdc_status_flags` struct fields have no kdoc.** The `drm_scd=
c_state` members are documented, but the nested `drm_scdc_status_flags` fie=
lds are not. Since these structs are in a public header and `drm_scdc_read_=
state` is exported, adding kdoc would be good practice.

---

---
Generated by Claude Code Patch Reviewer