Device / Driver PCI / Platform Rust abstractions

In order to access static metadata of a Rust kernel module, add the ModuleMetadata trait.

In particular, this trait provides the name of a Rust kernel module as specified by the module! macro.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/lib.rs    | 6 ++++++
 rust/macros/module.rs | 4 ++++
 2 files changed, 10 insertions(+)

rust/kernel/lib.rs +6 -0

index e1065a7551a3..61b82b78b915 100644
@@ -116,6 +116,12 @@ fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
     }
 }
 
+/// Metadata attached to a [`Module`] or [`InPlaceModule`].
+pub trait ModuleMetadata {
+    /// The name of the module as specified in the `module!` macro.
+    const NAME: &'static crate::str::CStr;
+}
+
 /// Equivalent to `THIS_MODULE` in the C API.
 ///
 /// C header: [`include/linux/init.h`](srctree/include/linux/init.h)

rust/macros/module.rs +4 -0

index 2587f41b0d39..cdf94f4982df 100644
@@ -228,6 +228,10 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
                 kernel::ThisModule::from_ptr(core::ptr::null_mut())
             }};
 
+            impl kernel::ModuleMetadata for {type_} {{
+                const NAME: &'static kernel::str::CStr = kernel::c_str!(\"{name}\");
+            }}
+
             // Double nested modules, since then nobody can access the public items inside.
             mod __module_init {{
                 mod __module_init {{

Implement the generic Registration type and the RegistrationOps trait.

The Registration structure is the common type that represents a driver registration and is typically bound to the lifetime of a module. However, it doesn't implement actual calls to the kernel's driver core to register drivers itself.

Instead the RegistrationOps trait is provided to subsystems, which have to implement RegistrationOps::register and RegistrationOps::unregister. Subsystems have to provide an implementation for both of those methods where the subsystem specific variants to register / unregister a driver have to implemented.

For instance, the PCI subsystem would call __pci_register_driver() from RegistrationOps::register and pci_unregister_driver() from DrvierOps::unregister.

Co-developed-by Wedson Almeida Filho <[email protected]>

Signed-off-by Wedson Almeida Filho <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 MAINTAINERS           |   1 +
 rust/kernel/driver.rs | 117 ++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs    |   1 +
 3 files changed, 119 insertions(+)
 create mode 100644 rust/kernel/driver.rs

MAINTAINERS +1 -0

index baf0eeb9a355..2ad58ed40079 100644
@@ -7033,6 +7033,7 @@ F:	include/linux/kobj*
 F:	include/linux/property.h
 F:	lib/kobj*
 F:	rust/kernel/device.rs
+F:	rust/kernel/driver.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
 M:	Nishanth Menon <[email protected]>

rust/kernel/driver.rs new file +18 -0

new file mode 100644
index 000000000000..c1957ee7bb7e
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.).
+//!
+//! Each bus / subsystem is expected to implement [`RegistrationOps`], which allows drivers to
+//! register using the [`Registration`] class.
+
+use crate::error::{Error, Result};
+use crate::{init::PinInit, str::CStr, try_pin_init, types::Opaque, ThisModule};
+use core::pin::Pin;
+use macros::{pin_data, pinned_drop};
+
+/// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform,
+/// Amba, etc.) to provide the corresponding subsystem specific implementation to register /
+/// unregister a driver of the particular type (`RegType`).
+///
+/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call
+/// `bindings::__pci_register_driver` from `RegistrationOps::register` and

rust/kernel/driver.rs new file +20 -0

+    fn unregister(reg: &Opaque<Self::RegType>);
+}
+
+/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
+/// `bindings::pci_driver`). Therefore a [`Registration`] must be initialized with a type that
+/// implements the [`RegistrationOps`] trait, such that the generic `T::register` and
+/// `T::unregister` calls result in the subsystem specific registration calls.
+///
+///Once the `Registration` structure is dropped, the driver is unregistered.
+#[pin_data(PinnedDrop)]
+pub struct Registration<T: RegistrationOps> {
+    #[pin]
+    reg: Opaque<T::RegType>,
+}
+
+// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to
+// share references to it with multiple threads as nothing can be done.
+unsafe impl<T: RegistrationOps> Sync for Registration<T> {}
+
+// SAFETY: Both registration and unregistration are implemented in C and safe to be performed from

rust/kernel/driver.rs new file +10 -0

+                let drv = unsafe { &*(ptr as *const Opaque<T::RegType>) };
+
+                T::register(drv, name, module)
+            }),
+        })
+    }
+}
+
+#[pinned_drop]
+impl<T: RegistrationOps> PinnedDrop for Registration<T> {

rust/kernel/driver.rs new file +29 -0

+macro_rules! module_driver {
+    (<$gen_type:ident>, $driver_ops:ty, { type: $type:ty, $($f:tt)* }) => {
+        type Ops<$gen_type> = $driver_ops;
+
+        #[$crate::prelude::pin_data]
+        struct DriverModule {
+            #[pin]
+            _driver: $crate::driver::Registration<Ops<$type>>,
+        }
+
+        impl $crate::InPlaceModule for DriverModule {
+            fn init(
+                module: &'static $crate::ThisModule
+            ) -> impl $crate::init::PinInit<Self, $crate::error::Error> {
+                $crate::try_pin_init!(Self {
+                    _driver <- $crate::driver::Registration::new(
+                        <Self as $crate::ModuleMetadata>::NAME,
+                        module,
+                    ),
+                })
+            }
+        }
+
+        $crate::prelude::module! {
+            type: DriverModule,
+            $($f)*
+        }
+    }
+}

rust/kernel/lib.rs +1 -0

index 61b82b78b915..7818407f9aac 100644
@@ -35,6 +35,7 @@
 mod build_assert;
 pub mod cred;
 pub mod device;
+pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
 pub mod firmware;

Quoted 18 lines · On Thu, 19 Dec 2024 18:04:04 +0100 Danilo Krummrich <[email protected]> wrote:

Implement the generic Registration type and the RegistrationOps trait.

The Registration structure is the common type that represents a driver registration and is typically bound to the lifetime of a module. However, it doesn't implement actual calls to the kernel's driver core to register drivers itself.

Instead the RegistrationOps trait is provided to subsystems, which have to implement RegistrationOps::register and RegistrationOps::unregister. Subsystems have to provide an implementation for both of those methods where the subsystem specific variants to register / unregister a driver have to implemented.

For instance, the PCI subsystem would call __pci_register_driver() from RegistrationOps::register and pci_unregister_driver() from DrvierOps::unregister.

Quoted 3 lines

Co-developed-by Wedson Almeida Filho <[email protected]>

Signed-off-by Wedson Almeida Filho <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

Hi Danilo,

I think there're soundness issues with this API, please see comments inlined below.

Best,
Gary

Quoted 43 lines

---
 MAINTAINERS           |   1 +
 rust/kernel/driver.rs | 117 ++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs    |   1 +
 3 files changed, 119 insertions(+)
 create mode 100644 rust/kernel/driver.rs

MAINTAINERS +1 -0

index baf0eeb9a355..2ad58ed40079 100644
@@ -7033,6 +7033,7 @@ F:	include/linux/kobj*
 F:	include/linux/property.h
 F:	lib/kobj*
 F:	rust/kernel/device.rs
+F:	rust/kernel/driver.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
 M:	Nishanth Menon <[email protected]>

rust/kernel/driver.rs new file +18 -0

new file mode 100644
index 000000000000..c1957ee7bb7e
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.).
+//!
+//! Each bus / subsystem is expected to implement [`RegistrationOps`], which allows drivers to
+//! register using the [`Registration`] class.
+
+use crate::error::{Error, Result};
+use crate::{init::PinInit, str::CStr, try_pin_init, types::Opaque, ThisModule};
+use core::pin::Pin;
+use macros::{pin_data, pinned_drop};
+
+/// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform,
+/// Amba, etc.) to provide the corresponding subsystem specific implementation to register /
+/// unregister a driver of the particular type (`RegType`).
+///
+/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call
+/// `bindings::__pci_register_driver` from `RegistrationOps::register` and

rust/kernel/driver.rs line 28

Quoted 10 lines

rust/kernel/driver.rs new file +10 -0

+/// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`.
+pub trait RegistrationOps {
+    /// The type that holds information about the registration. This is typically a struct defined
+    /// by the C portion of the kernel.
+    type RegType: Default;
+
+    /// Registers a driver.
+    ///
+    /// On success, `reg` must remain pinned and valid until the matching call to
+    /// [`RegistrationOps::unregister`].

This looks like an obligation for the caller, so this function should be unsafe?

rust/kernel/driver.rs line 35

Quoted 7 lines

rust/kernel/driver.rs new file +7 -0

+    fn register(
+        reg: &Opaque<Self::RegType>,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result;
+
+    /// Unregisters a driver previously registered with [`RegistrationOps::register`].

Similarly this is an obligation for the caller.

Quoted 26 lines

rust/kernel/driver.rs new file +25 -0

+    fn unregister(reg: &Opaque<Self::RegType>);
+}
+
+/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
+/// `bindings::pci_driver`). Therefore a [`Registration`] must be initialized with a type that
+/// implements the [`RegistrationOps`] trait, such that the generic `T::register` and
+/// `T::unregister` calls result in the subsystem specific registration calls.
+///
+///Once the `Registration` structure is dropped, the driver is unregistered.
+#[pin_data(PinnedDrop)]
+pub struct Registration<T: RegistrationOps> {
+    #[pin]
+    reg: Opaque<T::RegType>,
+}
+
+// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to
+// share references to it with multiple threads as nothing can be done.
+unsafe impl<T: RegistrationOps> Sync for Registration<T> {}
+
+// SAFETY: Both registration and unregistration are implemented in C and safe to be performed from
+// any thread, so `Registration` is `Send`.
+unsafe impl<T: RegistrationOps> Send for Registration<T> {}
+
+impl<T: RegistrationOps> Registration<T> {
+    /// Creates a new instance of the registration object.

rust/kernel/driver.rs line 65

Quoted 5 lines

rust/kernel/driver.rs new file +5 -0

+    pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {
+            reg <- Opaque::try_ffi_init(|ptr: *mut T::RegType| {
+                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
+                unsafe { ptr.write(T::RegType::default()) };

Any reason that this is initialised with a default, and not be up to T::register to initialise?

rust/kernel/driver.rs line 68

Quoted 4 lines

rust/kernel/driver.rs new file +3 -0

+
+                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write, and it has
+                // just been initialised above, so it's also valid for read.

Opaque can hold uninitialised value so as long as ptr is not dangling this is fine. There's no need to actually initialise.

Quoted 16 lines

rust/kernel/driver.rs new file +15 -0

+                let drv = unsafe { &*(ptr as *const Opaque<T::RegType>) };
+
+                T::register(drv, name, module)
+            }),
+        })
+    }
+}
+
+#[pinned_drop]
+impl<T: RegistrationOps> PinnedDrop for Registration<T> {
+    fn drop(self: Pin<&mut Self>) {
+        T::unregister(&self.reg);
+    }
+}
+

