[PATCH v3] drm/atomic: protect bridge private_obj during bridge removal

[PATCH v3] drm/atomic: protect bridge private_obj during bridge removal

[Luca Ceresoli]

This series prevents a race between DRM bridge removal and usage of the
bridge private_obj during DRM_MODESET_LOCK_ALL_BEGIN/END() and other
locking operations.

== Series description

The need for this series emerged during testing of DRM bridge
hot-plugging. Very rarely on hot-unplug the following warning has appeared:

  WARNING: CPU: 0 PID: 123 at include/drm/drm_modeset_lock.h:114 drm_atomic_private_obj_fini+0x64/0x80
  ...
  Call trace:
   drm_atomic_private_obj_fini+0x64/0x80
   drm_bridge_detach+0x38/0x98

This series does not depend on other series.

== Grand plan

This is part of the work to support hotplug of DRM bridges. The grand plan
was discussed in [0].

Here's the work breakdown (➜ marks the current series):

 1. … add refcounting to DRM bridges struct drm_bridge,
      based on devm_drm_bridge_alloc()
    A. ✔ add new alloc API and refcounting (v6.16)
    B. ✔ convert all bridge drivers to new API (v6.17)
    C. ✔ kunit tests (v6.17)
    D. ✔ add get/put to drm_bridge_add/remove() + attach/detach()
         and warn on old allocation pattern (v6.17)
    E. … add get/put on drm_bridge accessors
       1. ✔ drm_bridge_chain_get_first_bridge(), add cleanup action (v6.18)
       2. ✔ drm_bridge_get_prev_bridge() (v6.18)
       3. ✔ drm_bridge_get_next_bridge() (v6.19)
       4. ✔ drm_for_each_bridge_in_chain() (v6.19)
       5. ✔ drm_bridge_connector_init (v6.19)
       6. … protect encoder bridge chain with a mutex
       7. … of_drm_find_bridge
          a. ✔ add of_drm_get_bridge() (v7.0),
	       convert basic direct users (v7.0-v7.1)
	  b. ✔ convert direct of_drm_get_bridge() users, part 2 (v7.0)
	  c. ✔ convert direct of_drm_get_bridge() users, part 3 (v7.0)
	  d. ✔… convert direct of_drm_get_bridge() users, part 4
	        (some v7.1, some pending)
	  e.   convert bridge-only drm_of_find_panel_or_bridge() users
       8. drm_of_find_panel_or_bridge, *_of_get_bridge
       9. ✔ enforce drm_bridge_add before drm_bridge_attach (v6.19)
    F. ✔ debugfs improvements
       1. ✔ add top-level 'bridges' file (v6.16)
       2. ✔ show refcount and list lingering bridges (v6.19)
 2. ➜ handle gracefully atomic updates during bridge removal
    A. ✔ Add drm_bridge_enter/exit() to protect device resources (v7.0)
    B. ➜ protect private_obj removal from list
    C. ✔ Add drm_bridge_clear_and_put() (v7.1)
 3. … DSI host-device driver interaction
 4. ✔ removing the need for the "always-disconnected" connector
 5. … Migrate i.MX LCDIF driver to bridge-connector
 6.   DRM bridge hotplug
    A.   Bridge hotplug management in the DRM core
    B.   Device tree description

[0] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/#t

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Changes in v3:
- Rebased on current drm-misc-next (on 7.0-rc3)
- Small commit message clarification
- Link to v2: https://lore.kernel.org/r/20251021-drm-bridge-atomic-vs-remove-private_obj-v2-1-412a18399bac@bootlin.com

Changes in v2:
- Adapted to work on top of "drm/atomic: Add dev pointer to drm_private_obj"
- Removed 'To: jessica.zhang@oss.qualcomm.com', invalid address
- Link to v1: https://lore.kernel.org/r/20251013-drm-bridge-atomic-vs-remove-private_obj-v1-0-1fc2e58102e0@bootlin.com

---
Luca Ceresoli (1):
      drm/atomic: drm_atomic_private_obj_fini: protect private_obj removal from list

 drivers/gpu/drm/drm_atomic.c | 6 ++++++
 1 file changed, 6 insertions(+)
---
base-commit: 7ea0468380216c10b73633b976d33efa8c12d375
change-id: 20251013-drm-bridge-atomic-vs-remove-private_obj-d792805bebdc

Best regards,
--  
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH v3] drm/atomic: drm_atomic_private_obj_fini: protect private_obj removal from list

[Luca Ceresoli]

Currently drm_bridge_detach() expects that the bridge private_obj is not
locked by a drm_modeset_acquire_ctx, and it warns in case that happens:

  drm_bridge_detach()
  -> drm_atomic_private_obj_fini()
     -> list_del(&obj->head) // removes priv_obj from
                             // dev->mode_config.privobj_list
     -> obj->funcs->atomic_destroy_state()
     -> drm_modeset_lock_fini(&obj->lock)
        -> WARN_ON(!list_empty(&lock->head)) // warn if priv_obj->lock
	                                     // is still in ctx->locked

The expectation is not respected when introducing bridge hot-plugging. In
such case the warning triggers if the bridge is being removed concurrently
to an operation that locks the private object using a
drm_modeset_acquire_ctx, such as in this execution scenario:

  CPU0:
  drm_mode_obj_get_properties_ioctl() // userspace request
  -> DRM_MODESET_LOCK_ALL_BEGIN()
  .  -> drm_for_each_privobj() // loop on dev->mode_config.privobj_list
  .     - lock the privobj mutex
  .	- add priv_obj->lock to ctx->locked
  .	  (list of locks to be released later)
  .
  .                         CPU1:
  .                         drm_bridge_detach() // bridge hot-unplug
  .		            -> WARN triggers!
  .
  -> DRM_MODESET_LOCK_ALL_END()
     -> for each lock in ctx->locked
	- remove priv_obj->lock from ctx->locked
        - unlock the privobj mutex

