Claude review: drm/connector: Fix epoch_counter docs to reflect reality

Claude review: drm/connector: Fix epoch_counter docs to reflect reality

[Claude Code Review Bot]

Patch Review

**Verdict: Looks good.**

This is a straightforward documentation fix. The old comment:
```c
/** @epoch_counter: used to detect any other changes in connector, besides status */
```
is replaced with:
```c
/**
 * @epoch_counter: Used to detect changes in connector. Increased when
 * the connector, including its status, is changed.
 */
```

The fix is correct. The `epoch_counter` is bumped in `drm_connector_set_status_locked()` (among other places), so it very much does track status changes. The Fixes tag referencing the original commit `5186421cbfe2` is appropriate since the doc was wrong from the start.

No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/connector: Fix epoch_counter docs to reflect reality

[Claude Code Review Bot]

Patch Review

**Reviewed-by worthy.** This is a straightforward documentation fix.

The old doc says:
```c
/** @epoch_counter: used to detect any other changes in connector, besides status */
```

The new doc says:
```c
/**
 * @epoch_counter: Used to detect changes in connector. Increased when
 * the connector, including its status, is changed.
 */
```

This is correct. Looking at the code in `drm_probe_helper.c`, `check_connector_changed()` calls `drm_helper_probe_detect()` which updates the epoch counter when status changes, so the old "besides status" wording was wrong since introduction. The `Fixes:` tag pointing to the original commit is appropriate for a doc fix.

No issues.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v10 0/2] Pass down hot-plug CONNECTOR ID to user-space

[Nicolas Frattaroli]

This series addresses a shortcoming whereby a hot plug event is sent
without it being passed the actual connector that caused it. This takes
into consideration both the polling path and the HPD (Hot Plug Detect)
path.

The motivation is that user-space applications such as Weston would
previously receive non-connector-specific hotplug events, and then have
to figure out themselves which connector needs to have a modeset
executed on. This notably did not work when the hotplug events came in
too fast, resulting in Weston missing an on-off-on transition of a
connector, seeing that its state was unchanged from "on" so can't be the
one that was hotplugged, and skipping reinitialising it as it looks
through the other connectors that could've caused it.

The real world implication is that on setups with slightly sketchy HDMI
connections, a brief flicker in the HPD signal could result in video
output bidding farewell entirely until a manual proper re-plug was
performed.

By sending connector specific hotplug events, this ambiguity is resolved
without any change to the user-space API. Future work should however
make the kernel explicitly request a modeset from userspace through a
more robust path, either by using link-status (nb. the userspace side
may not handle it properly at present) or by exposing the epoch counter
through the uAPI such that fragile state comparison code is no longer
needed.

This change results in potentially more hotplug events being sent than
previously, as n changed connectors at a time result in n hotplug
events, rather than just a single one.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Changes in v10:
- Fix delayed_event processing in !drm_kms_helper_poll path. This
  necessitates taking the lock in that path for reading delayed_event as
  well. I chose not to transition delayed_event to an atomic_t yet, as I
  don't want to rock the boat too much in this series.
- Move delayed_event read/clear inside of the mode_config.mutex locked
  section.
- Link to v9: https://patch.msgid.link/20260422-hot-plug-passup-v9-0-aef804255986@collabora.com

Changes in v9:
- Get rid of the kmalloc_array by splitting the hotplug event into its
  two constituent parts to avoid the deadlock.
- Link to v8: https://patch.msgid.link/20260422-hot-plug-passup-v8-0-5cfae6ba4119@collabora.com

Changes in v8:
- Drop pending_hp patch
- Add patch correcting epoch_counter documentation
- Rework implementation such that hotplug events are still sent outside
  the mode_config mutex, but don't iterate through the connector list
  without the lock.
- Link to v7: https://patch.msgid.link/20260415-hot-plug-passup-v7-0-9a27ef5e2428@collabora.com

