Claude review: drm: Send per-connector hotplug events

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**`output_poll_execute` changes:**

The core change replaces the `changed` boolean with an array of connector pointers:

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

**Concern: `num_connector` is protected by `connector_list_lock` (a spinlock), 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 `changed_conns`. In practice, `drm_connector_list_iter` uses RCU + `connector_list_lock` and could see a connector that was added after the array was sized. This is a pre-existing window in the old code too (the old code iterated the same list), but the old code didn't index into a fixed-size array — it just set a boolean. The new code writes `changed_conns[num_changed++]` without any bounds check.

The probability is extremely low in practice (connectors are rarely added at 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 = true;
    goto out;
}
```

On allocation failure, the function jumps to `out:` which checks `delayed_hp`. This means if both `delayed_hp` is true AND the allocation fails, the delayed hotplug event will still fire. That's fine. And `repoll = true` ensures we'll retry. Reasonable.

**Connector refcounting pattern:**

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

Then outside the lock:
```c
for (i = 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. The `drm_connector_put` after sending the event balances it.

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

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

Good — clarifies that this variable only covers the case where a prior delayed event was pending, not connectors found changed during this poll cycle.

**`drm_helper_hpd_irq_event` changes:**

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

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

This is a clear improvement — 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 = 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** — and arguably more so, since `drm_helper_hpd_irq_event` is called from interrupt-related paths where dynamic connector registration is more plausible.

**Return value change:**

```c
return changed;
```

The old function returned `bool` from `int changed` — so `changed >= 1` became `true`. The new code returns `unsigned int changed` implicitly 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 `kmalloc_array(`, while the `drm_helper_hpd_irq_event` version has:

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

vs:

```c
changed_conns = 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` increases between the allocation and the iteration. A simple fix would be to track 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 practice, it may be acceptable with a comment noting the assumption, but it would be cleaner to guard against it.

---
Generated by Claude Code Patch Reviewer