Prevent this potential deadlock by using DRM_MODESET_LOCK_ALL_BEGIN/END()
around the list removal in drm_atomic_private_obj_fini(). This ensures that
exactly one of these happens:

 * the concurrent code (e.g. drm_mode_obj_get_properties_ioctl()) acquires
   all the locks first, so it can execute fully and release the
   privobj->lock before drm_atomic_private_obj_fini() calls list_del() and
   before the WARN_ON()
 * drm_atomic_private_obj_fini() acquires all the locks first, so it
   removes its privobj->lock from the dev->mode_config.privobj_list; the
   concurrent code will run afterwards and not acquire that lock because it
   is not present anymore

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changes in v3:
- Rebased on current drm-misc-next (on 7.0-rc3)
- Small commit message clarification

Changes in v2:
- added 'drm/atomic:' prefix to commit title
- Adapted to work on top of "drm/atomic: Add dev pointer to drm_private_obj"
- Slightly improved commit message
---
 drivers/gpu/drm/drm_atomic.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 41c57063f3b4..0f9c2528c9ee 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -962,7 +962,13 @@ EXPORT_SYMBOL(drm_atomic_private_obj_init);
 void
 drm_atomic_private_obj_fini(struct drm_private_obj *obj)
 {
+	struct drm_modeset_acquire_ctx ctx;
+	int ret = 0;
+
+	DRM_MODESET_LOCK_ALL_BEGIN(obj->dev, ctx, 0, ret);
 	list_del(&obj->head);
+	DRM_MODESET_LOCK_ALL_END(obj->dev, ctx, ret);
+
 	obj->funcs->atomic_destroy_state(obj, obj->state);
 	drm_modeset_lock_fini(&obj->lock);
 }

-- 
2.53.0

Claude review: drm/atomic: protect bridge private_obj during bridge removal

[Claude Code Review Bot]

Overall Series Review

Subject: drm/atomic: protect bridge private_obj during bridge removal
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>
Patches: 2
Reviewed: 2026-03-25T06:50:56.340536

---

This is a single-patch series addressing a real race condition in DRM bridge hot-unplug. The problem is well-understood and clearly documented in the commit message: when a bridge's `private_obj` is being finalized via `drm_bridge_detach()` concurrently with code that holds modeset locks (e.g., `drm_mode_obj_get_properties_ioctl()` using `DRM_MODESET_LOCK_ALL_BEGIN/END`), the `drm_modeset_lock_fini()` call hits a `WARN_ON` because the lock is still on the acquire context's locked list.

The fix is minimal and correct: wrap the `list_del(&obj->head)` in `drm_atomic_private_obj_fini()` with `DRM_MODESET_LOCK_ALL_BEGIN/END`. This serializes the list removal against concurrent lock acquisition, ensuring the private object is either fully locked by the concurrent user (who will finish and release before fini proceeds) or removed from the list before the concurrent user iterates it.

The series is well-motivated as part of ongoing DRM bridge hotplug work, and the roadmap in the cover letter is appreciated.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: protect bridge private_obj during bridge removal

[Claude Code Review Bot]

Patch Review

**Correctness**: The fix is correct. Looking at `drm_modeset_lock_all_ctx()` (in `drm_modeset_lock.c:451`), it iterates `drm_for_each_privobj()` and locks each private object. The race occurs when CPU1 calls `drm_atomic_private_obj_fini()` and does `list_del(&obj->head)` + `drm_modeset_lock_fini(&obj->lock)` while CPU0 still holds that lock in its acquire context. By wrapping `list_del` with `DRM_MODESET_LOCK_ALL_BEGIN/END`, the fini path must first acquire all modeset locks, which serializes against any concurrent `drm_modeset_lock_all_ctx()` user.

**One subtlety worth considering**: After `DRM_MODESET_LOCK_ALL_BEGIN` acquires all locks, it will also lock `obj->lock` itself (since `obj` is still in the `privobj_list` at that point). The `DRM_MODESET_LOCK_ALL_END` then drops all locks including `obj->lock`. Then `drm_modeset_lock_fini(&obj->lock)` is called outside the locked section, which checks `WARN_ON(!list_empty(&lock->head))` — at this point `lock->head` has been removed from `ctx->locked` by `drm_modeset_drop_locks()`, so the warn won't trigger. This is correct.

**Potential concern — `obj->dev` being NULL or stale**: The function uses `obj->dev` which was added by a prior patch ("drm/atomic: Add dev pointer to drm_private_obj"). This is fine as long as `obj->dev` is set during `drm_atomic_private_obj_init()` (confirmed at line 943: `obj->dev = dev;`).

**Ordering of operations**: The `atomic_destroy_state()` and `drm_modeset_lock_fini()` remain outside the locked region, which is correct — once the object is removed from the list, no concurrent code can find it, so no locking is needed for teardown.

**Minor nit**: The commit message says "Prevent this potential deadlock" but the issue described is actually a warning/use-after-remove, not a deadlock per se. The `DRM_MODESET_LOCK_ALL_BEGIN/END` approach handles actual deadlocks (via `-EDEADLK` backoff), but the original problem being solved is a race, not a deadlock. The v3 changelog mentions "Small commit message clarification" but this wording persists. Very minor.

**Overall**: Clean, minimal, correct fix. The approach of serializing via `DRM_MODESET_LOCK_ALL_BEGIN/END` is the right one — it reuses the existing deadlock avoidance infrastructure (ww_mutex) rather than introducing a new lock.

Reviewed-by worthy.

---
Generated by Claude Code Patch Reviewer