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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu Oct 15, 2026 at 12:45 AM JST, Vladislav Zaharov wrote:
<...>
> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index a8f6dc22057b..ee3a6600f320 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -4,6 +4,7 @@
>  
>  use kernel::{
>      debugfs,
> +    device,
>      driver::Registration,
>      pci,
>      prelude::*,
> @@ -32,40 +33,75 @@
>  
>  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 code running on behalf of a bound
> +/// device goes through here instead.
> +///
> +/// Written once, while the module data is built, and never again: what it points at is dropped
> +/// only after the driver is unregistered, so it is good for as long as any device is bound, and
> +/// is not read outside of that.
> +// TODO: Drop this once devices can reach the data of their module.
> +static mut DEBUGFS_DATA: *const DebugfsData = core::ptr::null();
>  
> -/// Guard that clears `DEBUGFS_ROOT` when dropped.
> -struct DebugfsRootGuard;
> +/// Data the module shares with every GPU it drives.
> +///
> +/// Reached from a device through [`debugfs_data()`].
> +#[pin_data]
> +pub(crate) struct DebugfsData {
> +    /// Root directory of the driver in debugfs.
> +    root: debugfs::Dir,
> +}
> +
> +impl DebugfsData {
> +    /// Creates the shared data.
> +    fn new() -> impl PinInit<Self> {
> +        pin_init!(Self {
> +            root: debugfs::Dir::new(c"nova-core"),
> +        })
> +    }

Another thing: since this is declaring now a proper interface, can you
move `DebugfsData`, `debugfs_data` and `DEBUGFS_DATA` into a sub-module,
say `debugfs.rs`? This will keep the root module free of clutter and
keep all the debugfs-related setup under a single module.

Tring doing that locally also made me notice another potential
improvement, see below.

>  
> -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 };
> +    /// Returns the root directory of the driver in debugfs.
> +    pub(crate) fn root(&self) -> &debugfs::Dir {
> +        &self.root
>      }
>  }
>  
> +/// Returns the data the module shares with its devices.
> +///
> +/// The bound device is what makes this sound: the data is built before the driver is registered
> +/// and dropped after it is unregistered, so it outlives every device that is bound, and the
> +/// returned reference cannot be held past the one it is taken for.
> +pub(crate) fn debugfs_data<'a>(_dev: &'a device::Device<device::Bound>) -> &'a DebugfsData {
> +    // SAFETY: A device can only be bound once the driver is registered, which happens after
> +    // `DEBUGFS_DATA` is written, so it points at the data of this module, which lives at least
> +    // as long as the caller's device is bound.
> +    unsafe { &*DEBUGFS_DATA }
> +}
> +
>  #[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(),
> +            _: {
> +                // SAFETY: Nothing reads `DEBUGFS_DATA` before a device is bound, which cannot
> +                // happen until the driver is registered below. What it points at is dropped only
> +                // once the driver is unregistered, so it is valid for as long as any device is
> +                // bound, and nothing reads it outside of that.
> +                unsafe { DEBUGFS_DATA = &*_debugfs };
> +            },

Let's move this initialization into `DebugfsData::new`, using
`pin_chain`:

    /// Returns an initializer that creates the shared debugfs data.
    ///
    /// # Safety
    ///
    /// The returned initializer must be run exactly once before the driver is registered. The
    /// initialized [`DebugfsData`] must outlive the driver registration.
    pub(super) unsafe fn new() -> impl PinInit<Self> {
        pin_init!(Self {
            root: debugfs::Dir::new(c"nova-core"),
        })
        .pin_chain(|debugfs| {
            // SAFETY: Per the method's safety block, the driver is not registered yet, meaning that
            // no device can be probed and thus nothing can access `DEBUGFS_DATA` concurrently.
            //
            // INVARIANT: Per the method's safety block, `DEBUGFS_DATA` is written before the driver
            // is registered and points to a valid instance of `DebugfsData`, which is pinned
            // and dropped after the driver is unregistered.
            unsafe { DEBUGFS_DATA = &*debugfs };
            Ok(())
        })
    }

That way, the burden of enforcing the invariants for `DEBUGFS_DATA`
relies exclusively on `new` (and we made it unsafe to force the caller
to do that). The only thing `nova_core` has to take care about is
calling it before the driver is registered, and keeping the result alive
until it is unregistered. Simple and trivial to check since `nova_core`
now doesn't contain any of the debugfs code stuff anymore and literally
fits within one screen worth of code.