From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections Date: Thu, 04 Jun 2026 12:03:17 +1000 Message-ID: In-Reply-To: <20260603011711.2077361-3-dakr@kernel.org> References: <20260603011711.2077361-1-dakr@kernel.org> <20260603011711.2077361-3-dakr@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the core safety patch. A few observations: **Good**: The switch from `drm_dev_unregister()` to `drm_dev_unplug()` is e= ssential and correctly placed. The C implementation of `drm_dev_unplug()` d= oes: ```c dev->unplugged =3D true; synchronize_srcu(&drm_unplug_srcu); drm_dev_unregister(dev); ``` So after `Registration::drop()` returns, all `drm_dev_enter()` critical sec= tions that saw the device as plugged have completed. **`UnbindGuard::deref()`** =E2=80=94 the safety argument: ```rust unsafe { T::ParentDevice::from_device(self.dev.as_ref().as_bound()) } ``` This calls `as_bound()` on the underlying `device::Device`. The safety comm= ent says the parent is bound because we hold a `drm_dev_enter()` critical s= ection. This is sound because `drm_dev_enter()` succeeding means `drm_dev_u= nplug()` hasn't completed `synchronize_srcu()` yet, which means the bus dri= ver's `remove()` hasn't returned (since `drm_dev_unplug()` is called from `= Registration::drop()`, which is called during unbind). So the device is ind= eed still bound. **Minor**: The `as_bound()` call suggests there's an unsafe method on `Devi= ce` to assert the Bound context. The safety invariant here is well-argued b= ut subtle =E2=80=94 it would benefit from explicitly mentioning that `Regis= tration::drop()` is the one calling `drm_dev_unplug()` so the temporal orde= ring is: `drm_dev_unplug()` =E2=86=92 `synchronize_srcu()` =E2=86=92 bus un= bind completes. Currently the safety comment only says "the parent device i= s bound for the lifetime of this guard" without tracing why. **`with_unbind_guard()`** is a nice convenience but note it doesn't give ac= cess to registration data =E2=80=94 that comes later in patch 3. Fine for i= ncremental development. --- Generated by Claude Code Patch Reviewer