Merge pull request #617 from wedsonaf/file-opener

rust: simplify file operations by removing `FileOpener`

Author: wedsonaf

drivers/android/process.rs

@@ -5,7 +5,7 @@ use kernel::{
     bindings, c_types,
     cred::Credential,
     file::File,
-    file_operations::{FileOpener, FileOperations, IoctlCommand, IoctlHandler, PollTable},
+    file_operations::{FileOperations, IoctlCommand, IoctlHandler, PollTable},
     io_buffer::{IoBufferReader, IoBufferWriter},
     linked_list::List,
     pages::Pages,
@@ -806,17 +806,16 @@ impl IoctlHandler for Process {
     }
 }
 
-impl FileOpener<Ref<Context>> for Process {
-    fn open(ctx: &Ref<Context>, file: &File) -> Result<Self::Wrapper> {
-        Self::new(ctx.clone(), file.cred().clone())
-    }
-}
-
 impl FileOperations for Process {
     type Wrapper = Ref<Self>;
+    type OpenData = Ref<Context>;
 
     kernel::declare_file_operations!(ioctl, compat_ioctl, mmap, poll);
 
+    fn open(ctx: &Ref<Context>, file: &File) -> Result<Self::Wrapper> {
+        Self::new(ctx.clone(), file.cred().clone())
+    }
+
     fn release(obj: Self::Wrapper, _file: &File) {
         // Mark this process as dead. We'll do the same for the threads later.
         obj.inner.lock().is_dead = true;

drivers/android/rust_binder.rs

@@ -102,13 +102,13 @@ const fn ptr_align(value: usize) -> usize {
 unsafe impl Sync for BinderModule {}
 
 struct BinderModule {
-    _reg: Pin<Box<Registration<Ref<Context>>>>,
+    _reg: Pin<Box<Registration<process::Process>>>,
 }
 
 impl KernelModule for BinderModule {
     fn init(name: &'static CStr, _module: &'static kernel::ThisModule) -> Result<Self> {
         let ctx = Context::new()?;
-        let reg = Registration::new_pinned::<process::Process>(name, None, ctx)?;
+        let reg = Registration::new_pinned(name, None, ctx)?;
         Ok(Self { _reg: reg })
     }
 }

drivers/char/hw_random/bcm2835_rng_rust.rs

@@ -18,12 +18,15 @@ module! {
     license: b"GPL v2",
 }
 
-#[derive(Default)]
 struct RngDevice;
 
 impl FileOperations for RngDevice {
     kernel::declare_file_operations!(read);
 
+    fn open(_open_data: &(), _file: &File) -> Result<Self::Wrapper> {
+        Ok(Box::try_new(RngDevice)?)
+    }
+
     fn read(_: &Self, _: &File, data: &mut impl IoBufferWriter, offset: u64) -> Result<usize> {
         // Succeed if the caller doesn't provide a buffer or if not at the start.
         if data.is_empty() || offset != 0 {
@@ -38,12 +41,11 @@ impl FileOperations for RngDevice {
 struct RngDriver;
 
 impl PlatformDriver for RngDriver {
-    type DrvData = Pin<Box<miscdev::Registration<()>>>;
+    type DrvData = Pin<Box<miscdev::Registration<RngDevice>>>;
 
     fn probe(device_id: i32) -> Result<Self::DrvData> {
         pr_info!("probing discovered hwrng with id {}\n", device_id);
-        let drv_data =
-            miscdev::Registration::new_pinned::<RngDevice>(c_str!("rust_hwrng"), None, ())?;
+        let drv_data = miscdev::Registration::new_pinned(c_str!("rust_hwrng"), None, ())?;
         Ok(drv_data)
     }
 

rust/kernel/chrdev.rs

@@ -134,7 +134,9 @@ impl<const N: usize> Registration<{ N }> {
     /// Registers a character device.
     ///
     /// You may call this once per device type, up to `N` times.
-    pub fn register<T: file_operations::FileOpener<()>>(self: Pin<&mut Self>) -> Result {
+    pub fn register<T: file_operations::FileOperations<OpenData = ()>>(
+        self: Pin<&mut Self>,
+    ) -> Result {
         // SAFETY: We must ensure that we never move out of `this`.
         let this = unsafe { self.get_unchecked_mut() };
         if this.inner.is_none() {
@@ -177,13 +179,8 @@ impl<const N: usize> Registration<{ N }> {
     }
 }
 
-impl<const N: usize> file_operations::FileOpenAdapter for Registration<{ N }> {
-    type Arg = ();
-
-    unsafe fn convert(
-        _inode: *mut bindings::inode,
-        _file: *mut bindings::file,
-    ) -> *const Self::Arg {
+impl<const N: usize> file_operations::FileOpenAdapter<()> for Registration<{ N }> {
+    unsafe fn convert(_inode: *mut bindings::inode, _file: *mut bindings::file) -> *const () {
         // TODO: Update the SAFETY comment on the call to `FileOperationsVTable::build` above once
         // this is updated to retrieve state.
         &()

rust/kernel/file_operations.rs

@@ -86,7 +86,7 @@ pub enum SeekFrom {
 ///
 /// The returned value of `A::convert` must be a valid non-null pointer and
 /// `T:open` must return a valid non-null pointer on an `Ok` result.
-unsafe extern "C" fn open_callback<A: FileOpenAdapter, T: FileOpener<A::Arg>>(
+unsafe extern "C" fn open_callback<A: FileOpenAdapter<T::OpenData>, T: FileOperations>(
     inode: *mut bindings::inode,
     file: *mut bindings::file,
 ) -> c_types::c_int {
@@ -312,7 +312,7 @@ unsafe extern "C" fn poll_callback<T: FileOperations>(
 
 pub(crate) struct FileOperationsVtable<A, T>(marker::PhantomData<A>, marker::PhantomData<T>);
 
-impl<A: FileOpenAdapter, T: FileOpener<A::Arg>> FileOperationsVtable<A, T> {
+impl<A: FileOpenAdapter<T::OpenData>, T: FileOperations> FileOperationsVtable<A, T> {
     const VTABLE: bindings::file_operations = bindings::file_operations {
         open: Some(open_callback::<A, T>),
         release: Some(release_callback::<T>),
@@ -568,10 +568,7 @@ impl IoctlCommand {
 /// Trait for extracting file open arguments from kernel data structures.
 ///
 /// This is meant to be implemented by registration managers.
-pub trait FileOpenAdapter {
-    /// The type of argument this adapter extracts.
-    type Arg;
-
+pub trait FileOpenAdapter<T: Sync> {
     /// Converts untyped data stored in [`struct inode`] and [`struct file`] (when [`struct
     /// file_operations::open`] is called) into the given type. For example, for `miscdev`
     /// devices, a pointer to the registered [`struct miscdev`] is stored in [`struct
@@ -582,27 +579,7 @@ pub trait FileOpenAdapter {
     /// This function must be called only when [`struct file_operations::open`] is being called for
     /// a file that was registered by the implementer. The returned pointer must be valid and
     /// not-null.
-    unsafe fn convert(_inode: *mut bindings::inode, _file: *mut bindings::file)
-        -> *const Self::Arg;
-}
-
-/// Trait for implementers of kernel files.
-///
-/// In addition to the methods in [`FileOperations`], implementers must also provide
-/// [`FileOpener::open`] with a customised argument. This allows a single implementation of
-/// [`FileOperations`] to be used for different types of registrations, for example, `miscdev` and
-/// `chrdev`.
-pub trait FileOpener<T: ?Sized>: FileOperations {
-    /// Creates a new instance of this file.
-    ///
-    /// Corresponds to the `open` function pointer in `struct file_operations`.
-    fn open(context: &T, file: &File) -> Result<Self::Wrapper>;
-}
-
-impl<T: FileOperations<Wrapper = Box<T>> + Default> FileOpener<()> for T {
-    fn open(_: &(), _file: &File) -> Result<Self::Wrapper> {
-        Ok(Box::try_new(T::default())?)
-    }
+    unsafe fn convert(_inode: *mut bindings::inode, _file: *mut bindings::file) -> *const T;
 }
 
 /// Corresponds to the kernel's `struct file_operations`.
@@ -619,6 +596,14 @@ pub trait FileOperations: Send + Sync + Sized + 'static {
     /// The pointer type that will be used to hold ourselves.
     type Wrapper: PointerWrapper = Box<Self>;
 
+    /// The type of the context data passed to [`FileOperations::open`].
+    type OpenData: Sync = ();
+
+    /// Creates a new instance of this file.
+    ///
+    /// Corresponds to the `open` function pointer in `struct file_operations`.
+    fn open(context: &Self::OpenData, file: &File) -> Result<Self::Wrapper>;
+
     /// Cleans up after the last reference to the file goes away.
     ///
     /// Note that the object is moved, so it will be freed automatically unless the implementation

rust/kernel/miscdev.rs

@@ -8,28 +8,28 @@
 
 use crate::bindings;
 use crate::error::{Error, Result};
-use crate::file_operations::{FileOpenAdapter, FileOpener, FileOperationsVtable};
+use crate::file_operations::{FileOpenAdapter, FileOperations, FileOperationsVtable};
 use crate::{str::CStr, KernelModule, ThisModule};
 use alloc::boxed::Box;
-use core::marker::{PhantomData, PhantomPinned};
+use core::marker::PhantomPinned;
 use core::{mem::MaybeUninit, pin::Pin};
 
 /// A registration of a miscellaneous device.
 ///
 /// # Invariants
 ///
 /// `Context` is always initialised when `registered` is `true`, and not initialised otherwise.
-pub struct Registration<T: Sync = ()> {
+pub struct Registration<T: FileOperations> {
     registered: bool,
     mdev: bindings::miscdevice,
     _pin: PhantomPinned,
 
     /// Context initialised on construction and made available to all file instances on
-    /// [`FileOpener::open`].
-    open_data: MaybeUninit<T>,
+    /// [`FileOperations::open`].
+    open_data: MaybeUninit<T::OpenData>,
 }
 
-impl<T: Sync> Registration<T> {
+impl<T: FileOperations> Registration<T> {
     /// Creates a new [`Registration`] but does not register it yet.
     ///
     /// It is allowed to move.
@@ -46,25 +46,25 @@ impl<T: Sync> Registration<T> {
     /// Registers a miscellaneous device.
     ///
     /// Returns a pinned heap-allocated representation of the registration.
-    pub fn new_pinned<F: FileOpener<T>>(
+    pub fn new_pinned(
         name: &'static CStr,
         minor: Option<i32>,
-        open_data: T,
+        open_data: T::OpenData,
     ) -> Result<Pin<Box<Self>>> {
         let mut r = Pin::from(Box::try_new(Self::new())?);
-        r.as_mut().register::<F>(name, minor, open_data)?;
+        r.as_mut().register(name, minor, open_data)?;
         Ok(r)
     }
 
     /// Registers a miscellaneous device with the rest of the kernel.
     ///
     /// It must be pinned because the memory block that represents the registration is
     /// self-referential. If a minor is not given, the kernel allocates a new one if possible.
-    pub fn register<F: FileOpener<T>>(
+    pub fn register(
         self: Pin<&mut Self>,
         name: &'static CStr,
         minor: Option<i32>,
-        open_data: T,
+        open_data: T::OpenData,
     ) -> Result {
         // SAFETY: We must ensure that we never move out of `this`.
         let this = unsafe { self.get_unchecked_mut() };
@@ -74,7 +74,7 @@ impl<T: Sync> Registration<T> {
         }
 
         // SAFETY: The adapter is compatible with `misc_register`.
-        this.mdev.fops = unsafe { FileOperationsVtable::<Self, F>::build() };
+        this.mdev.fops = unsafe { FileOperationsVtable::<Self, T>::build() };
         this.mdev.name = name.as_char_ptr();
         this.mdev.minor = minor.unwrap_or(bindings::MISC_DYNAMIC_MINOR as i32);
 
@@ -98,16 +98,17 @@ impl<T: Sync> Registration<T> {
     }
 }
 
-impl<T: Sync> Default for Registration<T> {
+impl<T: FileOperations> Default for Registration<T> {
     fn default() -> Self {
         Self::new()
     }
 }
 
-impl<T: Sync> FileOpenAdapter for Registration<T> {
-    type Arg = T;
-
-    unsafe fn convert(_inode: *mut bindings::inode, file: *mut bindings::file) -> *const Self::Arg {
+impl<T: FileOperations> FileOpenAdapter<T::OpenData> for Registration<T> {
+    unsafe fn convert(
+        _inode: *mut bindings::inode,
+        file: *mut bindings::file,
+    ) -> *const T::OpenData {
         // SAFETY: the caller must guarantee that `file` is valid.
         let reg = crate::container_of!(unsafe { (*file).private_data }, Self, mdev);
 
@@ -120,14 +121,13 @@ impl<T: Sync> FileOpenAdapter for Registration<T> {
 
 // SAFETY: The only method is `register()`, which requires a (pinned) mutable `Registration`, so it
 // is safe to pass `&Registration` to multiple threads because it offers no interior mutability.
-unsafe impl<T: Sync> Sync for Registration<T> {}
+unsafe impl<T: FileOperations> Sync for Registration<T> {}
 
 // SAFETY: All functions work from any thread. So as long as the `Registration::open_data` is
-// `Send`, so is `Registration<T>`. `T` needs to be `Sync` because it's a requirement of
-// `Registration<T>`.
-unsafe impl<T: Send + Sync> Send for Registration<T> {}
+// `Send`, so is `Registration<T>`.
+unsafe impl<T: FileOperations> Send for Registration<T> where T::OpenData: Send {}
 
-impl<T: Sync> Drop for Registration<T> {
+impl<T: FileOperations> Drop for Registration<T> {
     /// Removes the registration from the kernel if it has completed successfully before.
     fn drop(&mut self) {
         if self.registered {
@@ -142,17 +142,15 @@ impl<T: Sync> Drop for Registration<T> {
     }
 }
 
-/// Kernel module that exposes a single miscdev device implemented by `F`.
-pub struct Module<F: FileOpener<()>> {
-    _dev: Pin<Box<Registration>>,
-    _p: PhantomData<F>,
+/// Kernel module that exposes a single miscdev device implemented by `T`.
+pub struct Module<T: FileOperations<OpenData = ()>> {
+    _dev: Pin<Box<Registration<T>>>,
 }
 
-impl<F: FileOpener<()>> KernelModule for Module<F> {
+impl<T: FileOperations<OpenData = ()>> KernelModule for Module<T> {
     fn init(name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
         Ok(Self {
-            _p: PhantomData,
-            _dev: Registration::new_pinned::<F>(name, None, ())?,
+            _dev: Registration::new_pinned(name, None, ())?,
         })
     }
 }

samples/rust/rust_chrdev.rs

@@ -6,7 +6,7 @@
 #![feature(allocator_api, global_asm)]
 
 use kernel::prelude::*;
-use kernel::{chrdev, file_operations::FileOperations};
+use kernel::{chrdev, file, file_operations::FileOperations};
 
 module! {
     type: RustChrdev,
@@ -16,11 +16,14 @@ module! {
     license: b"GPL v2",
 }
 
-#[derive(Default)]
 struct RustFile;
 
 impl FileOperations for RustFile {
     kernel::declare_file_operations!();
+
+    fn open(_shared: &(), _file: &file::File) -> Result<Self::Wrapper> {
+        Ok(Box::try_new(RustFile)?)
+    }
 }
 
 struct RustChrdev {

samples/rust/rust_miscdev.rs

@@ -8,7 +8,7 @@
 use kernel::prelude::*;
 use kernel::{
     file::File,
-    file_operations::{FileOpener, FileOperations},
+    file_operations::FileOperations,
     io_buffer::{IoBufferReader, IoBufferWriter},
     miscdev,
     sync::{CondVar, Mutex, Ref, RefBorrow, UniqueRef},
@@ -55,18 +55,16 @@ impl SharedState {
 }
 
 struct Token;
-
-impl FileOpener<Ref<SharedState>> for Token {
-    fn open(shared: &Ref<SharedState>, _file: &File) -> Result<Self::Wrapper> {
-        Ok(shared.clone())
-    }
-}
-
 impl FileOperations for Token {
     type Wrapper = Ref<SharedState>;
+    type OpenData = Ref<SharedState>;
 
     kernel::declare_file_operations!(read, write);
 
+    fn open(shared: &Ref<SharedState>, _file: &File) -> Result<Self::Wrapper> {
+        Ok(shared.clone())
+    }
+
     fn read(
         shared: RefBorrow<'_, SharedState>,
         _: &File,
@@ -127,7 +125,7 @@ impl FileOperations for Token {
 }
 
 struct RustMiscdev {
-    _dev: Pin<Box<miscdev::Registration<Ref<SharedState>>>>,
+    _dev: Pin<Box<miscdev::Registration<Token>>>,
 }
 
 impl KernelModule for RustMiscdev {
@@ -137,7 +135,7 @@ impl KernelModule for RustMiscdev {
         let state = SharedState::try_new()?;
 
         Ok(RustMiscdev {
-            _dev: miscdev::Registration::new_pinned::<Token>(name, None, state)?,
+            _dev: miscdev::Registration::new_pinned(name, None, state)?,
         })
     }
 }