[PATCH v7 00/16] Device / Driver PCI / Platform Rust abstractions

[PATCH v7 00/16] Device / Driver PCI / Platform Rust abstractions

[Danilo Krummrich]

This patch series implements the necessary Rust abstractions to implement
device drivers in Rust.

This includes some basic generalizations for driver registration, handling of ID
tables, MMIO operations and device resource handling.

Those generalizations are used to implement device driver support for two
busses, the PCI and platform bus (with OF IDs) in order to provide some evidence
that the generalizations work as intended.

The patch series also includes two patches adding two driver samples, one PCI
driver and one platform driver.

The PCI bits are motivated by the Nova driver project [1], but are used by at
least one more OOT driver (rnvme [2]).

The platform bits, besides adding some more evidence to the base abstractions,
are required by a few more OOT drivers aiming at going upstream, i.e. rvkms [3],
cpufreq-dt [4], asahi [5] and the i2c work from Fabien [6].

The patches of this series can also be [7], [8] and [9].

Changes in v7:
==============
- Revocable:
  - replace `compare_exchange` with `swap`

- Driver:
  - fix warning when CONFIG_OF=n

- I/O:
  - remove unnecessary return statement in rust_helper_iounmap()
  - fix cast in doctest for `bindings::ioremap`

- Devres:
  - fix cast in doctest for `bindings::ioremap`

- PCI:
  - remove `Deref` of `pci::DeviceId`
  - rename `DeviceId` constructors
    - `new`       -> `from_id`
    - `with_class -> `from_class`

- MISC:
  - use `kernel::ffi::c_*` instead of `core::ffi::c_*`
  - rebase onto latest rust-next (0c5928deada15a8d075516e6e0d9ee19011bb000)

Changes in v6:
==============
- Module:
  - remove patch to pass the module name in `Module::init`
  - add a patch adding the `ModuleMetadata` trait to retreive the module name

- Driver:
  - generate module structure within `module_driver!`, such that we get a unique
    module type for which `ModuleMetadata` can be implemented to retreive the
    module name
  - let `Adapter::of_id_table` return `Option<of::IdTable<Self::IdInfo>>`

- Platform:
  - change type of `platform::Driver::OF_ID_TABLE` to `Option<of::IdTable>`

- RCU:
  - add RCU abstraction to MAINTAINERS file

- MISC:
  - pick up a couple of RBs

Changes in v5:
==============
- Opaque:
  - add `Opaque::pin_init`

- Revocable:
  - use const generic instead of bool in `revoke_internal`
  - don't open code things with `Opaque::try_ffi_init`, use
    `Opaque::pin_init` instead

- Driver:
  - fix `DriverOps` -> `RegistrationOps` in commit message
  - fix signature of `register` and `unregister` to take an
    `Opaque<T::RegType>` instead of a mutable reference
  - implement generic `driver::Adapter`

- ID table:
  - fix `module_device_table!` macro, according to commit 054a9cd395a7
   (modpost: rename alias symbol for MODULE_DEVICE_TABLE())

- PCI:
  - remove `pci::DeviceId` argument from probe()
  - provide `vendor_id` and `device_id` accessor functions
  - use `addr_of_mut!` for the device in probe()

- OF:
  - move OF `IdTable` type alias from platform.rs to of.rs

- Platform:
  - use `addr_of_mut!` for the device in probe()
  - move generic OF code to `driver::Adapter`

- MISC:
  - rebase onto latest rust-next (-rc2)

Changes in v4:
==============
- Revocable
  - convert `Revocable::data` to `Opaque`
  - introduce `revoke_nosync`, which does not wait for an RCU grace period
  - fix minor typo

- Device ID
  - fix ID table export name to always be unique
  - remove `#![allow(stable_features)]`
  - move `#![allow(const_refs_to_cell)]` to the "stable in 1.83" list

- I/O
  - split `Io<SIZE>` in `IoRaw<SIZE>` and `Io<SIZE>`
  - add rust helper for ioremap() and iounmap() (needed by doctests)

- Devres
  - optimze `drop` by using `revoke_nosync` (we're guaranteed there are no more
    concurrent users, hence no need to wait for a full grace period)

- OF
  - rename "Open Firmware" -> "Device Tree / Open Firmware" (Rob)
  - remove unused C header reference
  - add TODO comment to loop in `DeviceId::new`
  - remove `DeviceId::compatible`; was only used in sample code
  - add missing `#[repr(transparent)]`
  - use #[cfg(CONFIG_OF)] for the relevant functions

- PCI
  - let Rust helper for pci_resource_len() return resource_size_t

- PCI Sample
  - remove unnecessary `from_le`

- Platform
  - fix example compatible string
  - add example for `platform::Driver`
  - rename `ID_TABLE` to `OF_ID_TABLE`
  - don't allow public use of `of_match_table`; convert to `of_id_info` to
    retrieve the `of::DeviceId` to gather the corresponding `IdInfo`
  - remove wrong example reference in the documentation of `platform::Driver`
  - remove `as_dev`, as we already implement `AsRef` for `platform::Device`

- Platform Sample
  - fix compatible string; add corresponding entry to drivers/of/unittest-data/tests-platform.dtsi
  - remove usage of `of_match_table`

- MISC
  - fix a few spelling mistakes
  - rebase onto rust-next (v6.13-rc1)

Changes in v3:
==============
- add commits for `Opaque::try_ffi_init` and `InPlaceModule`
- rename `DriverOps` to `RegistrationOps`
- rework device ID abstractions to get rid of almost all macro magic (thanks to
  Gary Guo for working this out!)
  - this is possible due to recently stabilized language features
- add modpost alias generation for device ID tables
  - unfortunately, this is the part that still requires some macro magic in the
    device ID abstractions
- PCI
  - represent the driver private data with the driver specific `Driver` instance
    and bind it's lifetime to the time of the driver being bound to a device
    - this allows us to handle class / subsystem registrations in a cleaner way
  - get rid of `Driver::remove`
    - Rust drivers should bind cleanup code to the `Drop` implementation of the
      corresponding structure instead and put it into their driver structure for
      automatic cleanup
  - add a sample PCI driver
- add abstractions for `struct of_device_id`
- add abstractions for the platform bus, including a sample driver
- update the MAINTAINERS file accordingly
  - currently this turns out a bit messy, but it should become better once the
    build system supports a treewide distribution of the kernel crate
  - I didn't add myself as maintainer, but (if requested) I'm willing to do so
    and help with maintenance

Changes in v2:
==============
- statically initialize driver structures (Greg)
- move base device ID abstractions to a separate source file (Greg)
- remove `DeviceRemoval` trait in favor of using a `Devres` callback to
  unregister drivers
- remove `device::Data`, we don't need this abstraction anymore now that we
  `Devres` to revoke resources and registrations
- pass the module name to `Module::init` and `InPlaceModule::init` in a separate
  patch
- rework of `Io` including compile time boundary checks (Miguel, Wedson)
- adjust PCI abstractions accordingly and implement a `module_pci_driver!` macro
- rework `pci::Bar` to support a const SIZE
- increase the total amount of Documentation, rephrase some safety comments and
  commit messages for less ambiguity
- fix compilation issues with some documentation examples

Danilo Krummrich (14):
  rust: module: add trait `ModuleMetadata`
  rust: implement generic driver registration
  rust: implement `IdArray`, `IdTable` and `RawDeviceId`
  rust: types: add `Opaque::pin_init`
  rust: add `io::{Io, IoRaw}` base types
  rust: add devres abstraction
  rust: pci: add basic PCI device / driver abstractions
  rust: pci: implement I/O mappable `pci::Bar`
  samples: rust: add Rust PCI sample driver
  rust: of: add `of::DeviceId` abstraction
  rust: driver: implement `Adapter`
  rust: platform: add basic platform device / driver abstractions
  samples: rust: add Rust platform sample driver
  MAINTAINERS: add Danilo to DRIVER CORE

Wedson Almeida Filho (2):
  rust: add rcu abstraction
  rust: add `Revocable` type

 MAINTAINERS                                  |  10 +
 drivers/of/unittest-data/tests-platform.dtsi |   5 +
 rust/bindings/bindings_helper.h              |   3 +
 rust/helpers/device.c                        |  10 +
 rust/helpers/helpers.c                       |   5 +
 rust/helpers/io.c                            | 101 +++++
 rust/helpers/pci.c                           |  18 +
 rust/helpers/platform.c                      |  13 +
 rust/helpers/rcu.c                           |  13 +
 rust/kernel/device_id.rs                     | 165 +++++++
 rust/kernel/devres.rs                        | 178 ++++++++
 rust/kernel/driver.rs                        | 173 ++++++++
 rust/kernel/io.rs                            | 260 +++++++++++
 rust/kernel/lib.rs                           |  20 +
 rust/kernel/of.rs                            |  60 +++
 rust/kernel/pci.rs                           | 432 +++++++++++++++++++
 rust/kernel/platform.rs                      | 198 +++++++++
 rust/kernel/revocable.rs                     | 219 ++++++++++
 rust/kernel/sync.rs                          |   1 +
 rust/kernel/sync/rcu.rs                      |  47 ++
 rust/kernel/types.rs                         |  11 +
 rust/macros/module.rs                        |   4 +
 samples/rust/Kconfig                         |  21 +
 samples/rust/Makefile                        |   2 +
 samples/rust/rust_driver_pci.rs              | 110 +++++
 samples/rust/rust_driver_platform.rs         |  49 +++
 26 files changed, 2128 insertions(+)
 create mode 100644 rust/helpers/device.c
 create mode 100644 rust/helpers/io.c
 create mode 100644 rust/helpers/pci.c
 create mode 100644 rust/helpers/platform.c
 create mode 100644 rust/helpers/rcu.c
 create mode 100644 rust/kernel/device_id.rs
 create mode 100644 rust/kernel/devres.rs
 create mode 100644 rust/kernel/driver.rs
 create mode 100644 rust/kernel/io.rs
 create mode 100644 rust/kernel/of.rs
 create mode 100644 rust/kernel/pci.rs
 create mode 100644 rust/kernel/platform.rs
 create mode 100644 rust/kernel/revocable.rs
 create mode 100644 rust/kernel/sync/rcu.rs
 create mode 100644 samples/rust/rust_driver_pci.rs
 create mode 100644 samples/rust/rust_driver_platform.rs


base-commit: 0c5928deada15a8d075516e6e0d9ee19011bb000
-- 
2.47.1

[PATCH v7 01/16] rust: module: add trait `ModuleMetadata`

[Danilo Krummrich]

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 <dakr@kernel.org>
---
 rust/kernel/lib.rs    | 6 ++++++
 rust/macros/module.rs | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e1065a7551a3..61b82b78b915 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -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)
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 2587f41b0d39..cdf94f4982df 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -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 {{
-- 
2.47.1

[PATCH v7 02/16] rust: implement generic driver registration

[Danilo Krummrich]

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 <wedsonaf@gmail.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS           |   1 +
 rust/kernel/driver.rs | 117 ++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs    |   1 +
 3 files changed, 119 insertions(+)
 create mode 100644 rust/kernel/driver.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index baf0eeb9a355..2ad58ed40079 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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 <nm@ti.com>
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
new file mode 100644
index 000000000000..c1957ee7bb7e
--- /dev/null
+++ b/rust/kernel/driver.rs
@@ -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
+/// `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`].
+    fn register(
+        reg: &Opaque<Self::RegType>,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result;
+
+    /// Unregisters a driver previously registered with [`RegistrationOps::register`].
+    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.
+    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()) };
+
+                // 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.
+                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);
+    }
+}
+
+/// 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]
+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)*
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 61b82b78b915..7818407f9aac 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -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;
-- 
2.47.1

[PATCH v7 03/16] rust: implement `IdArray`, `IdTable` and `RawDeviceId`

[Danilo Krummrich]

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 <wedsonaf@gmail.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Co-developed-by: Fabien Parent <fabien.parent@linaro.org>
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 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

