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.

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 4/6 #305

Merged
merged 2 commits into from
Jun 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
46 changes: 39 additions & 7 deletions drivers/char/hw_random/bcm2835_rng_rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,16 @@

use alloc::boxed::Box;
use core::pin::Pin;
use kernel::of::OfMatchTable;
use kernel::platdev::PlatformDriver;
use kernel::prelude::*;
use kernel::{c_str, platdev};
use kernel::{
file::File,
file_operations::{FileOpener, FileOperations},
io_buffer::IoBufferWriter,
miscdev,
of::OfMatchTable,
platdev::PlatformDriver,
prelude::*,
{c_str, platdev},
};

module! {
type: RngModule,
Expand All @@ -20,15 +26,41 @@ module! {
license: b"GPL v2",
}

struct RngDevice;

impl FileOpener<()> for RngDevice {
fn open(_state: &()) -> Result<Self::Wrapper> {
Ok(Box::try_new(RngDevice)?)
}
}

impl FileOperations for RngDevice {
kernel::declare_file_operations!(read);

fn read<T: IoBufferWriter>(&self, _: &File, data: &mut T, 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 {
return Ok(0);
}

data.write(&0_u32)?;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a FIXME just in case?

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! While the PR x/6 effort is ongoing, the driver is a continual WIP. I'm just adding functionality, as it becomes available in the Rust kernel core.

Do you think there is merit in placing a "warning this is a work in progress, not fully functional" comment on top of the driver?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds reasonable to me. I'll add it.

Ok(4)
}
}

struct RngDriver;

impl PlatformDriver for RngDriver {
fn probe(device_id: i32) -> Result {
type DrvData = Pin<Box<miscdev::Registration<()>>>;

fn probe(device_id: i32) -> Result<Self::DrvData> {
pr_info!("probing discovered hwrng with id {}\n", device_id);
Ok(())
let drv_data =
miscdev::Registration::new_pinned::<RngDevice>(c_str!("rust_hwrng"), None, ())?;
Ok(drv_data)
}

fn remove(device_id: i32) -> Result {
fn remove(device_id: i32, _drv_data: Self::DrvData) -> Result {
pr_info!("removing hwrng with id {}\n", device_id);
Ok(())
}
Expand Down
16 changes: 16 additions & 0 deletions rust/helpers.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <linux/uio.h>
#include <linux/errname.h>
#include <linux/mutex.h>
#include <linux/platform_device.h>

void rust_helper_BUG(void)
{
Expand Down Expand Up @@ -130,6 +131,21 @@ void rust_helper_mutex_lock(struct mutex *lock)
}
EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);

void *
rust_helper_platform_get_drvdata(const struct platform_device *pdev)
{
return platform_get_drvdata(pdev);
}
EXPORT_SYMBOL_GPL(rust_helper_platform_get_drvdata);

void
rust_helper_platform_set_drvdata(struct platform_device *pdev,
void *data)
{
return platform_set_drvdata(pdev, data);
}
EXPORT_SYMBOL_GPL(rust_helper_platform_set_drvdata);

/* We use bindgen's --size_t-is-usize option to bind the C size_t type
* as the Rust usize type, so we can use it in contexts where Rust
* expects a usize like slice (array) indices. usize is defined to be
Expand Down
45 changes: 41 additions & 4 deletions rust/kernel/platdev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,31 @@ pub struct Registration {
// (it is fine for multiple threads to have a shared reference to it).
unsafe impl Sync for Registration {}

extern "C" {
#[allow(improper_ctypes)]
Copy link
Member

Choose a reason for hiding this comment

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

What's the improper ctype here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without it, I get the following warning:

  RUSTC L rust/kernel.o
warning: `extern` block uses type `arch_spinlock_t`, which is not FFI-safe
    --> rust/kernel/platdev.rs:36:15
     |
36   |         pdev: *const bindings::platform_device,
     |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
     |
     = note: `#[warn(improper_ctypes)]` on by default
     = help: consider adding a member to this struct
     = note: this struct has no fields
note: the type is defined here
    --> /home/sva/repos/kernel-arcx/rust/bindings_generated.rs:7331:1
     |
7331 | pub struct arch_spinlock_t {}
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: 1 warning emitted

Copy link
Member

Choose a reason for hiding this comment

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

What type is using arch_spinlock_t? Is it raw_spinlock? You can add the type to the opaque list:

--opaque-type spinlock
.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Gary, thanks for the help! I suspect that on arm 32-bit, arch_spinlock_t is just its own type, and not a typedef. So should I add it to --opaque-type?

typedef struct {
union {
u32 slock;
struct __raw_tickets {
#ifdef __ARMEB__
u16 next;
u16 owner;
#else
u16 owner;
u16 next;
#endif
} tickets;
};
} arch_spinlock_t;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, when I add arch_spinlock_t to --opaque-type, I seem to enter some kind of rabbit-hole where different structures trigger the warning, and when you add those to --opaque-type, there are more structures that trigger it. So far I have:

--opaque-type arch_spinlock_t
--opaque-type lock_class_key
--opaque-type arch_rwlock_t
--opaque-type vm_userfaultfd_ctx

and still the warnings keep coming.

Copy link
Member

Choose a reason for hiding this comment

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

Setting arch_spinlock_t itself to opaque type just propagate the issue one struct up :)

Do you need to peek into fields of platform_device? If not, just setting that to opaque type will be sufficient.

Copy link
Collaborator Author

@TheSven73 TheSven73 Jun 13, 2021

Choose a reason for hiding this comment

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

Edited wait a minute... yes, we do need to peek into platform_device. So that won't solve the issue...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But the peeking is just a hack, and will disappear once we proceed. I'll make a mental note to add platform_device to --opaque-type later, and remove the #[allow(improper_ctypes)].

fn rust_helper_platform_get_drvdata(
pdev: *const bindings::platform_device,
) -> *mut c_types::c_void;

#[allow(improper_ctypes)]
fn rust_helper_platform_set_drvdata(
pdev: *mut bindings::platform_device,
data: *mut c_types::c_void,
);
}

extern "C" fn probe_callback<P: PlatformDriver>(
pdev: *mut bindings::platform_device,
) -> c_types::c_int {
from_kernel_result! {
// SAFETY: `pdev` is guaranteed to be a valid, non-null pointer.
let device_id = unsafe { (*pdev).id };
P::probe(device_id)?;
let drv_data = P::probe(device_id)?;
let drv_data = drv_data.into_pointer() as *mut c_types::c_void;
// SAFETY: `pdev` is guaranteed to be a valid, non-null pointer.
unsafe {
rust_helper_platform_set_drvdata(pdev, drv_data);
}
Ok(0)
}
}
Expand All @@ -47,7 +65,16 @@ extern "C" fn remove_callback<P: PlatformDriver>(
from_kernel_result! {
// SAFETY: `pdev` is guaranteed to be a valid, non-null pointer.
let device_id = unsafe { (*pdev).id };
P::remove(device_id)?;
// SAFETY: `pdev` is guaranteed to be a valid, non-null pointer.
let ptr = unsafe { rust_helper_platform_get_drvdata(pdev) };
// SAFETY:
// - we allocated this pointer using `P::DrvData::into_pointer`,
// so it is safe to turn back into a `P::DrvData`.
// - the allocation happened in `probe`, no-one freed the memory,
// `remove` is the canonical kernel location to free driver data. so OK
// to convert the pointer back to a Rust structure here.
let drv_data = unsafe { P::DrvData::from_pointer(ptr) };
P::remove(device_id, drv_data)?;
Ok(0)
}
}
Expand Down Expand Up @@ -124,15 +151,25 @@ impl Drop for Registration {
///
/// Implement this trait whenever you create a platform driver.
pub trait PlatformDriver {
/// Device driver data.
///
/// Corresponds to the data set or retrieved via the kernel's
/// `platform_{set,get}_drvdata()` functions.
///
/// Require that `DrvData` implements `PointerWrapper`. We guarantee to
/// never move the underlying wrapped data structure. This allows
/// driver writers to use pinned or self-referential data structures.
type DrvData: PointerWrapper;

/// Platform driver probe.
///
/// Called when a new platform device is added or discovered.
/// Implementers should attempt to initialize the device here.
fn probe(device_id: i32) -> Result;
fn probe(device_id: i32) -> Result<Self::DrvData>;

/// Platform driver remove.
///
/// Called when a platform device is removed.
/// Implementers should prepare the device for complete removal here.
fn remove(device_id: i32) -> Result;
fn remove(device_id: i32, drv_data: Self::DrvData) -> Result;
}