rust/kernel/driver.rs line 88

Quoted 5 lines

rust/kernel/driver.rs new file +5 -0

+/// Declares a kernel module that exposes a single driver.
+///
+/// It is meant to be used as a helper by other subsystems so they can more easily expose their own
+/// macros.
+#[macro_export]

I think this is supposed to be used by other macros only? If so, please add #[doc(hidden)].

Quoted 42 lines

rust/kernel/driver.rs new file +29 -0

+macro_rules! module_driver {
+    (<$gen_type:ident>, $driver_ops:ty, { type: $type:ty, $($f:tt)* }) => {
+        type Ops<$gen_type> = $driver_ops;
+
+        #[$crate::prelude::pin_data]
+        struct DriverModule {
+            #[pin]
+            _driver: $crate::driver::Registration<Ops<$type>>,
+        }
+
+        impl $crate::InPlaceModule for DriverModule {
+            fn init(
+                module: &'static $crate::ThisModule
+            ) -> impl $crate::init::PinInit<Self, $crate::error::Error> {
+                $crate::try_pin_init!(Self {
+                    _driver <- $crate::driver::Registration::new(
+                        <Self as $crate::ModuleMetadata>::NAME,
+                        module,
+                    ),
+                })
+            }
+        }
+
+        $crate::prelude::module! {
+            type: DriverModule,
+            $($f)*
+        }
+    }
+}

rust/kernel/lib.rs +1 -0

index 61b82b78b915..7818407f9aac 100644
@@ -35,6 +35,7 @@
 mod build_assert;
 pub mod cred;
 pub mod device;
+pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
 pub mod firmware;

Most subsystems use some kind of ID to match devices and drivers. Hence, we have to provide Rust drivers an abstraction to register an ID table for the driver to match.

Generally, those IDs are subsystem specific and hence need to be implemented by the corresponding subsystem. However, the IdArray, IdTable and RawDeviceId types provide a generalized implementation that makes the life of subsystems easier to do so.

Co-developed-by Wedson Almeida Filho <[email protected]>

Signed-off-by Wedson Almeida Filho <[email protected]>

Co-developed-by Gary Guo <[email protected]>

Signed-off-by Gary Guo <[email protected]>

Co-developed-by Fabien Parent <[email protected]>

Signed-off-by Fabien Parent <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 MAINTAINERS              |   1 +
 rust/kernel/device_id.rs | 165 +++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs       |   6 ++
 3 files changed, 172 insertions(+)
 create mode 100644 rust/kernel/device_id.rs

MAINTAINERS +1 -0

index 2ad58ed40079..3cfb68650347 100644
@@ -7033,6 +7033,7 @@ F:	include/linux/kobj*
 F:	include/linux/property.h
 F:	lib/kobj*
 F:	rust/kernel/device.rs
+F:	rust/kernel/device_id.rs
 F:	rust/kernel/driver.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)

rust/kernel/device_id.rs new file +0 -0

new file mode 100644
index 000000000000..e5859217a579
 

<snip>

rust/kernel/device_id.rs new file +4 -0

+
+/// Create device table alias for modpost.
+#[macro_export]
+macro_rules! module_device_table {

rust/kernel/device_id.rs new file +2 -0

+    };
+}

rust/kernel/lib.rs +6 -0

index 7818407f9aac..66149ac5c0c9 100644
@@ -18,6 +18,11 @@
 #![feature(inline_const)]
 #![feature(lint_reasons)]
 #![feature(unsize)]
+// Stable in Rust 1.83
+#![feature(const_maybe_uninit_as_mut_ptr)]
+#![feature(const_mut_refs)]
+#![feature(const_ptr_write)]
+#![feature(const_refs_to_cell)]
 
 // Ensure conditional compilation based on the kernel configuration works;
 // otherwise we may silently break things like initcall handling.

@@ -35,6 +40,7 @@
 mod build_assert;
 pub mod cred;
 pub mod device;
+pub mod device_id;
 pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]

Quoted 9 lines · On Thu, 19 Dec 2024 18:04:05 +0100 Danilo Krummrich <[email protected]> wrote:

Most subsystems use some kind of ID to match devices and drivers. Hence, we have to provide Rust drivers an abstraction to register an ID table for the driver to match.

Generally, those IDs are subsystem specific and hence need to be implemented by the corresponding subsystem. However, the IdArray, IdTable and RawDeviceId types provide a generalized implementation that makes the life of subsystems easier to do so.

Quoted 7 lines

Co-developed-by Wedson Almeida Filho <[email protected]>

Signed-off-by Wedson Almeida Filho <[email protected]>

Co-developed-by Gary Guo <[email protected]>

Signed-off-by Gary Guo <[email protected]>

Co-developed-by Fabien Parent <[email protected]>

Signed-off-by Fabien Parent <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

Thank you for converting my prototype to a working patch. There's a nit below.

Quoted 25 lines

---
 MAINTAINERS              |   1 +
 rust/kernel/device_id.rs | 165 +++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs       |   6 ++
 3 files changed, 172 insertions(+)
 create mode 100644 rust/kernel/device_id.rs

MAINTAINERS +1 -0

index 2ad58ed40079..3cfb68650347 100644
@@ -7033,6 +7033,7 @@ F:	include/linux/kobj*
 F:	include/linux/property.h
 F:	lib/kobj*
 F:	rust/kernel/device.rs
+F:	rust/kernel/device_id.rs
 F:	rust/kernel/driver.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)

rust/kernel/device_id.rs new file +0 -0

new file mode 100644
index 000000000000..e5859217a579
 

rust/kernel/device_id.rs line 59

Quoted 10 lines

rust/kernel/device_id.rs new file +8 -0

<snip>
 
+
+impl<T: RawDeviceId, const N: usize> RawIdArray<T, N> {
+    #[doc(hidden)]
+    pub const fn size(&self) -> usize {
+        core::mem::size_of::<Self>()
+    }
+}
+

This is not necessary, see below.

Quoted 6 lines

<snip>

rust/kernel/device_id.rs new file +4 -0

+
+/// Create device table alias for modpost.
+#[macro_export]
+macro_rules! module_device_table {

rust/kernel/device_id.rs line 163

Quoted 10 lines

rust/kernel/device_id.rs new file +10 -0

+    ($table_type: literal, $module_table_name:ident, $table_name:ident) => {
+        #[rustfmt::skip]
+        #[export_name =
+            concat!("__mod_device_table__", $table_type,
+                    "__", module_path!(),
+                    "_", line!(),
+                    "_", stringify!($table_name))
+        ]
+        static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] =
+            unsafe { core::mem::transmute_copy($table_name.raw_ids()) };

const_size_of_val will be stable in Rust 1.85, so we can start using the feature and

static $module_table_name: [core::mem::MaybeUninit<u8>; core::mem::size_of_val($table_name.raw_ids())] =

    unsafe { core::mem::transmute_copy($table_name.raw_ids()) };

should work.

Quoted 27 lines

rust/kernel/device_id.rs new file +2 -0

+    };
+}

rust/kernel/lib.rs +6 -0

index 7818407f9aac..66149ac5c0c9 100644
@@ -18,6 +18,11 @@
 #![feature(inline_const)]
 #![feature(lint_reasons)]
 #![feature(unsize)]
+// Stable in Rust 1.83
+#![feature(const_maybe_uninit_as_mut_ptr)]
+#![feature(const_mut_refs)]
+#![feature(const_ptr_write)]
+#![feature(const_refs_to_cell)]
 
 // Ensure conditional compilation based on the kernel configuration works;
 // otherwise we may silently break things like initcall handling.

@@ -35,6 +40,7 @@
 mod build_assert;
 pub mod cred;
 pub mod device;
+pub mod device_id;
 pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]

rust/kernel/device_id.rs new file +25 -0

