Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

[Christian Schrefl <chrisi.schrefl@gmail.com>]

Hi Alice

On 27.01.25 11:27 AM, Alice Ryhl wrote:
><snip>
>>
>> to make sure that `misc_register` is called after data is initialized and to that
>> `data` will be dropped correctly in case `misc_register` fails.
>>
>> But I'm not very familiar with `(try_)pin_init!` so this might be unnecessary?
> 
> Using pin_chain is definitely incorrect because it will run the
> destructor of MiscDeviceRegistration if the misc_register call fails,
> but calling misc_deregister is incorrect in that case.
> 
> You should be able to just move the `data <-
> UnsafePinned::try_pin_init(data)` line to the top so that the field is
> initialized first.
> 

So if I understand correctly, the following should be correct:

pub fn register(
    opts: MiscDeviceOptions,
    data: impl PinInit<T::RegistrationData, Error>,
) -> impl PinInit<Self, Error> {
    try_pin_init!(Self {
        data <- UnsafePinned::try_pin_init(data),
        inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
            // SAFETY: The initializer can write to the provided `slot`.
            unsafe { slot.write(opts.into_raw::<T>()) };
            // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
            // get unregistered before `slot` is deallocated because the memory is pinned and
            // the destructor of this type deallocates the memory.
            // `data` is Initialized before `misc_register` so no race with `fops->open()`
            // is possible.
            // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
            // misc device.
            to_result(unsafe { bindings::misc_register(slot) })
        }),
        _t: PhantomData,
    })
}

Sorry I don't know the details of `(try_)pin_init` and the docs only say:
> The fields are initialized in the order that they appear in the initializer. So it is possible
> to read already initialized fields using raw pointers.
> 
> IMPORTANT: You are not allowed to create references to fields of the struct inside of the 
> initializer.

This says its invalid to create references, but as soon as `misc_register` its theoretically
possible that a `open()` call happens and creates a reference to the Registration. I assume
that is fine, because the Registration would be fully initialized at that point, but that's
technically against the Docs.

Also does `try_pin_init!()` automatically drop `data` when `bindings::misc_register` fails?

I couldn't find anything in the Docs about when and what is dropped.

Is there a equivalent to `cargo expand` for the kernel?
It would be nice to be able to look at the code generated by the macros.

Cheers
Christian