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: Wed, 27 May 2026 14:54:40 +1000
Message-ID: <review-patch2-20260526-hot-plug-passup-v10-2-f62351a9ea3e@collabora.com>
In-Reply-To: <20260526-hot-plug-passup-v10-2-f62351a9ea3e@collabora.com>
References: <20260526-hot-plug-passup-v10-0-f62351a9ea3e@collabora.com> <20260526-hot-plug-passup-v10-2-f62351a9ea3e@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

**Verdict: Correct, with minor observations.**

This patch modifies two functions: `output_poll_execute()` and `drm_helper_=
hpd_irq_event()`.

#### `output_poll_execute()` changes

**`!drm_kms_helper_poll` path (polling disabled):**

```c
if (!drm_kms_helper_poll) {
    if (dev->mode_config.poll_running) {
        drm_kms_helper_disable_hpd(dev);
        dev->mode_config.poll_running =3D false;
    }

    scoped_guard(mutex, &dev->mode_config.mutex) {
        changed =3D dev->mode_config.delayed_event;
        dev->mode_config.delayed_event =3D false;
    }

    if (changed)
        drm_kms_helper_hotplug_event(dev);

    return;
}
```

This is the v10 fix. Previously, `delayed_event` was read/cleared *before* =
the `!drm_kms_helper_poll` check and outside any lock, racing with the writ=
er in `drm_helper_probe_single_connector_modes()` (which sets it under `mod=
e_config.mutex`, line 568/627 of the same file). The `scoped_guard` is the =
right fix.

Note: this path falls back to `drm_kms_helper_hotplug_event(dev)` (a generi=
c, non-connector-specific event), because `delayed_event` is a bare `bool` =
that doesn't carry connector information. This is acceptable =E2=80=94 the =
polling loop isn't running here so there's no connector to identify, and a =
generic hotplug is better than none.

**`mutex_trylock` failure path:**

```c
if (!mutex_trylock(&dev->mode_config.mutex)) {
    schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
    return;
}
```

Old code set `repoll =3D true` and fell through to the `out:` label. New co=
de reschedules directly and returns. Functionally equivalent, and cleaner s=
ince it removes the `goto out` pattern.

**Connector iteration loop:**

```c
drm_sysfs_connector_hotplug_event(connector);
changed =3D true;
```

`drm_sysfs_connector_hotplug_event()` calls `kobject_uevent_env()` while `m=
ode_config.mutex` is held. This is intentional =E2=80=94 the comment at lin=
e 621-625 explains the deadlock concern is about "fb helpers" (i.e., `drm_c=
lient_dev_hotplug`), not about the sysfs uevent itself. `kobject_uevent_env=
` does a netlink broadcast which could be slow under memory pressure, but t=
hat's a latency concern, not a correctness issue.

**`delayed_event` handling after the loop:**

```c
if (dev->mode_config.delayed_event) {
    dev->mode_config.delayed_event =3D false;
    changed =3D true;
    drm_sysfs_hotplug_event(dev);
}
```

This picks up probes that happened between poll cycles (e.g., userspace sno=
oped via `drm_helper_probe_single_connector_modes`). It sends a generic (no=
n-connector-specific) uevent because the `delayed_event` bool doesn't ident=
ify which connector. If the poll loop already sent a connector-specific uev=
ent for the same connector, userspace sees both =E2=80=94 slightly redundan=
t but harmless.

**After mutex release:**

```c
if (changed)
    drm_client_dev_hotplug(dev);
```

Only `drm_client_dev_hotplug()` runs outside the lock (it takes `clientlist=
_mutex` and calls into fb helpers). This is the correct split that avoids t=
he deadlock. The uevent was already sent inside the lock.

**Minor observation:** If multiple connectors changed, `drm_client_dev_hotp=
lug(dev)` is still called only once at the end rather than per-connector. T=
his is fine =E2=80=94 the function doesn't take a connector argument and ju=
st notifies all registered DRM clients.

#### `drm_helper_hpd_irq_event()` changes

```c
if (check_connector_changed(connector)) {
    changed =3D true;
    drm_sysfs_connector_hotplug_event(connector);
}
```

The old code tracked `first_changed_connector` with a refcount get/put and =
branched on `changed =3D=3D 1` vs `changed > 0` to decide between connector=
-specific and generic hotplug. The new code is much simpler: send a connect=
or-specific uevent for *every* changed connector inside the loop. The `drm_=
connector_get`/`drm_connector_put` dance is gone since we no longer need to=
 stash a connector reference across the lock boundary.

```c
if (changed)
    drm_client_dev_hotplug(dev);
```

Again, client hotplug is deferred until after `mode_config.mutex` is releas=
ed. Correct.

**Return type change:** `int changed` =E2=86=92 `bool changed`. The functio=
n returns `bool`, so this is a cleanup. Note that `first_changed_connector`=
 is gone, which also removes the intermediate `drm_connector_get()` ref =E2=
=80=94 no leak risk.

#### Overall assessment

No correctness bugs found. The locking discipline is sound. The split of "s=
ysfs uevent under lock / client hotplug after lock" is well-justified by th=
e existing deadlock comment and the fb helper lock ordering. The `!drm_kms_=
helper_poll` path fix in v10 (reading `delayed_event` under the mutex) addr=
esses a real race. The code is simpler than what it replaces.

---
Generated by Claude Code Patch Reviewer