+/// Marker trait to indicate a Rust device ID type represents a corresponding C device ID type.
+///
+/// This is meant to be implemented by buses/subsystems so that they can use [`IdTable`] to
+/// guarantee (at compile-time) zero-termination of device id tables provided by drivers.
+///
+/// # Safety
+///
+/// Implementers must ensure that:
+///   - `Self` is layout-compatible with [`RawDeviceId::RawType`]; i.e. it's safe to transmute to
+///     `RawDeviceId`.
+///
+///     This requirement is needed so `IdArray::new` can convert `Self` to `RawType` when building
+///     the ID table.
+///
+///     Ideally, this should be achieved using a const function that does conversion instead of
+///     transmute; however, const trait functions relies on `const_trait_impl` unstable feature,
+///     which is broken/gone in Rust 1.73.
+///
+///   - `DRIVER_DATA_OFFSET` is the offset of context/data field of the device ID (usually named
+///     `driver_data`) of the device ID, the field is suitable sized to write a `usize` value.
+///
+///     Similar to the previous requirement, the data should ideally be added during `Self` to
+///     `RawType` conversion, but there's currently no way to do it when using traits in const.
+pub unsafe trait RawDeviceId {
+    /// The raw type that holds the device id.

Quoted 32 lines · Danilo Krummrich

rust/kernel/device_id.rs new file +31 -0

+/// Marker trait to indicate a Rust device ID type represents a corresponding C device ID type.
+///
+/// This is meant to be implemented by buses/subsystems so that they can use [`IdTable`] to
+/// guarantee (at compile-time) zero-termination of device id tables provided by drivers.
+///
+/// # Safety
+///
+/// Implementers must ensure that:
+///   - `Self` is layout-compatible with [`RawDeviceId::RawType`]; i.e. it's safe to transmute to
+///     `RawDeviceId`.
+///
+///     This requirement is needed so `IdArray::new` can convert `Self` to `RawType` when building
+///     the ID table.
+///
+///     Ideally, this should be achieved using a const function that does conversion instead of
+///     transmute; however, const trait functions relies on `const_trait_impl` unstable feature,
+///     which is broken/gone in Rust 1.73.
+///
+///   - `DRIVER_DATA_OFFSET` is the offset of context/data field of the device ID (usually named
+///     `driver_data`) of the device ID, the field is suitable sized to write a `usize` value.
+///
+///     Similar to the previous requirement, the data should ideally be added during `Self` to
+///     `RawType` conversion, but there's currently no way to do it when using traits in const.
+pub unsafe trait RawDeviceId {
+    /// The raw type that holds the device id.
+    ///
+    /// Id tables created from [`Self`] are going to hold this type in its zero-terminated array.
+    type RawType: Copy;
+
+    /// The offset to the context/data field.
+    const DRIVER_DATA_OFFSET: usize;

rust/kernel/device_id.rs line 44

Quoted 4 lines

rust/kernel/device_id.rs new file +4 -0

+
+    /// The index stored at `DRIVER_DATA_OFFSET` of the implementor of the [`RawDeviceId`] trait.
+    fn index(&self) -> usize;
+}

Very late to the game here, but have a question about the use of OFFSET here. Why is this preferred to a method that returns a pointer to the field?

Quoted 33 lines · Tamir Duberstein

Quoted 32 lines · Danilo Krummrich

rust/kernel/device_id.rs new file +31 -0

+/// Marker trait to indicate a Rust device ID type represents a corresponding C device ID type.
+///
+/// This is meant to be implemented by buses/subsystems so that they can use [`IdTable`] to
+/// guarantee (at compile-time) zero-termination of device id tables provided by drivers.
+///
+/// # Safety
+///
+/// Implementers must ensure that:
+///   - `Self` is layout-compatible with [`RawDeviceId::RawType`]; i.e. it's safe to transmute to
+///     `RawDeviceId`.
+///
+///     This requirement is needed so `IdArray::new` can convert `Self` to `RawType` when building
+///     the ID table.
+///
+///     Ideally, this should be achieved using a const function that does conversion instead of
+///     transmute; however, const trait functions relies on `const_trait_impl` unstable feature,
+///     which is broken/gone in Rust 1.73.
+///
+///   - `DRIVER_DATA_OFFSET` is the offset of context/data field of the device ID (usually named
+///     `driver_data`) of the device ID, the field is suitable sized to write a `usize` value.
+///
+///     Similar to the previous requirement, the data should ideally be added during `Self` to
+///     `RawType` conversion, but there's currently no way to do it when using traits in const.
+pub unsafe trait RawDeviceId {
+    /// The raw type that holds the device id.
+    ///
+    /// Id tables created from [`Self`] are going to hold this type in its zero-terminated array.
+    type RawType: Copy;
+
+    /// The offset to the context/data field.
+    const DRIVER_DATA_OFFSET: usize;

Quoted 8 lines

rust/kernel/device_id.rs line 44

Quoted 4 lines

rust/kernel/device_id.rs new file +4 -0

+
+    /// The index stored at `DRIVER_DATA_OFFSET` of the implementor of the [`RawDeviceId`] trait.
+    fn index(&self) -> usize;
+}

Very late to the game here, but have a question about the use of OFFSET here. Why is this preferred to a method that returns a pointer to the field?

We need it from const context, trait methods can't be const (yet).

From: Wedson Almeida Filho <[email protected]>

Add a simple abstraction to guard critical code sections with an rcu read lock.

Reviewed-by Boqun Feng <[email protected]>

Signed-off-by Wedson Almeida Filho <[email protected]>

Co-developed-by Danilo Krummrich <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 MAINTAINERS             |  1 +
 rust/helpers/helpers.c  |  1 +
 rust/helpers/rcu.c      | 13 ++++++++++++
 rust/kernel/sync.rs     |  1 +
 rust/kernel/sync/rcu.rs | 47 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 63 insertions(+)
 create mode 100644 rust/helpers/rcu.c
 create mode 100644 rust/kernel/sync/rcu.rs

MAINTAINERS +1 -0

index 3cfb68650347..0cc69e282889 100644
@@ -19690,6 +19690,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
 F:	Documentation/RCU/
 F:	include/linux/rcu*
 F:	kernel/rcu/
+F:	rust/kernel/sync/rcu.rs
 X:	Documentation/RCU/torture.rst
 X:	include/linux/srcu*.h
 X:	kernel/rcu/srcu*.c

rust/helpers/helpers.c +1 -0

index dcf827a61b52..060750af6524 100644
@@ -20,6 +20,7 @@
 #include "page.c"
 #include "pid_namespace.c"
 #include "rbtree.c"
+#include "rcu.c"
 #include "refcount.c"
 #include "security.c"
 #include "signal.c"

rust/helpers/rcu.c new file +13 -0

new file mode 100644
index 000000000000..f1cec6583513
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/rcupdate.h>
+
+void rust_helper_rcu_read_lock(void)
+{
+	rcu_read_lock();
+}
+
+void rust_helper_rcu_read_unlock(void)
+{
+	rcu_read_unlock();
+}

rust/kernel/sync.rs +1 -0

index 1eab7ebf25fd..0654008198b2 100644
@@ -12,6 +12,7 @@
 pub mod lock;
 mod locked_by;
 pub mod poll;
+pub mod rcu;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
 pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};

rust/kernel/sync/rcu.rs new file +18 -0

new file mode 100644
index 000000000000..b51d9150ffe2
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! RCU support.
+//!
+//! C header: [`include/linux/rcupdate.h`](srctree/include/linux/rcupdate.h)
+
+use crate::{bindings, types::NotThreadSafe};
+
+/// Evidence that the RCU read side lock is held on the current thread/CPU.
+///
+/// The type is explicitly not `Send` because this property is per-thread/CPU.
+///
+/// # Invariants
+///
+/// The RCU read side lock is actually held while instances of this guard exist.
+pub struct Guard(NotThreadSafe);
+
+impl Guard {

rust/kernel/sync/rcu.rs new file +12 -0

+
+impl Drop for Guard {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, the RCU read side is locked, so it is ok to unlock it.
+        unsafe { bindings::rcu_read_unlock() };
+    }
+}
+
+/// Acquires the RCU read side lock.
+pub fn read_lock() -> Guard {
+    Guard::new()
+}  

Quoted 9 lines · On Thu, 19 Dec 2024 18:04:06 +0100 Danilo Krummrich <[email protected]> wrote:

From: Wedson Almeida Filho <[email protected]>

Add a simple abstraction to guard critical code sections with an rcu read lock.

Reviewed-by Boqun Feng <[email protected]>

Signed-off-by Wedson Almeida Filho <[email protected]>

Co-developed-by Danilo Krummrich <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

Quoted 9 lines

---
 MAINTAINERS             |  1 +
 rust/helpers/helpers.c  |  1 +
 rust/helpers/rcu.c      | 13 ++++++++++++
 rust/kernel/sync.rs     |  1 +
 rust/kernel/sync/rcu.rs | 47 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 63 insertions(+)
 create mode 100644 rust/helpers/rcu.c
 create mode 100644 rust/kernel/sync/rcu.rs

[resend to the list]

Quoted 85 lines

MAINTAINERS +1 -0

index 3cfb68650347..0cc69e282889 100644
@@ -19690,6 +19690,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev
 F:	Documentation/RCU/
 F:	include/linux/rcu*
 F:	kernel/rcu/
+F:	rust/kernel/sync/rcu.rs
 X:	Documentation/RCU/torture.rst
 X:	include/linux/srcu*.h
 X:	kernel/rcu/srcu*.c

rust/helpers/helpers.c +1 -0

index dcf827a61b52..060750af6524 100644
@@ -20,6 +20,7 @@
 #include "page.c"
 #include "pid_namespace.c"
 #include "rbtree.c"
+#include "rcu.c"
 #include "refcount.c"
 #include "security.c"
 #include "signal.c"

rust/helpers/rcu.c new file +13 -0

new file mode 100644
index 000000000000..f1cec6583513
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/rcupdate.h>
+
+void rust_helper_rcu_read_lock(void)
+{
+	rcu_read_lock();
+}
+
+void rust_helper_rcu_read_unlock(void)
+{
+	rcu_read_unlock();
+}

rust/kernel/sync.rs +1 -0

index 1eab7ebf25fd..0654008198b2 100644
@@ -12,6 +12,7 @@
 pub mod lock;
 mod locked_by;
 pub mod poll;
+pub mod rcu;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
 pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};

rust/kernel/sync/rcu.rs new file +23 -0

new file mode 100644
index 000000000000..b51d9150ffe2
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! RCU support.
+//!
+//! C header: [`include/linux/rcupdate.h`](srctree/include/linux/rcupdate.h)
+
+use crate::{bindings, types::NotThreadSafe};
+
+/// Evidence that the RCU read side lock is held on the current thread/CPU.
+///
+/// The type is explicitly not `Send` because this property is per-thread/CPU.
+///
+/// # Invariants
+///
+/// The RCU read side lock is actually held while instances of this guard exist.
+pub struct Guard(NotThreadSafe);
+
+impl Guard {
+    /// Acquires the RCU read side lock and returns a guard.
+    pub fn new() -> Self {
+        // SAFETY: An FFI call with no additional requirements.
+        unsafe { bindings::rcu_read_lock() };
+        // INVARIANT: The RCU read side lock was just acquired above.

rust/kernel/sync/rcu.rs line 28

Quoted 5 lines

rust/kernel/sync/rcu.rs new file +5 -0

+        Self(NotThreadSafe)
+    }
+
+    /// Explicitly releases the RCU read side lock.
+    pub fn unlock(self) {}  

I don't think there's need for this, drop(rcu_guard) is equally clear.

There was a debate in Rust community about explicit lock methods, but the conclusion was to not have it,
see https://github.com/rust-lang/rust/issues/81872.

rust/kernel/sync/rcu.rs line 35

Quoted 7 lines

rust/kernel/sync/rcu.rs new file +7 -0

+}
+
+impl Default for Guard {
+    fn default() -> Self {
+        Self::new()
+    }
+}  

I don't think anyone would like to implicit acquire an RCU guard! I believe you included this because clippy is yelling, but in this case you shouldn't listen to clippy. Either suppress the warning or rename new to lock.

Quoted 13 lines

rust/kernel/sync/rcu.rs new file +12 -0

+
+impl Drop for Guard {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, the RCU read side is locked, so it is ok to unlock it.
+        unsafe { bindings::rcu_read_unlock() };
+    }
+}
+
+/// Acquires the RCU read side lock.
+pub fn read_lock() -> Guard {
+    Guard::new()
+}  

Analogous to Opaque::new add Opaque::pin_init, which instead of a value T takes a PinInit<T> and returns a PinInit<Opaque<T>>.

Reviewed-by Alice Ryhl <[email protected]>

Suggested-by Alice Ryhl <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/types.rs | 11 +++++++++++
 1 file changed, 11 insertions(+)