diff --git a/MAINTAINERS b/MAINTAINERS
index 2ad58ed40079..3cfb68650347 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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)
diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
new file mode 100644
index 000000000000..e5859217a579
--- /dev/null
+++ b/rust/kernel/device_id.rs
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic implementation of device IDs.
+//!
+//! Each bus / subsystem that matches device and driver through a bus / subsystem specific ID is
+//! expected to implement [`RawDeviceId`].
+
+use core::mem::MaybeUninit;
+
+/// 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;
+
+    /// The index stored at `DRIVER_DATA_OFFSET` of the implementor of the [`RawDeviceId`] trait.
+    fn index(&self) -> usize;
+}
+
+/// A zero-terminated device id array.
+#[repr(C)]
+pub struct RawIdArray<T: RawDeviceId, const N: usize> {
+    ids: [T::RawType; N],
+    sentinel: MaybeUninit<T::RawType>,
+}
+
+impl<T: RawDeviceId, const N: usize> RawIdArray<T, N> {
+    #[doc(hidden)]
+    pub const fn size(&self) -> usize {
+        core::mem::size_of::<Self>()
+    }
+}
+
+/// A zero-terminated device id array, followed by context data.
+#[repr(C)]
+pub struct IdArray<T: RawDeviceId, U, const N: usize> {
+    raw_ids: RawIdArray<T, N>,
+    id_infos: [U; N],
+}
+
+impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
+    /// Creates a new instance of the array.
+    ///
+    /// The contents are derived from the given identifiers and context information.
+    pub const fn new(ids: [(T, U); N]) -> Self {
+        let mut raw_ids = [const { MaybeUninit::<T::RawType>::uninit() }; N];
+        let mut infos = [const { MaybeUninit::uninit() }; N];
+
+        let mut i = 0usize;
+        while i < N {
+            // SAFETY: by the safety requirement of `RawDeviceId`, we're guaranteed that `T` is
+            // layout-wise compatible with `RawType`.
+            raw_ids[i] = unsafe { core::mem::transmute_copy(&ids[i].0) };
+            // SAFETY: by the safety requirement of `RawDeviceId`, this would be effectively
+            // `raw_ids[i].driver_data = i;`.
+            unsafe {
+                raw_ids[i]
+                    .as_mut_ptr()
+                    .byte_offset(T::DRIVER_DATA_OFFSET as _)
+                    .cast::<usize>()
+                    .write(i);
+            }
+
+            // SAFETY: this is effectively a move: `infos[i] = ids[i].1`. We make a copy here but
+            // later forget `ids`.
+            infos[i] = MaybeUninit::new(unsafe { core::ptr::read(&ids[i].1) });
+            i += 1;
+        }
+
+        core::mem::forget(ids);
+
+        Self {
+            raw_ids: RawIdArray {
+                // SAFETY: this is effectively `array_assume_init`, which is unstable, so we use
+                // `transmute_copy` instead. We have initialized all elements of `raw_ids` so this
+                // `array_assume_init` is safe.
+                ids: unsafe { core::mem::transmute_copy(&raw_ids) },
+                sentinel: MaybeUninit::zeroed(),
+            },
+            // SAFETY: We have initialized all elements of `infos` so this `array_assume_init` is
+            // safe.
+            id_infos: unsafe { core::mem::transmute_copy(&infos) },
+        }
+    }
+
+    /// Reference to the contained [`RawIdArray`].
+    pub const fn raw_ids(&self) -> &RawIdArray<T, N> {
+        &self.raw_ids
+    }
+}
+
+/// A device id table.
+///
+/// This trait is only implemented by `IdArray`.
+///
+/// The purpose of this trait is to allow `&'static dyn IdArray<T, U>` to be in context when `N` in
+/// `IdArray` doesn't matter.
+pub trait IdTable<T: RawDeviceId, U> {
+    /// Obtain the pointer to the ID table.
+    fn as_ptr(&self) -> *const T::RawType;
+
+    /// Obtain the pointer to the bus specific device ID from an index.
+    fn id(&self, index: usize) -> &T::RawType;
+
+    /// Obtain the pointer to the driver-specific information from an index.
+    fn info(&self, index: usize) -> &U;
+}
+
+impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> {
+    fn as_ptr(&self) -> *const T::RawType {
+        // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance
+        // to access the sentinel.
+        (self as *const Self).cast()
+    }
+
+    fn id(&self, index: usize) -> &T::RawType {
+        &self.raw_ids.ids[index]
+    }
+
+    fn info(&self, index: usize) -> &U {
+        &self.id_infos[index]
+    }
+}
+
+/// Create device table alias for modpost.
+#[macro_export]
+macro_rules! module_device_table {
+    ($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()) };
+    };
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 7818407f9aac..66149ac5c0c9 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -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)]
-- 
2.47.1

[PATCH v7 04/16] rust: add rcu abstraction

[Danilo Krummrich]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

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

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Co-developed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 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

diff --git a/MAINTAINERS b/MAINTAINERS
index 3cfb68650347..0cc69e282889 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index dcf827a61b52..060750af6524 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -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"
diff --git a/rust/helpers/rcu.c b/rust/helpers/rcu.c
new file mode 100644
index 000000000000..f1cec6583513
--- /dev/null
+++ b/rust/helpers/rcu.c
@@ -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();
+}
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 1eab7ebf25fd..0654008198b2 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -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};
diff --git a/rust/kernel/sync/rcu.rs b/rust/kernel/sync/rcu.rs
new file mode 100644
index 000000000000..b51d9150ffe2
--- /dev/null
+++ b/rust/kernel/sync/rcu.rs
@@ -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.
+        Self(NotThreadSafe)
+    }
+
+    /// Explicitly releases the RCU read side lock.
+    pub fn unlock(self) {}
+}
+
+impl Default for Guard {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+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()
+}
-- 
2.47.1

[PATCH v7 05/16] rust: types: add `Opaque::pin_init`

[Danilo Krummrich]

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 <aliceryhl@google.com>
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/types.rs | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index ec6457bb3084..3aea6af9a0bc 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -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
-- 
2.47.1

[PATCH v7 06/16] rust: add `Revocable` type

[Danilo Krummrich]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

Revocable allows access to objects to be safely revoked at run time.

This is useful, for example, for resources allocated during device probe;
when the device is removed, the driver should stop accessing the device
resources even if another state is kept in memory due to existing
references (i.e., device context data is ref-counted and has a non-zero
refcount after removal of the device).

Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Co-developed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/lib.rs       |   1 +
 rust/kernel/revocable.rs | 219 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 220 insertions(+)
 create mode 100644 rust/kernel/revocable.rs

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 66149ac5c0c9..5702ce32ec8e 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -60,6 +60,7 @@
 pub mod prelude;
 pub mod print;
 pub mod rbtree;
+pub mod revocable;
 pub mod security;
 pub mod seq_file;
 pub mod sizes;
diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
new file mode 100644
index 000000000000..1e5a9d25c21b
--- /dev/null
+++ b/rust/kernel/revocable.rs
@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Revocable objects.
+//!
+//! The [`Revocable`] type wraps other types and allows access to them to be revoked. The existence
+//! of a [`RevocableGuard`] ensures that objects remain valid.
+
+use crate::{bindings, prelude::*, sync::rcu, types::Opaque};
+use core::{
+    marker::PhantomData,
+    ops::Deref,
+    ptr::drop_in_place,
+    sync::atomic::{AtomicBool, Ordering},
+};
+
+/// An object that can become inaccessible at runtime.
+///
+/// Once access is revoked and all concurrent users complete (i.e., all existing instances of
+/// [`RevocableGuard`] are dropped), the wrapped object is also dropped.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::revocable::Revocable;
+///
+/// struct Example {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// fn add_two(v: &Revocable<Example>) -> Option<u32> {
+///     let guard = v.try_access()?;
+///     Some(guard.a + guard.b)
+/// }
+///
+/// let v = KBox::pin_init(Revocable::new(Example { a: 10, b: 20 }), GFP_KERNEL).unwrap();
+/// assert_eq!(add_two(&v), Some(30));
+/// v.revoke();
+/// assert_eq!(add_two(&v), None);
+/// ```
+///
+/// Sample example as above, but explicitly using the rcu read side lock.
+///
+/// ```
+/// # use kernel::revocable::Revocable;
+/// use kernel::sync::rcu;
+///
+/// struct Example {
+///     a: u32,
+///     b: u32,
+/// }
+///
+/// fn add_two(v: &Revocable<Example>) -> Option<u32> {
+///     let guard = rcu::read_lock();
+///     let e = v.try_access_with_guard(&guard)?;
+///     Some(e.a + e.b)
+/// }
+///
+/// let v = KBox::pin_init(Revocable::new(Example { a: 10, b: 20 }), GFP_KERNEL).unwrap();
+/// assert_eq!(add_two(&v), Some(30));
+/// v.revoke();
+/// assert_eq!(add_two(&v), None);
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct Revocable<T> {
+    is_available: AtomicBool,
+    #[pin]
+    data: Opaque<T>,
+}
+
+// SAFETY: `Revocable` is `Send` if the wrapped object is also `Send`. This is because while the
+// functionality exposed by `Revocable` can be accessed from any thread/CPU, it is possible that
+// this isn't supported by the wrapped object.
+unsafe impl<T: Send> Send for Revocable<T> {}
+
+// SAFETY: `Revocable` is `Sync` if the wrapped object is both `Send` and `Sync`. We require `Send`
+// from the wrapped object as well because  of `Revocable::revoke`, which can trigger the `Drop`
+// implementation of the wrapped object from an arbitrary thread.
+unsafe impl<T: Sync + Send> Sync for Revocable<T> {}
+
+impl<T> Revocable<T> {
+    /// Creates a new revocable instance of the given data.
+    pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
+        pin_init!(Self {
+            is_available: AtomicBool::new(true),
+            data <- Opaque::pin_init(data),
+        })
+    }
+
+    /// Tries to access the revocable wrapped object.
+    ///
+    /// Returns `None` if the object has been revoked and is therefore no longer accessible.
+    ///
+    /// Returns a guard that gives access to the object otherwise; the object is guaranteed to
+    /// remain accessible while the guard is alive. In such cases, callers are not allowed to sleep
+    /// because another CPU may be waiting to complete the revocation of this object.
+    pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
+        let guard = rcu::read_lock();
+        if self.is_available.load(Ordering::Relaxed) {
+            // Since `self.is_available` is true, data is initialised and has to remain valid
+            // because the RCU read side lock prevents it from being dropped.
+            Some(RevocableGuard::new(self.data.get(), guard))
+        } else {
+            None
+        }
+    }
+
+    /// Tries to access the revocable wrapped object.
+    ///
+    /// Returns `None` if the object has been revoked and is therefore no longer accessible.
+    ///
+    /// Returns a shared reference to the object otherwise; the object is guaranteed to
+    /// remain accessible while the rcu read side guard is alive. In such cases, callers are not
+    /// allowed to sleep because another CPU may be waiting to complete the revocation of this
+    /// object.
+    pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a T> {
+        if self.is_available.load(Ordering::Relaxed) {
+            // SAFETY: Since `self.is_available` is true, data is initialised and has to remain
+            // valid because the RCU read side lock prevents it from being dropped.
+            Some(unsafe { &*self.data.get() })
+        } else {
+            None
+        }
+    }
+
+    /// # Safety
+    ///
+    /// Callers must ensure that there are no more concurrent users of the revocable object.
+    unsafe fn revoke_internal<const SYNC: bool>(&self) {
+        if self.is_available.swap(false, Ordering::Relaxed) {
+            if SYNC {
+                // SAFETY: Just an FFI call, there are no further requirements.
+                unsafe { bindings::synchronize_rcu() };
+            }
+
+            // SAFETY: We know `self.data` is valid because only one CPU can succeed the
+            // `compare_exchange` above that takes `is_available` from `true` to `false`.
+            unsafe { drop_in_place(self.data.get()) };
+        }
+    }
+
+    /// Revokes access to and drops the wrapped object.
+    ///
+    /// Access to the object is revoked immediately to new callers of [`Revocable::try_access`],
+    /// expecting that there are no concurrent users of the object.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that there are no more concurrent users of the revocable object.
+    pub unsafe fn revoke_nosync(&self) {
+        // SAFETY: By the safety requirement of this function, the caller ensures that nobody is
+        // accessing the data anymore and hence we don't have to wait for the grace period to
+        // finish.
+        unsafe { self.revoke_internal::<false>() }
+    }
+
+    /// Revokes access to and drops the wrapped object.
+    ///
+    /// Access to the object is revoked immediately to new callers of [`Revocable::try_access`].
+    ///
+    /// If there are concurrent users of the object (i.e., ones that called
+    /// [`Revocable::try_access`] beforehand and still haven't dropped the returned guard), this
+    /// function waits for the concurrent access to complete before dropping the wrapped object.
+    pub fn revoke(&self) {
+        // SAFETY: By passing `true` we ask `revoke_internal` to wait for the grace period to
+        // finish.
+        unsafe { self.revoke_internal::<true>() }
+    }
+}
+
+#[pinned_drop]
+impl<T> PinnedDrop for Revocable<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // Drop only if the data hasn't been revoked yet (in which case it has already been
+        // dropped).
+        // SAFETY: We are not moving out of `p`, only dropping in place
+        let p = unsafe { self.get_unchecked_mut() };
+        if *p.is_available.get_mut() {
+            // SAFETY: We know `self.data` is valid because no other CPU has changed
+            // `is_available` to `false` yet, and no other CPU can do it anymore because this CPU
+            // holds the only reference (mutable) to `self` now.
+            unsafe { drop_in_place(p.data.get()) };
+        }
+    }
+}
+
+/// A guard that allows access to a revocable object and keeps it alive.
+///
+/// CPUs may not sleep while holding on to [`RevocableGuard`] because it's in atomic context
+/// holding the RCU read-side lock.
+///
+/// # Invariants
+///
+/// The RCU read-side lock is held while the guard is alive.
+pub struct RevocableGuard<'a, T> {
+    data_ref: *const T,
+    _rcu_guard: rcu::Guard,
+    _p: PhantomData<&'a ()>,
+}
+
+impl<T> RevocableGuard<'_, T> {
+    fn new(data_ref: *const T, rcu_guard: rcu::Guard) -> Self {
+        Self {
+            data_ref,
+            _rcu_guard: rcu_guard,
+            _p: PhantomData,
+        }
+    }
+}
+
+impl<T> Deref for RevocableGuard<'_, T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: By the type invariants, we hold the rcu read-side lock, so the object is
+        // guaranteed to remain valid.
+        unsafe { &*self.data_ref }
+    }
+}
-- 
2.47.1

[PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Danilo Krummrich]

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 <aliceryhl@google.com>
Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Daniel Almeida  <daniel.almeida@collabora.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 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

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 060750af6524..63f9b1da179f 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -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"
diff --git a/rust/helpers/io.c b/rust/helpers/io.c
new file mode 100644
index 000000000000..4c2401ccd720
--- /dev/null
+++ b/rust/helpers/io.c
@@ -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
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
new file mode 100644
index 000000000000..d4a73e52e3ee
--- /dev/null
+++ b/rust/kernel/io.rs
@@ -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
+        // 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
+    );
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 5702ce32ec8e..6c836ab73771 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -79,6 +79,7 @@
 
 #[doc(hidden)]
 pub use bindings;
+pub mod io;
 pub use macros;
 pub use uapi;
 
-- 
2.47.1

[PATCH v7 08/16] rust: add devres abstraction

[Danilo Krummrich]

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 <dakr@kernel.org>
---
 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

diff --git a/MAINTAINERS b/MAINTAINERS
index 0cc69e282889..a9ae2d69f961 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7034,6 +7034,7 @@ F:	include/linux/property.h
 F:	lib/kobj*
 F:	rust/kernel/device.rs
 F:	rust/kernel/device_id.rs
+F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
diff --git a/rust/helpers/device.c b/rust/helpers/device.c
new file mode 100644
index 000000000000..b2135c6686b0
--- /dev/null
+++ b/rust/helpers/device.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/device.h>
+
+int rust_helper_devm_add_action(struct device *dev,
+				void (*action)(void *),
+				void *data)
+{
+	return devm_add_action(dev, action, data);
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 63f9b1da179f..a3b52aa021de 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -12,6 +12,7 @@
 #include "build_assert.c"
 #include "build_bug.c"
 #include "cred.c"
+#include "device.c"
 #include "err.c"
 #include "fs.c"
 #include "io.c"
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
new file mode 100644
index 000000000000..9c9dd39584eb
--- /dev/null
+++ b/rust/kernel/devres.rs
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Devres abstraction
+//!
+//! [`Devres`] represents an abstraction for the kernel devres (device resource management)
+//! implementation.
+
+use crate::{
+    alloc::Flags,
+    bindings,
+    device::Device,
+    error::{Error, Result},
+    prelude::*,
+    revocable::Revocable,
+    sync::Arc,
+};
+
+use core::ops::Deref;
+
+#[pin_data]
+struct DevresInner<T> {
+    #[pin]
+    data: Revocable<T>,
+}
+
+/// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
+/// manage their lifetime.
+///
+/// [`Device`] bound resources should be freed when either the resource goes out of scope or the
+/// [`Device`] is unbound respectively, depending on what happens first.
+///
+/// To achieve that [`Devres`] registers a devres callback on creation, which is called once the
+/// [`Device`] is unbound, revoking access to the encapsulated resource (see also [`Revocable`]).
+///
+/// After the [`Devres`] has been unbound it is not possible to access the encapsulated resource
+/// anymore.
+///
+/// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
+/// [`Drop`] implementation.
+///
+/// # Example
+///
+/// ```no_run
+/// # use kernel::{bindings, c_str, device::Device, devres::Devres, 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; just for the example to get an `ARef<Device>` instance.
+/// # let dev = unsafe { Device::get_device(core::ptr::null_mut()) };
+///
+/// // SAFETY: Invalid usage for example purposes.
+/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
+/// let devres = Devres::new(&dev, iomem, GFP_KERNEL)?;
+///
+/// let res = devres.try_access().ok_or(ENXIO)?;
+/// res.writel(0x42, 0x0);
+/// # Ok(())
+/// # }
+/// ```
+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.
+        //
+        // `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()`.
+        //
+        // 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() };
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6c836ab73771..2b61bf99d1ee 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -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)]
-- 
2.47.1

[PATCH v7 09/16] rust: pci: add basic PCI device / driver abstractions

[Danilo Krummrich]

Implement the basic PCI abstractions required to write a basic PCI
driver. This includes the following data structures:

The `pci::Driver` trait represents the interface to the driver and
provides `pci::Driver::probe` for the driver to implement.

The `pci::Device` abstraction represents a `struct pci_dev` and provides
abstractions for common functions, such as `pci::Device::set_master`.

In order to provide the PCI specific parts to a generic
`driver::Registration` the `driver::RegistrationOps` trait is implemented
by `pci::Adapter`.

`pci::DeviceId` implements PCI device IDs based on the generic
`device_id::RawDevceId` abstraction.

Co-developed-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/pci.c              |  18 ++
 rust/kernel/lib.rs              |   2 +
 rust/kernel/pci.rs              | 292 ++++++++++++++++++++++++++++++++
 6 files changed, 315 insertions(+)
 create mode 100644 rust/helpers/pci.c
 create mode 100644 rust/kernel/pci.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index a9ae2d69f961..38d53b2f12d6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18104,6 +18104,7 @@ F:	include/asm-generic/pci*
 F:	include/linux/of_pci.h
 F:	include/linux/pci*
 F:	include/uapi/linux/pci*
+F:	rust/kernel/pci.rs
 
 PCIE BANDWIDTH CONTROLLER
 M:	Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 5c4dfe22f41a..6d7a68e2ecb7 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -20,6 +20,7 @@
 #include <linux/jump_label.h>
 #include <linux/mdio.h>
 #include <linux/miscdevice.h>
+#include <linux/pci.h>
 #include <linux/phy.h>
 #include <linux/pid_namespace.h>
 #include <linux/poll.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index a3b52aa021de..3fda33cd42d4 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -20,6 +20,7 @@
 #include "kunit.c"
 #include "mutex.c"
 #include "page.c"
+#include "pci.c"
 #include "pid_namespace.c"
 #include "rbtree.c"
 #include "rcu.c"
diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
new file mode 100644
index 000000000000..8ba22f911459
--- /dev/null
+++ b/rust/helpers/pci.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/pci.h>
+
+void rust_helper_pci_set_drvdata(struct pci_dev *pdev, void *data)
+{
+	pci_set_drvdata(pdev, data);
+}
+
+void *rust_helper_pci_get_drvdata(struct pci_dev *pdev)
+{
+	return pci_get_drvdata(pdev);
+}
+
+resource_size_t rust_helper_pci_resource_len(struct pci_dev *pdev, int bar)
+{
+	return pci_resource_len(pdev, bar);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 2b61bf99d1ee..1dc7eda6b480 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -82,6 +82,8 @@
 pub use bindings;
 pub mod io;
 pub use macros;
+#[cfg(all(CONFIG_PCI, CONFIG_PCI_MSI))]
+pub mod pci;
 pub use uapi;
 
 #[doc(hidden)]
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
new file mode 100644
index 000000000000..581a2d140c8d
--- /dev/null
+++ b/rust/kernel/pci.rs
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the PCI bus.
+//!
+//! C header: [`include/linux/pci.h`](srctree/include/linux/pci.h)
+
+use crate::{
+    bindings, container_of, device,
+    device_id::RawDeviceId,
+    driver,
+    error::{to_result, Result},
+    str::CStr,
+    types::{ARef, ForeignOwnable, Opaque},
+    ThisModule,
+};
+use core::ptr::addr_of_mut;
+use kernel::prelude::*;
+
+/// An adapter for the registration of PCI drivers.
+pub struct Adapter<T: Driver>(T);
+
+impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+    type RegType = bindings::pci_driver;
+
+    fn register(
+        pdrv: &Opaque<Self::RegType>,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        // SAFETY: It's safe to set the fields of `struct pci_driver` on initialization.
+        unsafe {
+            (*pdrv.get()).name = name.as_char_ptr();
+            (*pdrv.get()).probe = Some(Self::probe_callback);
+            (*pdrv.get()).remove = Some(Self::remove_callback);
+            (*pdrv.get()).id_table = T::ID_TABLE.as_ptr();
+        }
+
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        to_result(unsafe {
+            bindings::__pci_register_driver(pdrv.get(), module.0, name.as_char_ptr())
+        })
+    }
+
+    fn unregister(pdrv: &Opaque<Self::RegType>) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        unsafe { bindings::pci_unregister_driver(pdrv.get()) }
+    }
+}
+
+impl<T: Driver + 'static> Adapter<T> {
+    extern "C" fn probe_callback(
+        pdev: *mut bindings::pci_dev,
+        id: *const bindings::pci_device_id,
+    ) -> kernel::ffi::c_int {
+        // SAFETY: The PCI bus only ever calls the probe callback with a valid pointer to a
+        // `struct pci_dev`.
+        let dev = unsafe { device::Device::get_device(addr_of_mut!((*pdev).dev)) };
+        // SAFETY: `dev` is guaranteed to be embedded in a valid `struct pci_dev` by the call
+        // above.
+        let mut pdev = unsafe { Device::from_dev(dev) };
+
+        // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct pci_device_id` and
+        // does not add additional invariants, so it's safe to transmute.
+        let id = unsafe { &*id.cast::<DeviceId>() };
+        let info = T::ID_TABLE.info(id.index());
+
+        match T::probe(&mut pdev, info) {
+            Ok(data) => {
+                // Let the `struct pci_dev` own a reference of the driver's private data.
+                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
+                // `struct pci_dev`.
+                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
+            }
+            Err(err) => return Error::to_errno(err),
+        }
+
+        0
+    }
+
+    extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
+        // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
+        // `struct pci_dev`.
+        let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
+
+        // SAFETY: `remove_callback` is only ever called after a successful call to
+        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
+        // `KBox<T>` pointer created through `KBox::into_foreign`.
+        let _ = unsafe { KBox::<T>::from_foreign(ptr) };
+    }
+}
+
+/// Declares a kernel module that exposes a single PCI driver.
+///
+/// # Example
+///
+///```ignore
+/// kernel::module_pci_driver! {
+///     type: MyDriver,
+///     name: "Module name",
+///     author: "Author name",
+///     description: "Description",
+///     license: "GPL v2",
+/// }
+///```
+#[macro_export]
+macro_rules! module_pci_driver {
+($($f:tt)*) => {
+    $crate::module_driver!(<T>, $crate::pci::Adapter<T>, { $($f)* });
+};
+}
+
+/// Abstraction for bindings::pci_device_id.
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct DeviceId(bindings::pci_device_id);
+
+impl DeviceId {
+    const PCI_ANY_ID: u32 = !0;
+
+    /// Equivalent to C's `PCI_DEVICE` macro.
+    ///
+    /// Create a new `pci::DeviceId` from a vendor and device ID number.
+    pub const fn from_id(vendor: u32, device: u32) -> Self {
+        Self(bindings::pci_device_id {
+            vendor,
+            device,
+            subvendor: DeviceId::PCI_ANY_ID,
+            subdevice: DeviceId::PCI_ANY_ID,
+            class: 0,
+            class_mask: 0,
+            driver_data: 0,
+            override_only: 0,
+        })
+    }
+
+    /// Equivalent to C's `PCI_DEVICE_CLASS` macro.
+    ///
+    /// Create a new `pci::DeviceId` from a class number and mask.
+    pub const fn from_class(class: u32, class_mask: u32) -> Self {
+        Self(bindings::pci_device_id {
+            vendor: DeviceId::PCI_ANY_ID,
+            device: DeviceId::PCI_ANY_ID,
+            subvendor: DeviceId::PCI_ANY_ID,
+            subdevice: DeviceId::PCI_ANY_ID,
+            class,
+            class_mask,
+            driver_data: 0,
+            override_only: 0,
+        })
+    }
+}
+
+// SAFETY:
+// * `DeviceId` is a `#[repr(transparent)` wrapper of `pci_device_id` and does not add
+//   additional invariants, so it's safe to transmute to `RawType`.
+// * `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
+unsafe impl RawDeviceId for DeviceId {
+    type RawType = bindings::pci_device_id;
+
+    const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::pci_device_id, driver_data);
+
+    fn index(&self) -> usize {
+        self.0.driver_data as _
+    }
+}
+
+/// IdTable type for PCI
+pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
+
+/// Create a PCI `IdTable` with its alias for modpost.
+#[macro_export]
+macro_rules! pci_device_table {
+    ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => {
+        const $table_name: $crate::device_id::IdArray<
+            $crate::pci::DeviceId,
+            $id_info_type,
+            { $table_data.len() },
+        > = $crate::device_id::IdArray::new($table_data);
+
+        $crate::module_device_table!("pci", $module_table_name, $table_name);
+    };
+}
+
+/// The PCI driver trait.
+///
+/// # Example
+///
+///```
+/// # use kernel::{bindings, pci};
+///
+/// struct MyDriver;
+///
+/// kernel::pci_device_table!(
+///     PCI_TABLE,
+///     MODULE_PCI_TABLE,
+///     <MyDriver as pci::Driver>::IdInfo,
+///     [
+///         (pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, bindings::PCI_ANY_ID as _), ())
+///     ]
+/// );
+///
+/// impl pci::Driver for MyDriver {
+///     type IdInfo = ();
+///     const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
+///
+///     fn probe(
+///         _pdev: &mut pci::Device,
+///         _id_info: &Self::IdInfo,
+///     ) -> Result<Pin<KBox<Self>>> {
+///         Err(ENODEV)
+///     }
+/// }
+///```
+/// Drivers must implement this trait in order to get a PCI driver registered. Please refer to the
+/// `Adapter` documentation for an example.
+pub trait Driver {
+    /// The type holding information about each device id supported by the driver.
+    ///
+    /// TODO: Use associated_type_defaults once stabilized:
+    ///
+    /// type IdInfo: 'static = ();
+    type IdInfo: 'static;
+
+    /// The table of device ids supported by the driver.
+    const ID_TABLE: IdTable<Self::IdInfo>;
+
+    /// PCI driver probe.
+    ///
+    /// Called when a new platform device is added or discovered.
+    /// Implementers should attempt to initialize the device here.
+    fn probe(dev: &mut Device, id_info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>;
+}
+
+/// The PCI device representation.
+///
+/// A PCI device is based on an always reference counted `device:Device` instance. Cloning a PCI
+/// device, hence, also increments the base device' reference count.
+#[derive(Clone)]
+pub struct Device(ARef<device::Device>);
+
+impl Device {
+    /// Create a PCI Device instance from an existing `device::Device`.
+    ///
+    /// # Safety
+    ///
+    /// `dev` must be an `ARef<device::Device>` whose underlying `bindings::device` is a member of
+    /// a `bindings::pci_dev`.
+    pub unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
+        Self(dev)
+    }
+
+    fn as_raw(&self) -> *mut bindings::pci_dev {
+        // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
+        // embedded in `struct pci_dev`.
+        unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) as _ }
+    }
+
+    /// Returns the PCI vendor ID.
+    pub fn vendor_id(&self) -> u16 {
+        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        unsafe { (*self.as_raw()).vendor }
+    }
+
+    /// Returns the PCI device ID.
+    pub fn device_id(&self) -> u16 {
+        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        unsafe { (*self.as_raw()).device }
+    }
+
+    /// Enable memory resources for this device.
+    pub fn enable_device_mem(&self) -> Result {
+        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
+        let ret = unsafe { bindings::pci_enable_device_mem(self.as_raw()) };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Enable bus-mastering for this device.
+    pub fn set_master(&self) {
+        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
+        unsafe { bindings::pci_set_master(self.as_raw()) };
+    }
+}
+
+impl AsRef<device::Device> for Device {
+    fn as_ref(&self) -> &device::Device {
+        &self.0
+    }
+}
-- 
2.47.1

[PATCH v7 10/16] rust: pci: implement I/O mappable `pci::Bar`

[Danilo Krummrich]

Implement `pci::Bar`, `pci::Device::iomap_region` and
`pci::Device::iomap_region_sized` to allow for I/O mappings of PCI BARs.

To ensure that a `pci::Bar`, and hence the I/O memory mapping, can't
out-live the PCI device, the `pci::Bar` type is always embedded into a
`Devres` container, such that the `pci::Bar` is revoked once the device
is unbound and hence the I/O mapped memory is unmapped.

A `pci::Bar` can be requested with (`pci::Device::iomap_region_sized`) or
without (`pci::Device::iomap_region`) a const generic representing the
minimal requested size of the I/O mapped memory region. In case of the
latter only runtime checked I/O reads / writes are possible.

Co-developed-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/pci.rs | 142 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 141 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 581a2d140c8d..d5e7f0b15303 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -5,15 +5,19 @@
 //! C header: [`include/linux/pci.h`](srctree/include/linux/pci.h)
 
 use crate::{
+    alloc::flags::*,
     bindings, container_of, device,
     device_id::RawDeviceId,
+    devres::Devres,
     driver,
     error::{to_result, Result},
+    io::Io,
+    io::IoRaw,
     str::CStr,
     types::{ARef, ForeignOwnable, Opaque},
     ThisModule,
 };
-use core::ptr::addr_of_mut;
+use core::{ops::Deref, ptr::addr_of_mut};
 use kernel::prelude::*;
 
 /// An adapter for the registration of PCI drivers.
@@ -235,9 +239,115 @@ pub trait Driver {
 ///
 /// A PCI device is based on an always reference counted `device:Device` instance. Cloning a PCI
 /// device, hence, also increments the base device' reference count.
+///
+/// # Invariants
+///
+/// `Device` hold a valid reference of `ARef<device::Device>` whose underlying `struct device` is a
+/// member of a `struct pci_dev`.
 #[derive(Clone)]
 pub struct Device(ARef<device::Device>);
 
+/// A PCI BAR to perform I/O-Operations on.
+///
+/// # Invariants
+///
+/// `Bar` always holds an `IoRaw` inststance that holds a valid pointer to the start of the I/O
+/// memory mapped PCI bar and its size.
+pub struct Bar<const SIZE: usize = 0> {
+    pdev: Device,
+    io: IoRaw<SIZE>,
+    num: i32,
+}
+
+impl<const SIZE: usize> Bar<SIZE> {
+    fn new(pdev: Device, num: u32, name: &CStr) -> Result<Self> {
+        let len = pdev.resource_len(num)?;
+        if len == 0 {
+            return Err(ENOMEM);
+        }
+
+        // Convert to `i32`, since that's what all the C bindings use.
+        let num = i32::try_from(num)?;
+
+        // SAFETY:
+        // `pdev` is valid by the invariants of `Device`.
+        // `num` is checked for validity by a previous call to `Device::resource_len`.
+        // `name` is always valid.
+        let ret = unsafe { bindings::pci_request_region(pdev.as_raw(), num, name.as_char_ptr()) };
+        if ret != 0 {
+            return Err(EBUSY);
+        }
+
+        // SAFETY:
+        // `pdev` is valid by the invariants of `Device`.
+        // `num` is checked for validity by a previous call to `Device::resource_len`.
+        // `name` is always valid.
+        let ioptr: usize = unsafe { bindings::pci_iomap(pdev.as_raw(), num, 0) } as usize;
+        if ioptr == 0 {
+            // SAFETY:
+            // `pdev` valid by the invariants of `Device`.
+            // `num` is checked for validity by a previous call to `Device::resource_len`.
+            unsafe { bindings::pci_release_region(pdev.as_raw(), num) };
+            return Err(ENOMEM);
+        }
+
+        let io = match IoRaw::new(ioptr, len as usize) {
+            Ok(io) => io,
+            Err(err) => {
+                // SAFETY:
+                // `pdev` is valid by the invariants of `Device`.
+                // `ioptr` is guaranteed to be the start of a valid I/O mapped memory region.
+                // `num` is checked for validity by a previous call to `Device::resource_len`.
+                unsafe { Self::do_release(&pdev, ioptr, num) };
+                return Err(err);
+            }
+        };
+
+        Ok(Bar { pdev, io, num })
+    }
+
+    /// # Safety
+    ///
+    /// `ioptr` must be a valid pointer to the memory mapped PCI bar number `num`.
+    unsafe fn do_release(pdev: &Device, ioptr: usize, num: i32) {
+        // SAFETY:
+        // `pdev` is valid by the invariants of `Device`.
+        // `ioptr` is valid by the safety requirements.
+        // `num` is valid by the safety requirements.
+        unsafe {
+            bindings::pci_iounmap(pdev.as_raw(), ioptr as _);
+            bindings::pci_release_region(pdev.as_raw(), num);
+        }
+    }
+
+    fn release(&self) {
+        // SAFETY: The safety requirements are guaranteed by the type invariant of `self.pdev`.
+        unsafe { Self::do_release(&self.pdev, self.io.addr(), self.num) };
+    }
+}
+
+impl Bar {
+    fn index_is_valid(index: u32) -> bool {
+        // A `struct pci_dev` owns an array of resources with at most `PCI_NUM_RESOURCES` entries.
+        index < bindings::PCI_NUM_RESOURCES
+    }
+}
+
+impl<const SIZE: usize> Drop for Bar<SIZE> {
+    fn drop(&mut self) {
+        self.release();
+    }
+}
+
+impl<const SIZE: usize> Deref for Bar<SIZE> {
+    type Target = Io<SIZE>;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: By the type invariant of `Self`, the MMIO range in `self.io` is properly mapped.
+        unsafe { Io::from_raw(&self.io) }
+    }
+}
+
 impl Device {
     /// Create a PCI Device instance from an existing `device::Device`.
     ///
@@ -283,6 +393,36 @@ pub fn set_master(&self) {
         // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
         unsafe { bindings::pci_set_master(self.as_raw()) };
     }
+
+    /// Returns the size of the given PCI bar resource.
+    pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
+        if !Bar::index_is_valid(bar) {
+            return Err(EINVAL);
+        }
+
+        // SAFETY:
+        // - `bar` is a valid bar number, as guaranteed by the above call to `Bar::index_is_valid`,
+        // - by its type invariant `self.as_raw` is always a valid pointer to a `struct pci_dev`.
+        Ok(unsafe { bindings::pci_resource_len(self.as_raw(), bar.try_into()?) })
+    }
+
+    /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
+    /// can be performed on compile time for offsets (plus the requested type size) < SIZE.
+    pub fn iomap_region_sized<const SIZE: usize>(
+        &self,
+        bar: u32,
+        name: &CStr,
+    ) -> Result<Devres<Bar<SIZE>>> {
+        let bar = Bar::<SIZE>::new(self.clone(), bar, name)?;
+        let devres = Devres::new(self.as_ref(), bar, GFP_KERNEL)?;
+
+        Ok(devres)
+    }
+
+    /// Mapps an entire PCI-BAR after performing a region-request on it.
+    pub fn iomap_region(&self, bar: u32, name: &CStr) -> Result<Devres<Bar>> {
+        self.iomap_region_sized::<0>(bar, name)
+    }
 }
 
 impl AsRef<device::Device> for Device {
-- 
2.47.1

[PATCH v7 11/16] samples: rust: add Rust PCI sample driver

[Danilo Krummrich]

This commit adds a sample Rust PCI driver for QEMU's "pci-testdev"
device. To enable this device QEMU has to be called with
`-device pci-testdev`.

The same driver shows how to use the PCI device / driver abstractions,
as well as how to request and map PCI BARs, including a short sequence of
MMIO operations.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS                     |   1 +
 samples/rust/Kconfig            |  11 ++++
 samples/rust/Makefile           |   1 +
 samples/rust/rust_driver_pci.rs | 110 ++++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+)
 create mode 100644 samples/rust/rust_driver_pci.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 38d53b2f12d6..7cfcd5c6c8bd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18105,6 +18105,7 @@ F:	include/linux/of_pci.h
 F:	include/linux/pci*
 F:	include/uapi/linux/pci*
 F:	rust/kernel/pci.rs
