Re: [PATCH v4 1/3] gpu: nova-core: move the debugfs root into the module data

["Gary Guo" <gary@garyguo.net>]

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 <vladazaharova2018@gmail.com>
> ---
>  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,
>          })
>      }
>  }