Merge pull request torvalds#555 from wedsonaf/pin-cleanup

rust: remove all usages of `try_new_and_init` and `pin_init_and_share`.
ojeda · Nov 17, 2021 · 0f3ae60 · 0f3ae60
2 parents f8a2c63 + 36a85e4
commit 0f3ae60
Show file tree

Hide file tree

Showing 7 changed files with 149 additions and 181 deletions.
diff --git a/drivers/android/context.rs b/drivers/android/context.rs
@@ -4,7 +4,7 @@ use kernel::{
     bindings,
     prelude::*,
     security,
-    sync::{Mutex, Ref},
+    sync::{Mutex, Ref, UniqueRef},
 };
 
 use crate::{
@@ -26,22 +26,21 @@ unsafe impl Sync for Context {}
 
 impl Context {
     pub(crate) fn new() -> Result<Ref<Self>> {
-        Ref::try_new_and_init(
-            Self {
-                // SAFETY: Init is called below.
-                manager: unsafe {
-                    Mutex::new(Manager {
-                        node: None,
-                        uid: None,
-                    })
-                },
+        let mut ctx = Pin::from(UniqueRef::try_new(Self {
+            // SAFETY: Init is called below.
+            manager: unsafe {
+                Mutex::new(Manager {
+                    node: None,
+                    uid: None,
+                })
             },
-            |mut ctx| {
-                // SAFETY: `manager` is also pinned when `ctx` is.
-                let manager = unsafe { ctx.as_mut().map_unchecked_mut(|c| &mut c.manager) };
-                kernel::mutex_init!(manager, "Context::manager");
-            },
-        )
+        })?);
+
+        // SAFETY: `manager` is also pinned when `ctx` is.
+        let manager = unsafe { ctx.as_mut().map_unchecked_mut(|c| &mut c.manager) };
+        kernel::mutex_init!(manager, "Context::manager");
+
+        Ok(ctx.into())
     }
 
     pub(crate) fn set_manager_node(&self, node_ref: NodeRef) -> Result {

diff --git a/drivers/android/process.rs b/drivers/android/process.rs
@@ -261,24 +261,24 @@ unsafe impl Sync for Process {}
 
 impl Process {
     fn new(ctx: Ref<Context>) -> Result<Ref<Self>> {
-        Ref::try_new_and_init(
-            Self {
-                ctx,
-                task: Task::current().group_leader().clone(),
-                // SAFETY: `inner` is initialised in the call to `mutex_init` below.
-                inner: unsafe { Mutex::new(ProcessInner::new()) },
-                // SAFETY: `node_refs` is initialised in the call to `mutex_init` below.
-                node_refs: unsafe { Mutex::new(ProcessNodeRefs::new()) },
-            },
-            |mut process| {
-                // SAFETY: `inner` is pinned when `Process` is.
-                let pinned = unsafe { process.as_mut().map_unchecked_mut(|p| &mut p.inner) };
-                kernel::mutex_init!(pinned, "Process::inner");
-                // SAFETY: `node_refs` is pinned when `Process` is.
-                let pinned = unsafe { process.as_mut().map_unchecked_mut(|p| &mut p.node_refs) };
-                kernel::mutex_init!(pinned, "Process::node_refs");
-            },
-        )
+        let mut process = Pin::from(UniqueRef::try_new(Self {
+            ctx,
+            task: Task::current().group_leader().clone(),
+            // SAFETY: `inner` is initialised in the call to `mutex_init` below.
+            inner: unsafe { Mutex::new(ProcessInner::new()) },
+            // SAFETY: `node_refs` is initialised in the call to `mutex_init` below.
+            node_refs: unsafe { Mutex::new(ProcessNodeRefs::new()) },
+        })?);
+
+        // SAFETY: `inner` is pinned when `Process` is.
+        let pinned = unsafe { process.as_mut().map_unchecked_mut(|p| &mut p.inner) };
+        kernel::mutex_init!(pinned, "Process::inner");
+
+        // SAFETY: `node_refs` is pinned when `Process` is.
+        let pinned = unsafe { process.as_mut().map_unchecked_mut(|p| &mut p.node_refs) };
+        kernel::mutex_init!(pinned, "Process::node_refs");
+
+        Ok(process.into())
     }
 
     /// Attemps to fetch a work item from the process queue.
@@ -704,12 +704,14 @@ impl Process {
             return Ok(());
         }
 
-        let death = death
-            .write(
+        let death = {
+            let mut pinned = Pin::from(death.write(
                 // SAFETY: `init` is called below.
                 unsafe { NodeDeath::new(info.node_ref.node.clone(), self.clone(), cookie) },
-            )
-            .pin_init_and_share(|pinned_death| pinned_death.init());
+            ));
+            pinned.as_mut().init();
+            Ref::<NodeDeath>::from(pinned)
+        };
 
         info.death = Some(death.clone());
 

diff --git a/drivers/android/thread.rs b/drivers/android/thread.rs
@@ -13,7 +13,7 @@ use kernel::{
     linked_list::{GetLinks, Links, List},
     prelude::*,
     security,
-    sync::{CondVar, Ref, SpinLock},
+    sync::{CondVar, Ref, SpinLock, UniqueRef},
     user_ptr::{UserSlicePtr, UserSlicePtrWriter},
 };
 
@@ -241,31 +241,31 @@ impl Thread {
     pub(crate) fn new(id: i32, process: Ref<Process>) -> Result<Ref<Self>> {
         let return_work = Ref::try_new(ThreadError::new(InnerThread::set_return_work))?;
         let reply_work = Ref::try_new(ThreadError::new(InnerThread::set_reply_work))?;
-        Ref::try_new_and_init(
-            Self {
-                id,
-                process,
-                // SAFETY: `inner` is initialised in the call to `spinlock_init` below.
-                inner: unsafe { SpinLock::new(InnerThread::new()) },
-                // SAFETY: `work_condvar` is initalised in the call to `condvar_init` below.
-                work_condvar: unsafe { CondVar::new() },
-                links: Links::new(),
-            },
-            |mut thread| {
-                // SAFETY: `inner` is pinned when `thread` is.
-                let inner = unsafe { thread.as_mut().map_unchecked_mut(|t| &mut t.inner) };
-                kernel::spinlock_init!(inner, "Thread::inner");
-
-                // SAFETY: `work_condvar` is pinned when `thread` is.
-                let condvar = unsafe { thread.as_mut().map_unchecked_mut(|t| &mut t.work_condvar) };
-                kernel::condvar_init!(condvar, "Thread::work_condvar");
-                {
-                    let mut inner = thread.inner.lock();
-                    inner.set_reply_work(reply_work);
-                    inner.set_return_work(return_work);
-                }
-            },
-        )
+        let mut thread = Pin::from(UniqueRef::try_new(Self {
+            id,
+            process,
+            // SAFETY: `inner` is initialised in the call to `spinlock_init` below.
+            inner: unsafe { SpinLock::new(InnerThread::new()) },
+            // SAFETY: `work_condvar` is initalised in the call to `condvar_init` below.
+            work_condvar: unsafe { CondVar::new() },
+            links: Links::new(),
+        })?);
+
+        // SAFETY: `inner` is pinned when `thread` is.
+        let inner = unsafe { thread.as_mut().map_unchecked_mut(|t| &mut t.inner) };
+        kernel::spinlock_init!(inner, "Thread::inner");
+
+        // SAFETY: `work_condvar` is pinned when `thread` is.
+        let condvar = unsafe { thread.as_mut().map_unchecked_mut(|t| &mut t.work_condvar) };
+        kernel::condvar_init!(condvar, "Thread::work_condvar");
+
+        {
+            let mut inner = thread.inner.lock();
+            inner.set_reply_work(reply_work);
+            inner.set_return_work(return_work);
+        }
+
+        Ok(thread.into())
     }
 
     pub(crate) fn set_current_transaction(&self, transaction: Ref<Transaction>) {

diff --git a/drivers/android/transaction.rs b/drivers/android/transaction.rs
@@ -8,7 +8,7 @@ use kernel::{
     linked_list::List,
     linked_list::{GetLinks, Links},
     prelude::*,
-    sync::{Ref, SpinLock},
+    sync::{Ref, SpinLock, UniqueRef},
     user_ptr::UserSlicePtrWriter,
     ScopeGuard,
 };
@@ -55,29 +55,27 @@ impl Transaction {
         let data_address = alloc.ptr;
         let file_list = alloc.take_file_list();
         alloc.keep_alive();
-        let tr = Ref::try_new_and_init(
-            Self {
-                // SAFETY: `spinlock_init` is called below.
-                inner: unsafe { SpinLock::new(TransactionInner { file_list }) },
-                node_ref: Some(node_ref),
-                stack_next,
-                from: from.clone(),
-                to,
-                code: tr.code,
-                flags: tr.flags,
-                data_size: tr.data_size as _,
-                data_address,
-                offsets_size: tr.offsets_size as _,
-                links: Links::new(),
-                free_allocation: AtomicBool::new(true),
-            },
-            |mut tr| {
-                // SAFETY: `inner` is pinned when `tr` is.
-                let pinned = unsafe { tr.as_mut().map_unchecked_mut(|t| &mut t.inner) };
-                kernel::spinlock_init!(pinned, "Transaction::inner");
-            },
-        )?;
-        Ok(tr)
+        let mut tr = Pin::from(UniqueRef::try_new(Self {
+            // SAFETY: `spinlock_init` is called below.
+            inner: unsafe { SpinLock::new(TransactionInner { file_list }) },
+            node_ref: Some(node_ref),
+            stack_next,
+            from: from.clone(),
+            to,
+            code: tr.code,
+            flags: tr.flags,
+            data_size: tr.data_size as _,
+            data_address,
+            offsets_size: tr.offsets_size as _,
+            links: Links::new(),
+            free_allocation: AtomicBool::new(true),
+        })?);
+
+        // SAFETY: `inner` is pinned when `tr` is.
+        let pinned = unsafe { tr.as_mut().map_unchecked_mut(|t| &mut t.inner) };
+        kernel::spinlock_init!(pinned, "Transaction::inner");
+
+        Ok(tr.into())
     }
 
     pub(crate) fn new_reply(
@@ -90,29 +88,27 @@ impl Transaction {
         let data_address = alloc.ptr;
         let file_list = alloc.take_file_list();
         alloc.keep_alive();
-        let tr = Ref::try_new_and_init(
-            Self {
-                // SAFETY: `spinlock_init` is called below.
-                inner: unsafe { SpinLock::new(TransactionInner { file_list }) },
-                node_ref: None,
-                stack_next: None,
-                from: from.clone(),
-                to,
-                code: tr.code,
-                flags: tr.flags,
-                data_size: tr.data_size as _,
-                data_address,
-                offsets_size: tr.offsets_size as _,
-                links: Links::new(),
-                free_allocation: AtomicBool::new(true),
-            },
-            |mut tr| {
-                // SAFETY: `inner` is pinned when `tr` is.
-                let pinned = unsafe { tr.as_mut().map_unchecked_mut(|t| &mut t.inner) };
-                kernel::spinlock_init!(pinned, "Transaction::inner");
-            },
-        )?;
-        Ok(tr)
+        let mut tr = Pin::from(UniqueRef::try_new(Self {
+            // SAFETY: `spinlock_init` is called below.
+            inner: unsafe { SpinLock::new(TransactionInner { file_list }) },
+            node_ref: None,
+            stack_next: None,
+            from: from.clone(),
+            to,
+            code: tr.code,
+            flags: tr.flags,
+            data_size: tr.data_size as _,
+            data_address,
+            offsets_size: tr.offsets_size as _,
+            links: Links::new(),
+            free_allocation: AtomicBool::new(true),
+        })?);
+
+        // SAFETY: `inner` is pinned when `tr` is.
+        let pinned = unsafe { tr.as_mut().map_unchecked_mut(|t| &mut t.inner) };
+        kernel::spinlock_init!(pinned, "Transaction::inner");
+
+        Ok(tr.into())
     }
 
     /// Determines if the transaction is stacked on top of the given transaction.

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
@@ -95,13 +95,6 @@ impl<T> Ref<T> {
         Ok(unsafe { Self::from_inner(inner) })
     }
 
-    /// Constructs a new reference counted instance of `T` and calls the initialisation function.
-    ///
-    /// This is useful because it provides a mutable reference to `T` at its final location.
-    pub fn try_new_and_init<U: FnOnce(Pin<&mut T>)>(contents: T, init: U) -> Result<Self> {
-        Ok(UniqueRef::try_new(contents)?.pin_init_and_share(init))
-    }
-
     /// Deconstructs a [`Ref`] object into a `usize`.
     ///
     /// It can be reconstructed once via [`Ref::from_usize`].
@@ -410,9 +403,10 @@ impl<T: ?Sized> Deref for RefBorrow<T> {
 /// }
 ///
 /// fn test2() -> Result<Ref<Example>> {
-///     let mut x = UniqueRef::try_new(Example { a: 10, b: 20 })?;
+///     let mut pinned = Pin::from(UniqueRef::try_new(Example { a: 10, b: 20 })?);
 ///     // We can modify `pinned` because it is `Unpin`.
-///     Ok(x.pin_init_and_share(|mut pinned| pinned.a += 1))
+///     pinned.as_mut().a += 1;
+///     Ok(pinned.into())
 /// }
 /// ```
 pub struct UniqueRef<T: ?Sized> {
@@ -437,26 +431,6 @@ impl<T> UniqueRef<T> {
     }
 }
 
-impl<T: ?Sized> UniqueRef<T> {
-    /// Converts a [`UniqueRef<T>`] into a [`Ref<T>`].
-    ///
-    /// It allows callers to get a `Pin<&mut T>` that they can use to initialise the inner object
-    /// just before it becomes shareable.
-    pub fn pin_init_and_share<U: FnOnce(Pin<&mut T>)>(mut self, init: U) -> Ref<T> {
-        let inner = self.deref_mut();
-
-        // SAFETY: By the `Ref` invariant, `RefInner` is pinned and `T` is also pinned.
-        let pinned = unsafe { Pin::new_unchecked(inner) };
-
-        // INVARIANT: The only places where `&mut T` is available are here (where it is explicitly
-        // pinned, i.e. implementations of `init` will see a Pin<&mut T>), and in `Ref::drop`. Both
-        // are compatible with the pin requirements of the invariants of `Ref`.
-        init(pinned);
-
-        self.into()
-    }
-}
-
 impl<T> UniqueRef<MaybeUninit<T>> {
     /// Converts a `UniqueRef<MaybeUninit<T>>` into a `UniqueRef<T>` by writing a value into it.
     pub fn write(mut self, value: T) -> UniqueRef<T> {

diff --git a/samples/rust/rust_miscdev.rs b/samples/rust/rust_miscdev.rs
@@ -11,7 +11,7 @@ use kernel::{
     file_operations::{FileOpener, FileOperations},
     io_buffer::{IoBufferReader, IoBufferWriter},
     miscdev,
-    sync::{CondVar, Mutex, Ref},
+    sync::{CondVar, Mutex, Ref, UniqueRef},
 };
 
 module! {
@@ -35,22 +35,22 @@ struct SharedState {
 
 impl SharedState {
     fn try_new() -> Result<Ref<Self>> {
-        Ref::try_new_and_init(
-            Self {
-                // SAFETY: `condvar_init!` is called below.
-                state_changed: unsafe { CondVar::new() },
-                // SAFETY: `mutex_init!` is called below.
-                inner: unsafe { Mutex::new(SharedStateInner { token_count: 0 }) },
-            },
-            |mut state| {
-                // SAFETY: `state_changed` is pinned when `state` is.
-                let pinned = unsafe { state.as_mut().map_unchecked_mut(|s| &mut s.state_changed) };
-                kernel::condvar_init!(pinned, "SharedState::state_changed");
-                // SAFETY: `inner` is pinned when `state` is.
-                let pinned = unsafe { state.as_mut().map_unchecked_mut(|s| &mut s.inner) };
-                kernel::mutex_init!(pinned, "SharedState::inner");
-            },
-        )
+        let mut state = Pin::from(UniqueRef::try_new(Self {
+            // SAFETY: `condvar_init!` is called below.
+            state_changed: unsafe { CondVar::new() },
+            // SAFETY: `mutex_init!` is called below.
+            inner: unsafe { Mutex::new(SharedStateInner { token_count: 0 }) },
+        })?);
+
+        // SAFETY: `state_changed` is pinned when `state` is.
+        let pinned = unsafe { state.as_mut().map_unchecked_mut(|s| &mut s.state_changed) };
+        kernel::condvar_init!(pinned, "SharedState::state_changed");
+
+        // SAFETY: `inner` is pinned when `state` is.
+        let pinned = unsafe { state.as_mut().map_unchecked_mut(|s| &mut s.inner) };
+        kernel::mutex_init!(pinned, "SharedState::inner");
+
+        Ok(state.into())
     }
 }