Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple Rust driver that touches real hardware: PR 5 #381

Closed
wants to merge 2 commits into from
Closed

Simple Rust driver that touches real hardware: PR 5 #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbbd1dc
rust/kernel/platdev: get rid of `register()` private function
Jun 15, 2021
221d99b
rust/kernel/platdev: get rid of unnecessary `Pin`ning
Jun 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions drivers/char/hw_random/bcm2835_rng_rust.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,15 @@ impl PlatformDriver for RngDriver {
}

struct RngModule {
_pdev: Pin<Box<platdev::Registration>>,
_pdev: platdev::Registration,
}

impl KernelModule for RngModule {
fn init() -> Result<Self> {
const OF_MATCH_TBL: ConstOfMatchTable<1> =
ConstOfMatchTable::new_const([&c_str!("brcm,bcm2835-rng")]);

let pdev = platdev::Registration::new_pinned::<RngDriver>(
let pdev = platdev::Registration::new::<RngDriver>(
c_str!("bcm2835-rng-rust"),
Some(&OF_MATCH_TBL),
&THIS_MODULE,
Expand Down
59 changes: 18 additions & 41 deletions rust/kernel/platdev.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +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 {
registered: bool,
pdrv: bindings::platform_driver,
_pin: PhantomPinned,
// We never move out of `pdrv`'s `Box`.
pdrv: Box<bindings::platform_driver>,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the code be much more complicated if you had Pin<Box<...>> here?

My impression is that it wouldn't be too different (after the simplification you're applying), and the compiler would give you a little bit of help in enforcing the invariant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I'm glad you are asking this question. I can use it as a vehicle to check and update my understanding of the whole Pinning concept.

As far as I know, using Pin<Box<bindings::platform_driver>> on its own here won't change a thing. It won't prevent the platform_driver from moving out of the Box. Why? Because platform_driver: Unpin by default. So we can always call Pin::into_inner() safely, and the inner object will simply move out without complaints.

If we really want to prevent moving out, we need to mark platform_driver as !Unpin:

impl !Unpin for bindings::platform_driver {}

But Rust doesn't currently allow that!

error[E0658]: negative trait bounds are not yet fully implemented; use marker types for now
  --> rust/kernel/platdev.rs:20:6
   |
20 | impl !Unpin for bindings::platform_driver {}

So we'd have to use PhantomPinned, which in turn requires us to use some wrapper type:

struct pinned_platform_driver {
    inner: bindings::platform_driver,
    _pin: PhantomPinned,
}
// can't move out of Pin<Box<pinned_platform_driver>>, yay !

But now we are storing a different structure, one we can't get an inner mutable reference to without using unsafe blocks. Yet the kernel's platform_register()/platform_unregister() require mutable references !

During registration, it's not so bad, because we can register first, then pin. Which kind of defeats the reason for using Pin<> in the first place.. But the unregistration function now becomes problematic: it needs a mutable reference to a bindings::platform_driver, but that structure is wrapped in a !Unpin outer wrapper, so we need a bunch of unsafe acrobatics there to get the reference.

As far as I can tell, it all becomes a huge unreadable, unsafe mess. IMHO using a "do not move out" Invariant works pretty well. But I am open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would not be an invariant, no? i.e. an invariant is something that is guaranteed (as long as one does not break preconditions on e.g. constructors).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be an invariant, but not one the compiler will help enforcing.

Copy link
Collaborator Author

@TheSven73 TheSven73 Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to avoid using Pin<> here (and I think that's probably the way to go, subject to further feedback), then we'll need a way to document / enforce that no code inside platdev::Registration moves pdrv out of its Box. We should not need to enforce this externally, as pdrv is a private member field, and is never exposed externally. Except through a kernel function, but that doesn't count.

What is the best way to accomplish that? Using an invariant? Or some other way?

Copy link
Collaborator Author

@TheSven73 TheSven73 Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip! I'm sorry that my confusion is so confusing :) I'd like to ask a few questions, to update my internal model.

When we register a miscdev for example, we pass a pointer to a bindings::miscdevice to the kernel on registration. I'm assuming we need two guarantees:

  • the struct's memory area will continue to exist until after we unregister the miscdevice and
  • the struct's memory area won't be overwritten until after we unregister the miscdevice

The easiest way is probably like this: (a)

struct Registration {
    mdev: Box<bindings::miscdevice>,
    // etc
}
impl Registration {
    fn new() -> Self {
        let mut mdev = Box::new(default());
        unsafe { bindings::misc_register(&mut *mdev) };
        Self { mdev }
    }
}

Yet the existing codebase does the following: (b)

struct Registration {
    mdev: bindings::miscdevice,
    _pin: PhantomPinned,
}
impl Registration {
    fn new() -> Pin<Box<Self>> {
        let mut r = Pin::from(Box::new(Self::default()));
        let mut this = unsafe { /* r.get_mut() and get_unchecked_mut() gymnastics */ };
        unsafe { bindings::misc_register(&mut this.mdev) };
        r
    }
}

So my question is: why does the current codebase choose (b) over (a)? What are the advantages of (b) over (a)?
According to the comments in the code,

    /// It must be pinned because the memory block that represents the registration is
    /// self-referential.

but I am not sure that's a sufficient justification, because (a) is not Pinned, yet also allows self-referential structures.

Copy link
Member

@ojeda ojeda Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds correct to me. If one keeps the things that need to be pinned inside a Box, and the box remains private (i.e. others cannot mess with it), and the implementation of the abstraction takes care of never moving it, then it is OK. It is also OK to Pin the private Box to ensure it is not moved by mistake by the implementation.

Though I do not recall why Registration is pinned here, rather than leaving it a detail of the implementation. @wedsonaf?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option (b) isn't really what we have in the current codebase. What we have is (simplified):

impl Registration {
    fn new() -> Self {
        [...]
    }
}

Then as a helper we have:

    fn new_pinned() -> Result<Pin<Box<Self>>> {
        [...]
    }

And the reason is that we want to have a zero-cost abstraction. If we forced everyone to use new_pinned or option (a) above, we would require everyone to have an extra allocation. Here's an example (C Binder) on how miscdev is embedded in some outer structure: https://elixir.bootlin.com/linux/latest/source/drivers/android/binder_internal.h#L36 -- it would require an extra allocation if we were to require miscdev to be always boxed; such a solution would preclude the zero-cost case, but the current implementation allows both: if you want zero-cost, you embed Registration in your outer structure and ensure that it's pinned (by pinning the outer structure and ensuring that the projection is also pinned), but if you don't care about zero-cost, just use new_pinned() and pay the cost of the extra allocation.

Regarding Sven's example, that is perfectly fine: note that pd1.set(pd2) causes the destructor of whatever was pinned in pd1 to be called before it is overwritten by pd2, that is, if what was initially pinned was a Registration, its destructor would unregister it first, then it would be overwritten with the new one.

Copy link
Collaborator Author

@TheSven73 TheSven73 Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wedsonaf and @ojeda, thank you so much for your patience and willingness to explain, I really appreciate it !! Let’s see if I understand the underlying issues better now. I encourage you to update my understanding if something doesn’t sound quite right.

When we create an abstraction that uses a kernel C structure in Rust, e.g. via misc_register() or __mutex_init(), there are a couple of “gotchas”:

  1. We must not allow our users to modify the structure, other than through the proper C APIs and/or rules.
  2. We must not allow our users to move the kernel C structure around in memory.

(1) is easy. Users from outside the kernel crate can’t see private fields, so we can always make the C structure private, and ensure our public functions use it as prescribed in the kernel.

(2) is trickier! If the C structure is just a straight-forward field inside our Rust struct, then our users could move it around in memory, which breaks the C API. There are two solutions to this problem:

  1. Store the C structure in a private, Boxed field of the Rust struct. Our users can’t touch it because it’s private. They also can’t move it around - moving the Rust structure in memory is fine, because it’s just holding a pointer to the Boxed C struct.
  2. Store the C structure inside the Rust structure in a straightforward way. And prevent our users from moving it around using various techniques. Such as # Safety requirements, or method functions with a self type that’s Pin<&Self> or Pin<&mut Self>. All of these require our users to use unsafe blocks and make careful reasonings about pinning and safety.

All are partially flawed. Solution (1) leads to multiple allocations and indirections, which is a non-zero cost abstraction - hey that’s not what Rust is supposed to be about! Solution (2) is true zero-cost, yet requires users to write tricky unsafe code just to use what are essentially very simple abstractions. They have a good chance of getting it wrong, and go down in flames - hey that’s also not what Rust is all about!

I am not sure what the group consensus is here. Are the unsafe blocks and tricky pin projections in driver code not seen as a problem?

For platdev I will go with solution (1) for now, which is easy to use for driver writers, yet leads to multiple allocations and indirections. For a stone-cold path such as registering a platform driver (not a device), that shouldn't be a problem.

Edited to add: I'm assuming a solution along the lines of pin-project has been considered at some point, and rejected?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your understanding is correct!

I think it is reasonable to favor the easiest solution for cold paths, although people may still complain because, in the case of allocations, they may lead to other issues outside the cold path (say, memory fragmentation with SLOB).

Then there is the problem of having to return more Results. And, in general, the overall feeling that the language "forces" us to do allocations just to have a nicer API.

I don't think what way would be best for us, but I tend to think that medium-/long-term we should invest in finding out the best ergonomic solution while keeping the zero-cost (via the pin-*-like crates, a new language feature, or something else).

}

// SAFETY: `Registration` does not expose any of its state across threads
Expand Down Expand Up @@ -79,62 +77,41 @@ extern "C" fn remove_callback<P: PlatformDriver>(
}

impl Registration {
fn register<P: PlatformDriver>(
self: Pin<&mut Self>,
/// Registers a platform device.
///
/// Returns a representation of the registration.
pub fn new<P: PlatformDriver>(
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);
}
this.pdrv.driver.name = name.as_char_ptr();
) -> Result<Self> {
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::<P>);
this.pdrv.remove = Some(remove_callback::<P>);
pdrv.probe = Some(probe_callback::<P>);
pdrv.remove = Some(remove_callback::<P>);
// 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));
}
this.registered = true;
Ok(())
}

/// Registers a platform device.
///
/// Returns a pinned heap-allocated representation of the registration.
pub fn new_pinned<P: PlatformDriver>(
name: &'static CStr,
of_match_tbl: Option<&'static OfMatchTable>,
module: &'static crate::ThisModule,
) -> Result<Pin<Box<Self>>> {
let mut r = Pin::from(Box::try_new(Self::default())?);
r.as_mut().register::<P>(name, of_match_tbl, module)?;
Ok(r)
Ok(Self { pdrv })
}
}

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) }
}
}

Expand Down