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: Send per-connector hotplug events
Date: Thu, 23 Apr 2026 07:36:46 +1000
Message-ID: <review-patch2-20260422-hot-plug-passup-v8-2-5cfae6ba4119@collabora.com>
In-Reply-To: <20260422-hot-plug-passup-v8-2-5cfae6ba4119@collabora.com>
References: <20260422-hot-plug-passup-v8-0-5cfae6ba4119@collabora.com> <20260422-hot-plug-passup-v8-2-5cfae6ba4119@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

**`output_poll_execute` changes:**

The core change replaces the `changed` boolean with an array of connector p=
ointers:

```c
changed_conns =3D kmalloc_array(dev->mode_config.num_connector,
                              sizeof(*changed_conns), GFP_KERNEL);
```

**Concern: `num_connector` is protected by `connector_list_lock` (a spinloc=
k), not by `mode_config.mutex`.** The allocation uses `num_connector` read =
under `mode_config.mutex` (the `mutex_trylock` above), but `num_connector` =
is modified under `connector_list_lock`. A connector could be added between=
 the `kmalloc_array` and the iteration, causing a write past the end of `ch=
anged_conns`. In practice, `drm_connector_list_iter` uses RCU + `connector_=
list_lock` and could see a connector that was added after the array was siz=
ed. This is a pre-existing window in the old code too (the old code iterate=
d the same list), but the old code didn't index into a fixed-size array =E2=
=80=94 it just set a boolean. The new code writes `changed_conns[num_change=
d++]` without any bounds check.

The probability is extremely low in practice (connectors are rarely added a=
t runtime during polling), but for correctness, either:
- Read `num_connector` under `connector_list_lock`, or
- Add a bounds check `if (num_changed < array_size)` before storing, or
- Use a dynamically-growing list.

This is the most significant issue in the series.

**`kmalloc_array` failure handling:**

```c
if (!changed_conns) {
    repoll =3D true;
    goto out;
}
```

On allocation failure, the function jumps to `out:` which checks `delayed_h=
p`. This means if both `delayed_hp` is true AND the allocation fails, the d=
elayed hotplug event will still fire. That's fine. And `repoll =3D true` en=
sures we'll retry. Reasonable.

**Connector refcounting pattern:**

```c
drm_connector_get(connector);
changed_conns[num_changed++] =3D connector;
```

Then outside the lock:
```c
for (i =3D 0; i < num_changed; i++) {
    drm_kms_helper_connector_hotplug_event(changed_conns[i]);
    drm_connector_put(changed_conns[i]);
}
```

This is correct. The `drm_connector_get` inside the iter loop (which itself=
 holds a reference) ensures the connector survives past the lock release. T=
he `drm_connector_put` after sending the event balances it.

**Rename `changed` to `delayed_hp`:**

```c
delayed_hp =3D dev->mode_config.delayed_event;
```

Good =E2=80=94 clarifies that this variable only covers the case where a pr=
ior delayed event was pending, not connectors found changed during this pol=
l cycle.

**`drm_helper_hpd_irq_event` changes:**

The old code had special handling: if exactly 1 connector changed, send a p=
er-connector event; if >1 changed, send a global event. The new code always=
 sends per-connector events:

```c
for (i =3D 0; i < changed; i++) {
    drm_kms_helper_connector_hotplug_event(changed_conns[i]);
    drm_connector_put(changed_conns[i]);
}
```

This is a clear improvement =E2=80=94 every changed connector gets its own =
per-connector event regardless of count.

**Use of `scoped_guard`:**

```c
scoped_guard(mutex, &dev->mode_config.mutex) {
    changed_conns =3D kmalloc_array(...);
    ...
    drm_connector_list_iter_end(&conn_iter);
}
```

Clean use of `scoped_guard`. The `kmalloc_array` is called under mutex with=
 `GFP_KERNEL`, which is fine (the mutex is not a spinlock).

**Same `num_connector` race concern applies here** =E2=80=94 and arguably m=
ore so, since `drm_helper_hpd_irq_event` is called from interrupt-related p=
aths where dynamic connector registration is more plausible.

**Return value change:**

```c
return changed;
```

The old function returned `bool` from `int changed` =E2=80=94 so `changed >=
=3D 1` became `true`. The new code returns `unsigned int changed` implicitl=
y converted to `bool`, which works the same way. No issue.

**Minor nit:** The `kmalloc_array` indentation in both functions uses a tab=
 for the second line of args (`sizeof(*changed_conns), GFP_KERNEL`), which =
should align with the first argument per kernel style. Looking at the diff =
context, the `output_poll_execute` version aligns to the tab stop after `km=
alloc_array(`, while the `drm_helper_hpd_irq_event` version has:

```c
changed_conns =3D kmalloc_array(dev->mode_config.num_connector,
                              sizeof(*changed_conns), GFP_KERNEL);
```

vs:

```c
changed_conns =3D kmalloc_array(dev->mode_config.num_connector,
			sizeof(*changed_conns), GFP_KERNEL);
```

The latter (in `output_poll_execute`) uses tabs which doesn't align to the =
opening paren. The `drm_helper_hpd_irq_event` version looks better-aligned =
but both are within the range of acceptable kernel style.

**Summary for Patch 2:** The design is sound and the code is mostly correct=
. The main concern is the theoretical array overflow if `num_connector` inc=
reases between the allocation and the iteration. A simple fix would be to t=
rack the allocated size and add a bounds check, or to read `num_connector` =
under the appropriate lock. Given this is v8 and the window is tiny in prac=
tice, it may be acceptable with a comment noting the assumption, but it wou=
ld be cleaner to guard against it.

---
Generated by Claude Code Patch Reviewer