Changes in v7 RESEND:
- None, other than removing the leftover diffstat from the cover letter
- Link to v7: https://lore.kernel.org/r/20260217-hot-plug-passup-v7-0-f8221b2aab51@collabora.com

Changes in v7:
- Drop the two vkms patches, as I don't want them to be blocked on
  review. I still think they're correct, but they're not essential and
  don't need to block this series.
- Link to v6: https://lore.kernel.org/r/20260123-hot-plug-passup-v6-0-aaaf61d960bb@collabora.com

Changes in v6:
- Rewrote cover letter to explain the motivation for this series more
  plainly
- Rename "status_changed" to "pending_hp"
- Set "pending_hp" in the existing path that would also affect
  epoch_counter
- No longer set the boolean in drm_helper_probe_single_connector_modes,
  as it does not appear to be necessary
- Reword commits to better justify the changes
- Link to v5: https://lore.kernel.org/r/20251111162338.15141-1-marius.vlad@collabora.com/

Changes in v5:
- vkms: add support for sending the CONNECTOR ID when hot-plugging through
  ConfigFS - as reported by Louis, vkms can now make use of ConfigFS to
  simulate connector status.
- vkms: add a small change to ignore previous/old drm connector status
  when sending out hot-plug uevent.
- Link to v4: https://lore.kernel.org/r/20251103174558.7709-1-marius.vlad@collabora.com/

Changes in v4:
- removed the "This patch" bit - Dmitry
- added a short note when the flag is set and cleared - Dmitry
- address double dead-locking detected - kbot: https://lore.kernel.org/dri-devel/202509251410.fdfbcac3-lkp@intel.com/
- virtual connectors do not seem have any kind of hotplug - added
  polling in vkms - as noted by Ian
- Link to v3: https://lore.kernel.org/r/20250923083636.4749-1-marius.vlad@collabora.com/

Changes in v3:
- Address comments from Dmitry:
  - guard connector status write with mode_config.mutex
  - avoid setting up the connector status and immediately unset it. Do the
    unset in drm_kms_helper_hotplug_event/drm_kms_helper_connector_hotplug_event
- Link to v2: https://lore.kernel.org/r/20250729165708.9947-1-marius.vlad@collabora.com/

Changes in v2:
- Address comments from Daniel:
  - split patch into 2, one that introduces a bool to track connector
    connection status change and a patch that uses that to be able to send
    hot plug events with the proper CONNECTOR ID to udev and further pass
    that down to user-space
  - nuke out mutex when iterating connector list
  - fix typo
- Link to v1: https://lore.kernel.org/r/20250627131751.2004-1-marius.vlad@collabora.com/

---
Nicolas Frattaroli (2):
      drm/connector: Fix epoch_counter docs to reflect reality
      drm: Send per-connector hotplug events

 drivers/gpu/drm/drm_probe_helper.c | 53 ++++++++++++++++++++------------------
 include/drm/drm_connector.h        |  5 +++-
 2 files changed, 32 insertions(+), 26 deletions(-)
---
base-commit: 5fb5a9a63cf5ece68e0eeb6fa397da27712bccf0
change-id: 20260121-hot-plug-passup-f8ed03f7c202

Best regards,
--  
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH v10 1/2] drm/connector: Fix epoch_counter docs to reflect reality

[Nicolas Frattaroli]

Since the very day epoch_counter in drm_connector was introduced, its
documentation was not accurate. It claims it's used to detect "any other
changes [...] besides status", when in reality, it's used to detect
changes including status, as a status change also increases the epoch
counter.

Adjust the documentation to rectify this discrepancy.

