From dbbd1dc8e20c713b0d286a94e1658b006bea9124 Mon Sep 17 00:00:00 2001 From: Sven Van Asbroeck Date: Tue, 15 Jun 2021 09:48:15 -0400 Subject: [PATCH 1/2] rust/kernel/platdev: get rid of `register()` private function `register()` serves no purpose, except to complicate `Registration` construction. It's a copy-and-paste leftover from miscdev. Eliminate this function to get the following benefits: - `bool registered` disappears, which simplifies logic flow - an `unsafe` block disappears Signed-off-by: Sven Van Asbroeck --- rust/kernel/platdev.rs | 40 +++++++++------------------------------- 1 file changed, 9 insertions(+), 31 deletions(-) diff --git a/rust/kernel/platdev.rs b/rust/kernel/platdev.rs index 5f306b61321e36..27f49f8e5cb1d3 100644 --- a/rust/kernel/platdev.rs +++ b/rust/kernel/platdev.rs @@ -20,7 +20,6 @@ use core::{marker::PhantomPinned, pin::Pin}; /// A registration of a platform device. #[derive(Default)] pub struct Registration { - registered: bool, pdrv: bindings::platform_driver, _pin: PhantomPinned, } @@ -79,18 +78,15 @@ extern "C" fn remove_callback( } impl Registration { - fn register( - self: Pin<&mut Self>, + /// Registers a platform device. + /// + /// Returns a pinned heap-allocated representation of the registration. + pub fn new_pinned( name: &'static CStr, of_match_table: Option<&'static OfMatchTable>, module: &'static crate::ThisModule, - ) -> Result { - // SAFETY: We must ensure that we never move out of `this`. - let this = unsafe { self.get_unchecked_mut() }; - if this.registered { - // Already registered. - return Err(Error::EINVAL); - } + ) -> Result>> { + let mut this = Box::try_new(Self::default())?; this.pdrv.driver.name = name.as_char_ptr(); if let Some(tbl) = of_match_table { this.pdrv.driver.of_match_table = tbl.as_ptr(); @@ -109,32 +105,14 @@ impl Registration { if ret < 0 { return Err(Error::from_kernel_errno(ret)); } - this.registered = true; - Ok(()) - } - - /// Registers a platform device. - /// - /// Returns a pinned heap-allocated representation of the registration. - pub fn new_pinned( - name: &'static CStr, - of_match_tbl: Option<&'static OfMatchTable>, - module: &'static crate::ThisModule, - ) -> Result>> { - let mut r = Pin::from(Box::try_new(Self::default())?); - r.as_mut().register::

(name, of_match_tbl, module)?; - Ok(r) + Ok(Pin::from(this)) } } impl Drop for Registration { fn drop(&mut self) { - if self.registered { - // SAFETY: if `registered` is true, then `self.pdev` was registered - // previously, which means `platform_driver_unregister` is always - // safe to call. - unsafe { bindings::platform_driver_unregister(&mut self.pdrv) } - } + // SAFETY: `self.pdev` was registered previously. + unsafe { bindings::platform_driver_unregister(&mut self.pdrv) } } } From 221d99b96378a2e7c6a8cdf99c02001d542e4afe Mon Sep 17 00:00:00 2001 From: Sven Van Asbroeck Date: Tue, 29 Jun 2021 11:44:22 -0400 Subject: [PATCH 2/2] rust/kernel/platdev: get rid of unnecessary `Pin`ning `Registration` contains a self-referential structure. We need to ensure that users never move that structure around. This is currently done by marking `Registration` as `!Unpin`, and returning it as a `Pin>`, which users cannot move out of - not without the use of `unsafe`, anyway. However, the self-referenial structure is an implementation detail, which we are pushing onto users, by using `Pin` in the public function signature. Treat the self-referential structure as a true, internal implementation detail: store it into a private `Box` member of `Registration`. This is safe as long as `Registration` never moves that structure out of its `Box`. Now `Registration` can safely be moved by its users, and there is no longer a need to mark `Registration` as `!Unpin`, or return it `Pin`ned to users. This greatly simplifies and clarifies driver-side code. Signed-off-by: Sven Van Asbroeck --- drivers/char/hw_random/bcm2835_rng_rust.rs | 4 +-- rust/kernel/platdev.rs | 31 +++++++++++----------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/drivers/char/hw_random/bcm2835_rng_rust.rs b/drivers/char/hw_random/bcm2835_rng_rust.rs index 5f3ec20b295b8f..973fda2dc538af 100644 --- a/drivers/char/hw_random/bcm2835_rng_rust.rs +++ b/drivers/char/hw_random/bcm2835_rng_rust.rs @@ -67,7 +67,7 @@ impl PlatformDriver for RngDriver { } struct RngModule { - _pdev: Pin>, + _pdev: platdev::Registration, } impl KernelModule for RngModule { @@ -75,7 +75,7 @@ impl KernelModule for RngModule { const OF_MATCH_TBL: ConstOfMatchTable<1> = ConstOfMatchTable::new_const([&c_str!("brcm,bcm2835-rng")]); - let pdev = platdev::Registration::new_pinned::( + let pdev = platdev::Registration::new::( c_str!("bcm2835-rng-rust"), Some(&OF_MATCH_TBL), &THIS_MODULE, diff --git a/rust/kernel/platdev.rs b/rust/kernel/platdev.rs index 27f49f8e5cb1d3..e443ddd57bbf52 100644 --- a/rust/kernel/platdev.rs +++ b/rust/kernel/platdev.rs @@ -15,13 +15,12 @@ use crate::{ types::PointerWrapper, }; use alloc::boxed::Box; -use core::{marker::PhantomPinned, pin::Pin}; /// A registration of a platform device. #[derive(Default)] pub struct Registration { - pdrv: bindings::platform_driver, - _pin: PhantomPinned, + // We never move out of `pdrv`'s `Box`. + pdrv: Box, } // SAFETY: `Registration` does not expose any of its state across threads @@ -80,39 +79,39 @@ extern "C" fn remove_callback( impl Registration { /// Registers a platform device. /// - /// Returns a pinned heap-allocated representation of the registration. - pub fn new_pinned( + /// Returns a representation of the registration. + pub fn new( name: &'static CStr, of_match_table: Option<&'static OfMatchTable>, module: &'static crate::ThisModule, - ) -> Result>> { - let mut this = Box::try_new(Self::default())?; - this.pdrv.driver.name = name.as_char_ptr(); + ) -> Result { + let mut pdrv = Box::try_new(bindings::platform_driver::default())?; + pdrv.driver.name = name.as_char_ptr(); if let Some(tbl) = of_match_table { - this.pdrv.driver.of_match_table = tbl.as_ptr(); + pdrv.driver.of_match_table = tbl.as_ptr(); } - this.pdrv.probe = Some(probe_callback::

); - this.pdrv.remove = Some(remove_callback::

); + pdrv.probe = Some(probe_callback::

); + pdrv.remove = Some(remove_callback::

); // SAFETY: - // - `this.pdrv` lives at least until the call to `platform_driver_unregister()` returns. - // - `name` pointer has static lifetime. + // - `pdrv` will never move out of its `Box`, and lives at least + // until the call to `platform_driver_unregister()` returns. // - `module.0` lives at least as long as the module. // - `probe()` and `remove()` are static functions. // - `of_match_table` is either a raw pointer with static lifetime, // as guaranteed by the [`of::OfMatchTable::as_ptr()`] return type, // or null. - let ret = unsafe { bindings::__platform_driver_register(&mut this.pdrv, module.0) }; + let ret = unsafe { bindings::__platform_driver_register(&mut *pdrv, module.0) }; if ret < 0 { return Err(Error::from_kernel_errno(ret)); } - Ok(Pin::from(this)) + Ok(Self { pdrv }) } } impl Drop for Registration { fn drop(&mut self) { // SAFETY: `self.pdev` was registered previously. - unsafe { bindings::platform_driver_unregister(&mut self.pdrv) } + unsafe { bindings::platform_driver_unregister(&mut *self.pdrv) } } }