+F:	samples/rust/rust_driver_pci.rs
 
 PCIE BANDWIDTH CONTROLLER
 M:	Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..6d468193cdd8 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -30,6 +30,17 @@ config SAMPLE_RUST_PRINT
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DRIVER_PCI
+	tristate "PCI Driver"
+	depends on PCI
+	help
+	  This option builds the Rust PCI driver sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called driver_pci.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_HOSTPROGS
 	bool "Host programs"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index c1a5c1655395..2f5b6bdb2fa5 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -3,6 +3,7 @@ ccflags-y += -I$(src)				# needed for trace events
 
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
+obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
 
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
new file mode 100644
index 000000000000..1fb6e44f3395
--- /dev/null
+++ b/samples/rust/rust_driver_pci.rs
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust PCI driver sample (based on QEMU's `pci-testdev`).
+//!
+//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
+
+use kernel::{bindings, c_str, devres::Devres, pci, prelude::*};
+
+struct Regs;
+
+impl Regs {
+    const TEST: usize = 0x0;
+    const OFFSET: usize = 0x4;
+    const DATA: usize = 0x8;
+    const COUNT: usize = 0xC;
+    const END: usize = 0x10;
+}
+
+type Bar0 = pci::Bar<{ Regs::END }>;
+
+#[derive(Debug)]
+struct TestIndex(u8);
+
+impl TestIndex {
+    const NO_EVENTFD: Self = Self(0);
+}
+
+struct SampleDriver {
+    pdev: pci::Device,
+    bar: Devres<Bar0>,
+}
+
+kernel::pci_device_table!(
+    PCI_TABLE,
+    MODULE_PCI_TABLE,
+    <SampleDriver as pci::Driver>::IdInfo,
+    [(
+        pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, 0x5),
+        TestIndex::NO_EVENTFD
+    )]
+);
+
+impl SampleDriver {
+    fn testdev(index: &TestIndex, bar: &Bar0) -> Result<u32> {
+        // Select the test.
+        bar.writeb(index.0, Regs::TEST);
+
+        let offset = u32::from_le(bar.readl(Regs::OFFSET)) as usize;
+        let data = bar.readb(Regs::DATA);
+
+        // Write `data` to `offset` to increase `count` by one.
+        //
+        // Note that we need `try_writeb`, since `offset` can't be checked at compile-time.
+        bar.try_writeb(data, offset)?;
+
+        Ok(bar.readl(Regs::COUNT))
+    }
+}
+
+impl pci::Driver for SampleDriver {
+    type IdInfo = TestIndex;
+
+    const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
+
+    fn probe(pdev: &mut pci::Device, info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
+        dev_dbg!(
+            pdev.as_ref(),
+            "Probe Rust PCI driver sample (PCI ID: 0x{:x}, 0x{:x}).\n",
+            pdev.vendor_id(),
+            pdev.device_id()
+        );
+
+        pdev.enable_device_mem()?;
+        pdev.set_master();
+
+        let bar = pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci"))?;
+
+        let drvdata = KBox::new(
+            Self {
+                pdev: pdev.clone(),
+                bar,
+            },
+            GFP_KERNEL,
+        )?;
+
+        let bar = drvdata.bar.try_access().ok_or(ENXIO)?;
+
+        dev_info!(
+            pdev.as_ref(),
+            "pci-testdev data-match count: {}\n",
+            Self::testdev(info, &bar)?
+        );
+
+        Ok(drvdata.into())
+    }
+}
+
+impl Drop for SampleDriver {
+    fn drop(&mut self) {
+        dev_dbg!(self.pdev.as_ref(), "Remove Rust PCI driver sample.\n");
+    }
+}
+
+kernel::module_pci_driver! {
+    type: SampleDriver,
+    name: "rust_driver_pci",
+    author: "Danilo Krummrich",
+    description: "Rust PCI driver",
+    license: "GPL v2",
+}
-- 
2.47.1

[PATCH v7 12/16] rust: of: add `of::DeviceId` abstraction

[Danilo Krummrich]

`of::DeviceId` is an abstraction around `struct of_device_id`.

This is used by subsequent patches, in particular the platform bus
abstractions, to create OF device ID tables.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS        |  1 +
 rust/kernel/lib.rs |  1 +
 rust/kernel/of.rs  | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)
 create mode 100644 rust/kernel/of.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 7cfcd5c6c8bd..472cb259483e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17504,6 +17504,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
 F:	Documentation/ABI/testing/sysfs-firmware-ofw
 F:	drivers/of/
 F:	include/linux/of*.h
+F:	rust/kernel/of.rs
 F:	scripts/dtc/
 F:	tools/testing/selftests/dt/
 K:	of_overlay_notifier_
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 1dc7eda6b480..27f914b0769b 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -56,6 +56,7 @@
 pub mod miscdevice;
 #[cfg(CONFIG_NET)]
 pub mod net;
+pub mod of;
 pub mod page;
 pub mod pid_namespace;
 pub mod prelude;
diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
new file mode 100644
index 000000000000..04f2d8ef29cb
--- /dev/null
+++ b/rust/kernel/of.rs
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Device Tree / Open Firmware abstractions.
+
+use crate::{bindings, device_id::RawDeviceId, prelude::*};
+
+/// IdTable type for OF drivers.
+pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
+
+/// An open firmware device id.
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct DeviceId(bindings::of_device_id);
+
+// SAFETY:
+// * `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and does not add
+//   additional invariants, so it's safe to transmute to `RawType`.
+// * `DRIVER_DATA_OFFSET` is the offset to the `data` field.
+unsafe impl RawDeviceId for DeviceId {
+    type RawType = bindings::of_device_id;
+
+    const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data);
+
+    fn index(&self) -> usize {
+        self.0.data as _
+    }
+}
+
+impl DeviceId {
+    /// Create a new device id from an OF 'compatible' string.
+    pub const fn new(compatible: &'static CStr) -> Self {
+        let src = compatible.as_bytes_with_nul();
+        // Replace with `bindings::of_device_id::default()` once stabilized for `const`.
+        // SAFETY: FFI type is valid to be zero-initialized.
+        let mut of: bindings::of_device_id = unsafe { core::mem::zeroed() };
+
+        // TODO: Use `clone_from_slice` once the corresponding types do match.
+        let mut i = 0;
+        while i < src.len() {
+            of.compatible[i] = src[i] as _;
+            i += 1;
+        }
+
+        Self(of)
+    }
+}
+
+/// Create an OF `IdTable` with an "alias" for modpost.
+#[macro_export]
+macro_rules! of_device_table {
+    ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => {
+        const $table_name: $crate::device_id::IdArray<
+            $crate::of::DeviceId,
+            $id_info_type,
+            { $table_data.len() },
+        > = $crate::device_id::IdArray::new($table_data);
+
+        $crate::module_device_table!("of", $module_table_name, $table_name);
+    };
+}
-- 
2.47.1

[PATCH v7 13/16] rust: driver: implement `Adapter`

[Danilo Krummrich]

In order to not duplicate code in bus specific implementations (e.g.
platform), implement a generic `driver::Adapter` to represent the
connection of matched drivers and devices.

Bus specific `Adapter` implementations can simply implement this trait
to inherit generic functionality, such as matching OF or ACPI device IDs
and ID table entries.

Suggested-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/driver.rs           | 58 ++++++++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 6d7a68e2ecb7..8fe70183a392 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -20,6 +20,7 @@
 #include <linux/jump_label.h>
 #include <linux/mdio.h>
 #include <linux/miscdevice.h>
+#include <linux/of_device.h>
 #include <linux/pci.h>
 #include <linux/phy.h>
 #include <linux/pid_namespace.h>
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index c1957ee7bb7e..c630e65098ed 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -6,7 +6,7 @@
 //! register using the [`Registration`] class.
 
 use crate::error::{Error, Result};
-use crate::{init::PinInit, str::CStr, try_pin_init, types::Opaque, ThisModule};
+use crate::{device, init::PinInit, of, str::CStr, try_pin_init, types::Opaque, ThisModule};
 use core::pin::Pin;
 use macros::{pin_data, pinned_drop};
 
@@ -115,3 +115,59 @@ fn init(
         }
     }
 }
+
+/// The bus independent adapter to match a drivers and a devices.
+///
+/// This trait should be implemented by the bus specific adapter, which represents the connection
+/// of a device and a driver.
+///
+/// It provides bus independent functions for device / driver interactions.
+pub trait Adapter {
+    /// The type holding driver private data about each device id supported by the driver.
+    type IdInfo: 'static;
+
+    /// The [`of::IdTable`] of the corresponding driver.
+    fn of_id_table() -> Option<of::IdTable<Self::IdInfo>>;
+
+    /// Returns the driver's private data from the matching entry in the [`of::IdTable`], if any.
+    ///
+    /// If this returns `None`, it means there is no match with an entry in the [`of::IdTable`].
+    #[cfg(CONFIG_OF)]
+    fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
+        let table = Self::of_id_table()?;
+
+        // SAFETY:
+        // - `table` has static lifetime, hence it's valid for read,
+        // - `dev` is guaranteed to be valid while it's alive, and so is `pdev.as_ref().as_raw()`.
+        let raw_id = unsafe { bindings::of_match_device(table.as_ptr(), dev.as_raw()) };
+
+        if raw_id.is_null() {
+            None
+        } else {
+            // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and
+            // does not add additional invariants, so it's safe to transmute.
+            let id = unsafe { &*raw_id.cast::<of::DeviceId>() };
+
+            Some(table.info(<of::DeviceId as crate::device_id::RawDeviceId>::index(id)))
+        }
+    }
+
+    #[cfg(not(CONFIG_OF))]
+    #[allow(missing_docs)]
+    fn of_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> {
+        None
+    }
+
+    /// Returns the driver's private data from the matching entry of any of the ID tables, if any.
+    ///
+    /// If this returns `None`, it means that there is no match in any of the ID tables directly
+    /// associated with a [`device::Device`].
+    fn id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
+        let id = Self::of_id_info(dev);
+        if id.is_some() {
+            return id;
+        }
+
+        None
+    }
+}
-- 
2.47.1

[PATCH v7 14/16] rust: platform: add basic platform device / driver abstractions

[Danilo Krummrich]

Implement the basic platform bus abstractions required to write a basic
platform driver. This includes the following data structures:

The `platform::Driver` trait represents the interface to the driver and
provides `platform::Driver::probe` for the driver to implement.

The `platform::Device` abstraction represents a `struct platform_device`.

In order to provide the platform bus specific parts to a generic
`driver::Registration` the `driver::RegistrationOps` trait is implemented
by `platform::Adapter`.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/platform.c         |  13 +++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/platform.rs         | 198 ++++++++++++++++++++++++++++++++
 6 files changed, 215 insertions(+)
 create mode 100644 rust/helpers/platform.c
 create mode 100644 rust/kernel/platform.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 472cb259483e..798387acaefc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7036,6 +7036,7 @@ F:	rust/kernel/device.rs
 F:	rust/kernel/device_id.rs
 F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
