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: Implement parsing and printing HDMI 2.1 fields
Date: Mon, 25 May 2026 21:49:33 +1000
Message-ID: <review-patch3-20260520-scdc-link-health-v2-3-511af18cd64b@collabora.com>
In-Reply-To: <20260520-scdc-link-health-v2-3-511af18cd64b@collabora.com>
References: <20260520-scdc-link-health-v2-0-511af18cd64b@collabora.com> <20260520-scdc-link-health-v2-3-511af18cd64b@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 patch extends for HDMI 2.1's FRL mode, adding lane 3 support, FRL rate=
, FFE levels, and status flags 1/2. Several bugs here:

**1. BUG: Lane 3 error counter missing `SCDC_CHANNEL_VALID` check**

```c
if (num_lanes =3D=3D 4)
    counter[3] =3D buf[ERR_DET_OFF(SCDC_ERR_DET_3_L)] |
                (buf[ERR_DET_OFF(SCDC_ERR_DET_3_H)] & ~SCDC_CHANNEL_VALID) =
<< 8;
else
    counter[3] =3D 0;
```

Channels 0=E2=80=932 check `if (buf[i + 1] & SCDC_CHANNEL_VALID)` before us=
ing the counter value, setting it to 0 if invalid. Lane 3 unconditionally u=
ses the value. This should mirror the existing pattern:

```c
if (num_lanes =3D=3D 4) {
    if (buf[ERR_DET_OFF(SCDC_ERR_DET_3_H)] & SCDC_CHANNEL_VALID)
        counter[3] =3D buf[ERR_DET_OFF(SCDC_ERR_DET_3_L)] |
                    (buf[ERR_DET_OFF(SCDC_ERR_DET_3_H)] & ~SCDC_CHANNEL_VAL=
ID) << 8;
    else
        counter[3] =3D 0;
}
```

**2. BUG: Lane 3 error counters not cleared on write-back**

After reading, the loop zeroes `buf[0..5]` (channels 0=E2=80=932) and `buf[=
6]` (checksum), but `buf[7]` and `buf[8]` (lane 3) are left with their orig=
inal read values. The subsequent write-back:

```c
return drm_scdc_write(connector->ddc, SCDC_ERR_DET_0_L, buf, buf_sz);
```

writes those un-cleared values back, so lane 3 counters are never reset. Ad=
d:

```c
buf[ERR_DET_OFF(SCDC_ERR_DET_3_L)] =3D 0;
buf[ERR_DET_OFF(SCDC_ERR_DET_3_H)] =3D 0;
```

before the write-back (and after extracting the counter value).

**3. Checksum scope for 4-lane case =E2=80=94 verify against HDMI 2.1 spec**

```c
for (i =3D 0; i < buf_sz; i++)
    sum =3D wrapping_add(u8, sum, buf[i]);
```

For `buf_sz =3D 9` (4 lanes), this sums registers 0x50=E2=80=930x58 includi=
ng the lane 3 bytes. The HDMI 2.0 checksum at register 0x56 was designed to=
 make bytes 0x50=E2=80=930x56 sum to zero. If the HDMI 2.1 spec extends the=
 checksum to cover 0x50=E2=80=930x58, this is correct. If it doesn't, this =
will produce false `EPROTO` failures whenever lane 3 has non-zero error cou=
nts. Please verify this matches the HDMI 2.1 spec =E2=80=94 the code should=
 have a comment citing the relevant spec section.

**4. `drm_scdc_frl_rate_str` returns NULL in default case**

```c
default:
    return NULL;
```

This value is passed to `seq_printf` via `scdc_print_str` with `%s`. While =
the default case should be unreachable (all 4-bit values are covered by the=
 enum), returning NULL for `%s` is a latent kernel-oops-in-waiting. Return =
`"(Unknown)"` instead, or add a `WARN_ON` and return a string.

**5. `drm_scdc_read_status1_2_flags` =E2=80=94 same clear-before-read patte=
rn**

```c
ret =3D drm_scdc_writeb(connector->ddc, SCDC_UPDATE_0, SCDC_FLT_UPDATE);
```

Same concern as patch 1's `drm_scdc_read_status0_flags` =E2=80=94 the FLT_U=
PDATE flag is cleared before reading STATUS_FLAGS_1/2. For debugfs, fine; a=
s a public API, document the behavior.

**6. Minor: `__pure` on `static inline` function**

```c
static inline __pure int drm_scdc_num_frl_lanes(enum drm_scdc_frl_rate rate)
```

`__pure` on a `static inline` is redundant =E2=80=94 the compiler already h=
as full visibility of the function body. It's harmless but unnecessary.

---

**Summary**: The series needs fixes for the lane 3 error counter bugs (miss=
ing valid check, missing clear-on-writeback), removal of the inappropriate =
comment, and ideally a spec citation for the 4-lane checksum scope. The bit=
field portability concern should at least be acknowledged. The overall feat=
ure is useful and the design is sound.

---
Generated by Claude Code Patch Reviewer