Message-Id: <DA9RLBPS7QKE.3CGXHMYG1CDOU@kernel.org>
Date: Fri, 30 May 2025 22:06:55 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>, <gregkh@...uxfoundation.org>,
 <rafael@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <benno.lossin@...ton.me>, <a.hindborg@...nel.org>, <aliceryhl@...gle.com>,
 <tmgross@...ch.edu>, <chrisi.schrefl@...il.com>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

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>`)

>  }
>  
> -// 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. 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?

> +                    // `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.

> +}
> +
> +/// 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`?

> +    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`.

> +            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.

> +
> +                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).

> +                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() },

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 :)

> +            _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.

---
Cheers,
Benno

>  
>      /// Called when the misc device is released.
>      fn release(device: Self::Ptr, _file: &File) {

Powered by blists - more mailing lists