rust/kernel/types.rs +11 -0

index ec6457bb3084..3aea6af9a0bc 100644
@@ -281,6 +281,17 @@ pub const fn uninit() -> Self {
         }
     }
 
+    /// Create an opaque pin-initializer from the given pin-initializer.
+    pub fn pin_init(slot: impl PinInit<T>) -> impl PinInit<Self> {
+        Self::ffi_init(|ptr: *mut T| {
+            // SAFETY:
+            //   - `ptr` is a valid pointer to uninitialized memory,
+            //   - `slot` is not accessed on error; the call is infallible,
+            //   - `slot` is pinned in memory.
+            let _ = unsafe { init::PinInit::<T>::__pinned_init(slot, ptr) };
+        })
+    }
+
     /// Creates a pin-initializer from the given initializer closure.
     ///
     /// The returned initializer calls the given closure with the pointer to the inner `T` of this

I/O memory is typically either mapped through direct calls to ioremap() or subsystem / bus specific ones such as pci_iomap().

Even though subsystem / bus specific functions to map I/O memory are based on ioremap() / iounmap() it is not desirable to re-implement them in Rust.

Instead, implement a base type for I/O mapped memory, which generically provides the corresponding accessors, such as Io::readb or Io:try_readb.

Io supports an optional const generic, such that a driver can indicate the minimal expected and required size of the mapping at compile time. Correspondingly, calls to the 'non-try' accessors, support compile time checks of the I/O memory offset to read / write, while the 'try' accessors, provide boundary checks on runtime.

IoRaw is meant to be embedded into a structure (e.g. pci::Bar or io::IoMem) which creates the actual I/O memory mapping and initializes IoRaw accordingly.

To ensure that I/O mapped memory can't out-live the device it may be bound to, subsystems must embed the corresponding I/O memory type (e.g. pci::Bar) into a Devres container, such that it gets revoked once the device is unbound.

Reviewed-by Alice Ryhl <[email protected]>

Tested-by Daniel Almeida <[email protected]>

Reviewed-by Daniel Almeida <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/helpers/helpers.c |   1 +
 rust/helpers/io.c      | 101 ++++++++++++++++
 rust/kernel/io.rs      | 260 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   1 +
 4 files changed, 363 insertions(+)
 create mode 100644 rust/helpers/io.c
 create mode 100644 rust/kernel/io.rs

rust/helpers/helpers.c +1 -0

index 060750af6524..63f9b1da179f 100644
@@ -14,6 +14,7 @@
 #include "cred.c"
 #include "err.c"
 #include "fs.c"
+#include "io.c"
 #include "jump_label.c"
 #include "kunit.c"
 #include "mutex.c"

rust/helpers/io.c new file +101 -0

new file mode 100644
index 000000000000..4c2401ccd720
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/io.h>
+
+void __iomem *rust_helper_ioremap(phys_addr_t offset, size_t size)
+{
+	return ioremap(offset, size);
+}
+
+void rust_helper_iounmap(volatile void __iomem *addr)
+{
+	iounmap(addr);
+}
+
+u8 rust_helper_readb(const volatile void __iomem *addr)
+{
+	return readb(addr);
+}
+
+u16 rust_helper_readw(const volatile void __iomem *addr)
+{
+	return readw(addr);
+}
+
+u32 rust_helper_readl(const volatile void __iomem *addr)
+{
+	return readl(addr);
+}
+
+#ifdef CONFIG_64BIT
+u64 rust_helper_readq(const volatile void __iomem *addr)
+{
+	return readq(addr);
+}
+#endif
+
+void rust_helper_writeb(u8 value, volatile void __iomem *addr)
+{
+	writeb(value, addr);
+}
+
+void rust_helper_writew(u16 value, volatile void __iomem *addr)
+{
+	writew(value, addr);
+}
+
+void rust_helper_writel(u32 value, volatile void __iomem *addr)
+{
+	writel(value, addr);
+}
+
+#ifdef CONFIG_64BIT
+void rust_helper_writeq(u64 value, volatile void __iomem *addr)
+{
+	writeq(value, addr);
+}
+#endif
+
+u8 rust_helper_readb_relaxed(const volatile void __iomem *addr)
+{
+	return readb_relaxed(addr);
+}
+
+u16 rust_helper_readw_relaxed(const volatile void __iomem *addr)
+{
+	return readw_relaxed(addr);
+}
+
+u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
+{
+	return readl_relaxed(addr);
+}
+
+#ifdef CONFIG_64BIT
+u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
+{
+	return readq_relaxed(addr);
+}
+#endif
+
+void rust_helper_writeb_relaxed(u8 value, volatile void __iomem *addr)
+{
+	writeb_relaxed(value, addr);
+}
+
+void rust_helper_writew_relaxed(u16 value, volatile void __iomem *addr)
+{
+	writew_relaxed(value, addr);
+}
+
+void rust_helper_writel_relaxed(u32 value, volatile void __iomem *addr)
+{
+	writel_relaxed(value, addr);
+}
+
+#ifdef CONFIG_64BIT
+void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
+{
+	writeq_relaxed(value, addr);
+}
+#endif

rust/kernel/io.rs new file +209 -0

