From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/atomic: protect bridge private_obj during bridge removal Date: Wed, 25 Mar 2026 06:50:56 +1000 Message-ID: In-Reply-To: <20260324-drm-bridge-atomic-vs-remove-private_obj-v3-0-64deefe84044@bootlin.com> References: <20260324-drm-bridge-atomic-vs-remove-private_obj-v3-0-64deefe84044@bootlin.com> <20260324-drm-bridge-atomic-vs-remove-private_obj-v3-0-64deefe84044@bootlin.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 **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 l= ocks each private object. The race occurs when CPU1 calls `drm_atomic_priva= te_obj_fini()` and does `list_del(&obj->head)` + `drm_modeset_lock_fini(&ob= j->lock)` while CPU0 still holds that lock in its acquire context. By wrapp= ing `list_del` with `DRM_MODESET_LOCK_ALL_BEGIN/END`, the fini path must fi= rst 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` acqu= ires 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 d= rops all locks including `obj->lock`. Then `drm_modeset_lock_fini(&obj->loc= k)` is called outside the locked section, which checks `WARN_ON(!list_empty= (&lock->head))` =E2=80=94 at this point `lock->head` has been removed from = `ctx->locked` by `drm_modeset_drop_locks()`, so the warn won't trigger. Thi= s is correct. **Potential concern =E2=80=94 `obj->dev` being NULL or stale**: The functio= n uses `obj->dev` which was added by a prior patch ("drm/atomic: Add dev po= inter to drm_private_obj"). This is fine as long as `obj->dev` is set durin= g `drm_atomic_private_obj_init()` (confirmed at line 943: `obj->dev =3D dev= ;`). **Ordering of operations**: The `atomic_destroy_state()` and `drm_modeset_l= ock_fini()` remain outside the locked region, which is correct =E2=80=94 on= ce 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" bu= t the issue described is actually a warning/use-after-remove, not a deadloc= k per se. The `DRM_MODESET_LOCK_ALL_BEGIN/END` approach handles actual dead= locks (via `-EDEADLK` backoff), but the original problem being solved is a = race, not a deadlock. The v3 changelog mentions "Small commit message clari= fication" 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 =E2=80=94 it reuses the ex= isting deadlock avoidance infrastructure (ww_mutex) rather than introducing= a new lock. Reviewed-by worthy. --- Generated by Claude Code Patch Reviewer