+F:	rust/kernel/platform.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
 M:	Nishanth Menon <nm@ti.com>
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8fe70183a392..e9fdceb568b8 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -24,6 +24,7 @@
 #include <linux/pci.h>
 #include <linux/phy.h>
 #include <linux/pid_namespace.h>
+#include <linux/platform_device.h>
 #include <linux/poll.h>
 #include <linux/refcount.h>
 #include <linux/sched.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 3fda33cd42d4..0640b7e115be 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -20,6 +20,7 @@
 #include "kunit.c"
 #include "mutex.c"
 #include "page.c"
+#include "platform.c"
 #include "pci.c"
 #include "pid_namespace.c"
 #include "rbtree.c"
diff --git a/rust/helpers/platform.c b/rust/helpers/platform.c
new file mode 100644
index 000000000000..ab9b9f317301
--- /dev/null
+++ b/rust/helpers/platform.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/platform_device.h>
+
+void *rust_helper_platform_get_drvdata(const struct platform_device *pdev)
+{
+	return platform_get_drvdata(pdev);
+}
+
+void rust_helper_platform_set_drvdata(struct platform_device *pdev, void *data)
+{
+	platform_set_drvdata(pdev, data);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 27f914b0769b..e59250dc6c6e 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -59,6 +59,7 @@
 pub mod of;
 pub mod page;
 pub mod pid_namespace;
+pub mod platform;
 pub mod prelude;
 pub mod print;
 pub mod rbtree;
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
new file mode 100644
index 000000000000..03287794f9d0
--- /dev/null
+++ b/rust/kernel/platform.rs
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the platform bus.
+//!
+//! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
+
+use crate::{
+    bindings, container_of, device, driver,
+    error::{to_result, Result},
+    of,
+    prelude::*,
+    str::CStr,
+    types::{ARef, ForeignOwnable, Opaque},
+    ThisModule,
+};
+
+use core::ptr::addr_of_mut;
+
+/// An adapter for the registration of platform drivers.
+pub struct Adapter<T: Driver>(T);
+
+impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+    type RegType = bindings::platform_driver;
+
+    fn register(
+        pdrv: &Opaque<Self::RegType>,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        let of_table = match T::OF_ID_TABLE {
+            Some(table) => table.as_ptr(),
+            None => core::ptr::null(),
+        };
+
+        // SAFETY: It's safe to set the fields of `struct platform_driver` on initialization.
+        unsafe {
+            (*pdrv.get()).driver.name = name.as_char_ptr();
+            (*pdrv.get()).probe = Some(Self::probe_callback);
+            (*pdrv.get()).remove = Some(Self::remove_callback);
+            (*pdrv.get()).driver.of_match_table = of_table;
+        }
+
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
+    }
+
+    fn unregister(pdrv: &Opaque<Self::RegType>) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        unsafe { bindings::platform_driver_unregister(pdrv.get()) };
+    }
+}
+
+impl<T: Driver + 'static> Adapter<T> {
+    extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ffi::c_int {
+        // SAFETY: The platform bus only ever calls the probe callback with a valid `pdev`.
+        let dev = unsafe { device::Device::get_device(addr_of_mut!((*pdev).dev)) };
+        // SAFETY: `dev` is guaranteed to be embedded in a valid `struct platform_device` by the
+        // call above.
+        let mut pdev = unsafe { Device::from_dev(dev) };
+
+        let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
+        match T::probe(&mut pdev, info) {
+            Ok(data) => {
+                // Let the `struct platform_device` own a reference of the driver's private data.
+                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
+                // `struct platform_device`.
+                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
+            }
+            Err(err) => return Error::to_errno(err),
+        }
+
+        0
+    }
+
+    extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
+        // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
+        let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
+
+        // SAFETY: `remove_callback` is only ever called after a successful call to
+        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
+        // `KBox<T>` pointer created through `KBox::into_foreign`.
+        let _ = unsafe { KBox::<T>::from_foreign(ptr) };
+    }
+}
+
+impl<T: Driver + 'static> driver::Adapter for Adapter<T> {
+    type IdInfo = T::IdInfo;
+
+    fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> {
+        T::OF_ID_TABLE
+    }
+}
+
+/// Declares a kernel module that exposes a single platform driver.
+///
+/// # Examples
+///
+/// ```ignore
+/// kernel::module_platform_driver! {
+///     type: MyDriver,
+///     name: "Module name",
+///     author: "Author name",
+///     description: "Description",
+///     license: "GPL v2",
+/// }
+/// ```
+#[macro_export]
+macro_rules! module_platform_driver {
+    ($($f:tt)*) => {
+        $crate::module_driver!(<T>, $crate::platform::Adapter<T>, { $($f)* });
+    };
+}
+
+/// The platform driver trait.
+///
+/// Drivers must implement this trait in order to get a platform driver registered.
+///
+/// # Example
+///
+///```
+/// # use kernel::{bindings, c_str, of, platform};
+///
+/// struct MyDriver;
+///
+/// kernel::of_device_table!(
+///     OF_TABLE,
+///     MODULE_OF_TABLE,
+///     <MyDriver as platform::Driver>::IdInfo,
+///     [
+///         (of::DeviceId::new(c_str!("test,device")), ())
+///     ]
+/// );
+///
+/// impl platform::Driver for MyDriver {
+///     type IdInfo = ();
+///     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+///
+///     fn probe(
+///         _pdev: &mut platform::Device,
+///         _id_info: Option<&Self::IdInfo>,
+///     ) -> Result<Pin<KBox<Self>>> {
+///         Err(ENODEV)
+///     }
+/// }
+///```
+pub trait Driver {
+    /// The type holding driver private data about each device id supported by the driver.
+    ///
+    /// TODO: Use associated_type_defaults once stabilized:
+    ///
+    /// type IdInfo: 'static = ();
+    type IdInfo: 'static;
+
+    /// The table of OF device ids supported by the driver.
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>>;
+
+    /// Platform driver probe.
+    ///
+    /// Called when a new platform device is added or discovered.
+    /// Implementers should attempt to initialize the device here.
+    fn probe(dev: &mut Device, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
+}
+
+/// The platform device representation.
+///
+/// A platform device is based on an always reference counted `device:Device` instance. Cloning a
+/// platform device, hence, also increments the base device' reference count.
+///
+/// # Invariants
+///
+/// `Device` holds a valid reference of `ARef<device::Device>` whose underlying `struct device` is a
+/// member of a `struct platform_device`.
+#[derive(Clone)]
+pub struct Device(ARef<device::Device>);
+
+impl Device {
+    /// Convert a raw kernel device into a `Device`
+    ///
+    /// # Safety
+    ///
+    /// `dev` must be an `Aref<device::Device>` whose underlying `bindings::device` is a member of a
+    /// `bindings::platform_device`.
+    unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
+        Self(dev)
+    }
+
+    fn as_raw(&self) -> *mut bindings::platform_device {
+        // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
+        // embedded in `struct platform_device`.
+        unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
+    }
+}
+
+impl AsRef<device::Device> for Device {
+    fn as_ref(&self) -> &device::Device {
+        &self.0
+    }
+}
-- 
2.47.1

[PATCH v7 15/16] samples: rust: add Rust platform sample driver

[Danilo Krummrich]

Add a sample Rust platform driver illustrating the usage of the platform
bus abstractions.

This driver probes through either a match of device / driver name or a
match within the OF ID table.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS                                  |  1 +
 drivers/of/unittest-data/tests-platform.dtsi |  5 ++
 samples/rust/Kconfig                         | 10 ++++
 samples/rust/Makefile                        |  1 +
 samples/rust/rust_driver_platform.rs         | 49 ++++++++++++++++++++
 5 files changed, 66 insertions(+)
 create mode 100644 samples/rust/rust_driver_platform.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 798387acaefc..ed4b293b1baa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7037,6 +7037,7 @@ F:	rust/kernel/device_id.rs
 F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
 F:	rust/kernel/platform.rs
+F:	samples/rust/rust_driver_platform.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
 M:	Nishanth Menon <nm@ti.com>
diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
index fa39611071b3..2caaf1c10ee6 100644
--- a/drivers/of/unittest-data/tests-platform.dtsi
+++ b/drivers/of/unittest-data/tests-platform.dtsi
@@ -33,6 +33,11 @@ dev@100 {
 					reg = <0x100>;
 				};
 			};
+
+			test-device@2 {
+				compatible = "test,rust-device";
+				reg = <0x2>;
+			};
 		};
 	};
 };
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 6d468193cdd8..70126b750426 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -41,6 +41,16 @@ config SAMPLE_RUST_DRIVER_PCI
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DRIVER_PLATFORM
+	tristate "Platform Driver"
+	help
+	  This option builds the Rust Platform driver sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_driver_platform.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_HOSTPROGS
 	bool "Host programs"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 2f5b6bdb2fa5..761d13fff018 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -4,6 +4,7 @@ ccflags-y += -I$(src)				# needed for trace events
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
+obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
 
diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
new file mode 100644
index 000000000000..8120609e2940
--- /dev/null
+++ b/samples/rust/rust_driver_platform.rs
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust Platform driver sample.
+
+use kernel::{c_str, of, platform, prelude::*};
+
+struct SampleDriver {
+    pdev: platform::Device,
+}
+
+struct Info(u32);
+
+kernel::of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <SampleDriver as platform::Driver>::IdInfo,
+    [(of::DeviceId::new(c_str!("test,rust-device")), Info(42))]
+);
+
+impl platform::Driver for SampleDriver {
+    type IdInfo = Info;
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+    fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
+        dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
+
+        if let Some(info) = info {
+            dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
+        }
+
+        let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
+
+        Ok(drvdata.into())
+    }
+}
+
+impl Drop for SampleDriver {
+    fn drop(&mut self) {
+        dev_dbg!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n");
+    }
+}
+
+kernel::module_platform_driver! {
+    type: SampleDriver,
+    name: "rust_driver_platform",
+    author: "Danilo Krummrich",
+    description: "Rust Platform driver",
+    license: "GPL v2",
+}
-- 
2.47.1

[PATCH v7 16/16] MAINTAINERS: add Danilo to DRIVER CORE

[Danilo Krummrich]

Let's keep an eye on the Rust abstractions; add myself as reviewer to
DRIVER CORE.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ed4b293b1baa..15c8df5b3590 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7021,6 +7021,7 @@ F:	include/linux/component.h
 DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 R:	"Rafael J. Wysocki" <rafael@kernel.org>
+R:	Danilo Krummrich <dakr@kernel.org>
 S:	Supported
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
 F:	Documentation/core-api/kobject.rst
-- 
2.47.1

Re: [PATCH v7 00/16] Device / Driver PCI / Platform Rust abstractions

[Dirk Behme]