new file mode 100644
index 000000000000..d4a73e52e3ee
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Memory-mapped IO.
+//!
+//! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h)
+
+use crate::error::{code::EINVAL, Result};
+use crate::{bindings, build_assert};
+
+/// Raw representation of an MMIO region.
+///
+/// By itself, the existence of an instance of this structure does not provide any guarantees that
+/// the represented MMIO region does exist or is properly mapped.
+///
+/// Instead, the bus specific MMIO implementation must convert this raw representation into an `Io`
+/// instance providing the actual memory accessors. Only by the conversion into an `Io` structure
+/// any guarantees are given.
+pub struct IoRaw<const SIZE: usize = 0> {
+    addr: usize,
+    maxsize: usize,
+}
+
+impl<const SIZE: usize> IoRaw<SIZE> {
+    /// Returns a new `IoRaw` instance on success, an error otherwise.
+    pub fn new(addr: usize, maxsize: usize) -> Result<Self> {
+        if maxsize < SIZE {
+            return Err(EINVAL);
+        }
+
+        Ok(Self { addr, maxsize })
+    }
+
+    /// Returns the base address of the MMIO region.
+    #[inline]
+    pub fn addr(&self) -> usize {
+        self.addr
+    }
+
+    /// Returns the maximum size of the MMIO region.
+    #[inline]
+    pub fn maxsize(&self) -> usize {
+        self.maxsize
+    }
+}
+
+/// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
+///
+/// The creator (usually a subsystem / bus such as PCI) is responsible for creating the
+/// mapping, performing an additional region request etc.
+///
+/// # Invariant
+///
+/// `addr` is the start and `maxsize` the length of valid I/O mapped memory region of size
+/// `maxsize`.
+///
+/// # Examples
+///
+/// ```no_run
+/// # use kernel::{bindings, io::{Io, IoRaw}};
+/// # use core::ops::Deref;
+///
+/// // See also [`pci::Bar`] for a real example.
+/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
+///
+/// impl<const SIZE: usize> IoMem<SIZE> {
+///     /// # Safety
+///     ///
+///     /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
+///     /// virtual address space.
+///     unsafe fn new(paddr: usize) -> Result<Self>{
+///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
+///         // valid for `ioremap`.
+///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) };
+///         if addr.is_null() {
+///             return Err(ENOMEM);
+///         }
+///
+///         Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
+///     }
+/// }
+///
+/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
+///     fn drop(&mut self) {
+///         // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
+///         unsafe { bindings::iounmap(self.0.addr() as _); };
+///     }
+/// }
+///
+/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
+///    type Target = Io<SIZE>;
+///
+///    fn deref(&self) -> &Self::Target {
+///         // SAFETY: The memory range stored in `self` has been properly mapped in `Self::new`.
+///         unsafe { Io::from_raw(&self.0) }
+///    }
+/// }
+///
+///# fn no_run() -> Result<(), Error> {
+/// // SAFETY: Invalid usage for example purposes.
+/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
+/// iomem.writel(0x42, 0x0);
+/// assert!(iomem.try_writel(0x42, 0x0).is_ok());
+/// assert!(iomem.try_writel(0x42, 0x4).is_err());
+/// # Ok(())
+/// # }
+/// ```
+#[repr(transparent)]
+pub struct Io<const SIZE: usize = 0>(IoRaw<SIZE>);
+
+macro_rules! define_read {
+    ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
+        /// Read IO data from a given offset known at compile time.
+        ///
+        /// Bound checks are performed on compile time, hence if the offset is not known at compile
+        /// time, the build will fail.
+        $(#[$attr])*
+        #[inline]
+        pub fn $name(&self, offset: usize) -> $type_name {
+            let addr = self.io_addr_assert::<$type_name>(offset);
+
+            // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+            unsafe { bindings::$name(addr as _) }
+        }
+
+        /// Read IO data from a given offset.
+        ///
+        /// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
+        /// out of bounds.
+        $(#[$attr])*
+        pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
+            let addr = self.io_addr::<$type_name>(offset)?;
+
+            // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+            Ok(unsafe { bindings::$name(addr as _) })
+        }
+    };
+}
+
+macro_rules! define_write {
+    ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
+        /// Write IO data from a given offset known at compile time.
+        ///
+        /// Bound checks are performed on compile time, hence if the offset is not known at compile
+        /// time, the build will fail.
+        $(#[$attr])*
+        #[inline]
+        pub fn $name(&self, value: $type_name, offset: usize) {
+            let addr = self.io_addr_assert::<$type_name>(offset);
+
+            // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+            unsafe { bindings::$name(value, addr as _, ) }
+        }
+
+        /// Write IO data from a given offset.
+        ///
+        /// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
+        /// out of bounds.
+        $(#[$attr])*
+        pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
+            let addr = self.io_addr::<$type_name>(offset)?;
+
+            // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+            unsafe { bindings::$name(value, addr as _) }
+            Ok(())
+        }
+    };
+}
+
+impl<const SIZE: usize> Io<SIZE> {
+    /// Converts an `IoRaw` into an `Io` instance, providing the accessors to the MMIO mapping.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
+    /// `maxsize`.
+    pub unsafe fn from_raw(raw: &IoRaw<SIZE>) -> &Self {
+        // SAFETY: `Io` is a transparent wrapper around `IoRaw`.
+        unsafe { &*core::ptr::from_ref(raw).cast() }
+    }
+
+    /// Returns the base address of this mapping.
+    #[inline]
+    pub fn addr(&self) -> usize {
+        self.0.addr()
+    }
+
+    /// Returns the maximum size of this mapping.
+    #[inline]
+    pub fn maxsize(&self) -> usize {
+        self.0.maxsize()
+    }
+
+    #[inline]
+    const fn offset_valid<U>(offset: usize, size: usize) -> bool {
+        let type_size = core::mem::size_of::<U>();
+        if let Some(end) = offset.checked_add(type_size) {
+            end <= size && offset % type_size == 0
+        } else {
+            false
+        }
+    }
+
+    #[inline]
+    fn io_addr<U>(&self, offset: usize) -> Result<usize> {
+        if !Self::offset_valid::<U>(offset, self.maxsize()) {
+            return Err(EINVAL);
+        }
+
+        // Probably no need to check, since the safety requirements of `Self::new` guarantee that

rust/kernel/io.rs new file +41 -0

+
+    define_read!(readb, try_readb, u8);
+    define_read!(readw, try_readw, u16);
+    define_read!(readl, try_readl, u32);
+    define_read!(
+        #[cfg(CONFIG_64BIT)]
+        readq,
+        try_readq,
+        u64
+    );
+
+    define_read!(readb_relaxed, try_readb_relaxed, u8);
+    define_read!(readw_relaxed, try_readw_relaxed, u16);
+    define_read!(readl_relaxed, try_readl_relaxed, u32);
+    define_read!(
+        #[cfg(CONFIG_64BIT)]
+        readq_relaxed,
+        try_readq_relaxed,
+        u64
+    );
+
+    define_write!(writeb, try_writeb, u8);
+    define_write!(writew, try_writew, u16);
+    define_write!(writel, try_writel, u32);
+    define_write!(
+        #[cfg(CONFIG_64BIT)]
+        writeq,
+        try_writeq,
+        u64
+    );
+
+    define_write!(writeb_relaxed, try_writeb_relaxed, u8);
+    define_write!(writew_relaxed, try_writew_relaxed, u16);
+    define_write!(writel_relaxed, try_writel_relaxed, u32);
+    define_write!(
+        #[cfg(CONFIG_64BIT)]
+        writeq_relaxed,
+        try_writeq_relaxed,
+        u64
+    );
+}

rust/kernel/lib.rs +1 -0

index 5702ce32ec8e..6c836ab73771 100644
@@ -79,6 +79,7 @@
 
 #[doc(hidden)]
 pub use bindings;
+pub mod io;
 pub use macros;
 pub use uapi;
 

Signature · 1 line

2.47.1

Quoted 48 lines · Danilo Krummrich

rust/kernel/io.rs new file +48 -0

+impl<const SIZE: usize> Io<SIZE> {
+    /// Converts an `IoRaw` into an `Io` instance, providing the accessors to the MMIO mapping.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
+    /// `maxsize`.
+    pub unsafe fn from_raw(raw: &IoRaw<SIZE>) -> &Self {
+        // SAFETY: `Io` is a transparent wrapper around `IoRaw`.
+        unsafe { &*core::ptr::from_ref(raw).cast() }
+    }
+
+    /// Returns the base address of this mapping.
+    #[inline]
+    pub fn addr(&self) -> usize {
+        self.0.addr()
+    }
+
+    /// Returns the maximum size of this mapping.
+    #[inline]
+    pub fn maxsize(&self) -> usize {
+        self.0.maxsize()
+    }
+
+    #[inline]
+    const fn offset_valid<U>(offset: usize, size: usize) -> bool {
+        let type_size = core::mem::size_of::<U>();
+        if let Some(end) = offset.checked_add(type_size) {
+            end <= size && offset % type_size == 0
+        } else {
+            false
+        }
+    }
+
+    #[inline]
+    fn io_addr<U>(&self, offset: usize) -> Result<usize> {
+        if !Self::offset_valid::<U>(offset, self.maxsize()) {
+            return Err(EINVAL);
+        }
+
+        // Probably no need to check, since the safety requirements of `Self::new` guarantee that
+        // this can't overflow.
+        self.addr().checked_add(offset).ok_or(EINVAL)
+    }
+
+    #[inline]
+    fn io_addr_assert<U>(&self, offset: usize) -> usize {
+        build_assert!(Self::offset_valid::<U>(offset, SIZE));

rust/kernel/io.rs line 219

Quoted 3 lines

rust/kernel/io.rs new file +3 -0

+
+        self.addr() + offset
+    }

Currently reworking the portmem abstractions I wrote for the LED/SE10 driver.
Right now I’m wondering if it would make sense to move the 3 functions above (offset_valid, io_addr and io_addr_assert into IoRaw), as I’m considering reusing the IoRaw in the portmem and then just offer the outb/outw/outl functions on a wraping type around IoRaw. For this I would also use the same functions to check bounds.

I/O memory is typically either mapped through direct calls to ioremap() or subsystem / bus specific ones such as pci_iomap().

Even though subsystem / bus specific functions to map I/O memory are based on ioremap() / iounmap() it is not desirable to re-implement them in Rust.

Instead, implement a base type for I/O mapped memory, which generically provides the corresponding accessors, such as Io::readb or Io:try_readb.

Io supports an optional const generic, such that a driver can indicate the minimal expected and required size of the mapping at compile time. Correspondingly, calls to the 'non-try' accessors, support compile time checks of the I/O memory offset to read / write, while the 'try' accessors, provide boundary checks on runtime.

IoRaw is meant to be embedded into a structure (e.g. pci::Bar or io::IoMem) which creates the actual I/O memory mapping and initializes IoRaw accordingly.

To ensure that I/O mapped memory can't out-live the device it may be bound to, subsystems must embed the corresponding I/O memory type (e.g. pci::Bar) into a Devres container, such that it gets revoked once the device is unbound.

rust/kernel/io.rs new file +141 -0

+
+            // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+            unsafe { bindings::$name(addr as _) }
+        }
+
+        /// Read IO data from a given offset.
+        ///
+        /// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
+        /// out of bounds.
+        $(#[$attr])*
+        pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
+            let addr = self.io_addr::<$type_name>(offset)?;
+
+            // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+            Ok(unsafe { bindings::$name(addr as _) })
+        }
+    };
+}
+
+macro_rules! define_write {
+    ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
+        /// Write IO data from a given offset known at compile time.
+        ///
+        /// Bound checks are performed on compile time, hence if the offset is not known at compile
+        /// time, the build will fail.
+        $(#[$attr])*
+        #[inline]
+        pub fn $name(&self, value: $type_name, offset: usize) {
+            let addr = self.io_addr_assert::<$type_name>(offset);
+
+            // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+            unsafe { bindings::$name(value, addr as _, ) }
+        }
+
+        /// Write IO data from a given offset.
+        ///
+        /// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
+        /// out of bounds.
+        $(#[$attr])*
+        pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
+            let addr = self.io_addr::<$type_name>(offset)?;
+
+            // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+            unsafe { bindings::$name(value, addr as _) }
+            Ok(())
+        }
+    };
+}
+
+impl<const SIZE: usize> Io<SIZE> {
+    /// Converts an `IoRaw` into an `Io` instance, providing the accessors to the MMIO mapping.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
+    /// `maxsize`.
+    pub unsafe fn from_raw(raw: &IoRaw<SIZE>) -> &Self {
+        // SAFETY: `Io` is a transparent wrapper around `IoRaw`.
+        unsafe { &*core::ptr::from_ref(raw).cast() }
+    }
+
+    /// Returns the base address of this mapping.
+    #[inline]
+    pub fn addr(&self) -> usize {
+        self.0.addr()
+    }
+
+    /// Returns the maximum size of this mapping.
+    #[inline]
+    pub fn maxsize(&self) -> usize {
+        self.0.maxsize()
+    }
+
+    #[inline]
+    const fn offset_valid<U>(offset: usize, size: usize) -> bool {
+        let type_size = core::mem::size_of::<U>();
+        if let Some(end) = offset.checked_add(type_size) {
+            end <= size && offset % type_size == 0
+        } else {
+            false
+        }
+    }
+
+    #[inline]
+    fn io_addr<U>(&self, offset: usize) -> Result<usize> {
+        if !Self::offset_valid::<U>(offset, self.maxsize()) {
+            return Err(EINVAL);
+        }
+
+        // Probably no need to check, since the safety requirements of `Self::new` guarantee that
+        // this can't overflow.
+        self.addr().checked_add(offset).ok_or(EINVAL)
+    }
+
+    #[inline]
+    fn io_addr_assert<U>(&self, offset: usize) -> usize {
+        build_assert!(Self::offset_valid::<U>(offset, SIZE));
+
+        self.addr() + offset
+    }
+
+    define_read!(readb, try_readb, u8);
+    define_read!(readw, try_readw, u16);
+    define_read!(readl, try_readl, u32);
+    define_read!(
+        #[cfg(CONFIG_64BIT)]
+        readq,
+        try_readq,
+        u64
+    );
+
+    define_read!(readb_relaxed, try_readb_relaxed, u8);
+    define_read!(readw_relaxed, try_readw_relaxed, u16);
+    define_read!(readl_relaxed, try_readl_relaxed, u32);
+    define_read!(
+        #[cfg(CONFIG_64BIT)]
+        readq_relaxed,
+        try_readq_relaxed,
+        u64
+    );
+
+    define_write!(writeb, try_writeb, u8);
+    define_write!(writew, try_writew, u16);
+    define_write!(writel, try_writel, u32);
+    define_write!(
+        #[cfg(CONFIG_64BIT)]
+        writeq,
+        try_writeq,
+        u64
+    );
+
+    define_write!(writeb_relaxed, try_writeb_relaxed, u8);
+    define_write!(writew_relaxed, try_writew_relaxed, u16);
+    define_write!(writel_relaxed, try_writel_relaxed, u32);
+    define_write!(
+        #[cfg(CONFIG_64BIT)]
+        writeq_relaxed,
+        try_writeq_relaxed,
+        u64
+    );
+}

rust/kernel/lib.rs +1 -0

index 5702ce32ec8e..6c836ab73771 100644
@@ -79,6 +79,7 @@
 
 #[doc(hidden)]
 pub use bindings;
+pub mod io;
 pub use macros;
 pub use uapi;
 

Signature · 1 line

2.47.1

Is this a known issue or limitation? Or is this a bug/rough edge that still needs fixing? Or alternatively am I just doing something wrong? Would appreciate any insights as figuring out what I'd done wrong here was a bit of a rough introduction!

Hi Alistair,

Quoted 5 lines · Alistair Popple

