Merge pull request #89 from Rust-for-Linux/pin-chrdev

Use Pin<T> in the chrdev API

Author: alex

drivers/char/rust_example.rs

@@ -7,7 +7,7 @@
 use alloc::boxed::Box;
 use core::pin::Pin;
 use kernel::prelude::*;
-use kernel::{cstr, file_operations::FileOperations, miscdev};
+use kernel::{chrdev, cstr, file_operations::FileOperations, miscdev};
 
 module! {
     type: RustExample,
@@ -47,6 +47,7 @@ impl FileOperations for RustFile {
 
 struct RustExample {
     message: String,
+    _chrdev: Pin<Box<chrdev::Registration<2>>>,
     _dev: Pin<Box<miscdev::Registration>>,
 }
 
@@ -72,9 +73,15 @@ impl KernelModule for RustExample {
         let x: [u64; 1028] = core::hint::black_box([5; 1028]);
         println!("Large array has length: {}", x.len());
 
+        let mut chrdev_reg = chrdev::Registration::new_pinned(
+            cstr!("rust_chrdev"), 0, &THIS_MODULE)?;
+        chrdev_reg.as_mut().register::<RustFile>()?;
+        chrdev_reg.as_mut().register::<RustFile>()?;
+
         Ok(RustExample {
             message: "on the heap!".to_owned(),
             _dev: miscdev::Registration::new_pinned::<RustFile>(cstr!("rust_miscdev"), None)?,
+            _chrdev: chrdev_reg,
         })
     }
 }

rust/kernel/chrdev.rs

@@ -1,100 +1,116 @@
 // SPDX-License-Identifier: GPL-2.0
 
-use core::convert::TryInto;
-use core::mem;
-use core::ops::Range;
-
 use alloc::boxed::Box;
-use alloc::vec;
-use alloc::vec::Vec;
+use core::convert::TryInto;
+use core::marker::PhantomPinned;
+use core::mem::MaybeUninit;
+use core::pin::Pin;
 
 use crate::bindings;
 use crate::c_types;
 use crate::error::{Error, KernelResult};
 use crate::file_operations;
 use crate::types::CStr;
 
-pub fn builder(name: CStr<'static>, minors: Range<u16>) -> KernelResult<Builder> {
-    Ok(Builder {
-        name,
-        minors,
-        file_ops: vec![],
-    })
+struct RegistrationInner<const N: usize> {
+    dev: bindings::dev_t,
+    used: usize,
+    cdevs: [MaybeUninit<bindings::cdev>; N],
+    _pin: PhantomPinned,
 }
 
-pub struct Builder {
+/// chrdev registration. May contain up to a fixed number (`N`) of devices.
+/// Must be pinned.
+pub struct Registration<const N: usize> {
     name: CStr<'static>,
-    minors: Range<u16>,
-    file_ops: Vec<&'static bindings::file_operations>,
+    minors_start: u16,
+    this_module: &'static crate::ThisModule,
+    inner: Option<RegistrationInner<N>>,
 }
 
-impl Builder {
-    pub fn register_device<T: file_operations::FileOperations>(mut self) -> Builder {
-        if self.file_ops.len() >= self.minors.len() {
-            panic!("More devices registered than minor numbers allocated.")
+impl<const N: usize> Registration<{ N }> {
+    pub fn new(
+        name: CStr<'static>,
+        minors_start: u16,
+        this_module: &'static crate::ThisModule,
+    ) -> Self {
+        Registration {
+            name,
+            minors_start,
+            this_module,
+            inner: None,
         }
-        self.file_ops
-            .push(&file_operations::FileOperationsVtable::<T>::VTABLE);
-        self
     }
 
-    pub fn build(self, this_module: &'static crate::ThisModule) -> KernelResult<Registration> {
-        let mut dev: bindings::dev_t = 0;
-        let res = unsafe {
-            bindings::alloc_chrdev_region(
-                &mut dev,
-                self.minors.start.into(),
-                self.minors.len().try_into()?,
-                self.name.as_ptr() as *const c_types::c_char,
-            )
-        };
-        if res != 0 {
-            return Err(Error::from_kernel_errno(res));
+    pub fn new_pinned(
+        name: CStr<'static>,
+        minors_start: u16,
+        this_module: &'static crate::ThisModule,
+    ) -> KernelResult<Pin<Box<Self>>> {
+        Ok(Pin::from(Box::try_new(Self::new(name, minors_start, this_module))?))
+    }
+    /// Register a character device with this range. Call this once per device
+    /// type (up to `N` times).
+    pub fn register<T: file_operations::FileOperations>(self: Pin<&mut Self>) -> KernelResult<()> {
+        // SAFETY: we must ensure that we never move out of `this`.
+        let this = unsafe { self.get_unchecked_mut() };
+        if this.inner.is_none() {
+            let mut dev: bindings::dev_t = 0;
+            // SAFETY: Calling unsafe function. `this.name` has 'static
+            // lifetime 
+            let res = unsafe {
+                bindings::alloc_chrdev_region(
+                    &mut dev,
+                    this.minors_start.into(),
+                    N.try_into()?,
+                    this.name.as_ptr() as *const c_types::c_char,
+                )
+            };
+            if res != 0 {
+                return Err(Error::from_kernel_errno(res));
+            }
+            this.inner = Some(RegistrationInner {
+                dev,
+                used: 0,
+                cdevs: [MaybeUninit::<bindings::cdev>::uninit(); N],
+                _pin: PhantomPinned,
+            });
         }
 
-        // Turn this into a boxed slice immediately because the kernel stores pointers into it, and
-        // so that data should never be moved.
-        let mut cdevs = vec![unsafe { mem::zeroed() }; self.file_ops.len()].into_boxed_slice();
-        for (i, file_op) in self.file_ops.iter().enumerate() {
-            unsafe {
-                bindings::cdev_init(&mut cdevs[i], *file_op);
-                cdevs[i].owner = this_module.0;
-                let rc = bindings::cdev_add(&mut cdevs[i], dev + i as bindings::dev_t, 1);
-                if rc != 0 {
-                    // Clean up the ones that were allocated.
-                    for j in 0..=i {
-                        bindings::cdev_del(&mut cdevs[j]);
-                    }
-                    bindings::unregister_chrdev_region(dev, self.minors.len() as _);
-                    return Err(Error::from_kernel_errno(rc));
-                }
+        let mut inner = this.inner.as_mut().unwrap();
+        if inner.used == N {
+            return Err(Error::EINVAL);
+        }
+        let cdev = inner.cdevs[inner.used].as_mut_ptr();
+        // SAFETY: calling unsafe functions and manipulating MaybeUninit ptr.
+        unsafe {
+            bindings::cdev_init(cdev, &file_operations::FileOperationsVtable::<T>::VTABLE);
+            (*cdev).owner = this.this_module.0;
+            let rc = bindings::cdev_add(cdev, inner.dev + inner.used as bindings::dev_t, 1);
+            if rc != 0 {
+                return Err(Error::from_kernel_errno(rc));
             }
         }
-
-        Ok(Registration {
-            dev,
-            count: self.minors.len(),
-            cdevs,
-        })
+        inner.used += 1;
+        Ok(())
     }
 }
 
-pub struct Registration {
-    dev: bindings::dev_t,
-    count: usize,
-    cdevs: Box<[bindings::cdev]>,
-}
-
-// This is safe because Registration doesn't actually expose any methods.
-unsafe impl Sync for Registration {}
+// SAFETY: `Registration` doesn't expose any of its state across threads (it's
+// fine for multiple threads to have a shared reference to it).
+unsafe impl<const N: usize> Sync for Registration<{ N }> {}
 
-impl Drop for Registration {
+impl<const N: usize> Drop for Registration<{ N }> {
     fn drop(&mut self) {
-        unsafe {
-            for dev in self.cdevs.iter_mut() {
-                bindings::cdev_del(dev);
+        if let Some(inner) = self.inner.as_mut() {
+            // SAFETY: calling unsafe functions, `0..inner.used` of
+            // `inner.cdevs` are initialized in `Registration::register`.
+            unsafe {
+                for i in 0..inner.used {
+                    bindings::cdev_del(inner.cdevs[i].as_mut_ptr());
+                }
+                bindings::unregister_chrdev_region(inner.dev, N.try_into().unwrap());
             }
-            bindings::unregister_chrdev_region(self.dev, self.count as _);
         }
     }
 }