Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

[Danilo Krummrich <dakr@kernel.org>]

On Fri, May 30, 2025 at 10:06:55PM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> > @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
> >      }
> >  }
> >  
> > -/// A registration of a miscdevice.
> > -///
> >  /// # Invariants
> >  ///
> > -/// `inner` is a registered misc device.
> > +/// - `inner` is a registered misc device,
> > +/// - `data` is valid for the entire lifetime of `Self`.
> >  #[repr(C)]
> >  #[pin_data(PinnedDrop)]
> > -pub struct MiscDeviceRegistration<T: MiscDevice> {
> > +struct RawDeviceRegistration<T: MiscDevice> {
> >      #[pin]
> >      inner: Opaque<bindings::miscdevice>,
> > -    #[pin]
> > -    data: Opaque<T::RegistrationData>,
> > +    data: NonNull<T::RegistrationData>,
> >      _t: PhantomData<T>,
> 
> You shouldn't need the `PhantomData` here.
> 
> Also, do we need to ask for `T: MiscDevice` here? Could we instead have
> just `T` and then below you write
> `RawDeviceRegistration<T::RegistrationData>` instead? (`new` of course
> needs to have a new generic: `U: MiscDevice<RegistrationData = T>`)

Sure, is there any advantage? Your proposal seems more complicated at a first
glance.

> >  }
> >  
> > -// SAFETY:
> > -// - It is allowed to call `misc_deregister` on a different thread from where you called
> > -//   `misc_register`.
> > -// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> > -unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> > -
> > -// SAFETY:
> > -// - All `&self` methods on this type are written to ensure that it is safe to call them in
> > -//   parallel.
> > -// - `MiscDevice::RegistrationData` is always `Sync`.
> > -unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> > -
> > -impl<T: MiscDevice> MiscDeviceRegistration<T> {
> > -    /// Register a misc device.
> > -    pub fn register(
> > +impl<T: MiscDevice> RawDeviceRegistration<T> {
> > +    fn new<'a>(
> >          opts: MiscDeviceOptions,
> > -        data: impl PinInit<T::RegistrationData, Error>,
> > -    ) -> impl PinInit<Self, Error> {
> > +        parent: Option<&'a Device<Bound>>,
> > +        data: &'a T::RegistrationData,
> > +    ) -> impl PinInit<Self, Error> + 'a
> > +    where
> > +        T: 'a,
> > +    {
> >          try_pin_init!(Self {
> > -            data <- Opaque::pin_init(data),
> > +            // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> > +            // is guaranteed to be valid for the entire lifetime of `Self`.
> > +            data: NonNull::from(data),
> 
> Both the argument in the INVARIANT comment and way this works are a bit
> flawed.

Why is the argument flawed? Let's say we go with your proposal below, what would
the safety requirement for RawDeviceRegistration::new and the invariant of
RawDeviceRegistration look like? Wouldn't it be the exact same argument?

> Instead, I'd recommend directly taking the `NonNull` as a
> parameter. Yes the function will need to be `unsafe`, but the lifetime
> that you're creating below only lives for `'a`, but the object might
> live much longer. You might still be fine, but I'd just recommend
> staying in raw pointer land (or in this case `NonNull`).
> 
> >              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> > +                let mut value = opts.into_raw::<T>();
> > +
> > +                if let Some(parent) = parent {
> > +                    // The device core code will take care to take a reference of `parent` in
> 
> Just a question: with "take a reference of" you mean that it will
> increment the refcount?

Exactly -- will change it to "increment the refcount" for clarity.

> 
> > +                    // `device_add()` called by `misc_register()`.
> > +                    value.parent = parent.as_raw();
> > +                }
> > +
> >                  // SAFETY: The initializer can write to the provided `slot`.
> > -                unsafe { slot.write(opts.into_raw::<T>()) };
> > +                unsafe { slot.write(value) };
> >  
> >                  // SAFETY:
> >                  // * We just wrote the misc device options to the slot. The miscdevice will
> > @@ -94,12 +94,12 @@ pub fn register(
> >      }
> >  
> >      /// Returns a raw pointer to the misc device.
> > -    pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > +    fn as_raw(&self) -> *mut bindings::miscdevice {
> >          self.inner.get()
> >      }
> >  
> >      /// Access the `this_device` field.
> > -    pub fn device(&self) -> &Device {
> > +    fn device(&self) -> &Device {
> >          // SAFETY: This can only be called after a successful register(), which always
> >          // initialises `this_device` with a valid device. Furthermore, the signature of this
> >          // function tells the borrow-checker that the `&Device` reference must not outlive the
> > @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
> >          unsafe { Device::as_ref((*self.as_raw()).this_device) }
> >      }
> >  
> > +    fn data(&self) -> &T::RegistrationData {
> > +        // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
> > +        // `Self`.
> > +        unsafe { self.data.as_ref() }
> > +    }
> > +}
> > +
> > +#[pinned_drop]
> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<T> {
> > +    fn drop(self: Pin<&mut Self>) {
> > +        // SAFETY: We know that the device is registered by the type invariants.
> > +        unsafe { bindings::misc_deregister(self.inner.get()) };
> > +    }
> > +}
> > +
> > +#[expect(dead_code)]
> > +enum DeviceRegistrationInner<T: MiscDevice> {
> > +    Raw(Pin<KBox<RawDeviceRegistration<T>>>),
> > +    Managed(Devres<RawDeviceRegistration<T>>),
> 
> These two names could be shortened (`DeviceRegistrationInner` and
> `RawDeviceRegistration`) as they are only implementation details of this
> file. How about `InnerRegistration` and `RawRegistration`? Or maybe
> something even shorter.

There's a reason why I keep them something with "DeviceRegistration" everywhere,
which is to make it clear that it's both a device instance *and* a registration,
which is actually rather uncommon and caused by the fact that device creation
and registration needs to be done under the misc_mtx in misc_register().

This is also the reason for those data structures to be a bit complicated; it
would be much simpler if device creation and registration would be independent
things.

> > +}
> > +
> > +/// A registration of a miscdevice.
> > +#[pin_data(PinnedDrop)]
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> > +    inner: DeviceRegistrationInner<T>,
> > +    #[pin]
> > +    data: Opaque<T::RegistrationData>,
> 
> Why is it necessary to store `data` inside of `Opaque`?

It was UnsafePinned before, but Alice proposed to go with Opaque for the
meantime. Anyways, this is not introduced by this patch, it comes from
Christians patch adding T::RegistrationData.

> 
> > +    this_device: ARef<Device>,
> > +    _t: PhantomData<T>,
> > +}
> > +
> > +// SAFETY:
> > +// - It is allowed to call `misc_deregister` on a different thread from where you called
> > +//   `misc_register`.
> > +// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> > +unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> > +
> > +// SAFETY:
> > +// - All `&self` methods on this type are written to ensure that it is safe to call them in
> > +//   parallel.
> > +// - `MiscDevice::RegistrationData` is always `Sync`.
> > +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> > +
> > +impl<T: MiscDevice> MiscDeviceRegistration<T> {
> > +    /// Register a misc device.
> > +    pub fn register<'a>(
> > +        opts: MiscDeviceOptions,
> > +        data: impl PinInit<T::RegistrationData, Error> + 'a,
> > +        parent: Option<&'a Device<Bound>>,
> > +    ) -> impl PinInit<Self, Error> + 'a
> > +    where
> > +        T: 'a,
> > +    {
> > +        let mut dev: Option<ARef<Device>> = None;
> > +
> > +        try_pin_init!(&this in Self {
> > +            data <- Opaque::pin_init(data),
> > +            // TODO: make `inner` in-place when enums get supported by pin-init.
> > +            //
> > +            // Link: https://github.com/Rust-for-Linux/pin-init/issues/59
> 
> You might want to add that this would avoid the extra allocation in
> `DeviceRegistrationInner`.

Sure, will do.

> > +            inner: {
> > +                // SAFETY:
> > +                //   - `this` is a valid pointer to `Self`,
> > +                //   - `data` was properly initialized above.
> > +                let data = unsafe { &*(*this.as_ptr()).data.get() };
> 
> As mentioned above, this creates a reference that is valid for this
> *block*. So its lifetime will end after the `},` and before
> `this_device` is initialized.
> 
> It *might* be ok to turn it back into a raw pointer in
> `RawDeviceRegistration::new`, but I wouldn't bet on it.

Why? The reference is still valid in RawDeviceRegistration::new, no?

> > +
> > +                let raw = RawDeviceRegistration::new(opts, parent, data);
> > +
> > +                // FIXME: Work around a bug in rustc, to prevent the following warning:
> > +                //
> > +                //   "warning: value captured by `dev` is never read."
> > +                //
> > +                // Link: https://github.com/rust-lang/rust/issues/141615
> 
> Note that the bug is that the compiler complains about the wrong span.
> The original value of `dev` is `None` and that value is never used, so
> the warning is justified. So this `let _ = dev;` still needs to stay
> until `pin-init` supports accessing previously initialized fields (now
> I'm pretty certain that I will implement that soon).

Do you want to propose an alternative comment about this?

> > +                let _ = dev;
> > +
> > +                if let Some(parent) = parent {
> > +                    let devres = Devres::new(parent, raw, GFP_KERNEL)?;
> > +
> > +                    dev = Some(devres.access(parent)?.device().into());
> > +                    DeviceRegistrationInner::Managed(devres)
> > +                } else {
> > +                    let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
> > +
> > +                    dev = Some(boxed.device().into());
> > +                    DeviceRegistrationInner::Raw(boxed)
> > +                }
> > +            },
> > +            // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
> > +            // case.
> > +            this_device: {
> > +                // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
> > +                unsafe { dev.unwrap_unchecked() }
> > +            },
> 
> No need for the extra block, just do:
> 
>     // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
>     // case.
>     // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
>     this_device: unsafe { dev.unwrap_unchecked() },

Yes, I know, but I found the above a bit cleaner -- I don't mind changing it
though.

> I'm also pretty sure that the compiler would optimize `.take().unwrap()`
> and also this is only executed once per `MiscDeviceRegistration`, so
> even if it isn't it wouldn't really matter. So I'd prefer if we don't
> use `unsafe` here even if it is painfully obvious (if I'm fast enough
> with implementing, you can rebase on top before you merge and then this
> will be gone anyways :)

Sounds good! :)

But I think that unsafe is better than unwrap() in such cases; unsafe requires
us to explain why it's OK to do it, which makes it less likely to create bugs.

(Just recently I wrote some code, hit the need for unsafe and, while writing up
the safety comment, I had to explain to myself, why the way I was about to
implement this was pretty broken.)

unwrap() on the other hand, doesn't require any explanation, but panics the
kernel in the worst case.

> > +            _t: PhantomData,
> > +        })
> > +    }
> > +
> > +    /// Access the `this_device` field.
> > +    pub fn device(&self) -> &Device {
> > +        &self.this_device
> > +    }
> > +
> >      /// Access the additional data stored in this registration.
> >      pub fn data(&self) -> &T::RegistrationData {
> >          // SAFETY:
> > @@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData {
> >  #[pinned_drop]
> >  impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
> >      fn drop(self: Pin<&mut Self>) {
> > -        // SAFETY: We know that the device is registered by the type invariants.
> > -        unsafe { bindings::misc_deregister(self.inner.get()) };
> > -
> >          // SAFETY: `self.data` is valid for dropping.
> >          unsafe { core::ptr::drop_in_place(self.data.get()) };
> >      }
> > @@ -137,14 +236,13 @@ pub trait MiscDevice: Sized {
> >      /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
> >      /// If no additional data is required than the unit type `()` should be used.
> >      ///
> > -    /// This data can be accessed in [`MiscDevice::open()`] using
> > -    /// [`MiscDeviceRegistration::data()`].
> > +    /// This data can be accessed in [`MiscDevice::open()`].
> >      type RegistrationData: Sync;
> >  
> >      /// Called when the misc device is opened.
> >      ///
> >      /// The returned pointer will be stored as the private data for the file.
> > -    fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
> > +    fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;
> 
> What is the reason that these parameters begin with `_`? In a trait
> function without a body, the compiler shouldn't war about unused
> parameters.

No idea, I just tried to be complient with the existing style of the file. :)