Fixes: 5186421cbfe2 ("drm: Introduce epoch counter to drm_connector")
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 include/drm/drm_connector.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 5ad62c207d00..abd58e98e4aa 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2264,7 +2264,10 @@ struct drm_connector {
 	 */
 	struct mutex edid_override_mutex;
 
-	/** @epoch_counter: used to detect any other changes in connector, besides status */
+	/**
+	 * @epoch_counter: Used to detect changes in connector. Increased when
+	 * the connector, including its status, is changed.
+	 */
 	u64 epoch_counter;
 
 	/**

-- 
2.54.0

[PATCH v10 2/2] drm: Send per-connector hotplug events

[Nicolas Frattaroli]

Try to send per-connector hotplug events as often as possible, rather
than connector-less global hotplug events. This does result in more
hotplug events if multiple connectors changed at the same time, but
give userspace more actionable information.

Since the hotplug event needs to be sent outside of the mode_config
mutex to avoid a deadlock, the drm_client_dev_hotplug() call is split
off from the drm_sysfs_(connector_)?hotplug_event calls.

Co-developed-by: Marius Vlad <marius.vlad@collabora.com>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/drm_probe_helper.c | 53 ++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index d4dc8cb45bce..e26ead5b5e58 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -760,27 +760,32 @@ static void output_poll_execute(struct work_struct *work)
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	enum drm_connector_status old_status;
-	bool repoll = false, changed;
+	bool repoll = false, changed = false;
 	u64 old_epoch_counter;
 
 	if (!dev->mode_config.poll_enabled)
 		return;
 
-	/* Pick up any changes detected by the probe functions. */
-	changed = dev->mode_config.delayed_event;
-	dev->mode_config.delayed_event = false;
-
 	if (!drm_kms_helper_poll) {
 		if (dev->mode_config.poll_running) {
 			drm_kms_helper_disable_hpd(dev);
 			dev->mode_config.poll_running = false;
 		}
-		goto out;
+
+		scoped_guard(mutex, &dev->mode_config.mutex) {
+			changed = dev->mode_config.delayed_event;
+			dev->mode_config.delayed_event = false;
+		}
+
+		if (changed)
+			drm_kms_helper_hotplug_event(dev);
+
+		return;
 	}
 
 	if (!mutex_trylock(&dev->mode_config.mutex)) {
-		repoll = true;
-		goto out;
+		schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
+		return;
 	}
 
 	drm_connector_list_iter_begin(dev, &conn_iter);
@@ -836,16 +841,23 @@ static void output_poll_execute(struct work_struct *work)
 				    connector->base.id, connector->name,
 				    old_epoch_counter, connector->epoch_counter);
 
+			drm_sysfs_connector_hotplug_event(connector);
 			changed = true;
 		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
 
+	/* Pick up any changes detected by the probe functions. */
+	if (dev->mode_config.delayed_event) {
+		dev->mode_config.delayed_event = false;
+		changed = true;
+		drm_sysfs_hotplug_event(dev);
+	}
+
 	mutex_unlock(&dev->mode_config.mutex);
 
-out:
 	if (changed)
-		drm_kms_helper_hotplug_event(dev);
+		drm_client_dev_hotplug(dev);
 
 	if (repoll)
 		schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
