Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Using `drm_dev_unplug()` instead of `drm_dev_unregister()` breaks normal hardware shutdown for display drivers on module unload. --
commit f4a9e1c28c30b5e7af0c36efbe74ccb2e5047e20 Author: Danilo Krummrich <[email protected]> rust: drm: Add RegistrationGuard for drm_dev_enter/exit critical sections This commit adds a RegistrationGuard to represent a drm_dev_enter/exit SRCU protection area. It also changes Registration::drop to use drm_dev_unplug() instead of drm_dev_unregister() to provide an SRCU barrier. > diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs > index 5152a18a8312b..3cda8dceb498d 100644 > --- a/rust/kernel/drm/driver.rs > +++ b/rust/kernel/drm/driver.rs [ ... ] > @@ -199,8 +199,14 @@ unsafe impl<T: Driver> Send for Registration<T> {} > > impl<T: Driver> Drop for Registration<T> { > fn drop(&mut self) { > + // Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure > that existing > + // `drm_dev_enter()` critical sections complete before > unregistration proceeds. This > + // is required for the safety of `RegistrationGuard`, which relies > on the SRCU barrier in > + // `drm_dev_unplug()` to guarantee that the parent device is still > bound within the > + // critical section. > + // > // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The > existence of this > - // `Registration` also guarantees the this `drm::Device` is actually > registered. > - unsafe { bindings::drm_dev_unregister(self.0.as_raw()) }; > + // `Registration` also guarantees that this `drm::Device` is > actually registered. > + unsafe { bindings::drm_dev_unplug(self.0.as_raw()) }; [Severity: High] Will replacing drm_dev_unregister() with drm_dev_unplug() here break normal hardware shutdown for display drivers on module unload? When drm_dev_unplug() is called, it sets dev->unplugged = true, which causes any subsequent calls to drm_dev_enter() to return false. The standard pattern for DRM driver removal is to unregister the device, which now calls drm_dev_unplug() through this Drop implementation. The driver then typically calls drm_atomic_helper_shutdown() to gracefully disable the hardware. Since dev->unplugged is now true, the KMS disable code paths (which rely on drm_dev_enter() to prevent accessing hot-unplugged hardware) will immediately abort. Does this cause the shutdown sequence to silently fail and leave the display hardware enabled, potentially continuing to scan out from freed memory or trigger IOMMU faults? > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