Is this a known issue or limitation? Or is this a bug/rough edge that still needs fixing? Or alternatively am I just doing something wrong? Would appreciate any insights as figuring out what I'd done wrong here was a bit of a rough introduction!

Yeah, it is a result of our build_assert! machinery:

https://rust.docs.kernel.org/kernel/macro.build_assert.html

which works by producing a build (link) error rather than the usual compiler error and thus the bad error message.

build_assert! is really the biggest hammer we have to assert something is true at build time, since it may rely on the optimizer. For instance, if static_assert! is usable in that context, it should be instead of build_assert!.

Ideally we would have a way to get the message propagated somehow, because it is indeed confusing.

I hope that helps.

Cheers,
Miguel

Kind of, but given the current state of build_assert's and the impossiblity of debugging them should we avoid adding them until they can be fixed?

Unless the code absolutely cannot compile without them I think it would be better to turn them into runtime errors that can at least hint at what might have gone wrong. For example I think a run-time check would have been much more appropriate and easy to debug here, rather than having to bisect my changes.

I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes, but testing with that also didn't yeild great results - it creates a backtrace but that doesn't seem to point anywhere terribly close to where the bad access was, I'm guessing maybe due to inlining and other optimisations - or is decode_stacktrace.sh not the right tool for this job?

Quoted 2 lines · Alistair Popple

Kind of, but given the current state of build_assert's and the impossiblity of debugging them should we avoid adding them until they can be fixed?

If you build the module as built-in the linker gives you some more hints, but I agree it's still not great.

I think build_assert() is not widely used yet and, until the situation improves, we could also keep a list of common pitfalls if that helps?

Quoted 4 lines

Unless the code absolutely cannot compile without them I think it would be better to turn them into runtime errors that can at least hint at what might have gone wrong. For example I think a run-time check would have been much more appropriate and easy to debug here, rather than having to bisect my changes.

No, especially for I/O the whole purpose of the non-try APIs is to ensure that boundary checks happen at compile time.

Quoted 5 lines

I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes, but testing with that also didn't yeild great results - it creates a backtrace but that doesn't seem to point anywhere terribly close to where the bad access was, I'm guessing maybe due to inlining and other optimisations - or is decode_stacktrace.sh not the right tool for this job?

I was about to suggest CONFIG_RUST_BUILD_ASSERT_ALLOW=y to you, since this will make the kernel panic when hitting a build_assert().

I gave this a quick try with [1] in qemu and it lead to the following hint, right before the oops:

[    0.957932] rust_kernel: panicked at /home/danilo/projects/linux/nova/nova-next/rust/kernel/io.rs:216:9:

Seeing this immediately tells me that I'm trying to do out of bound I/O accesses in my driver, which indeed doesn't tell me the exact line (in case things are inlined too much to gather it from the backtrace of the oops), but it should be good enough, no?

or is decode_stacktrace.sh not the right tool for this job?

[1]

samples/rust/rust_driver_pci.rs +1 -1

index 1fb6e44f3395..2ff3af11d711 100644
@@ -13,7 +13,7 @@ impl Regs {
     const OFFSET: usize = 0x4;
     const DATA: usize = 0x8;
     const COUNT: usize = 0xC;
-    const END: usize = 0x10;
+    const END: usize = 0x2;
 }
 
 type Bar0 = pci::Bar<{ Regs::END }>;

Quoted 4 lines · Danilo Krummrich

On Tue, Feb 25, 2025 at 04:50:05PM +1100, Alistair Popple wrote:

I think build_assert() is not widely used yet and, until the situation improves, we could also keep a list of common pitfalls if that helps?

I've asked a few times, but are there any plans/ideas on how to improve the situation?

I'm kind of suprised we're building things on top of a fairly broken feature without an idea of how we might make that feature work. I'd love to help, but being new to R4L no immediately useful ideas come to mind.

Quoted 5 lines

Quoted 4 lines

Unless the code absolutely cannot compile without them I think it would be better to turn them into runtime errors that can at least hint at what might have gone wrong. For example I think a run-time check would have been much more appropriate and easy to debug here, rather than having to bisect my changes.

Quoted 2 lines

No, especially for I/O the whole purpose of the non-try APIs is to ensure that boundary checks happen at compile time.

To be honest I don't really understand the utility here because the compile-time check can't be a definitive check. You're always going to have to fallback to a run-time check because at least for PCI (and likely others) you can't know for at compile time if the IO region is big enough or matches the compile-time constraint.

So this seems more like a quiz for developers to check if they really do want to access the given offset. It's not really doing any useful compile-time bounds check that is preventing something bad from happening, becasue that has to happen at run-time. Especially as the whole BAR is mapped anyway.

Hence why I think an obvious run-time error instead of an obtuse and difficult to figure out build error would be better. But maybe I'm missing some usecase here that makes this more useful.

Quoted 13 lines

Quoted 6 lines

I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes, but testing with that also didn't yeild great results - it creates a backtrace but that doesn't seem to point anywhere terribly close to where the bad access was, I'm guessing maybe due to inlining and other optimisations - or is decode_stacktrace.sh not the right tool for this job?

I was about to suggest CONFIG_RUST_BUILD_ASSERT_ALLOW=y to you, since this will make the kernel panic when hitting a build_assert().

I gave this a quick try with [1] in qemu and it lead to the following hint, right before the oops:

Quoted 6 lines

[    0.957932] rust_kernel: panicked at /home/danilo/projects/linux/nova/nova-next/rust/kernel/io.rs:216:9:

Seeing this immediately tells me that I'm trying to do out of bound I/O accesses in my driver, which indeed doesn't tell me the exact line (in case things are inlined too much to gather it from the backtrace of the oops), but it should be good enough, no?

*smacks forehead*

Yes. So to answer this question:

Quoted 1 line

or is decode_stacktrace.sh not the right tool for this job?

No, it isn't. Just reading the kernel logs properly would have been a better option! I guess coming from C I'm just too used to jumping straight to the stack trace in the case of BUG_ON(), etc. Thanks for point that out.

Quoted 8 lines · Alistair Popple

Quoted 4 lines · Danilo Krummrich

On Tue, Feb 25, 2025 at 04:50:05PM +1100, Alistair Popple wrote:

I think build_assert() is not widely used yet and, until the situation improves, we could also keep a list of common pitfalls if that helps?

I've asked a few times, but are there any plans/ideas on how to improve the situation?

I just proposed a few ones. If the limitation can be resolved I don't know.

There are two different cases.

(1) build_assert() is evaluated in const context to false
(2) the compiler can't guarantee that build_assert() is evaluated to true at compile time

For (1) you get a proper backtrace by the compiler. For (2) there's currently only the option to make the linker fail, which doesn't produce the most useful output.

If we wouldn't do (2) we'd cause a kernel panic on runtime, which can be enforced with CONFIG_RUST_BUILD_ASSERT_ALLOW=y.

Quoted 3 lines

I'm kind of suprised we're building things on top of a fairly broken feature without an idea of how we might make that feature work. I'd love to help, but being new to R4L no immediately useful ideas come to mind.

The feature is not broken at all, it works perfectly fine. It's just that for
(2) it has an ergonomic limitation.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Unless the code absolutely cannot compile without them I think it would be better to turn them into runtime errors that can at least hint at what might have gone wrong. For example I think a run-time check would have been much more appropriate and easy to debug here, rather than having to bisect my changes.

Quoted 8 lines

Quoted 2 lines

No, especially for I/O the whole purpose of the non-try APIs is to ensure that boundary checks happen at compile time.

To be honest I don't really understand the utility here because the compile-time check can't be a definitive check. You're always going to have to fallback to a run-time check because at least for PCI (and likely others) you can't know for at compile time if the IO region is big enough or matches the compile-time constraint.

That's not true, let me explain.

When you write a driver, you absolutely have to know the register layout. This means that you also know what the minimum PCI bar size has to be for your driver to work. If it would be smaller than what your driver expects, it can't function anyways. In Rust we make use of this fact.

When you map a PCI bar through pdev.iomap_region_sized you pass in a const generic (SIZE) representing the *expected* PCI bar size. This can indeed fail on run-time, but that's fine, as mentioned, if the bar is smaller than what your driver expect, it's useless anyways.

If the call succeeds, it means that the actual PCI bar size is greater or equal to SIZE. Since SIZE is known at compile time all subsequent I/O operations can be boundary checked against SIZE at compile time, which additionally makes the call infallible. This works for most I/O operations drivers do.

However, sometimes we need to do I/O ops at a PCI bar offset that is only known at run-time. In this case you can use the try_* variants, such as try_read32(). Those do boundary checks against the actual size of the PCI bar, which is only known at run-time and hence they're fallible.

Quoted 5 lines

So this seems more like a quiz for developers to check if they really do want to access the given offset. It's not really doing any useful compile-time bounds check that is preventing something bad from happening, becasue that has to happen at run-time. Especially as the whole BAR is mapped anyway.

See the explanation above.

Quoted 4 lines

Hence why I think an obvious run-time error instead of an obtuse and difficult to figure out build error would be better. But maybe I'm missing some usecase here that makes this more useful.

No, failing the boundary check at compile time (if possible) is always better than failing it at run-time for obvious reasons.

Quoted 22 lines

Quoted 13 lines

Quoted 6 lines

I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes, but testing with that also didn't yeild great results - it creates a backtrace but that doesn't seem to point anywhere terribly close to where the bad access was, I'm guessing maybe due to inlining and other optimisations - or is decode_stacktrace.sh not the right tool for this job?

I was about to suggest CONFIG_RUST_BUILD_ASSERT_ALLOW=y to you, since this will make the kernel panic when hitting a build_assert().

I gave this a quick try with [1] in qemu and it lead to the following hint, right before the oops:

Quoted 6 lines

[    0.957932] rust_kernel: panicked at /home/danilo/projects/linux/nova/nova-next/rust/kernel/io.rs:216:9:

Seeing this immediately tells me that I'm trying to do out of bound I/O accesses in my driver, which indeed doesn't tell me the exact line (in case things are inlined too much to gather it from the backtrace of the oops), but it should be good enough, no?

*smacks forehead*

Quoted 7 lines

Yes. So to answer this question:

Quoted 1 line

or is decode_stacktrace.sh not the right tool for this job?

No, it isn't. Just reading the kernel logs properly would have been a better option! I guess coming from C I'm just too used to jumping straight to the stack trace in the case of BUG_ON(), etc. Thanks for point that out.

Happy I could help.

Quoted 20 lines · Danilo Krummrich

Quoted 5 lines · Alistair Popple

