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

["Danilo Krummrich" <dakr@kernel.org>]

On Sat Nov 7, 2026 at 2:32 PM CET, Alexandre Courbot wrote:
> On Sat Nov 7, 2026 at 10:14 PM JST, Danilo Krummrich wrote:
>> On Sat Nov 7, 2026 at 7:47 AM CET, Alexandre Courbot wrote:
>>> On Thu Oct 15, 2026 at 12:45 AM JST, Vladislav Zaharov wrote:
>>> 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`?
>>
>> I think that makes sense.
>>
>>>>  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).
>>
>> Actually, I would leave that part as is.
>>
>> Eventually we'll have registration data for driver registrations to share module
>> data. Once we have that, we'd need to change DebugfsData::new() again and move
>> that part bnack into NovaCoreModule::init().
>>
>> Besides that, DebugfsData has nothing to do with the current limitation of
>> publishing module data to driver registrations; the module code seems like the
>> correct place to deal with this.
>
> Makes sense, let's reduce the churn indeed. In this case I guess
> `DEBUGFS_DATA` will need to be made `pub(super)` so this module can
> initialize it.

I think DEBUGFS_DATA still belongs into nova_core.rs (including the
debugfs_data() accessor) since it is a workaround to make module data
accessible.

I'd also like the accessor to be an associated function:
NovaCoreModule::debugfs_data().

Thanks,
Danilo