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

[Alice Ryhl <aliceryhl@google.com>]

On Sat, May 31, 2025 at 02:23:23PM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> > @@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
> >  /// # Invariants
> >  ///
> >  /// `inner` is a registered misc device.
> > -#[repr(transparent)]
> > +#[repr(C)]
> 
> Why do we need linear layout? `container_of!` also works with the `Rust`
> layout.
> 
> >  #[pin_data(PinnedDrop)]
> > -pub struct MiscDeviceRegistration<T> {
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> >      #[pin]
> >      inner: Opaque<bindings::miscdevice>,
> > +    #[pin]
> > +    data: Opaque<T::RegistrationData>,
> >      _t: PhantomData<T>,
> 
> No need to keep the `PhantomData` field around, since you're using `T`
> above.
> 
> >  }
> >  
> > -// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
> > -// `misc_register`.
> > -unsafe impl<T> Send for MiscDeviceRegistration<T> {}
> > -// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
> > -// parallel.
> > -unsafe impl<T> Sync for MiscDeviceRegistration<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> {}
> 
> I would feel better if we still add the `T::RegistrationData: Sync`
> bound here even if it is vacuous today.
> 
> >  impl<T: MiscDevice> MiscDeviceRegistration<T> {
> >      /// Register a misc device.
> > -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > +    pub fn register(
> > +        opts: MiscDeviceOptions,
> > +        data: impl PinInit<T::RegistrationData, Error>,
> > +    ) -> impl PinInit<Self, Error> {
> >          try_pin_init!(Self {
> > +            data <- Opaque::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.
> > +                // 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) })
> > @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
> >          // before the underlying `struct miscdevice` is destroyed.
> >          unsafe { Device::as_ref((*self.as_raw()).this_device) }
> >      }
> > +
> > +    /// Access the additional data stored in this registration.
> > +    pub fn data(&self) -> &T::RegistrationData {
> > +        // SAFETY:
> > +        // * No mutable reference to the value contained by `self.data` can ever be created.
> > +        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
> 
> Please add type invariants for these two requirements.
> 
> > +        unsafe { &*self.data.get() }
> > +    }
> >  }
> >  
> >  #[pinned_drop]
> > -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
> > +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 and nothing uses it anymore.
> 
> Ditto.
> 
> > +        unsafe { core::ptr::drop_in_place(self.data.get()) };
> >      }
> >  }
> >  
> > @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
> >      /// What kind of pointer should `Self` be wrapped in.
> >      type Ptr: ForeignOwnable + Send + Sync;
> >  
> > +    /// 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()`].
> > +    type RegistrationData: Sync;
> 
> Why do we require `Sync` here?
> 
> We might want to give this a shorter name?