On Sun Sep 13, 2026 at 7:37 PM BST, Vladislav Zaharov wrote:
> The debugfs root lives in a static that init() fills in and a guard
> field of the module data clears again. That costs a `static mut`, an
> unsafe write on each side and a guard type whose only job is to undo
> the write.
>
> It also leaks. try_pin_init! drops only the fields it has already
> built, and the guard is written after the Registration, so a
> registration that fails leaves the guard unbuilt and the static set.
> Statics are never dropped, and the module is unloaded right away, so
> the directory outlives everything that could remove it. The next load
> then finds the name taken: debugfs_create_dir() returns -EEXIST, which
> Entry keeps as it would any other pointer, and the driver comes up with
> no debugfs at all until the machine is rebooted.
>
> Have the module data own a DebugfsData instead, built before the
> registration and dropped after it, and keep only a pointer to it in the
> static, for devices that have no other way to reach the data of their
> module. What is left is one unsafe read for the users and one write on
> each side, with no guard type. A registration that fails now drops the
> data that was built before it, and the directory goes with it.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Vladislav Zaharov <[email protected]>
> ---
> drivers/gpu/nova-core/gsp.rs | 15 +++---
> drivers/gpu/nova-core/nova_core.rs | 82 +++++++++++++++++++++++-------
> 2 files changed, 69 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/nova_core.rs
> b/drivers/gpu/nova-core/nova_core.rs
> index 1133c6ce5c55..0f8501c26e05 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -30,40 +30,84 @@
>
> pub(crate) const MODULE_NAME: &core::ffi::CStr = <LocalModule as
> kernel::ModuleMetadata>::NAME;
>
> -// TODO: Move this into per-module data once that exists.
> -static mut DEBUGFS_ROOT: Option<debugfs::Dir> = None;
> +/// Pointer to the [`DebugfsData`] the module owns.
> +///
> +/// A device has no way to reach the data of its module, so probe() goes
> through here instead.
> +// TODO: Drop this once devices can reach the data of their module.
> +static mut DEBUGFS_DATA: *const DebugfsData = core::ptr::null();
>
> [snip]
>
> -impl Drop for DebugfsRootGuard {
> - fn drop(&mut self) {
> - // SAFETY: This guard is dropped after `_driver` (due to field
> order),
> - // so the driver is unregistered and no probe() can be running.
> - unsafe { DEBUGFS_ROOT = None };
> +#[pinned_drop]
> +impl PinnedDrop for DebugfsData {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: This runs after the registration is dropped, as the
> fields of `NovaCoreModule`
> + // are dropped in declaration order, so the driver is unregistered
> and neither a probe()
> + // nor the teardown of a device can be reading `DEBUGFS_DATA`.
> + unsafe { DEBUGFS_DATA = core::ptr::null() };
I think we can drop this. If we're still accessing it after registration is
dropped and just before module unload, we have a bigger problem.
Dropping this would allow this pointer to be completely uncoupled of the struct
itself (it really is a module-level mechanism and not coupled to this type).
> }
> }
>
> +/// Returns the data the module shares with its devices, or [`None`] if
> there is none yet.
> +///
> +/// Only ever call this while the driver is registered, which is to say from
> probe() or from the
> +/// teardown of a device that is bound: the data is built before the
> registration and dropped
> +/// after it, and nothing else keeps what is returned here alive.
> +pub(crate) fn debugfs_data() -> Option<&'static DebugfsData> {
> + // SAFETY: `DEBUGFS_DATA` is written while the module data is
> initialized, before the driver
> + // is registered, and again when that data is dropped, after the driver
> is unregistered. Both
> + // happen with no device bound, so a caller in probe() or in the
> teardown of a device cannot
> + // race with either, and by the type invariant what it gets points at
> live data that outlives
> + // the device it is used from.
> + unsafe { DEBUGFS_DATA.as_ref() }
> +}
The `'static` signature would be lying here. And also it is not ideal that this
returns a `Option`; the user would be always unwrapping it.
Instead, you can do this:
pub(crate) fn debugfs_data<'a>(dev: &'a Device<Bound>) -> &'a DebugfsData {
// SAFETY: `_debugfs` field of module data is dropped after
// registration. So it must be alive while a device is bound.
unsafe { &*DEBUGFS_DATA }
}
See? This way the `= null()` becomes truly redundant because we use lifetime to
restrict how long that data can be accessed.
> +
> #[pin_data]
> struct NovaCoreModule {
> - // Fields are dropped in declaration order, so `_driver` is dropped
> first,
> - // then `_debugfs_guard` clears `DEBUGFS_ROOT`.
> + // Fields are dropped in declaration order, so the registration goes
> first and no probe() can
> + // still be running once the shared data is torn down. `init()` builds
> them the other way
> + // round, as the data has to be there before the first probe() reaches
> for it.
> #[pin]
> _driver: Registration<pci::Adapter<driver::NovaCoreDriver>>,
> - _debugfs_guard: DebugfsRootGuard,
> + #[pin]
> + _debugfs: DebugfsData,
> }
>
> impl InPlaceModule for NovaCoreModule {
> fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self,
> Error> {
> - let dir = debugfs::Dir::new(c"nova-core");
> -
> - // SAFETY: We are the only driver code running during init, so there
> - // cannot be any concurrent access to `DEBUGFS_ROOT`.
> - unsafe { DEBUGFS_ROOT = Some(dir) };
> -
> try_pin_init!(Self {
> + _debugfs <- DebugfsData::new(),
The pointer should be set here instead, not part of `DebugfsData`. `pin-init`
gives you initialized pointer of `_debugfs`. So something like (untested):
_: {
unsafe { DEBUGFS_DATA = _debugfs.get_ref(); }
},
should work.
Best,
Gary
> _driver <- Registration::new(MODULE_NAME, module),
> - _debugfs_guard: DebugfsRootGuard,
> })
> }
> }