To be honest I don't really understand the utility here because the compile-time check can't be a definitive check. You're always going to have to fallback to a run-time check because at least for PCI (and likely others) you can't know for at compile time if the IO region is big enough or matches the compile-time constraint.

That's not true, let me explain.

When you write a driver, you absolutely have to know the register layout. This means that you also know what the minimum PCI bar size has to be for your driver to work. If it would be smaller than what your driver expects, it can't function anyways. In Rust we make use of this fact.

When you map a PCI bar through pdev.iomap_region_sized you pass in a const generic (SIZE) representing the *expected* PCI bar size. This can indeed fail on run-time, but that's fine, as mentioned, if the bar is smaller than what your driver expect, it's useless anyways.

Quoted 4 lines

If the call succeeds, it means that the actual PCI bar size is greater or equal to SIZE. Since SIZE is known at compile time all subsequent I/O operations can be boundary checked against SIZE at compile time, which additionally makes the call infallible. This works for most I/O operations drivers do.

Argh! That's the piece I was missing - that this makes the IO call infallible and thus removes the need to write run-time error handling code. Sadly of course that's not actually true, because I/O operations can always fail for reasons other than what can be checked at compile time (eg. in particular PCI devices can fall off the bus and return all 0xF's). But I guess existing drivers don't really handle those cases either.

I'm not sure I agree it works perfectly fine. Developer ergonomics are an important aspect of any build environment, and I'd argue the ergonomic limitation for (2) means it is at least somewhat broken and needs fixing.

Anyway thanks for your time and detailed explainations, I really just started this thread as I think reducing friction for existing kernel developers to start looking at Rust in the kernel is important.

I've asked a few times, but are there any plans/ideas on how to improve the situation? I'm kind of suprised we're building things on top of a fairly broken feature without an idea of how we might make that feature work. I'd love to help, but being new to R4L no immediately useful ideas come to mind.

Add a Rust abstraction for the kernel's devres (device resource management) implementation.

The Devres type acts as a container to manage the lifetime and accessibility of device bound resources. Therefore it registers a devres callback and revokes access to the resource on invocation.

Users of the Devres abstraction can simply free the corresponding resources in their Drop implementation, which is invoked when either the Devres instance goes out of scope or the devres callback leads to the resource being revoked, which implies a call to drop_in_place().

Signed-off-by Danilo Krummrich <[email protected]>

---
 MAINTAINERS            |   1 +
 rust/helpers/device.c  |  10 +++
 rust/helpers/helpers.c |   1 +
 rust/kernel/devres.rs  | 178 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   1 +
 5 files changed, 191 insertions(+)
 create mode 100644 rust/helpers/device.c
 create mode 100644 rust/kernel/devres.rs

<snip>

rust/kernel/devres.rs new file +68 -0

+pub struct Devres<T>(Arc<DevresInner<T>>);
+
+impl<T> DevresInner<T> {
+    fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
+        let inner = Arc::pin_init(
+            pin_init!( DevresInner {
+                data <- Revocable::new(data),
+            }),
+            flags,
+        )?;
+
+        // Convert `Arc<DevresInner>` into a raw pointer and make devres own this reference until
+        // `Self::devres_callback` is called.
+        let data = inner.clone().into_raw();
+
+        // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is
+        // detached.
+        let ret = unsafe {
+            bindings::devm_add_action(dev.as_raw(), Some(Self::devres_callback), data as _)
+        };
+
+        if ret != 0 {
+            // SAFETY: We just created another reference to `inner` in order to pass it to
+            // `bindings::devm_add_action`. If `bindings::devm_add_action` fails, we have to drop
+            // this reference accordingly.
+            let _ = unsafe { Arc::from_raw(data) };
+            return Err(Error::from_errno(ret));
+        }
+
+        Ok(inner)
+    }
+
+    #[allow(clippy::missing_safety_doc)]
+    unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
+        let ptr = ptr as *mut DevresInner<T>;
+        // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the
+        // reference.
+        // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in
+        //         `DevresInner::new`.
+        let inner = unsafe { Arc::from_raw(ptr) };
+
+        inner.data.revoke();
+    }
+}
+
+impl<T> Devres<T> {
+    /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
+    /// returned `Devres` instance' `data` will be revoked once the device is detached.
+    pub fn new(dev: &Device, data: T, flags: Flags) -> Result<Self> {
+        let inner = DevresInner::new(dev, data, flags)?;
+
+        Ok(Devres(inner))
+    }
+
+    /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
+    /// is owned by devres and will be revoked / dropped, once the device is detached.
+    pub fn new_foreign_owned(dev: &Device, data: T, flags: Flags) -> Result {
+        let _ = DevresInner::new(dev, data, flags)?;
+
+        Ok(())
+    }
+}
+
+impl<T> Deref for Devres<T> {
+    type Target = Revocable<T>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0.data

rust/kernel/devres.rs new file +6 -0

+        //
+        // SAFETY: When `drop` runs, it's guaranteed that nobody is accessing the revocable data
+        // anymore, hence it is safe not to wait for the grace period to finish.
+        unsafe { self.revoke_nosync() };
+    }
+}

rust/kernel/lib.rs +1 -0

index 6c836ab73771..2b61bf99d1ee 100644
@@ -41,6 +41,7 @@
 pub mod cred;
 pub mod device;
 pub mod device_id;
+pub mod devres;
 pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]

Quoted 99 lines · On Thu, 19 Dec 2024 18:04:10 +0100 Danilo Krummrich <[email protected]> wrote:

Add a Rust abstraction for the kernel's devres (device resource management) implementation.

The Devres type acts as a container to manage the lifetime and accessibility of device bound resources. Therefore it registers a devres callback and revokes access to the resource on invocation.

Users of the Devres abstraction can simply free the corresponding resources in their Drop implementation, which is invoked when either the Devres instance goes out of scope or the devres callback leads to the resource being revoked, which implies a call to drop_in_place().

Signed-off-by Danilo Krummrich <[email protected]>

---
 MAINTAINERS            |   1 +
 rust/helpers/device.c  |  10 +++
 rust/helpers/helpers.c |   1 +
 rust/kernel/devres.rs  | 178 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   1 +
 5 files changed, 191 insertions(+)
 create mode 100644 rust/helpers/device.c
 create mode 100644 rust/kernel/devres.rs

<snip>

rust/kernel/devres.rs new file +74 -0

+pub struct Devres<T>(Arc<DevresInner<T>>);
+
+impl<T> DevresInner<T> {
+    fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
+        let inner = Arc::pin_init(
+            pin_init!( DevresInner {
+                data <- Revocable::new(data),
+            }),
+            flags,
+        )?;
+
+        // Convert `Arc<DevresInner>` into a raw pointer and make devres own this reference until
+        // `Self::devres_callback` is called.
+        let data = inner.clone().into_raw();
+
+        // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is
+        // detached.
+        let ret = unsafe {
+            bindings::devm_add_action(dev.as_raw(), Some(Self::devres_callback), data as _)
+        };
+
+        if ret != 0 {
+            // SAFETY: We just created another reference to `inner` in order to pass it to
+            // `bindings::devm_add_action`. If `bindings::devm_add_action` fails, we have to drop
+            // this reference accordingly.
+            let _ = unsafe { Arc::from_raw(data) };
+            return Err(Error::from_errno(ret));
+        }
+
+        Ok(inner)
+    }
+
+    #[allow(clippy::missing_safety_doc)]
+    unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
+        let ptr = ptr as *mut DevresInner<T>;
+        // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the
+        // reference.
+        // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in
+        //         `DevresInner::new`.
+        let inner = unsafe { Arc::from_raw(ptr) };
+
+        inner.data.revoke();
+    }
+}
+
+impl<T> Devres<T> {
+    /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
+    /// returned `Devres` instance' `data` will be revoked once the device is detached.
+    pub fn new(dev: &Device, data: T, flags: Flags) -> Result<Self> {
+        let inner = DevresInner::new(dev, data, flags)?;
+
+        Ok(Devres(inner))
+    }
+
+    /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
+    /// is owned by devres and will be revoked / dropped, once the device is detached.
+    pub fn new_foreign_owned(dev: &Device, data: T, flags: Flags) -> Result {
+        let _ = DevresInner::new(dev, data, flags)?;
+
+        Ok(())
+    }
+}
+
+impl<T> Deref for Devres<T> {
+    type Target = Revocable<T>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0.data
+    }
+}
+
+impl<T> Drop for Devres<T> {
+    fn drop(&mut self) {
+        // Revoke the data, such that it gets dropped already and the actual resource is freed.

rust/kernel/devres.rs line 172

Quoted 4 lines

rust/kernel/devres.rs new file +4 -0

+        //
+        // `DevresInner` has to stay alive until the devres callback has been called. This is
+        // necessary since we don't know when `Devres` is dropped and calling
+        // `devm_remove_action()` instead could race with `devres_release_all()`.

IIUC, the outcome of that race is the WARN if devres_release_all takes the spinlock first and has already remvoed the action?

Could you do a custom devres_release here that mimick devm_remove_action but omit the WARN? This way it allows the memory behind DevresInner to be freed early without keeping it allocated until the end of device lifetime.

Quoted 18 lines

rust/kernel/devres.rs new file +6 -0

+        //
+        // SAFETY: When `drop` runs, it's guaranteed that nobody is accessing the revocable data
+        // anymore, hence it is safe not to wait for the grace period to finish.
+        unsafe { self.revoke_nosync() };
+    }
+}

rust/kernel/lib.rs +1 -0

index 6c836ab73771..2b61bf99d1ee 100644
@@ -41,6 +41,7 @@
 pub mod cred;
 pub mod device;
 pub mod device_id;
+pub mod devres;
 pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]

Quoted 102 lines · Gary Guo

Quoted 99 lines · On Thu, 19 Dec 2024 18:04:10 +0100 Danilo Krummrich <[email protected]> wrote:

Add a Rust abstraction for the kernel's devres (device resource management) implementation.

The Devres type acts as a container to manage the lifetime and accessibility of device bound resources. Therefore it registers a devres callback and revokes access to the resource on invocation.

Users of the Devres abstraction can simply free the corresponding resources in their Drop implementation, which is invoked when either the Devres instance goes out of scope or the devres callback leads to the resource being revoked, which implies a call to drop_in_place().

Signed-off-by Danilo Krummrich <[email protected]>

---
 MAINTAINERS            |   1 +
 rust/helpers/device.c  |  10 +++
 rust/helpers/helpers.c |   1 +
 rust/kernel/devres.rs  | 178 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   1 +
 5 files changed, 191 insertions(+)
 create mode 100644 rust/helpers/device.c
 create mode 100644 rust/kernel/devres.rs