@@ -1081,9 +1093,9 @@ EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event);
  */
 bool drm_helper_hpd_irq_event(struct drm_device *dev)
 {
-	struct drm_connector *connector, *first_changed_connector = NULL;
 	struct drm_connector_list_iter conn_iter;
-	int changed = 0;
+	struct drm_connector *connector;
+	bool changed = false;
 
 	if (!dev->mode_config.poll_enabled)
 		return false;
@@ -1096,24 +1108,15 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 			continue;
 
 		if (check_connector_changed(connector)) {
-			if (!first_changed_connector) {
-				drm_connector_get(connector);
-				first_changed_connector = connector;
-			}
-
-			changed++;
+			changed = true;
+			drm_sysfs_connector_hotplug_event(connector);
 		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
 	mutex_unlock(&dev->mode_config.mutex);
 
-	if (changed == 1)
-		drm_kms_helper_connector_hotplug_event(first_changed_connector);
-	else if (changed > 0)
-		drm_kms_helper_hotplug_event(dev);
-
-	if (first_changed_connector)
-		drm_connector_put(first_changed_connector);
+	if (changed)
+		drm_client_dev_hotplug(dev);
 
 	return changed;
 }

-- 
2.54.0

Claude review: Pass down hot-plug CONNECTOR ID to user-space

[Claude Code Review Bot]

Overall Series Review

Subject: Pass down hot-plug CONNECTOR ID to user-space
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Patches: 3
Reviewed: 2026-05-27T14:54:39.574822

---

This is a well-motivated 2-patch series (v10) that passes connector IDs in hotplug uevents to userspace. The real-world problem is clear: compositors like Weston receiving generic hotplug events can miss fast on-off-on transitions on a connector, leaving it dead until manually replugged. The design is sound — the sysfs uevent (`kobject_uevent_env`) is sent inside the `mode_config.mutex` while iterating connectors, and `drm_client_dev_hotplug()` (which calls into fb helpers that may re-acquire the mutex) is deferred until after the lock is released. This avoids the AB-BA deadlock that plagued earlier versions.

The series is at a good level of maturity after 10 revisions and has a Reviewed-by from Daniel Stone on patch 1. No correctness bugs found. A few minor observations follow.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/connector: Fix epoch_counter docs to reflect reality

[Claude Code Review Bot]

Patch Review

**Verdict: Looks good.**

Trivial documentation fix. The original kerneldoc for `epoch_counter` said:

```c
/** @epoch_counter: used to detect any other changes in connector, besides status */
```

But `epoch_counter` is also bumped on status changes (confirmed in `drm_probe_helper.c` where `drm_helper_probe_detect` changes `epoch_counter` alongside `connector->status`). The replacement is accurate:

```c
/**
 * @epoch_counter: Used to detect changes in connector. Increased when
 * the connector, including its status, is changed.
 */
```

Has `Fixes:` tag and `Reviewed-by:` from Daniel Stone. No issues.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm: Send per-connector hotplug events

[Claude Code Review Bot]

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 = false;
    }

    scoped_guard(mutex, &dev->mode_config.mutex) {
        changed = dev->mode_config.delayed_event;
        dev->mode_config.delayed_event = 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 writer in `drm_helper_probe_single_connector_modes()` (which sets it under `mode_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 generic, non-connector-specific event), because `delayed_event` is a bare `bool` that doesn't carry connector information. This is acceptable — 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 = true` and fell through to the `out:` label. New code reschedules directly and returns. Functionally equivalent, and cleaner since it removes the `goto out` pattern.

**Connector iteration loop:**

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

`drm_sysfs_connector_hotplug_event()` calls `kobject_uevent_env()` while `mode_config.mutex` is held. This is intentional — the comment at line 621-625 explains the deadlock concern is about "fb helpers" (i.e., `drm_client_dev_hotplug`), not about the sysfs uevent itself. `kobject_uevent_env` does a netlink broadcast which could be slow under memory pressure, but that'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 = false;
    changed = true;
    drm_sysfs_hotplug_event(dev);
}
```

This picks up probes that happened between poll cycles (e.g., userspace snooped via `drm_helper_probe_single_connector_modes`). It sends a generic (non-connector-specific) uevent because the `delayed_event` bool doesn't identify which connector. If the poll loop already sent a connector-specific uevent for the same connector, userspace sees both — slightly redundant 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 the deadlock. The uevent was already sent inside the lock.

**Minor observation:** If multiple connectors changed, `drm_client_dev_hotplug(dev)` is still called only once at the end rather than per-connector. This is fine — the function doesn't take a connector argument and just notifies all registered DRM clients.

#### `drm_helper_hpd_irq_event()` changes

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

The old code tracked `first_changed_connector` with a refcount get/put and branched on `changed == 1` vs `changed > 0` to decide between connector-specific and generic hotplug. The new code is much simpler: send a connector-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 released. Correct.

**Return type change:** `int changed` → `bool changed`. The function returns `bool`, so this is a cleanup. Note that `first_changed_connector` is gone, which also removes the intermediate `drm_connector_get()` ref — no leak risk.

#### Overall assessment

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

---
Generated by Claude Code Patch Reviewer