On 19.12.24 18:04, Danilo Krummrich wrote:
> This patch series implements the necessary Rust abstractions to implement
> device drivers in Rust.
> 
> This includes some basic generalizations for driver registration, handling of ID
> tables, MMIO operations and device resource handling.
> 
> Those generalizations are used to implement device driver support for two
> busses, the PCI and platform bus (with OF IDs) in order to provide some evidence
> that the generalizations work as intended.
> 
> The patch series also includes two patches adding two driver samples, one PCI
> driver and one platform driver.
> 
> The PCI bits are motivated by the Nova driver project [1], but are used by at
> least one more OOT driver (rnvme [2]).
> 
> The platform bits, besides adding some more evidence to the base abstractions,
> are required by a few more OOT drivers aiming at going upstream, i.e. rvkms [3],
> cpufreq-dt [4], asahi [5] and the i2c work from Fabien [6].
> 
> The patches of this series can also be [7], [8] and [9].
> 
> Changes in v7:
> ==============
> - Revocable:
>   - replace `compare_exchange` with `swap`
> 
> - Driver:
>   - fix warning when CONFIG_OF=n
> 
> - I/O:
>   - remove unnecessary return statement in rust_helper_iounmap()
>   - fix cast in doctest for `bindings::ioremap`
> 
> - Devres:
>   - fix cast in doctest for `bindings::ioremap`
> 
> - PCI:
>   - remove `Deref` of `pci::DeviceId`
>   - rename `DeviceId` constructors
>     - `new`       -> `from_id`
>     - `with_class -> `from_class`
> 
> - MISC:
>   - use `kernel::ffi::c_*` instead of `core::ffi::c_*`
>   - rebase onto latest rust-next (0c5928deada15a8d075516e6e0d9ee19011bb000)


Not sure if resending is required, but after a quick retest the
Tested-by from v6 still stands:

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>

Thanks

Dirk

Re: [PATCH v7 00/16] Device / Driver PCI / Platform Rust abstractions

[Greg KH]

On Thu, Dec 19, 2024 at 06:04:02PM +0100, Danilo Krummrich wrote:
> This patch series implements the necessary Rust abstractions to implement
> device drivers in Rust.
> 
> This includes some basic generalizations for driver registration, handling of ID
> tables, MMIO operations and device resource handling.
> 
> Those generalizations are used to implement device driver support for two
> busses, the PCI and platform bus (with OF IDs) in order to provide some evidence
> that the generalizations work as intended.
> 
> The patch series also includes two patches adding two driver samples, one PCI
> driver and one platform driver.
> 
> The PCI bits are motivated by the Nova driver project [1], but are used by at
> least one more OOT driver (rnvme [2]).
> 
> The platform bits, besides adding some more evidence to the base abstractions,
> are required by a few more OOT drivers aiming at going upstream, i.e. rvkms [3],
> cpufreq-dt [4], asahi [5] and the i2c work from Fabien [6].
> 
> The patches of this series can also be [7], [8] and [9].
> 
> Changes in v7:
> ==============
> - Revocable:
>   - replace `compare_exchange` with `swap`
> 
> - Driver:
>   - fix warning when CONFIG_OF=n
> 
> - I/O:
>   - remove unnecessary return statement in rust_helper_iounmap()
>   - fix cast in doctest for `bindings::ioremap`
> 
> - Devres:
>   - fix cast in doctest for `bindings::ioremap`
> 
> - PCI:
>   - remove `Deref` of `pci::DeviceId`
>   - rename `DeviceId` constructors
>     - `new`       -> `from_id`
>     - `with_class -> `from_class`
> 
> - MISC:
>   - use `kernel::ffi::c_*` instead of `core::ffi::c_*`
>   - rebase onto latest rust-next (0c5928deada15a8d075516e6e0d9ee19011bb000)

Thanks for this last round of changes, looks great to me!  I've added
them all to my driver-core-testing branch, and if 0-day doesn't
complain, will move it to driver-core-next for merging into 6.14-rc1.

Thanks for persisting with this, looks great.  Now the real work starts :)

thanks,

greg k-h

Re: [PATCH v7 02/16] rust: implement generic driver registration

[Gary Guo]

On Thu, 19 Dec 2024 18:04:04 +0100
Danilo Krummrich <dakr@kernel.org> 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`.
> 
> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Hi Danilo,

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

Best,
Gary

> ---
>  MAINTAINERS           |   1 +
>  rust/kernel/driver.rs | 117 ++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs    |   1 +
>  3 files changed, 119 insertions(+)
>  create mode 100644 rust/kernel/driver.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index baf0eeb9a355..2ad58ed40079 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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 <nm@ti.com>
> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> new file mode 100644
> index 000000000000..c1957ee7bb7e
> --- /dev/null
> +++ b/rust/kernel/driver.rs
> @@ -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
> +/// `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?

> +    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.

> +    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.
> +    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?

> +
> +                // 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.

> +                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);
> +    }
> +}
> +
> +/// 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)]`.

> +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)*
> +        }
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 61b82b78b915..7818407f9aac 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -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;

Re: [PATCH v7 03/16] rust: implement `IdArray`, `IdTable` and `RawDeviceId`

[Gary Guo]

On Thu, 19 Dec 2024 18:04:05 +0100
Danilo Krummrich <dakr@kernel.org> 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.
> 
> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Co-developed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Co-developed-by: Fabien Parent <fabien.parent@linaro.org>
> Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

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

> ---
>  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
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2ad58ed40079..3cfb68650347 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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)
> diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
> new file mode 100644
> index 000000000000..e5859217a579
> --- /dev/null
> +++ b/rust/kernel/device_id.rs
> 
> <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.

> <snip>
>
> +
> +/// Create device table alias for modpost.
> +#[macro_export]
> +macro_rules! module_device_table {
> +    ($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.

> +    };
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 7818407f9aac..66149ac5c0c9 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -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)]

Re: [PATCH v7 05/16] rust: types: add `Opaque::pin_init`

[Gary Guo]

On Thu, 19 Dec 2024 18:04:07 +0100
Danilo Krummrich <dakr@kernel.org> wrote:

> 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 <aliceryhl@google.com>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/types.rs | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index ec6457bb3084..3aea6af9a0bc 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -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

Re: [PATCH v7 01/16] rust: module: add trait `ModuleMetadata`

[Gary Guo]

On Thu, 19 Dec 2024 18:04:03 +0100
Danilo Krummrich <dakr@kernel.org> wrote:

> 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 <dakr@kernel.org>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/lib.rs    | 6 ++++++
>  rust/macros/module.rs | 4 ++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e1065a7551a3..61b82b78b915 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -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)
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index 2587f41b0d39..cdf94f4982df 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -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 {{

Re: [PATCH v7 04/16] rust: add rcu abstraction

[Gary Guo]

On Thu, 19 Dec 2024 18:04:06 +0100
Danilo Krummrich <dakr@kernel.org> wrote:

> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> Add a simple abstraction to guard critical code sections with an rcu
> read lock.
> 
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Co-developed-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  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]

> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3cfb68650347..0cc69e282889 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index dcf827a61b52..060750af6524 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -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"
> diff --git a/rust/helpers/rcu.c b/rust/helpers/rcu.c
> new file mode 100644
> index 000000000000..f1cec6583513
> --- /dev/null
> +++ b/rust/helpers/rcu.c
> @@ -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();
> +}
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 1eab7ebf25fd..0654008198b2 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -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};
> diff --git a/rust/kernel/sync/rcu.rs b/rust/kernel/sync/rcu.rs
> new file mode 100644
> index 000000000000..b51d9150ffe2
> --- /dev/null
> +++ b/rust/kernel/sync/rcu.rs
> @@ -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.
> +        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.

> +}
> +
> +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`.

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

Re: [PATCH v7 08/16] rust: add devres abstraction

[Gary Guo]

On Thu, 19 Dec 2024 18:04:10 +0100
Danilo Krummrich <dakr@kernel.org> 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 <dakr@kernel.org>
> ---
>  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>
>
> +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.
> +        //
> +        // `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.

> +        //
> +        // 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() };
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6c836ab73771..2b61bf99d1ee 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -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)]

Re: [PATCH v7 02/16] rust: implement generic driver registration

[Danilo Krummrich]

Hi Gary,

On Tue, Dec 24, 2024 at 07:58:21PM +0000, Gary Guo wrote:
> On Thu, 19 Dec 2024 18:04:04 +0100
> Danilo Krummrich <dakr@kernel.org> 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`.
> > 
> > Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> 
> Hi Danilo,
> 
> I think there're soundness issues with this API, please see comments
> inlined below.

Just in case you did not note, this series has been applied to the driver-core
tree already.

> 
> Best,
> Gary
> 
> > ---
> >  MAINTAINERS           |   1 +
> >  rust/kernel/driver.rs | 117 ++++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs    |   1 +
> >  3 files changed, 119 insertions(+)
> >  create mode 100644 rust/kernel/driver.rs
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index baf0eeb9a355..2ad58ed40079 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -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 <nm@ti.com>
> > diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> > new file mode 100644
> > index 000000000000..c1957ee7bb7e
> > --- /dev/null
> > +++ b/rust/kernel/driver.rs
> > @@ -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
> > +/// `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?
> 
> > +    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.

I think you're right. I didn't want this to be unsafe, and when I got rid of the
raw pointer, and hence removed the unsafe, I did miss the fact you point out
here. It's a bit unfortunate, especially, since `register` and `unregister` are
only ever called from this file.

I'll send a patch to fix it up, unless you want to send one yourself.

> 
> > +    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.
> > +    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?

`T::RegType` is always an FFI type, so this can be in the common code.

> 
> > +
> > +                // 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.

I think the "initialized" part is from before it was an `Opaque`.

> 
> > +                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);
> > +    }
> > +}
> > +
> > +/// 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)]`.

Why? It's a public kernel API to be used by bus abstractions. I don't think
documentation should be for drivers only.

> 
> > +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)*
> > +        }
> > +    }
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 61b82b78b915..7818407f9aac 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -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;
> 

Re: [PATCH v7 03/16] rust: implement `IdArray`, `IdTable` and `RawDeviceId`

[Danilo Krummrich]

On Tue, Dec 24, 2024 at 08:10:02PM +0000, Gary Guo wrote:
> On Thu, 19 Dec 2024 18:04:05 +0100
> Danilo Krummrich <dakr@kernel.org> 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.
> > 
> > Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Co-developed-by: Gary Guo <gary@garyguo.net>
> > Signed-off-by: Gary Guo <gary@garyguo.net>
> > Co-developed-by: Fabien Parent <fabien.parent@linaro.org>
> > Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> 
> Thank you for converting my prototype to a working patch. There's a nit below.
> 
> > ---
> >  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
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2ad58ed40079..3cfb68650347 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -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)
> > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
> > new file mode 100644
> > index 000000000000..e5859217a579
> > --- /dev/null
> > +++ b/rust/kernel/device_id.rs
> > 
> > <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.
> 
> > <snip>
> >
> > +
> > +/// Create device table alias for modpost.
> > +#[macro_export]
> > +macro_rules! module_device_table {
> > +    ($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.

Thanks for the note, this indeed saves a few lines.

Since the series has been applied already, feel free to send a patch.

> 
> > +    };
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 7818407f9aac..66149ac5c0c9 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -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)]
> 

Re: [PATCH v7 04/16] rust: add rcu abstraction

[Danilo Krummrich]

On Tue, Dec 24, 2024 at 08:54:50PM +0000, Gary Guo wrote:
> On Thu, 19 Dec 2024 18:04:06 +0100
> Danilo Krummrich <dakr@kernel.org> wrote:
> 
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > 
> > Add a simple abstraction to guard critical code sections with an rcu
> > read lock.
> > 
> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Co-developed-by: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  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]
> 
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3cfb68650347..0cc69e282889 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -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
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index dcf827a61b52..060750af6524 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -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"
> > diff --git a/rust/helpers/rcu.c b/rust/helpers/rcu.c
> > new file mode 100644
> > index 000000000000..f1cec6583513
> > --- /dev/null
> > +++ b/rust/helpers/rcu.c
> > @@ -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();
> > +}
> > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> > index 1eab7ebf25fd..0654008198b2 100644
> > --- a/rust/kernel/sync.rs
> > +++ b/rust/kernel/sync.rs
> > @@ -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};
> > diff --git a/rust/kernel/sync/rcu.rs b/rust/kernel/sync/rcu.rs
> > new file mode 100644
> > index 000000000000..b51d9150ffe2
> > --- /dev/null
> > +++ b/rust/kernel/sync/rcu.rs
> > @@ -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.
> > +        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.

I don't mind one or the other, feel free to send a patch to remove it. :)

> 
> 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.
> 
> > +}
> > +
> > +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`.

I picked up this patch from Wedson, so I can't tell for sure. I don't see
any other reason for this though, so we could remove it.

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

Re: [PATCH v7 08/16] rust: add devres abstraction

[Danilo Krummrich]

On Tue, Dec 24, 2024 at 09:53:23PM +0000, Gary Guo wrote:
> On Thu, 19 Dec 2024 18:04:10 +0100
> Danilo Krummrich <dakr@kernel.org> 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 <dakr@kernel.org>
> > ---
> >  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>
> >
> > +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.
> > +        //
> > +        // `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.

> 
> 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.
> 
> > +        //
> > +        // 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() };
> > +    }
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 6c836ab73771..2b61bf99d1ee 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -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)]
> 

Re: [PATCH v7 08/16] rust: add devres abstraction

[Danilo Krummrich]