<snip>

rust/kernel/devres.rs new file +74 -0

+pub struct Devres<T>(Arc<DevresInner<T>>);
+
+impl<T> DevresInner<T> {
+    fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
+        let inner = Arc::pin_init(
+            pin_init!( DevresInner {
+                data <- Revocable::new(data),
+            }),
+            flags,
+        )?;
+
+        // Convert `Arc<DevresInner>` into a raw pointer and make devres own this reference until
+        // `Self::devres_callback` is called.
+        let data = inner.clone().into_raw();
+
+        // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is
+        // detached.
+        let ret = unsafe {
+            bindings::devm_add_action(dev.as_raw(), Some(Self::devres_callback), data as _)
+        };
+
+        if ret != 0 {
+            // SAFETY: We just created another reference to `inner` in order to pass it to
+            // `bindings::devm_add_action`. If `bindings::devm_add_action` fails, we have to drop
+            // this reference accordingly.
+            let _ = unsafe { Arc::from_raw(data) };
+            return Err(Error::from_errno(ret));
+        }
+
+        Ok(inner)
+    }
+
+    #[allow(clippy::missing_safety_doc)]
+    unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
+        let ptr = ptr as *mut DevresInner<T>;
+        // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the
+        // reference.
+        // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in
+        //         `DevresInner::new`.
+        let inner = unsafe { Arc::from_raw(ptr) };
+
+        inner.data.revoke();
+    }
+}
+
+impl<T> Devres<T> {
+    /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
+    /// returned `Devres` instance' `data` will be revoked once the device is detached.
+    pub fn new(dev: &Device, data: T, flags: Flags) -> Result<Self> {
+        let inner = DevresInner::new(dev, data, flags)?;
+
+        Ok(Devres(inner))
+    }
+
+    /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
+    /// is owned by devres and will be revoked / dropped, once the device is detached.
+    pub fn new_foreign_owned(dev: &Device, data: T, flags: Flags) -> Result {
+        let _ = DevresInner::new(dev, data, flags)?;
+
+        Ok(())
+    }
+}
+
+impl<T> Deref for Devres<T> {
+    type Target = Revocable<T>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0.data
+    }
+}
+
+impl<T> Drop for Devres<T> {
+    fn drop(&mut self) {
+        // Revoke the data, such that it gets dropped already and the actual resource is freed.

Quoted 8 lines

rust/kernel/devres.rs line 172

Quoted 4 lines

rust/kernel/devres.rs new file +4 -0

+        //
+        // `DevresInner` has to stay alive until the devres callback has been called. This is
+        // necessary since we don't know when `Devres` is dropped and calling
+        // `devm_remove_action()` instead could race with `devres_release_all()`.

IIUC, the outcome of that race is the WARN if devres_release_all takes the spinlock first and has already remvoed the action?

Yes, this was one issue. But I think there was another when you have a class Registration that is owned by a Devres, which holds private data that is encapsulated in a Devres too.

I have this case in Nova where the DRM device' private data holds the PCI bar, and the DRM device registration has a reference of the corresponding DRM device.

But maybe this also was something else. I will double check and if I can confirm that the WARN_ON() in devm_remove_action() is the only issue, we can certainly change this.

Quoted 26 lines

Could you do a custom devres_release here that mimick devm_remove_action but omit the WARN? This way it allows the memory behind DevresInner to be freed early without keeping it allocated until the end of device lifetime.

Quoted 18 lines

rust/kernel/devres.rs new file +6 -0

+        //
+        // SAFETY: When `drop` runs, it's guaranteed that nobody is accessing the revocable data
+        // anymore, hence it is safe not to wait for the grace period to finish.
+        unsafe { self.revoke_nosync() };
+    }
+}

rust/kernel/lib.rs +1 -0

index 6c836ab73771..2b61bf99d1ee 100644
@@ -41,6 +41,7 @@
 pub mod cred;
 pub mod device;
 pub mod device_id;
+pub mod devres;
 pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]

Quoted 93 lines · On Thu, 19 Dec 2024 18:04:10 +0100 Danilo Krummrich <[email protected]> wrote:

Add a Rust abstraction for the kernel's devres (device resource management) implementation.

The Devres type acts as a container to manage the lifetime and accessibility of device bound resources. Therefore it registers a devres callback and revokes access to the resource on invocation.

Users of the Devres abstraction can simply free the corresponding resources in their Drop implementation, which is invoked when either the Devres instance goes out of scope or the devres callback leads to the resource being revoked, which implies a call to drop_in_place().

Signed-off-by Danilo Krummrich <[email protected]>

---
 MAINTAINERS            |   1 +
 rust/helpers/device.c  |  10 +++
 rust/helpers/helpers.c |   1 +
 rust/kernel/devres.rs  | 178 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   1 +
 5 files changed, 191 insertions(+)
 create mode 100644 rust/helpers/device.c
 create mode 100644 rust/kernel/devres.rs

<snip>

rust/kernel/devres.rs new file +68 -0

+pub struct Devres<T>(Arc<DevresInner<T>>);
+
+impl<T> DevresInner<T> {
+    fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
+        let inner = Arc::pin_init(
+            pin_init!( DevresInner {
+                data <- Revocable::new(data),
+            }),
+            flags,
+        )?;
+
+        // Convert `Arc<DevresInner>` into a raw pointer and make devres own this reference until
+        // `Self::devres_callback` is called.
+        let data = inner.clone().into_raw();
+
+        // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is
+        // detached.
+        let ret = unsafe {
+            bindings::devm_add_action(dev.as_raw(), Some(Self::devres_callback), data as _)
+        };
+
+        if ret != 0 {
+            // SAFETY: We just created another reference to `inner` in order to pass it to
+            // `bindings::devm_add_action`. If `bindings::devm_add_action` fails, we have to drop
+            // this reference accordingly.
+            let _ = unsafe { Arc::from_raw(data) };
+            return Err(Error::from_errno(ret));
+        }
+
+        Ok(inner)
+    }
+
+    #[allow(clippy::missing_safety_doc)]
+    unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
+        let ptr = ptr as *mut DevresInner<T>;
+        // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the
+        // reference.
+        // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in
+        //         `DevresInner::new`.
+        let inner = unsafe { Arc::from_raw(ptr) };
+
+        inner.data.revoke();
+    }
+}
+
+impl<T> Devres<T> {
+    /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
+    /// returned `Devres` instance' `data` will be revoked once the device is detached.
+    pub fn new(dev: &Device, data: T, flags: Flags) -> Result<Self> {
+        let inner = DevresInner::new(dev, data, flags)?;
+
+        Ok(Devres(inner))
+    }
+
+    /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
+    /// is owned by devres and will be revoked / dropped, once the device is detached.
+    pub fn new_foreign_owned(dev: &Device, data: T, flags: Flags) -> Result {
+        let _ = DevresInner::new(dev, data, flags)?;
+
+        Ok(())
+    }
+}
+
+impl<T> Deref for Devres<T> {
+    type Target = Revocable<T>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0.data

rust/kernel/devres.rs line 172

Quoted 10 lines

rust/kernel/devres.rs new file +10 -0

+    }
+}
+
+impl<T> Drop for Devres<T> {
+    fn drop(&mut self) {
+        // Revoke the data, such that it gets dropped already and the actual resource is freed.
+        //
+        // `DevresInner` has to stay alive until the devres callback has been called. This is
+        // necessary since we don't know when `Devres` is dropped and calling
+        // `devm_remove_action()` instead could race with `devres_release_all()`.

IIUC, the outcome of that race is the WARN if devres_release_all takes the spinlock first and has already remvoed the action?

Could you do a custom devres_release here that mimick devm_remove_action but omit the WARN? This way it allows the memory behind DevresInner to be freed early without keeping it allocated until the end of device lifetime.

Quoted 19 lines

rust/kernel/devres.rs new file +6 -0

+        //
+        // SAFETY: When `drop` runs, it's guaranteed that nobody is accessing the revocable data
+        // anymore, hence it is safe not to wait for the grace period to finish.
+        unsafe { self.revoke_nosync() };
+    }
+}

rust/kernel/lib.rs +1 -0

index 6c836ab73771..2b61bf99d1ee 100644
@@ -41,6 +41,7 @@
 pub mod cred;
 pub mod device;
 pub mod device_id;
+pub mod devres;
 pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]

Quoted 8 lines

rust/kernel/devres.rs line 172

Quoted 7 lines

rust/kernel/devres.rs new file +7 -0

+impl<T> Drop for Devres<T> {
+    fn drop(&mut self) {
+        // Revoke the data, such that it gets dropped already and the actual resource is freed.
+        //
+        // `DevresInner` has to stay alive until the devres callback has been called. This is
+        // necessary since we don't know when `Devres` is dropped and calling
+        // `devm_remove_action()` instead could race with `devres_release_all()`.

Quoted 8 lines

IIUC, the outcome of that race is the WARN if devres_release_all takes the spinlock first and has already remvoed the action?

Could you do a custom devres_release here that mimick devm_remove_action but omit the WARN? This way it allows the memory behind DevresInner to be freed early without keeping it allocated until the end of device lifetime.

Better late than never, I now remember what's the *actual* race I was preventing here:

  | Task 0                               | Task 1
--|----------------------------------------------------------------------------
1 | Devres::drop() {                     | devres_release_all() {
2 |    DevresInner::remove_action() {    |    spin_lock_irqsave();
3 |                                      |    remove_nodes(&todo);
4 |                                      |    spin_unlock_irqrestore();
5 |       devm_remove_action_nowarn();   |
6 |       let _ = Arc::from_raw();       |
7 |    }                                 |
8 | }                                    |
9 |                                      |    release_nodes(&todo);
10|                                      | }

So, if devres_release_all() wins the race it stores the devres action within the temporary todo list. Which means that in 9 we enter DevresInner::devres_callback() even though DevresInner has been freed already.

Unfortunately, this race can happen with [1], but not with this original version of Devres.

I see two ways to fix it:

1) Just revert [1] and stick with the original version.

2) Use devm_release_action() instead of devm_remove_action() and don't drop the reference in DevresInner::remove_action() (6). This way the reference is always dropped from the callback.

With 2) we still have an unnecessary call to revoke() if Devres is dropped before the device is detached from the driver, but that's still better than keeping DevresInner alive until the device is detached from the driver, which the original version did. Hence, I'll go ahead and prepare a patch for this, unless there are any concerns.

- Danilo

[1] https://lore.kernel.org/rust-for-linux/[email protected]/