On Thu, Jan 02, 2025 at 11:30:11AM +0100, Danilo Krummrich wrote:
> On Tue, Dec 24, 2024 at 09:53:23PM +0000, Gary Guo wrote:
> > On Thu, 19 Dec 2024 18:04:10 +0100
> > Danilo Krummrich <dakr@kernel.org> 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 <dakr@kernel.org>
> > > ---
> > >  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>
> > >
> > > +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.
> > > +        //
> > > +        // `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.

Ok, I double checked and this should indeed be the only issue.

I can reproduce the race and can confirm that a devm_remove_action_nowarn()
works, as long as we make it return the integer that indicates whether the
action has been removed already, such that we can handle it from the Rust side
accordingly.

Generally, I don't think it's a big deal, drivers usually hold the resources
they request anyways until remove(). But it's still an improvement, so I'll send
a patch for that.

> 
> > 
> > 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.
> > 
> > > +        //
> > > +        // 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() };
> > > +    }
> > > +}
> > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > index 6c836ab73771..2b61bf99d1ee 100644
> > > --- a/rust/kernel/lib.rs
> > > +++ b/rust/kernel/lib.rs
> > > @@ -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)]
> > 

Re: [PATCH v7 08/16] rust: add devres abstraction

[Danilo Krummrich]

On Tue, Dec 24, 2024 at 09:53:23PM +0000, Gary Guo wrote:
> On Thu, 19 Dec 2024 18:04:10 +0100
> Danilo Krummrich <dakr@kernel.org> 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 <dakr@kernel.org>
> > ---
> >  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>
> >
> > +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.
> > +        //
> > +        // `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.

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/20250107122609.8135-2-dakr@kernel.org/

> 
> > +        //
> > +        // 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() };
> > +    }
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 6c836ab73771..2b61bf99d1ee 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -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)]
> 

Re: [PATCH v7 08/16] rust: add devres abstraction

[Danilo Krummrich]

On Tue, Jan 14, 2025 at 05:33:45PM +0100, Danilo Krummrich wrote:
> > > +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.
> 
> 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.

Well, actually this can't happen, since obviously we know whether Devres:drop
removed it or whether it will be released by devres_release_all() and we only
free DevresInner conditionally.

So, the code in [1] is fine; I somehow managed to confuse myself.

Sorry for the noise.

- Danilo

> 
> 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/20250107122609.8135-2-dakr@kernel.org/

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Fiona Behrens]

On 19 Dec 2024, at 18:04, Danilo Krummrich wrote:

> 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 <aliceryhl@google.com>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Reviewed-by: Daniel Almeida  <daniel.almeida@collabora.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  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
>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 060750af6524..63f9b1da179f 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -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"
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> new file mode 100644
> index 000000000000..4c2401ccd720
> --- /dev/null
> +++ b/rust/helpers/io.c
> @@ -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
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> new file mode 100644
> index 000000000000..d4a73e52e3ee
> --- /dev/null
> +++ b/rust/kernel/io.rs
> @@ -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
> +        // 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
> +    }

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.

Thanks,
Fiona

> +
> +    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
> +    );
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 5702ce32ec8e..6c836ab73771 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -79,6 +79,7 @@
>
>  #[doc(hidden)]
>  pub use bindings;
> +pub mod io;
>  pub use macros;
>  pub use uapi;
>
> -- 
> 2.47.1

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Danilo Krummrich]

On 1/16/25 11:31 AM, Fiona Behrens wrote:
> On 19 Dec 2024, at 18:04, Danilo Krummrich wrote:

>> +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
>> +    }
> 
> 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.

Sure, feel free to move them. I think I should have moved those functions to
IoRaw from the get-go.

- Danilo

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Alistair Popple]

On Thu, Dec 19, 2024 at 06:04:09PM +0100, Danilo Krummrich wrote:
> 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.

[...]

> +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);

I'm rather new to Rust in the kernel but I've been playing
around with the nova-core stub (https://lore.kernel.org/
dri-devel/20250209173048.17398-1-dakr@kernel.org/) a bit and the first thing I
tried to do was add a new register access. Of course when doing that I forgot to
update the BAR size definition:

const BAR0_SIZE: usize = 8;
pub(crate) type Bar0 = pci::Bar<BAR0_SIZE>;

That lead to this rather cryptic build error message:

  RUSTC [M] drivers/gpu/nova-core/nova_core.o
drivers/gpu/nova-core/nova_core.o: warning: objtool: _RNvXNtCs3LxSlPxFOnk_9nova_core6driverNtB2_8NovaCoreNtNtCsgupvMsqqUAi_6kernel3pci6Driver5probe() falls through to next function _RNvXs_NtCs3LxSlPxFOnk_9nova_core3gpuNtB4_7ChipsetNtNtCs2wKxHFORdeQ_4core3fmt7Display3fmt()
  MODPOST Module.symvers
ERROR: modpost: "rust_build_error" [drivers/gpu/nova-core/nova_core.ko] undefined!

Building it into the kernel instead of as a module provides some more hints:

vmlinux.o: warning: objtool: _RNvXNtCs3LxSlPxFOnk_9nova_core6driverNtB2_8NovaCoreNtNtCsgupvMsqqUAi_6kernel3pci6Driver5probe() falls through to next function _RNvXs_NtCs3LxSlPxFOnk_9nova_core3gpuNtB4_7ChipsetNtNtCs2wKxHFORdeQ_4core3fmt7Display3fmt()
  OBJCOPY modules.builtin.modinfo
  GEN     modules.builtin
  MODPOST Module.symvers
  UPD     include/generated/utsversion.h
  CC      init/version-timestamp.o
  KSYMS   .tmp_vmlinux0.kallsyms.S
  AS      .tmp_vmlinux0.kallsyms.o
  LD      .tmp_vmlinux1
/usr/bin/ld: vmlinux.o: in function `<kernel::io::Io<4>>::io_addr_assert::<u32>':
/data/source/linux/rust/kernel/build_assert.rs:78: undefined reference to `rust_build_error'

That was just enough to let me figure out what I'd done wrong based on the small
change I'd made. However the lack of reference to the actual offending line of
code that triggered the assert still made it more difficult than needed.

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!

 - Alistair

> +
> +            // 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
> +    );
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 5702ce32ec8e..6c836ab73771 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -79,6 +79,7 @@
>  
>  #[doc(hidden)]
>  pub use bindings;
> +pub mod io;
>  pub use macros;
>  pub use uapi;
>  
> -- 
> 2.47.1
> 
> 

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Miguel Ojeda]

Hi Alistair,

On Fri, Feb 21, 2025 at 2:20 AM Alistair Popple <apopple@nvidia.com> wrote:
>
> 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

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Alistair Popple]

On Fri, Feb 21, 2025 at 04:58:59AM +0100, Miguel Ojeda wrote:
> Hi Alistair,
> 
> On Fri, Feb 21, 2025 at 2:20 AM Alistair Popple <apopple@nvidia.com> wrote:
> >
> > 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.

Are there any proposals or ideas for how we could do that?

> I hope that helps.

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?

Thanks.

 - Alistair

> Cheers,
> Miguel

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Danilo Krummrich]

On Tue, Feb 25, 2025 at 04:50:05PM +1100, Alistair Popple wrote:
> 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?

> 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.

> 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?

[1]

diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 1fb6e44f3395..2ff3af11d711 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -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 }>;

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Alistair Popple]

On Tue, Feb 25, 2025 at 12:04:35PM +0100, Danilo Krummrich wrote:
> On Tue, Feb 25, 2025 at 04:50:05PM +1100, Alistair Popple wrote:
> > 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.

Yeah, that is how I eventually figured it out as a result of trying to resolve
the "undefined symbol" build error.

> 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.

At the very least if we could produce something more informative in the output
than the objtool "falls through to next function" warning and the undefined
reference that would help. I'm not sure if there is something we could do in the
build system to detect that and print a build-time hint along the lines of "hey,
you probably hit a build assert".

> > 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.

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.

> > 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?

*smacks forehead*

Yes. So to answer this question:

> 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.

> [1]
> 
> diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> index 1fb6e44f3395..2ff3af11d711 100644
> --- a/samples/rust/rust_driver_pci.rs
> +++ b/samples/rust/rust_driver_pci.rs
> @@ -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 }>;

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Danilo Krummrich]

On Thu, Feb 27, 2025 at 11:25:55AM +1100, Alistair Popple wrote:
> On Tue, Feb 25, 2025 at 12:04:35PM +0100, Danilo Krummrich wrote:
> > 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.

> 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.

> > > 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.
> 
> 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.

> 
> 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.

> 
> 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.

> 
> > > 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?
> 
> *smacks forehead*
> 
> Yes. So to answer this question:
> 
> > 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.

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Miguel Ojeda]

On Thu, Feb 27, 2025 at 1:26 AM Alistair Popple <apopple@nvidia.com> wrote:
>
> 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.

It is not "broken" -- after all, it works as it was intended/designed
when it was introduced, though it is definitely a hack and thus indeed
the message could be improved greatly. :)

As for how to improve it, e.g. Gary suggested the other day to use the
DWARF information to locate the call site.

I guess another way would be to generate different symbol names per
call site, so that we can embed the path and line number into it (more
or less), so that the user at least has a hint, though that may have
disadvantages.

Cheers,
Miguel

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Alistair Popple]

On Thu, Feb 27, 2025 at 11:01:55AM +0100, Danilo Krummrich wrote:
> On Thu, Feb 27, 2025 at 11:25:55AM +1100, Alistair Popple wrote:
> > On Tue, Feb 25, 2025 at 12:04:35PM +0100, Danilo Krummrich wrote:
> > > 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.

Thanks. The limitation is what I was asking about. The work around for (2)
works, but is not terribly discoverable. I will see if I can come up with
something better to help with discoverability that at least.

> 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.
> 
> > 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.

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.

> > > > 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.
> > 
> > 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.

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.

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. So I wanted to highlight that the
build_assert as linker error is really confusing when coming from C, and that
it's an area that I think needs to improve to make Rust more successful in the
kernel. Sadly I don't have any immediately ideas but if I do I will post them.

> 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.
> 
> > 
> > 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.
>
> > 
> > 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.
>
> > 
> > > > 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?
> > 
> > *smacks forehead*
> > 
> > Yes. So to answer this question:
> > 
> > > 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.

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Danilo Krummrich]

On Fri, Feb 28, 2025 at 04:29:04PM +1100, Alistair Popple wrote:
> On Thu, Feb 27, 2025 at 11:01:55AM +0100, Danilo Krummrich wrote:
> > On Thu, Feb 27, 2025 at 11:25:55AM +1100, Alistair Popple wrote:
> 
> > > 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.
> 
> 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.

We handle this case too by giving out a Devres<pci::Bar> rather than just a
pci::Bar. The former gets revoked when the device falls off the bus.

Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

[Miguel Ojeda]

On Fri, Feb 28, 2025 at 6:29 AM Alistair Popple <apopple@nvidia.com> wrote:
>
> 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.

+1, it is indeed very, very important.

But, just to clarify, we have been caring about ergonomics and
reducing friction for kernel developers since the very beginning,
including asking upstream Rust for features and so on when applicable.

In general, it has been a factor in most topics we have discussed in
the team since 2020, not just for source code or debugging features,
but also docs, KUnit, and so on.

That is why we would like to improve it and why we have it in our
upstream Rust wishlist and so on. In other words, it is not that we do
not see the issue!

I hope that clarifies.

Cheers,
Miguel

Re: [PATCH v7 03/16] rust: implement `IdArray`, `IdTable` and `RawDeviceId`

[Tamir Duberstein]

On Thu, Dec 19, 2024 at 12:08 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> +/// 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;
> +
> +    /// 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?

Re: [PATCH v7 03/16] rust: implement `IdArray`, `IdTable` and `RawDeviceId`

[Danilo Krummrich]

On Sat, Mar 15, 2025 at 12:52:27PM -0400, Tamir Duberstein wrote:
> On Thu, Dec 19, 2024 at 12:08 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > +/// 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;
> > +
> > +    /// 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).

Re: [PATCH v7 03/16] rust: implement `IdArray`, `IdTable` and `RawDeviceId`

[Tamir Duberstein]

On Sat, Mar 15, 2025 at 12:59 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sat, Mar 15, 2025 at 12:52:27PM -0400, Tamir Duberstein wrote:
> > On Thu, Dec 19, 2024 at 12:08 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > +/// 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;
> > > +
> > > +    /// 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).

Thanks for explaining!