[PATCH v2 00/10] Device / Driver and PCI Rust abstractions

[PATCH v2 00/10] Device / Driver and PCI Rust abstractions

[Danilo Krummrich]

This patch series implements basic device / driver, PCI and devres Rust
abstractions.

This patch series is sent in the context of [1], and the corresponding patch
series [2], which contains some basic DRM Rust abstractions and a stub
implementation of the Nova GPU driver.

Nova is intended to be developed upstream, starting out with just a stub driver
to lift some initial required infrastructure upstream. A more detailed
explanation can be found in [1].

As mentioned above, a driver serving as example on how these abstractions are
used within a (DRM) driver can be found in [2].

Additionally, the device / driver bits can also be found in [3], all
abstractions required for Nova in [4] and Nova in [5].

This patch series is based on [6] (which has been merged in the driver-core tree
already), as well as two more patches:

- "rust: init: introduce Opaque::try_ffi_init" [7]
- "rust: introduce InPlaceModule" [8]

@Wedson, please let me know if you want to send the two separately or if you
want me to pick them up for this series.

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

[1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
[2] https://lore.kernel.org/dri-devel/20240618233324.14217-1-dakr@redhat.com/
[3] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device
[4] https://github.com/Rust-for-Linux/linux/tree/staging/dev
[5] https://gitlab.freedesktop.org/drm/nova/-/tree/nova-next
[6] https://lore.kernel.org/rust-for-linux/20240618154841.6716-1-dakr@redhat.com/
[7] https://github.com/Rust-for-Linux/linux/commit/9c49161db95f4eb4e55e62873b835fb6c1a0bb39
[8] https://github.com/Rust-for-Linux/linux/commit/e74d5d33dd2b9361e8cebae77227e3f924b50034

Danilo Krummrich (6):
  rust: pass module name to `Module::init`
  rust: implement generic driver registration
  rust: add `io::Io` base type
  rust: add devres abstraction
  rust: pci: add basic PCI device / driver abstractions
  rust: pci: implement I/O mappable `pci::Bar`

Wedson Almeida Filho (4):
  rust: implement `IdArray`, `IdTable` and `RawDeviceId`
  rust: add rcu abstraction
  rust: add `Revocable` type
  rust: add `dev_*` print macros.

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers.c                  | 145 ++++++++++
 rust/kernel/device.rs           | 319 +++++++++++++++++++++-
 rust/kernel/device_id.rs        | 336 +++++++++++++++++++++++
 rust/kernel/devres.rs           | 168 ++++++++++++
 rust/kernel/driver.rs           | 128 +++++++++
 rust/kernel/io.rs               | 219 +++++++++++++++
 rust/kernel/lib.rs              |  22 +-
 rust/kernel/net/phy.rs          |   2 +-
 rust/kernel/pci.rs              | 467 ++++++++++++++++++++++++++++++++
 rust/kernel/prelude.rs          |   2 +
 rust/kernel/revocable.rs        | 209 ++++++++++++++
 rust/kernel/sync.rs             |   1 +
 rust/kernel/sync/rcu.rs         |  52 ++++
 rust/macros/module.rs           |   3 +-
 samples/rust/rust_minimal.rs    |   2 +-
 samples/rust/rust_print.rs      |   2 +-
 17 files changed, 2069 insertions(+), 9 deletions(-)
 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/pci.rs
 create mode 100644 rust/kernel/revocable.rs
 create mode 100644 rust/kernel/sync/rcu.rs


base-commit: e74d5d33dd2b9361e8cebae77227e3f924b50034
-- 
2.45.1

[PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

In a subsequent patch we introduce the `Registration` abstraction used
to register driver structures. Some subsystems require the module name on
driver registration (e.g. PCI in __pci_register_driver()), hence pass
the module name to `Module::init`.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/lib.rs           | 14 ++++++++++----
 rust/kernel/net/phy.rs       |  2 +-
 rust/macros/module.rs        |  3 ++-
 samples/rust/rust_minimal.rs |  2 +-
 samples/rust/rust_print.rs   |  2 +-
 5 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index a791702b4fee..5af00e072a58 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
     /// should do.
     ///
     /// Equivalent to the `module_init` macro in the C API.
-    fn init(module: &'static ThisModule) -> error::Result<Self>;
+    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;
 }
 
 /// A module that is pinned and initialised in-place.
@@ -79,13 +79,19 @@ pub trait InPlaceModule: Sync + Send {
     /// Creates an initialiser for the module.
     ///
     /// It is called when the module is loaded.
-    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error>;
+    fn init(
+        name: &'static str::CStr,
+        module: &'static ThisModule,
+    ) -> impl init::PinInit<Self, error::Error>;
 }
 
 impl<T: Module> InPlaceModule for T {
-    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
+    fn init(
+        name: &'static str::CStr,
+        module: &'static ThisModule,
+    ) -> impl init::PinInit<Self, error::Error> {
         let initer = move |slot: *mut Self| {
-            let m = <Self as Module>::init(module)?;
+            let m = <Self as Module>::init(name, module)?;
 
             // SAFETY: `slot` is valid for write per the contract with `pin_init_from_closure`.
             unsafe { slot.write(m) };
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index fd40b703d224..ccb2552dc107 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -887,7 +887,7 @@ struct Module {
                 [$($crate::net::phy::create_phy_driver::<$driver>()),+];
 
             impl $crate::Module for Module {
-                fn init(module: &'static ThisModule) -> Result<Self> {
+                fn init(_name: &'static CStr, module: &'static ThisModule) -> Result<Self> {
                     // SAFETY: The anonymous constant guarantees that nobody else can access
                     // the `DRIVERS` static. The array is used only in the C side.
                     let drivers = unsafe { &mut DRIVERS };
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 105be4797f85..be03b2cf77a1 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -302,7 +302,8 @@ mod __module_init {{
                     ///
                     /// This function must only be called once.
                     unsafe fn __init() -> core::ffi::c_int {{
-                        let initer = <{type_} as kernel::InPlaceModule>::init(&super::super::THIS_MODULE);
+                        let initer = <{type_} as kernel::InPlaceModule>::init(kernel::c_str!(\"{name}\"),
+                                                                              &super::super::THIS_MODULE);
                         // SAFETY: No data race, since `__MOD` can only be accessed by this module
                         // and there only `__init` and `__exit` access it. These functions are only
                         // called once and `__exit` cannot be called before or during `__init`.
diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs
index 2a9eaab62d1c..3b918ff5eebb 100644
--- a/samples/rust/rust_minimal.rs
+++ b/samples/rust/rust_minimal.rs
@@ -17,7 +17,7 @@ struct RustMinimal {
 }
 
 impl kernel::Module for RustMinimal {
-    fn init(_module: &'static ThisModule) -> Result<Self> {
+    fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
         pr_info!("Rust minimal sample (init)\n");
         pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
 
diff --git a/samples/rust/rust_print.rs b/samples/rust/rust_print.rs
index 6eabb0d79ea3..722275a735f1 100644
--- a/samples/rust/rust_print.rs
+++ b/samples/rust/rust_print.rs
@@ -40,7 +40,7 @@ fn arc_print() -> Result {
 }
 
 impl kernel::Module for RustPrint {
-    fn init(_module: &'static ThisModule) -> Result<Self> {
+    fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
         pr_info!("Rust printing macros sample (init)\n");
 
         pr_emerg!("Emergency message (level 0) without args\n");
-- 
2.45.1

[PATCH v2 02/10] rust: implement generic driver registration

[Danilo Krummrich]

Implement the generic `Registration` type and the `DriverOps` 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 `DriverOps` trait is provided to subsystems, which have to
implement `DriverOps::register` and `DrvierOps::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
`DriverOps::register` and pci_unregister_driver() from
`DrvierOps::unregister`.

This patch is based on previous work from Wedson Almeida Filho.

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

diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
new file mode 100644
index 000000000000..e04406b93b56
--- /dev/null
+++ b/rust/kernel/driver.rs
@@ -0,0 +1,128 @@
+// 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 [`DriverOps`], 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 [`DriverOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, Amba,
+/// etc.) to privide 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 `DriverOps::register` and
+/// `bindings::pci_unregister_driver` from `DriverOps::unregister`.
+pub trait DriverOps {
+    /// 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.
+    ///
+    /// # Safety
+    ///
+    /// `reg` must point to valid, initialised, and writable memory. It may be modified by this
+    /// function to hold registration state.
+    ///
+    /// On success, `reg` must remain pinned and valid until the matching call to
+    /// [`DriverOps::unregister`].
+    fn register(
+        reg: &mut Self::RegType,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result;
+
+    /// Unregisters a driver previously registered with [`DriverOps::register`].
+    ///
+    /// # Safety
+    ///
+    /// `reg` must point to valid writable memory, initialised by a previous successful call to
+    /// [`DriverOps::register`].
+    fn unregister(reg: &mut Self::RegType);
+}
+
+/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
+/// `bindings::pci_driver`). Therefore a [`Registration`] is initialized with some type that
+/// implements the [`DriverOps`] 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: DriverOps> {
+    #[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: DriverOps> 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: DriverOps> Send for Registration<T> {}
+
+impl<T: DriverOps> 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 { &mut *ptr };
+
+                T::register(drv, name, module)
+            }),
+        })
+    }
+}
+
+#[pinned_drop]
+impl<T: DriverOps> PinnedDrop for Registration<T> {
+    fn drop(self: Pin<&mut Self>) {
+        let drv = unsafe { &mut *self.reg.get() };
+
+        T::unregister(drv);
+    }
+}
+
+/// A kernel module that only registers the given driver on init.
+///
+/// This is a helper struct to make it easier to define single-functionality modules, in this case,
+/// modules that offer a single driver.
+#[pin_data]
+pub struct Module<T: DriverOps> {
+    #[pin]
+    _driver: Registration<T>,
+}
+
+impl<T: DriverOps + Sync + Send> crate::InPlaceModule for Module<T> {
+    fn init(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {
+            _driver <- Registration::<T>::new(name, module),
+        })
+    }
+}
+
+/// 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;
+        type ModuleType = $crate::driver::Module<Ops<$type>>;
+        $crate::prelude::module! {
+            type: ModuleType,
+            $($f)*
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 5af00e072a58..5382402cd3db 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -29,6 +29,7 @@
 pub mod alloc;
 mod build_assert;
 pub mod device;
+pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
 pub mod firmware;
-- 
2.45.1

[PATCH v2 03/10] rust: implement `IdArray`, `IdTable` and `RawDeviceId`

[Danilo Krummrich]

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

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: Asahi Lina <lina@asahilina.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Co-developed-by: Danilo Krummrich <dakr@redhat.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/device_id.rs | 336 +++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs       |   2 +
 2 files changed, 338 insertions(+)
 create mode 100644 rust/kernel/device_id.rs

diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
new file mode 100644
index 000000000000..c490300f29bb
--- /dev/null
+++ b/rust/kernel/device_id.rs
@@ -0,0 +1,336 @@
+// 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::marker::PhantomData;
+
+/// Conversion from a device id to a raw device id.
+///
+/// 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.
+///
+/// Originally, RawDeviceId was implemented as a const trait. However, this unstable feature is
+/// broken/gone in 1.73. To work around this, turn IdArray::new() into a macro such that it can use
+/// concrete types (which can still have const associated functions) instead of a trait.
+///
+/// # Safety
+///
+/// Implementers must ensure that:
+///   - [`RawDeviceId::ZERO`] is actually a zeroed-out version of the raw device id.
+///   - `to_rawid` is implemented and stores `offset` in the context/data field of the raw device
+///     id so that buses can recover the pointer to the data. (This should actually be a trait
+///     function, however, this requires `const_trait_impl`, and hence has to changed once the
+///     feature is stabilized.)
+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;
+
+    /// A zeroed-out representation of the raw device id.
+    ///
+    /// Id tables created from [`Self`] use [`Self::ZERO`] as the sentinel to indicate the end of
+    /// the table.
+    const ZERO: Self::RawType;
+}
+
+/// A zero-terminated device id array, followed by context data.
+#[repr(C)]
+pub struct IdArray<T: RawDeviceId, U, const N: usize> {
+    ids: [T::RawType; N],
+    sentinel: T::RawType,
+    id_infos: [Option<U>; N],
+}
+
+impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
+    const U_NONE: Option<U> = None;
+
+    /// Returns an `IdTable` backed by `self`.
+    ///
+    /// This is used to essentially erase the array size.
+    pub const fn as_table(&self) -> IdTable<'_, T, U> {
+        IdTable {
+            first: &self.ids[0],
+            _p: PhantomData,
+        }
+    }
+
+    /// Creates a new instance of the array.
+    ///
+    /// The contents are derived from the given identifiers and context information.
+    #[doc(hidden)]
+    pub const unsafe fn new(raw_ids: [T::RawType; N], infos: [Option<U>; N]) -> Self
+    where
+        T: RawDeviceId + Copy,
+        T::RawType: Copy + Clone,
+    {
+        Self {
+            ids: raw_ids,
+            sentinel: T::ZERO,
+            id_infos: infos,
+        }
+    }
+
+    #[doc(hidden)]
+    pub const fn get_offset(idx: usize) -> isize
+    where
+        T: RawDeviceId + Copy,
+        T::RawType: Copy + Clone,
+    {
+        // SAFETY: We are only using this dummy value to get offsets.
+        let array = unsafe { Self::new([T::ZERO; N], [Self::U_NONE; N]) };
+        // SAFETY: Both pointers are within `array` (or one byte beyond), consequently they are
+        // derived from the same allocated object. We are using a `u8` pointer, whose size 1,
+        // so the pointers are necessarily 1-byte aligned.
+        let ret = unsafe {
+            (&array.id_infos[idx] as *const _ as *const u8)
+                .offset_from(&array.ids[idx] as *const _ as _)
+        };
+        core::mem::forget(array);
+        ret
+    }
+}
+
+// Creates a new ID array. This is a macro so it can take the concrete ID type as a parameter in
+// order to call to_rawid() on it, and still remain const. This is necessary until a new
+// const_trait_impl implementation lands, since the existing implementation was removed in Rust
+// 1.73.
+#[macro_export]
+#[doc(hidden)]
+macro_rules! _new_id_array {
+    (($($args:tt)*), $id_type:ty) => {{
+        /// Creates a new instance of the array.
+        ///
+        /// The contents are derived from the given identifiers and context information.
+        const fn new< U, const N: usize>(ids: [$id_type; N], infos: [Option<U>; N])
+            -> $crate::device_id::IdArray<$id_type, U, N>
+        where
+            $id_type: $crate::device_id::RawDeviceId + Copy,
+            <$id_type as $crate::device_id::RawDeviceId>::RawType: Copy + Clone,
+        {
+            let mut raw_ids =
+                [<$id_type as $crate::device_id::RawDeviceId>::ZERO; N];
+            let mut i = 0usize;
+            while i < N {
+                let offset: isize = $crate::device_id::IdArray::<$id_type, U, N>::get_offset(i);
+                raw_ids[i] = ids[i].to_rawid(offset);
+                i += 1;
+            }
+
+            // SAFETY: We are passing valid arguments computed with the correct offsets.
+            unsafe {
+                $crate::device_id::IdArray::<$id_type, U, N>::new(raw_ids, infos)
+            }
+       }
+
+        new($($args)*)
+    }}
+}
+
+/// A device id table.
+///
+/// The table is guaranteed to be zero-terminated and to be followed by an array of context data of
+/// type `Option<U>`.
+pub struct IdTable<'a, T: RawDeviceId, U> {
+    first: &'a T::RawType,
+    _p: PhantomData<&'a U>,
+}
+
+impl<T: RawDeviceId, U> AsRef<T::RawType> for IdTable<'_, T, U> {
+    fn as_ref(&self) -> &T::RawType {
+        self.first
+    }
+}
+
+/// Counts the number of parenthesis-delimited, comma-separated items.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::count_paren_items;
+///
+/// assert_eq!(0, count_paren_items!());
+/// assert_eq!(1, count_paren_items!((A)));
+/// assert_eq!(1, count_paren_items!((A),));
+/// assert_eq!(2, count_paren_items!((A), (B)));
+/// assert_eq!(2, count_paren_items!((A), (B),));
+/// assert_eq!(3, count_paren_items!((A), (B), (C)));
+/// assert_eq!(3, count_paren_items!((A), (B), (C),));
+/// ```
+#[macro_export]
+macro_rules! count_paren_items {
+    (($($item:tt)*), $($remaining:tt)*) => { 1 + $crate::count_paren_items!($($remaining)*) };
+    (($($item:tt)*)) => { 1 };
+    () => { 0 };
+}
+
+/// Converts a comma-separated list of pairs into an array with the first element. That is, it
+/// discards the second element of the pair.
+///
+/// Additionally, it automatically introduces a type if the first element is warpped in curly
+/// braces, for example, if it's `{v: 10}`, it becomes `X { v: 10 }`; this is to avoid repeating
+/// the type.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::first_item;
+///
+/// #[derive(PartialEq, Debug)]
+/// struct X {
+///     v: u32,
+/// }
+///
+/// assert_eq!([] as [X; 0], first_item!(X, ));
+/// assert_eq!([X { v: 10 }], first_item!(X, ({ v: 10 }, Y)));
+/// assert_eq!([X { v: 10 }], first_item!(X, ({ v: 10 }, Y),));
+/// assert_eq!([X { v: 10 }], first_item!(X, (X { v: 10 }, Y)));
+/// assert_eq!([X { v: 10 }], first_item!(X, (X { v: 10 }, Y),));
+/// assert_eq!([X { v: 10 }, X { v: 20 }], first_item!(X, ({ v: 10 }, Y), ({ v: 20 }, Y)));
+/// assert_eq!([X { v: 10 }, X { v: 20 }], first_item!(X, ({ v: 10 }, Y), ({ v: 20 }, Y),));
+/// assert_eq!([X { v: 10 }, X { v: 20 }], first_item!(X, (X { v: 10 }, Y), (X { v: 20 }, Y)));
+/// assert_eq!([X { v: 10 }, X { v: 20 }], first_item!(X, (X { v: 10 }, Y), (X { v: 20 }, Y),));
+/// assert_eq!([X { v: 10 }, X { v: 20 }, X { v: 30 }],
+///            first_item!(X, ({ v: 10 }, Y), ({ v: 20 }, Y), ({v: 30}, Y)));
+/// assert_eq!([X { v: 10 }, X { v: 20 }, X { v: 30 }],
+///            first_item!(X, ({ v: 10 }, Y), ({ v: 20 }, Y), ({v: 30}, Y),));
+/// assert_eq!([X { v: 10 }, X { v: 20 }, X { v: 30 }],
+///            first_item!(X, (X { v: 10 }, Y), (X { v: 20 }, Y), (X {v: 30}, Y)));
+/// assert_eq!([X { v: 10 }, X { v: 20 }, X { v: 30 }],
+///            first_item!(X, (X { v: 10 }, Y), (X { v: 20 }, Y), (X {v: 30}, Y),));
+/// ```
+#[macro_export]
+macro_rules! first_item {
+    ($id_type:ty, $(({$($first:tt)*}, $second:expr)),* $(,)?) => {
+        {
+            type IdType = $id_type;
+            [$(IdType{$($first)*},)*]
+        }
+    };
+    ($id_type:ty, $(($first:expr, $second:expr)),* $(,)?) => { [$($first,)*] };
+}
+
+/// Converts a comma-separated list of pairs into an array with the second element. That is, it
+/// discards the first element of the pair.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::second_item;
+///
+/// assert_eq!([] as [u32; 0], second_item!());
+/// assert_eq!([10u32], second_item!((X, 10u32)));
+/// assert_eq!([10u32], second_item!((X, 10u32),));
+/// assert_eq!([10u32], second_item!(({ X }, 10u32)));
+/// assert_eq!([10u32], second_item!(({ X }, 10u32),));
+/// assert_eq!([10u32, 20], second_item!((X, 10u32), (X, 20)));
+/// assert_eq!([10u32, 20], second_item!((X, 10u32), (X, 20),));
+/// assert_eq!([10u32, 20], second_item!(({ X }, 10u32), ({ X }, 20)));
+/// assert_eq!([10u32, 20], second_item!(({ X }, 10u32), ({ X }, 20),));
+/// assert_eq!([10u32, 20, 30], second_item!((X, 10u32), (X, 20), (X, 30)));
+/// assert_eq!([10u32, 20, 30], second_item!((X, 10u32), (X, 20), (X, 30),));
+/// assert_eq!([10u32, 20, 30], second_item!(({ X }, 10u32), ({ X }, 20), ({ X }, 30)));
+/// assert_eq!([10u32, 20, 30], second_item!(({ X }, 10u32), ({ X }, 20), ({ X }, 30),));
+/// ```
+#[macro_export]
+macro_rules! second_item {
+    ($(({$($first:tt)*}, $second:expr)),* $(,)?) => { [$($second,)*] };
+    ($(($first:expr, $second:expr)),* $(,)?) => { [$($second,)*] };
+}
+
+/// Defines a new constant [`IdArray`] with a concise syntax.
+///
+/// It is meant to be used by buses and subsystems to create a similar macro with their device id
+/// type already specified, i.e., with fewer parameters to the end user.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::{define_id_array, device_id::RawDeviceId};
+///
+/// #[derive(Copy, Clone)]
+/// struct Id(u32);
+///
+/// // SAFETY: `ZERO` is all zeroes and `to_rawid` stores `offset` as the second element of the raw
+/// // device id pair.
+/// unsafe impl RawDeviceId for Id {
+///     type RawType = (u64, isize);
+///     const ZERO: Self::RawType = (0, 0);
+/// }
+///
+/// impl Id {
+///     #[allow(clippy::wrong_self_convention)]
+///     const fn to_rawid(&self, offset: isize) -> <Id as RawDeviceId>::RawType {
+///         (self.0 as u64 + 1, offset)
+///     }
+/// }
+///
+/// define_id_array!(A1, Id, (), []);
+/// define_id_array!(A2, Id, &'static [u8], [(Id(10), None)]);
+/// define_id_array!(A3, Id, &'static [u8], [(Id(10), Some(b"id1")), ]);
+/// define_id_array!(A4, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), Some(b"id2"))]);
+/// define_id_array!(A5, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), Some(b"id2")), ]);
+/// define_id_array!(A6, Id, &'static [u8], [(Id(10), None), (Id(20), Some(b"id2")), ]);
+/// define_id_array!(A7, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), None), ]);
+/// define_id_array!(A8, Id, &'static [u8], [(Id(10), None), (Id(20), None), ]);
+/// ```
+#[macro_export]
+macro_rules! define_id_array {
+    ($table_name:ident, $id_type:ty, $data_type:ty, [ $($t:tt)* ]) => {
+        const $table_name: $crate::device_id::IdArray<$id_type,
+                                                      $data_type, {
+                                                          $crate::count_paren_items!($($t)*)
+                                                      }> = $crate::_new_id_array!(
+                                                          ($crate::first_item!($id_type, $($t)*),
+                                                           $crate::second_item!($($t)*)),
+                                                          $id_type);
+    };
+}
+
+/// Defines a new constant [`IdTable`] with a concise syntax.
+///
+/// It is meant to be used by buses and subsystems to create a similar macro with their device id
+/// type already specified, i.e., with fewer parameters to the end user.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::{define_id_table, device_id::RawDeviceId};
+///
+/// #[derive(Copy, Clone)]
+/// struct Id(u32);
+///
+/// // SAFETY: `ZERO` is all zeroes and `to_rawid` stores `offset` as the second element of the raw
+/// // device id pair.
+/// unsafe impl RawDeviceId for Id {
+///     type RawType = (u64, isize);
+///     const ZERO: Self::RawType = (0, 0);
+/// }
+///
+/// impl Id {
+///     #[allow(clippy::wrong_self_convention)]
+///     const fn to_rawid(&self, offset: isize) -> <Id as RawDeviceId>::RawType {
+///         (self.0 as u64 + 1, offset)
+///     }
+/// }
+///
+/// define_id_table!(T1, Id, &'static [u8], [(Id(10), None)]);
+/// define_id_table!(T2, Id, &'static [u8], [(Id(10), Some(b"id1")), ]);
+/// define_id_table!(T3, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), Some(b"id2"))]);
+/// define_id_table!(T4, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), Some(b"id2")), ]);
+/// define_id_table!(T5, Id, &'static [u8], [(Id(10), None), (Id(20), Some(b"id2")), ]);
+/// define_id_table!(T6, Id, &'static [u8], [(Id(10), Some(b"id1")), (Id(20), None), ]);
+/// define_id_table!(T7, Id, &'static [u8], [(Id(10), None), (Id(20), None), ]);
+/// ```
+#[macro_export]
+macro_rules! define_id_table {
+    ($table_name:ident, $id_type:ty, $data_type:ty, [ $($t:tt)* ]) => {
+        const $table_name: Option<$crate::device_id::IdTable<'static, $id_type, $data_type>> = {
+            $crate::define_id_array!(ARRAY, $id_type, $data_type, [ $($t)* ]);
+            Some(ARRAY.as_table())
+        };
+    };
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 5382402cd3db..98e1a1425d17 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -13,6 +13,7 @@
 
 #![no_std]
 #![feature(coerce_unsized)]
+#![feature(const_refs_to_cell)]
 #![feature(dispatch_from_dyn)]
 #![feature(new_uninit)]
 #![feature(receiver_trait)]
@@ -29,6 +30,7 @@
 pub mod alloc;
 mod build_assert;
 pub mod device;
+pub mod device_id;
 pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
-- 
2.45.1

[PATCH v2 04/10] 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.

Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/helpers.c          | 15 ++++++++++++
 rust/kernel/sync.rs     |  1 +
 rust/kernel/sync/rcu.rs | 52 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+)
 create mode 100644 rust/kernel/sync/rcu.rs

diff --git a/rust/helpers.c b/rust/helpers.c
index 0e02b2c64c72..0ce40ccb978b 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -27,6 +27,7 @@
 #include <linux/err.h>
 #include <linux/errname.h>
 #include <linux/mutex.h>
+#include <linux/rcupdate.h>
 #include <linux/refcount.h>
 #include <linux/sched/signal.h>
 #include <linux/slab.h>
@@ -166,6 +167,20 @@ rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
 }
 EXPORT_SYMBOL_GPL(rust_helper_krealloc);
 
+/* rcu */
+void rust_helper_rcu_read_lock(void)
+{
+	rcu_read_lock();
+}
+EXPORT_SYMBOL_GPL(rust_helper_rcu_read_lock);
+
+void rust_helper_rcu_read_unlock(void)
+{
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(rust_helper_rcu_read_unlock);
+/* end rcu */
+
 /*
  * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
  * use it in contexts where Rust expects a `usize` like slice (array) indices.
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 0ab20975a3b5..1806767359fe 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -11,6 +11,7 @@
 mod condvar;
 pub mod lock;
 mod locked_by;
+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..5a35495f69a4
--- /dev/null
+++ b/rust/kernel/sync/rcu.rs
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! RCU support.
+//!
+//! C header: [`include/linux/rcupdate.h`](srctree/include/linux/rcupdate.h)
+
+use crate::bindings;
+use core::marker::PhantomData;
+
+/// 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 {
+    _not_send: PhantomData<*mut ()>,
+}
+
+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 {
+            _not_send: PhantomData,
+        }
+    }
+
+    /// 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.45.1

[PATCH v2 05/10] 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>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/lib.rs       |   1 +
 rust/kernel/revocable.rs | 209 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 210 insertions(+)
 create mode 100644 rust/kernel/revocable.rs

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 98e1a1425d17..601c3d3c9d54 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -43,6 +43,7 @@
 pub mod net;
 pub mod prelude;
 pub mod print;
+pub mod revocable;
 mod static_assert;
 #[doc(hidden)]
 pub mod std_vendor;
diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
new file mode 100644
index 000000000000..3d13e7b2f2e8
--- /dev/null
+++ b/rust/kernel/revocable.rs
@@ -0,0 +1,209 @@
+// 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,
+    init::{self},
+    prelude::*,
+    sync::rcu,
+};
+use core::{
+    cell::UnsafeCell,
+    marker::PhantomData,
+    mem::MaybeUninit,
+    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 = Box::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 = Box::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: MaybeUninit<UnsafeCell<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 <- unsafe {
+                init::pin_init_from_closure(move |slot: *mut MaybeUninit<UnsafeCell<T>>| {
+                    init::PinInit::<T, core::convert::Infallible>::__pinned_init(data,
+                                                                                 slot as *mut T)?;
+                    Ok::<(), core::convert::Infallible>(())
+                })
+            },
+        })
+    }
+
+    /// 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) {
+            // 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 { RevocableGuard::new(self.data.assume_init_ref().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.assume_init_ref().get() })
+        } else {
+            None
+        }
+    }
+
+    /// 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) {
+        if self
+            .is_available
+            .compare_exchange(true, false, Ordering::Relaxed, Ordering::Relaxed)
+            .is_ok()
+        {
+            // 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.assume_init_ref().get()) };
+        }
+    }
+}
+
+#[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.assume_init_ref().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.45.1

[PATCH v2 06/10] rust: add `dev_*` print macros.

[Danilo Krummrich]

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

Implement `dev_*` print macros for `device::Device`.

They behave like the macros with the same names in C, i.e., they print
messages to the kernel ring buffer with the given level, prefixing the
messages with corresponding device information.

Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/device.rs  | 319 ++++++++++++++++++++++++++++++++++++++++-
 rust/kernel/prelude.rs |   2 +
 2 files changed, 320 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index e445e87fb7d7..058767339a64 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -8,7 +8,10 @@
     bindings,
     types::{ARef, Opaque},
 };
-use core::ptr;
+use core::{fmt, ptr};
+
+#[cfg(CONFIG_PRINTK)]
+use crate::c_str;
 
 /// A reference-counted device.
 ///
@@ -79,6 +82,110 @@ pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self {
         // SAFETY: Guaranteed by the safety requirements of the function.
         unsafe { &*ptr.cast() }
     }
+
+    /// Prints an emergency-level message (level 0) prefixed with device information.
+    ///
+    /// More details are available from [`dev_emerg`].
+    ///
+    /// [`dev_emerg`]: crate::dev_emerg
+    pub fn pr_emerg(&self, args: fmt::Arguments<'_>) {
+        // SAFETY: `klevel` is null-terminated, uses one of the kernel constants.
+        unsafe { self.printk(bindings::KERN_EMERG, args) };
+    }
+
+    /// Prints an alert-level message (level 1) prefixed with device information.
+    ///
+    /// More details are available from [`dev_alert`].
+    ///
+    /// [`dev_alert`]: crate::dev_alert
+    pub fn pr_alert(&self, args: fmt::Arguments<'_>) {
+        // SAFETY: `klevel` is null-terminated, uses one of the kernel constants.
+        unsafe { self.printk(bindings::KERN_ALERT, args) };
+    }
+
+    /// Prints a critical-level message (level 2) prefixed with device information.
+    ///
+    /// More details are available from [`dev_crit`].
+    ///
+    /// [`dev_crit`]: crate::dev_crit
+    pub fn pr_crit(&self, args: fmt::Arguments<'_>) {
+        // SAFETY: `klevel` is null-terminated, uses one of the kernel constants.
+        unsafe { self.printk(bindings::KERN_CRIT, args) };
+    }
+
+    /// Prints an error-level message (level 3) prefixed with device information.
+    ///
+    /// More details are available from [`dev_err`].
+    ///
+    /// [`dev_err`]: crate::dev_err
+    pub fn pr_err(&self, args: fmt::Arguments<'_>) {
+        // SAFETY: `klevel` is null-terminated, uses one of the kernel constants.
+        unsafe { self.printk(bindings::KERN_ERR, args) };
+    }
+
+    /// Prints a warning-level message (level 4) prefixed with device information.
+    ///
+    /// More details are available from [`dev_warn`].
+    ///
+    /// [`dev_warn`]: crate::dev_warn
+    pub fn pr_warn(&self, args: fmt::Arguments<'_>) {
+        // SAFETY: `klevel` is null-terminated, uses one of the kernel constants.
+        unsafe { self.printk(bindings::KERN_WARNING, args) };
+    }
+
+    /// Prints a notice-level message (level 5) prefixed with device information.
+    ///
+    /// More details are available from [`dev_notice`].
+    ///
+    /// [`dev_notice`]: crate::dev_notice
+    pub fn pr_notice(&self, args: fmt::Arguments<'_>) {
+        // SAFETY: `klevel` is null-terminated, uses one of the kernel constants.
+        unsafe { self.printk(bindings::KERN_NOTICE, args) };
+    }
+
+    /// Prints an info-level message (level 6) prefixed with device information.
+    ///
+    /// More details are available from [`dev_info`].
+    ///
+    /// [`dev_info`]: crate::dev_info
+    pub fn pr_info(&self, args: fmt::Arguments<'_>) {
+        // SAFETY: `klevel` is null-terminated, uses one of the kernel constants.
+        unsafe { self.printk(bindings::KERN_INFO, args) };
+    }
+
+    /// Prints a debug-level message (level 7) prefixed with device information.
+    ///
+    /// More details are available from [`dev_dbg`].
+    ///
+    /// [`dev_dbg`]: crate::dev_dbg
+    pub fn pr_dbg(&self, args: fmt::Arguments<'_>) {
+        if cfg!(debug_assertions) {
+            // SAFETY: `klevel` is null-terminated, uses one of the kernel constants.
+            unsafe { self.printk(bindings::KERN_DEBUG, args) };
+        }
+    }
+
+    /// Prints the provided message to the console.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `klevel` is null-terminated; in particular, one of the
+    /// `KERN_*`constants, for example, `KERN_CRIT`, `KERN_ALERT`, etc.
+    #[cfg_attr(not(CONFIG_PRINTK), allow(unused_variables))]
+    unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
+        // SAFETY: `klevel` is null-terminated and one of the kernel constants. `self.as_raw`
+        // is valid because `self` is valid. The "%pA" format string expects a pointer to
+        // `fmt::Arguments`, which is what we're passing as the last argument.
+        #[cfg(CONFIG_PRINTK)]
+        unsafe {
+            bindings::_dev_printk(
+                klevel as *const _ as *const core::ffi::c_char,
+                self.as_raw(),
+                c_str!("%pA").as_char_ptr(),
+                &msg as *const _ as *const core::ffi::c_void,
+            )
+        };
+    }
 }
 
 // SAFETY: Instances of `Device` are always reference-counted.
@@ -100,3 +207,213 @@ unsafe impl Send for Device {}
 // SAFETY: `Device` can be shared among threads because all immutable methods are protected by the
 // synchronization in `struct device`.
 unsafe impl Sync for Device {}
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! dev_printk {
+    ($method:ident, $dev:expr, $($f:tt)*) => {
+        {
+            ($dev).$method(core::format_args!($($f)*));
+        }
+    }
+}
+
+/// Prints an emergency-level message (level 0) prefixed with device information.
+///
+/// This level should be used if the system is unusable.
+///
+/// Equivalent to the kernel's `dev_emerg` macro.
+///
+/// Mimics the interface of [`std::print!`]. More information about the syntax is available from
+/// [`core::fmt`] and [`alloc::format!`].
+///
+/// [`std::print!`]: https://doc.rust-lang.org/std/macro.print.html
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::device::Device;
+///
+/// fn example(dev: &Device) {
+///     dev_emerg!(dev, "hello {}\n", "there");
+/// }
+/// ```
+#[macro_export]
+macro_rules! dev_emerg {
+    ($($f:tt)*) => { $crate::dev_printk!(pr_emerg, $($f)*); }
+}
+
+/// Prints an alert-level message (level 1) prefixed with device information.
+///
+/// This level should be used if action must be taken immediately.
+///
+/// Equivalent to the kernel's `dev_alert` macro.
+///
+/// Mimics the interface of [`std::print!`]. More information about the syntax is available from
+/// [`core::fmt`] and [`alloc::format!`].
+///
+/// [`std::print!`]: https://doc.rust-lang.org/std/macro.print.html
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::device::Device;
+///
+/// fn example(dev: &Device) {
+///     dev_alert!(dev, "hello {}\n", "there");
+/// }
+/// ```
+#[macro_export]
+macro_rules! dev_alert {
+    ($($f:tt)*) => { $crate::dev_printk!(pr_alert, $($f)*); }
+}
+
+/// Prints a critical-level message (level 2) prefixed with device information.
+///
+/// This level should be used in critical conditions.
+///
+/// Equivalent to the kernel's `dev_crit` macro.
+///
+/// Mimics the interface of [`std::print!`]. More information about the syntax is available from
+/// [`core::fmt`] and [`alloc::format!`].
+///
+/// [`std::print!`]: https://doc.rust-lang.org/std/macro.print.html
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::device::Device;
+///
+/// fn example(dev: &Device) {
+///     dev_crit!(dev, "hello {}\n", "there");
+/// }
+/// ```
+#[macro_export]
+macro_rules! dev_crit {
+    ($($f:tt)*) => { $crate::dev_printk!(pr_crit, $($f)*); }
+}
+
+/// Prints an error-level message (level 3) prefixed with device information.
+///
+/// This level should be used in error conditions.
+///
+/// Equivalent to the kernel's `dev_err` macro.
+///
+/// Mimics the interface of [`std::print!`]. More information about the syntax is available from
+/// [`core::fmt`] and [`alloc::format!`].
+///
+/// [`std::print!`]: https://doc.rust-lang.org/std/macro.print.html
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::device::Device;
+///
+/// fn example(dev: &Device) {
+///     dev_err!(dev, "hello {}\n", "there");
+/// }
+/// ```
+#[macro_export]
+macro_rules! dev_err {
+    ($($f:tt)*) => { $crate::dev_printk!(pr_err, $($f)*); }
+}
+
+/// Prints a warning-level message (level 4) prefixed with device information.
+///
+/// This level should be used in warning conditions.
+///
+/// Equivalent to the kernel's `dev_warn` macro.
+///
+/// Mimics the interface of [`std::print!`]. More information about the syntax is available from
+/// [`core::fmt`] and [`alloc::format!`].
+///
+/// [`std::print!`]: https://doc.rust-lang.org/std/macro.print.html
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::device::Device;
+///
+/// fn example(dev: &Device) {
+///     dev_warn!(dev, "hello {}\n", "there");
+/// }
+/// ```
+#[macro_export]
+macro_rules! dev_warn {
+    ($($f:tt)*) => { $crate::dev_printk!(pr_warn, $($f)*); }
+}
+
+/// Prints a notice-level message (level 5) prefixed with device information.
+///
+/// This level should be used in normal but significant conditions.
+///
+/// Equivalent to the kernel's `dev_notice` macro.
+///
+/// Mimics the interface of [`std::print!`]. More information about the syntax is available from
+/// [`core::fmt`] and [`alloc::format!`].
+///
+/// [`std::print!`]: https://doc.rust-lang.org/std/macro.print.html
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::device::Device;
+///
+/// fn example(dev: &Device) {
+///     dev_notice!(dev, "hello {}\n", "there");
+/// }
+/// ```
+#[macro_export]
+macro_rules! dev_notice {
+    ($($f:tt)*) => { $crate::dev_printk!(pr_notice, $($f)*); }
+}
+
+/// Prints an info-level message (level 6) prefixed with device information.
+///
+/// This level should be used for informational messages.
+///
+/// Equivalent to the kernel's `dev_info` macro.
+///
+/// Mimics the interface of [`std::print!`]. More information about the syntax is available from
+/// [`core::fmt`] and [`alloc::format!`].
+///
+/// [`std::print!`]: https://doc.rust-lang.org/std/macro.print.html
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::device::Device;
+///
+/// fn example(dev: &Device) {
+///     dev_info!(dev, "hello {}\n", "there");
+/// }
+/// ```
+#[macro_export]
+macro_rules! dev_info {
+    ($($f:tt)*) => { $crate::dev_printk!(pr_info, $($f)*); }
+}
+
+/// Prints a debug-level message (level 7) prefixed with device information.
+///
+/// This level should be used for debug messages.
+///
+/// Equivalent to the kernel's `dev_dbg` macro, except that it doesn't support dynamic debug yet.
+///
+/// Mimics the interface of [`std::print!`]. More information about the syntax is available from
+/// [`core::fmt`] and [`alloc::format!`].
+///
+/// [`std::print!`]: https://doc.rust-lang.org/std/macro.print.html
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::device::Device;
+///
+/// fn example(dev: &Device) {
+///     dev_dbg!(dev, "hello {}\n", "there");
+/// }
+/// ```
+#[macro_export]
+macro_rules! dev_dbg {
+    ($($f:tt)*) => { $crate::dev_printk!(pr_dbg, $($f)*); }
+}
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index b37a0b3180fb..c5765ab863d6 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -27,6 +27,8 @@
 // `super::std_vendor` is hidden, which makes the macro inline for some reason.
 #[doc(no_inline)]
 pub use super::dbg;
+pub use super::fmt;
+pub use super::{dev_alert, dev_crit, dev_dbg, dev_emerg, dev_err, dev_info, dev_notice, dev_warn};
 pub use super::{pr_alert, pr_crit, pr_debug, pr_emerg, pr_err, pr_info, pr_notice, pr_warn};
 
 pub use super::{init, pin_init, try_init, try_pin_init};
-- 
2.45.1

[PATCH v2 07/10] rust: add `io::Io` base type

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

`Io` 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
`Io` accordingly.

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

Co-developed-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/helpers.c     | 106 ++++++++++++++++++++++
 rust/kernel/io.rs  | 219 +++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs |   1 +
 3 files changed, 326 insertions(+)
 create mode 100644 rust/kernel/io.rs

diff --git a/rust/helpers.c b/rust/helpers.c
index 0ce40ccb978b..824b7c0b98dc 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -26,6 +26,7 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/errname.h>
+#include <linux/io.h>
 #include <linux/mutex.h>
 #include <linux/rcupdate.h>
 #include <linux/refcount.h>
@@ -181,6 +182,111 @@ void rust_helper_rcu_read_unlock(void)
 EXPORT_SYMBOL_GPL(rust_helper_rcu_read_unlock);
 /* end rcu */
 
+/* io.h */
+u8 rust_helper_readb(const volatile void __iomem *addr)
+{
+	return readb(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readb);
+
+u16 rust_helper_readw(const volatile void __iomem *addr)
+{
+	return readw(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readw);
+
+u32 rust_helper_readl(const volatile void __iomem *addr)
+{
+	return readl(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readl);
+
+#ifdef CONFIG_64BIT
+u64 rust_helper_readq(const volatile void __iomem *addr)
+{
+	return readq(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readq);
+#endif
+
+void rust_helper_writeb(u8 value, volatile void __iomem *addr)
+{
+	writeb(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writeb);
+
+void rust_helper_writew(u16 value, volatile void __iomem *addr)
+{
+	writew(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writew);
+
+void rust_helper_writel(u32 value, volatile void __iomem *addr)
+{
+	writel(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writel);
+
+#ifdef CONFIG_64BIT
+void rust_helper_writeq(u64 value, volatile void __iomem *addr)
+{
+	writeq(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writeq);
+#endif
+
+u8 rust_helper_readb_relaxed(const volatile void __iomem *addr)
+{
+	return readb_relaxed(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readb_relaxed);
+
+u16 rust_helper_readw_relaxed(const volatile void __iomem *addr)
+{
+	return readw_relaxed(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readw_relaxed);
+
+u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
+{
+	return readl_relaxed(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readl_relaxed);
+
+#ifdef CONFIG_64BIT
+u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
+{
+	return readq_relaxed(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readq_relaxed);
+#endif
+
+void rust_helper_writeb_relaxed(u8 value, volatile void __iomem *addr)
+{
+	writeb_relaxed(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writeb_relaxed);
+
+void rust_helper_writew_relaxed(u16 value, volatile void __iomem *addr)
+{
+	writew_relaxed(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writew_relaxed);
+
+void rust_helper_writel_relaxed(u32 value, volatile void __iomem *addr)
+{
+	writel_relaxed(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writel_relaxed);
+
+#ifdef CONFIG_64BIT
+void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
+{
+	writeq_relaxed(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writeq_relaxed);
+#endif
+
 /*
  * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
  * use it in contexts where Rust expects a `usize` like slice (array) indices.
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
new file mode 100644
index 000000000000..a19a1226181d
--- /dev/null
+++ b/rust/kernel/io.rs
@@ -0,0 +1,219 @@
+// 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};
+
+/// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
+///
+/// The creator (usually a subsystem 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 remapped memory region.
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::{bindings, io::Io};
+/// # use core::ops::Deref;
+///
+/// // See also [`pci::Bar`] for a real example.
+/// struct IoMem<const SIZE: usize>(Io<SIZE>);
+///
+/// impl<const SIZE: usize> IoMem<SIZE> {
+///     fn new(paddr: usize) -> Result<Self>{
+///
+///         // SAFETY: assert safety for this example
+///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE.try_into().unwrap()) };
+///         if addr.is_null() {
+///             return Err(ENOMEM);
+///         }
+///
+///         // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
+///         // size `SIZE`.
+///         let io = unsafe { Io::new(addr as _, SIZE)? };
+///
+///         Ok(IoMem(io))
+///     }
+/// }
+///
+/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
+///     fn drop(&mut self) {
+///         // SAFETY: Safe as by the invariant of `Io`.
+///         unsafe { bindings::iounmap(self.0.base_addr() as _); };
+///     }
+/// }
+///
+/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
+///    type Target = Io<SIZE>;
+///
+///    fn deref(&self) -> &Self::Target {
+///        &self.0
+///    }
+/// }
+///
+/// let iomem = IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD).unwrap();
+/// iomem.writel(0x42, 0x0);
+/// assert!(iomem.try_writel(0x42, 0x0).is_ok());
+/// assert!(iomem.try_writel(0x42, 0x4).is_err());
+/// ```
+pub struct Io<const SIZE: usize = 0> {
+    addr: usize,
+    maxsize: usize,
+}
+
+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);
+
+            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)?;
+
+            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);
+
+            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)?;
+
+            unsafe { bindings::$name(value, addr as _) }
+            Ok(())
+        }
+    };
+}
+
+impl<const SIZE: usize> Io<SIZE> {
+    ///
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
+    /// `maxsize`.
+    pub unsafe fn new(addr: usize, maxsize: usize) -> Result<Self> {
+        if maxsize < SIZE {
+            return Err(EINVAL);
+        }
+
+        Ok(Self { addr, maxsize })
+    }
+
+    /// Returns the base address of this mapping.
+    #[inline]
+    pub fn base_addr(&self) -> usize {
+        self.addr
+    }
+
+    /// Returns the size of this mapping.
+    #[inline]
+    pub fn maxsize(&self) -> usize {
+        self.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.base_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.base_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 601c3d3c9d54..f4dd11014a65 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -56,6 +56,7 @@
 
 #[doc(hidden)]
 pub use bindings;
+pub mod io;
 pub use macros;
 pub use uapi;
 
-- 
2.45.1

[PATCH v2 08/10] 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@redhat.com>
---
 rust/helpers.c        |   6 ++
 rust/kernel/devres.rs | 168 ++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs    |   1 +
 3 files changed, 175 insertions(+)
 create mode 100644 rust/kernel/devres.rs

diff --git a/rust/helpers.c b/rust/helpers.c
index 824b7c0b98dc..269f97698588 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -287,6 +287,12 @@ void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
 EXPORT_SYMBOL_GPL(rust_helper_writeq_relaxed);
 #endif
 
+int rust_helper_devm_add_action(struct device *dev, void (*action)(void *), void *data)
+{
+	return devm_add_action(dev, action, data);
+}
+EXPORT_SYMBOL_GPL(rust_helper_devm_add_action);
+
 /*
  * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
  * use it in contexts where Rust expects a `usize` like slice (array) indices.
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
new file mode 100644
index 000000000000..ab0a3eb1ea4f
--- /dev/null
+++ b/rust/kernel/devres.rs
@@ -0,0 +1,168 @@
+// 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::ffi::c_void;
+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
+///
+/// ```
+/// # use kernel::{bindings, c_str, device::Device, devres::Devres, io::Io};
+/// # use core::ops::Deref;
+///
+/// // See also [`pci::Bar`] for a real example.
+/// struct IoMem<const SIZE: usize>(Io<SIZE>);
+///
+/// impl<const SIZE: usize> IoMem<SIZE> {
+///     fn new(paddr: usize) -> Result<Self>{
+///
+///         // SAFETY: assert safety for this example
+///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE.try_into().unwrap()) };
+///         if addr.is_null() {
+///             return Err(ENOMEM);
+///         }
+///
+///         // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
+///         // size `SIZE`.
+///         let io = unsafe { Io::new(addr as _, SIZE)? };
+///
+///         Ok(IoMem(io))
+///     }
+/// }
+///
+/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
+///     fn drop(&mut self) {
+///         // SAFETY: Safe as by the invariant of `Io`.
+///         unsafe { bindings::iounmap(self.0.base_addr() as _); };
+///     }
+/// }
+///
+/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
+///    type Target = Io<SIZE>;
+///
+///    fn deref(&self) -> &Self::Target {
+///        &self.0
+///    }
+/// }
+///
+/// # // SAFETY: *NOT* safe, just for the example to get an `ARef<Device>` instance
+/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) };
+///
+/// let iomem = IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD).unwrap();
+/// let devres = Devres::new(&dev, iomem, GFP_KERNEL).unwrap();
+///
+/// let res = devres.try_access().ok_or(ENXIO).unwrap();
+/// res.writel(0x42, 0x0);
+/// ```
+///
+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();
+        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)
+    }
+
+    unsafe extern "C" fn devres_callback(ptr: *mut 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()`.
+        self.revoke();
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index f4dd11014a65..ef9426e32c18 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -31,6 +31,7 @@
 mod build_assert;
 pub mod device;
 pub mod device_id;
+pub mod devres;
 pub mod driver;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
-- 
2.45.1

[PATCH v2 09/10] 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` and `pci::Driver::remove` 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::DriverOps` trait is implemented by
the `pci::Adapter`.

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

This patch is based on previous work from FUJITA Tomonori.

Co-developed-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Co-developed-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers.c                  |  18 ++
 rust/kernel/lib.rs              |   2 +
 rust/kernel/pci.rs              | 325 ++++++++++++++++++++++++++++++++
 4 files changed, 346 insertions(+)
 create mode 100644 rust/kernel/pci.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 18a3f05115cb..30ad2a0e22d7 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -12,6 +12,7 @@
 #include <linux/firmware.h>
 #include <linux/jiffies.h>
 #include <linux/mdio.h>
+#include <linux/pci.h>
 #include <linux/phy.h>
 #include <linux/refcount.h>
 #include <linux/sched.h>
diff --git a/rust/helpers.c b/rust/helpers.c
index 269f97698588..c7f90b457af5 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -28,6 +28,7 @@
 #include <linux/errname.h>
 #include <linux/io.h>
 #include <linux/mutex.h>
+#include <linux/pci.h>
 #include <linux/rcupdate.h>
 #include <linux/refcount.h>
 #include <linux/sched/signal.h>
@@ -293,6 +294,23 @@ int rust_helper_devm_add_action(struct device *dev, void (*action)(void *), void
 }
 EXPORT_SYMBOL_GPL(rust_helper_devm_add_action);
 
+void rust_helper_pci_set_drvdata(struct pci_dev *pdev, void *data)
+{
+	pci_set_drvdata(pdev, data);
+}
+EXPORT_SYMBOL_GPL(rust_helper_pci_set_drvdata);
+
+void *rust_helper_pci_get_drvdata(struct pci_dev *pdev)
+{
+	return pci_get_drvdata(pdev);
+}
+EXPORT_SYMBOL_GPL(rust_helper_pci_get_drvdata);
+
+u64 rust_helper_pci_resource_len(struct pci_dev *pdev, int barnr)
+{
+	return pci_resource_len(pdev, barnr);
+}
+
 /*
  * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
  * use it in contexts where Rust expects a `usize` like slice (array) indices.
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ef9426e32c18..4a02946dbbd9 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -59,6 +59,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..a8230474e9b8
--- /dev/null
+++ b/rust/kernel/pci.rs
@@ -0,0 +1,325 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Wrappers for the PCI subsystem
+//!
+//! C header: [`include/linux/pci.h`](srctree/include/linux/pci.h)
+
+use crate::{
+    bindings, container_of, device,
+    device_id::{IdTable, RawDeviceId},
+    driver,
+    error::{to_result, Result},
+    str::CStr,
+    types::{ARef, ForeignOwnable},
+    ThisModule,
+};
+use kernel::prelude::*; // for pinned_drop
+
+/// An adapter for the registration of PCI drivers.
+pub struct Adapter<T: Driver>(T);
+
+impl<T: Driver> driver::DriverOps for Adapter<T> {
+    type RegType = bindings::pci_driver;
+
+    fn register(
+        pdrv: &mut Self::RegType,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        pdrv.name = name.as_char_ptr();
+        pdrv.probe = Some(Self::probe_callback);
+        pdrv.remove = Some(Self::remove_callback);
+        pdrv.id_table = T::ID_TABLE.as_ref();
+
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        to_result(unsafe {
+            bindings::__pci_register_driver(pdrv as _, module.0, name.as_char_ptr())
+        })
+    }
+
+    fn unregister(pdrv: &mut Self::RegType) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        unsafe { bindings::pci_unregister_driver(pdrv) }
+    }
+}
+
+impl<T: Driver> Adapter<T> {
+    extern "C" fn probe_callback(
+        pdev: *mut bindings::pci_dev,
+        id: *const bindings::pci_device_id,
+    ) -> core::ffi::c_int {
+        // SAFETY: Safe because the core kernel only ever calls the probe callback with a valid
+        // `pdev`.
+        let dev = unsafe { device::Device::from_raw(&mut (*pdev).dev) };
+        // SAFETY: Guaranteed by the rules described above.
+        let mut pdev = unsafe { Device::from_dev(dev) };
+
+        // SAFETY: `id` is a pointer within the static table, so it's always valid.
+        let offset = unsafe { (*id).driver_data };
+        let info = {
+            // SAFETY: The offset comes from a previous call to `offset_from` in `IdArray::new`,
+            // which guarantees that the resulting pointer is within the table.
+            let ptr = unsafe {
+                id.cast::<u8>()
+                    .offset(offset as _)
+                    .cast::<Option<T::IdInfo>>()
+            };
+            // SAFETY: Guaranteed by the preceding safety requirement.
+            unsafe { (*ptr).as_ref() }
+        };
+        match T::probe(&mut pdev, info) {
+            Ok(data) => {
+                // Let the `struct pci_dev` own a reference of the driver's private data.
+                // SAFETY: The core kernel only ever calls the probe callback with a valid `pdev`.
+                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 core kernel only ever calls the probe callback with a valid `pdev`. `ptr`
+        // points to a valid reference of the driver's private data, as it was set by
+        // `Adapter::probe_callback`.
+        let data = unsafe {
+            let ptr = bindings::pci_get_drvdata(pdev);
+
+            T::Data::from_foreign(ptr)
+        };
+
+        T::remove(&data);
+    }
+}
+
+/// 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.
+#[derive(Clone, Copy)]
+pub struct DeviceId {
+    /// Vendor ID
+    pub vendor: u32,
+    /// Device ID
+    pub device: u32,
+    /// Subsystem vendor ID
+    pub subvendor: u32,
+    /// Subsystem device ID
+    pub subdevice: u32,
+    /// Device class and subclass
+    pub class: u32,
+    /// Limit which sub-fields of the class
+    pub class_mask: u32,
+}
+
+impl DeviceId {
+    const PCI_ANY_ID: u32 = !0;
+
+    /// PCI_DEVICE macro.
+    pub const fn new(vendor: u32, device: u32) -> Self {
+        Self {
+            vendor,
+            device,
+            subvendor: DeviceId::PCI_ANY_ID,
+            subdevice: DeviceId::PCI_ANY_ID,
+            class: 0,
+            class_mask: 0,
+        }
+    }
+
+    /// PCI_DEVICE_CLASS macro.
+    pub const fn with_class(class: u32, class_mask: u32) -> Self {
+        Self {
+            vendor: DeviceId::PCI_ANY_ID,
+            device: DeviceId::PCI_ANY_ID,
+            subvendor: DeviceId::PCI_ANY_ID,
+            subdevice: DeviceId::PCI_ANY_ID,
+            class,
+            class_mask,
+        }
+    }
+
+    /// PCI_DEVICE_ID macro.
+    pub const fn to_rawid(&self, offset: isize) -> bindings::pci_device_id {
+        bindings::pci_device_id {
+            vendor: self.vendor,
+            device: self.device,
+            subvendor: self.subvendor,
+            subdevice: self.subdevice,
+            class: self.class,
+            class_mask: self.class_mask,
+            driver_data: offset as _,
+            override_only: 0,
+        }
+    }
+}
+
+// SAFETY: `ZERO` is all zeroed-out and `to_rawid` stores `offset` in `pci_device_id::driver_data`.
+unsafe impl RawDeviceId for DeviceId {
+    type RawType = bindings::pci_device_id;
+
+    const ZERO: Self::RawType = bindings::pci_device_id {
+        vendor: 0,
+        device: 0,
+        subvendor: 0,
+        subdevice: 0,
+        class: 0,
+        class_mask: 0,
+        driver_data: 0,
+        override_only: 0,
+    };
+}
+
+/// Define a const pci device id table
+///
+/// # Examples
+///
+/// See [`Driver`]
+///
+#[macro_export]
+macro_rules! define_pci_id_table {
+    ($data_type:ty, $($t:tt)*) => {
+        type IdInfo = $data_type;
+        const ID_TABLE: $crate::device_id::IdTable<'static, $crate::pci::DeviceId, $data_type> = {
+            $crate::define_id_array!(ARRAY, $crate::pci::DeviceId, $data_type, $($t)* );
+            ARRAY.as_table()
+        };
+    };
+}
+pub use define_pci_id_table;
+
+/// The PCI driver trait.
+///
+/// # Example
+///
+///```
+/// # use kernel::{bindings, define_pci_id_table, pci, sync::Arc};
+///
+/// struct MyDriver;
+/// struct MyDeviceData;
+///
+/// impl pci::Driver for MyDriver {
+///     type Data = Arc<MyDeviceData>;
+///
+///     define_pci_id_table! {
+///         (),
+///         [ (pci::DeviceId::new(bindings::PCI_VENDOR_ID_REDHAT,
+///                               bindings::PCI_ANY_ID as u32),
+///            None)
+///         ]
+///     }
+///
+///     fn probe(
+///         _pdev: &mut pci::Device,
+///         _id_info: Option<&Self::IdInfo>
+///     ) -> Result<Self::Data> {
+///         Err(ENODEV)
+///     }
+///
+///     fn remove(_data: &Self::Data) {
+///     }
+/// }
+///```
+/// 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 {
+    /// Data stored on device by driver.
+    ///
+    /// Corresponds to the data set or retrieved via the kernel's
+    /// `pci_{set,get}_drvdata()` functions.
+    ///
+    /// Require that `Data` implements `ForeignOwnable`. We guarantee to
+    /// never move the underlying wrapped data structure.
+    ///
+    /// TODO: Use associated_type_defaults once stabilized:
+    ///
+    /// `type Data: ForeignOwnable = ();`
+    type Data: ForeignOwnable;
+
+    /// 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<'static, DeviceId, 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: Option<&Self::IdInfo>) -> Result<Self::Data>;
+
+    /// PCI driver remove.
+    ///
+    /// Called when a platform device is removed.
+    /// Implementers should prepare the device for complete removal here.
+    fn remove(data: &Self::Data);
+}
+
+/// 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: Guaranteed by the type invaraints.
+        unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) as _ }
+    }
+
+    /// Enable the Device's memory.
+    pub fn enable_device_mem(&self) -> Result {
+        // SAFETY: Safe by the type invariants.
+        let ret = unsafe { bindings::pci_enable_device_mem(self.as_raw()) };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Set the Device's master.
+    pub fn set_master(&self) {
+        // SAFETY: Safe by the type invariants.
+        unsafe { bindings::pci_set_master(self.as_raw()) };
+    }
+}
+
+impl AsRef<device::Device> for Device {
+    fn as_ref(&self) -> &device::Device {
+        &self.0
+    }
+}
-- 
2.45.1

[PATCH v2 10/10] 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@redhat.com>
---
 rust/kernel/pci.rs | 142 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index a8230474e9b8..2b61fb59d4a7 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -5,14 +5,18 @@
 //! C header: [`include/linux/pci.h`](srctree/include/linux/pci.h)
 
 use crate::{
+    alloc::flags::*,
     bindings, container_of, device,
     device_id::{IdTable, RawDeviceId},
+    devres::Devres,
     driver,
     error::{to_result, Result},
+    io::Io,
     str::CStr,
     types::{ARef, ForeignOwnable},
     ThisModule,
 };
+use core::ops::Deref;
 use kernel::prelude::*; // for pinned_drop
 
 /// An adapter for the registration of PCI drivers.
@@ -281,9 +285,114 @@ 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 `Io` 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: Io<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);
+        }
+
+        // SAFETY: `ioptr` is guaranteed to be the start of a valid I/O mapped memory region of size
+        // `len`.
+        let io = match unsafe { Io::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: Safe by the invariants of `Device` and `Bar`.
+        unsafe { Self::do_release(&self.pdev, self.io.base_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 {
+        &self.io
+    }
+}
+
 impl Device {
     /// Create a PCI Device instance from an existing `device::Device`.
     ///
@@ -316,6 +425,39 @@ pub fn set_master(&self) {
         // SAFETY: Safe by the type invariants.
         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: Safe by the type invariant.
+        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)
+    }
+
+    /// Returns a new `ARef` of the base `device::Device`.
+    pub fn as_dev(&self) -> ARef<device::Device> {
+        self.0.clone()
+    }
 }
 
 impl AsRef<device::Device> for Device {
-- 
2.45.1

Re: [PATCH v2 00/10] Device / Driver and PCI Rust abstractions

[Viresh Kumar]

On 19-06-24, 01:39, Danilo Krummrich wrote:
> - 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

Hi Danilo,

I am working on writing bindings for CPUFreq drivers [1] and was
looking to rebase over staging/rust-device, and I am not sure how to
proceed after device::Data is dropped now.

What I was doing at probe() was something like this:

    fn probe(dev: &mut platform::Device, id_info: Option<&Self::IdInfo>) -> Result<Self::Data> {
        let data = Arc::<DeviceData>::from(kernel::new_device_data!(
            cpufreq::Registration::new(),
            (),
            "CPUFreqDT::Registration"
        )?);

        ...

        // Need a mutable object to be passed to register() here.
        data.registrations()
            .ok_or(ENXIO)?
            .as_pinned_mut()
            .register(c_str!("cpufreq-dt"), ...)?;

        Ok(data)
    }

The register() function of cpufreq core needs a mutable pointer to
`self` and it worked earlier as Data used a RevocableMutex. But with
Devres, we don't have a Mutex anymore and devres.try_access() doesn't
give a mutable object.

I am a bit confused on how to get this going. I looked at how PCI bus
is implemented in the staging/dev but I couldn't find an end driver
using this work.

Maybe I am making an mistake and missing the obvious.

Thanks.

-- 
viresh

[1] https://lore.kernel.org/all/cover.1717750631.git.viresh.kumar@linaro.org/

Re: [PATCH v2 00/10] Device / Driver and PCI Rust abstractions

[Greg KH]

On Wed, Jun 19, 2024 at 05:34:07PM +0530, Viresh Kumar wrote:
> On 19-06-24, 01:39, Danilo Krummrich wrote:
> > - 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
> 
> Hi Danilo,
> 
> I am working on writing bindings for CPUFreq drivers [1] and was
> looking to rebase over staging/rust-device, and I am not sure how to
> proceed after device::Data is dropped now.

As it should be dropped :)

A struct device does not have a "data" pointer, it has specific other
pointers to hold data in, but they better be accessed by their proper
name if you want rust code to be reviewable by anyone.

Also, you shouldn't be accessing that field directly anyway, that's what
the existing dev_set_drvdata/dev_get_drvdata() calls are for.  Just use
them please.

thanks,

greg k-h

Re: [PATCH v2 00/10] Device / Driver and PCI Rust abstractions

[Danilo Krummrich]

On Wed, Jun 19, 2024 at 05:34:07PM +0530, Viresh Kumar wrote:
> On 19-06-24, 01:39, Danilo Krummrich wrote:
> > - 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
> 
> Hi Danilo,
> 
> I am working on writing bindings for CPUFreq drivers [1] and was
> looking to rebase over staging/rust-device, and I am not sure how to
> proceed after device::Data is dropped now.
> 
> What I was doing at probe() was something like this:
> 
>     fn probe(dev: &mut platform::Device, id_info: Option<&Self::IdInfo>) -> Result<Self::Data> {
>         let data = Arc::<DeviceData>::from(kernel::new_device_data!(
>             cpufreq::Registration::new(),
>             (),
>             "CPUFreqDT::Registration"
>         )?);
> 
>         ...
> 
>         // Need a mutable object to be passed to register() here.
>         data.registrations()
>             .ok_or(ENXIO)?
>             .as_pinned_mut()
>             .register(c_str!("cpufreq-dt"), ...)?;
> 
>         Ok(data)
>     }
> 
> The register() function of cpufreq core needs a mutable pointer to
> `self` and it worked earlier as Data used a RevocableMutex. But with
> Devres, we don't have a Mutex anymore and devres.try_access() doesn't
> give a mutable object.

If you want to split `cpufreq::Registration` in `new()` and `register()`, you
probably want to pass the registration object to `Devres` in `register()`
instead.

However, I wouldn't recommend splitting it up (unless you absolutely have to),
it's way cleaner (and probably less racy) if things are registered once the
registration is created.

> 
> I am a bit confused on how to get this going. I looked at how PCI bus
> is implemented in the staging/dev but I couldn't find an end driver
> using this work.

The PCI abstraction did not need to change for that, since it uses the
generalized `driver::Registration`, which is handled by the `Module` structure
instead.

However, staging/dev also contains the `drm::drv::Registration` type [1], which
in principle does the same thing as `cpufreq::Registration` just for a DRM
device.

If you're looking for an example driver making use of this, please have a look
at Nova [1].

[1] https://github.com/Rust-for-Linux/linux/blob/staging/dev/rust/kernel/drm/drv.rs
[2] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/drivers/gpu/drm/nova/driver.rs

> 
> Maybe I am making an mistake and missing the obvious.
> 
> Thanks.
> 
> -- 
> viresh
> 
> [1] https://lore.kernel.org/all/cover.1717750631.git.viresh.kumar@linaro.org/
> 

Re: [PATCH v2 00/10] Device / Driver and PCI Rust abstractions

[Danilo Krummrich]

On Wed, Jun 19, 2024 at 02:17:58PM +0200, Greg KH wrote:
> On Wed, Jun 19, 2024 at 05:34:07PM +0530, Viresh Kumar wrote:
> > On 19-06-24, 01:39, Danilo Krummrich wrote:
> > > - 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
> > 
> > Hi Danilo,
> > 
> > I am working on writing bindings for CPUFreq drivers [1] and was
> > looking to rebase over staging/rust-device, and I am not sure how to
> > proceed after device::Data is dropped now.
> 
> As it should be dropped :)
> 
> A struct device does not have a "data" pointer, it has specific other
> pointers to hold data in, but they better be accessed by their proper
> name if you want rust code to be reviewable by anyone.
> 
> Also, you shouldn't be accessing that field directly anyway, that's what
> the existing dev_set_drvdata/dev_get_drvdata() calls are for.  Just use
> them please.

Sorry that this was confusing. `device::Data` was a generic type for drivers to
store their private data in. It was meant to be handled by subsystems to store
it in their particular driver structure. Is most cases of course this eventually
ends up in a call to dev_set_drvdata() (e.g. through pci_set_drvdata()).

The reason we had this in the first place was that `device::Data` was also used
to store resources and registrations. With my rework to `Devres` this isn't the
case anymore, and hence `device::Data` does not add any value anymore and was
removed.

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v2 00/10] Device / Driver and PCI Rust abstractions

[Viresh Kumar]

On 19-06-24, 14:36, Danilo Krummrich wrote:
> If you want to split `cpufreq::Registration` in `new()` and `register()`, you
> probably want to pass the registration object to `Devres` in `register()`
> instead.
> 
> However, I wouldn't recommend splitting it up (unless you absolutely have to),
> it's way cleaner (and probably less racy) if things are registered once the
> registration is created.

> The PCI abstraction did not need to change for that, since it uses the
> generalized `driver::Registration`, which is handled by the `Module` structure
> instead.
> 
> However, staging/dev also contains the `drm::drv::Registration` type [1], which
> in principle does the same thing as `cpufreq::Registration` just for a DRM
> device.
> 
> If you're looking for an example driver making use of this, please have a look
> at Nova [1].

Thanks for the pointers Danilo.

There is more to it now and I still don't know what's the best way
forward. :(

Devres will probably work well with the frameworks that provide a bus,
where a device and driver are matched and probe/remove are called.
Obviously Devres needs a struct device, whose probing/removal can
allocate/free resources.

The CPUFreq framework is a bit different. There is no bus, device or
driver there. The device for the framework is the CPU device, but we
don't (can't) really bind a struct driver to it. There are more layers
in the kernel which use the CPU devices directly, like cpuidle, etc.
And so the CPU device isn't really private to the cpufreq/cpuidle
frameworks.

Most of the cpufreq drivers register with the cpufreq core from their
module_init() function, and unregister from module_exit(). There is no
probe/remove() callbacks available. Some drivers though have a
platform device/driver model implemented over an imaginary platform
device, a hack implemented to get them working because of special
requirements (one of them is to allow defer probing to work). The
driver I am implementing, cpufreq-dt, also has one such platform
device which is created at runtime. But there will be others without a
platform device.

The point is that the Rust cpufreq core can't do the Devres stuff
itself and it can't expect a struct device to be made available to it
by the driver. Some cpufreq drivers will have a platform device, some
won't.

One way to make this whole work is to reintroduce the Data part, just
for cpufreq core, but I really don't want to do it. What else can be
done ?

-- 
viresh

Re: [PATCH v2 00/10] Device / Driver and PCI Rust abstractions

[Danilo Krummrich]

On Thu, Jun 20, 2024 at 03:35:56PM +0530, Viresh Kumar wrote:
> On 19-06-24, 14:36, Danilo Krummrich wrote:
> > If you want to split `cpufreq::Registration` in `new()` and `register()`, you
> > probably want to pass the registration object to `Devres` in `register()`
> > instead.
> > 
> > However, I wouldn't recommend splitting it up (unless you absolutely have to),
> > it's way cleaner (and probably less racy) if things are registered once the
> > registration is created.
> 
> > The PCI abstraction did not need to change for that, since it uses the
> > generalized `driver::Registration`, which is handled by the `Module` structure
> > instead.
> > 
> > However, staging/dev also contains the `drm::drv::Registration` type [1], which
> > in principle does the same thing as `cpufreq::Registration` just for a DRM
> > device.
> > 
> > If you're looking for an example driver making use of this, please have a look
> > at Nova [1].
> 
> Thanks for the pointers Danilo.
> 
> There is more to it now and I still don't know what's the best way
> forward. :(
> 
> Devres will probably work well with the frameworks that provide a bus,
> where a device and driver are matched and probe/remove are called.
> Obviously Devres needs a struct device, whose probing/removal can
> allocate/free resources.

Indeed, but please note that this was the case before as well. When we had
`device::Data` with a `Revokable<T>` for Registrations this revokable was
revoked through the `DeviceRemoval` trait when the driver was unbound from the
device.

> 
> The CPUFreq framework is a bit different. There is no bus, device or
> driver there. The device for the framework is the CPU device, but we
> don't (can't) really bind a struct driver to it. There are more layers
> in the kernel which use the CPU devices directly, like cpuidle, etc.
> And so the CPU device isn't really private to the cpufreq/cpuidle
> frameworks.

If there is no bus, device or driver, then those abstractions aren't for your
use case. Those are abstractions around the device / driver core.

> 
> Most of the cpufreq drivers register with the cpufreq core from their
> module_init() function, and unregister from module_exit(). There is no
> probe/remove() callbacks available. Some drivers though have a
> platform device/driver model implemented over an imaginary platform
> device, a hack implemented to get them working because of special
> requirements (one of them is to allow defer probing to work). The
> driver I am implementing, cpufreq-dt, also has one such platform
> device which is created at runtime. But there will be others without a
> platform device.
> 
> The point is that the Rust cpufreq core can't do the Devres stuff
> itself and it can't expect a struct device to be made available to it
> by the driver. Some cpufreq drivers will have a platform device, some
> won't.

That seems to be purely a design question for cpufreq drivers then.

What prevents you from always creating a corresponding platform device?

If you really want some drivers to bypass the device / driver model (not sure
if that's a good idea though), you need separate abstractions for that.

> 
> One way to make this whole work is to reintroduce the Data part, just
> for cpufreq core, but I really don't want to do it.

That doesn't help you either. As mentioned above, `device::Data` was supposed to
receive a callback (`DeviceRemoval`) from the underlying driver (platform_driver
in your case) on device detach to revoke the registration.

By using `Devres` instead, nothing changes semantically, but it makes the
resulting code cleaner.

> What else can be done ?

Think about what you want the lifetime of your cpufreq registration to be.

Currently, it seems you want to do both, bind it to probe() / remove(), in case
the driver is implemented as platform_driver, and to module_init() /
module_exit(), in case the device / driver model is bypassed.

> 
> -- 
> viresh
> 

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Greg KH]

On Wed, Jun 19, 2024 at 01:39:47AM +0200, Danilo Krummrich wrote:
> In a subsequent patch we introduce the `Registration` abstraction used
> to register driver structures. Some subsystems require the module name on
> driver registration (e.g. PCI in __pci_register_driver()), hence pass
> the module name to `Module::init`.

I understand the need/want here, but it feels odd that you have to
change anything to do it.

> 
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/lib.rs           | 14 ++++++++++----
>  rust/kernel/net/phy.rs       |  2 +-
>  rust/macros/module.rs        |  3 ++-
>  samples/rust/rust_minimal.rs |  2 +-
>  samples/rust/rust_print.rs   |  2 +-
>  5 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index a791702b4fee..5af00e072a58 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
>      /// should do.
>      ///
>      /// Equivalent to the `module_init` macro in the C API.
> -    fn init(module: &'static ThisModule) -> error::Result<Self>;
> +    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;

Why can't the name come directly from the build system?  Why must it be
passed into the init function of the module "class"?  What is it going
to do with it?

A PCI, or other bus, driver "knows" it's name already by virtue of the
build system, so it can pass that string into whatever function needs
that, but the module init function itself does NOT need that.

So I fail to understand why we need to burden ALL module init functions
with this, when only a very very very tiny subset of all drivers will
ever need to know this, and even then, they don't need to know it at
init module time, they know it at build time and it will be a static
string at that point, it will not be coming in through an init call.

thanks,

greg k-h

Re: [PATCH v2 02/10] rust: implement generic driver registration

[Greg KH]

On Wed, Jun 19, 2024 at 01:39:48AM +0200, Danilo Krummrich wrote:
> Implement the generic `Registration` type and the `DriverOps` trait.

I don't think this is needed, more below...

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

But that's not what normally happens, more below...

> Instead the `DriverOps` trait is provided to subsystems, which have to
> implement `DriverOps::register` and `DrvierOps::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.

So you are saying this should be something that a "bus" would do?
Please be explicit as to what you mean by "subsystem" here.

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

So this is a BusOps, or more in general, a "subsystem" if it's not a
bus (i.e. it's a class).  Note, we used to use the term "subsystem" a
very long time ago but got rid of them in the driver core, let's not
bring it back unless we REALLY know we want it this time.

So why isn't this just a BusOps?
> This patch is based on previous work from Wedson Almeida Filho.
> 
> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/driver.rs | 128 ++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs    |   1 +
>  2 files changed, 129 insertions(+)
>  create mode 100644 rust/kernel/driver.rs
> 
> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> new file mode 100644
> index 000000000000..e04406b93b56
> --- /dev/null
> +++ b/rust/kernel/driver.rs
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.).

See, you think it's a bus, let's call it a bus!  :)

> +//!
> +//! Each bus / subsystem is expected to implement [`DriverOps`], 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 [`DriverOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, Amba,
> +/// etc.) to privide 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 `DriverOps::register` and
> +/// `bindings::pci_unregister_driver` from `DriverOps::unregister`.
> +pub trait DriverOps {
> +    /// 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.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `reg` must point to valid, initialised, and writable memory. It may be modified by this
> +    /// function to hold registration state.
> +    ///
> +    /// On success, `reg` must remain pinned and valid until the matching call to
> +    /// [`DriverOps::unregister`].
> +    fn register(
> +        reg: &mut Self::RegType,
> +        name: &'static CStr,
> +        module: &'static ThisModule,
> +    ) -> Result;
> +
> +    /// Unregisters a driver previously registered with [`DriverOps::register`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// `reg` must point to valid writable memory, initialised by a previous successful call to
> +    /// [`DriverOps::register`].
> +    fn unregister(reg: &mut Self::RegType);
> +}

So you are getting into what a bus wants/needs to support here, why is
register/unregister the "big" things to be implemented first?  Why not
just map the current register/unregister bus functions to a bus-specific
trait for now?  And then, if you think it really should be generic, we
can make it that way then.  I don't see why this needs to be generic now
as you aren't implementing a bus in rust at this point in time, right?

> +
> +/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
> +/// `bindings::pci_driver`). Therefore a [`Registration`] is initialized with some type that
> +/// implements the [`DriverOps`] 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: DriverOps> {
> +    #[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: DriverOps> 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: DriverOps> Send for Registration<T> {}
> +
> +impl<T: DriverOps> Registration<T> {
> +    /// Creates a new instance of the registration object.
> +    pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {

Drivers have modules, not busses.  So you are registering a driver with
a bus here, it's not something that a driver itself implements as you
have named here.


> +        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 { &mut *ptr };
> +
> +                T::register(drv, name, module)
> +            }),
> +        })
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T: DriverOps> PinnedDrop for Registration<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        let drv = unsafe { &mut *self.reg.get() };
> +
> +        T::unregister(drv);
> +    }
> +}
> +
> +/// A kernel module that only registers the given driver on init.
> +///
> +/// This is a helper struct to make it easier to define single-functionality modules, in this case,
> +/// modules that offer a single driver.
> +#[pin_data]
> +pub struct Module<T: DriverOps> {
> +    #[pin]
> +    _driver: Registration<T>,
> +}

While these are nice, let's not add them just yet, let's keep it simple
for now until we work out the proper model and see what is, and is not,
common for drivers to do.

That way we keep the review simpler as well, and saves you time
rewriting things as we rename / modify stuff.

> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -29,6 +29,7 @@
>  pub mod alloc;
>  mod build_assert;
>  pub mod device;
> +pub mod driver;

Nope, this is a bus :)

thanks,

greg k-h

Re: [PATCH v2 03/10] rust: implement `IdArray`, `IdTable` and `RawDeviceId`

[Greg KH]

On Wed, Jun 19, 2024 at 01:39:49AM +0200, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> 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.

Let's not.

For now, let's stick with the C arrays please.  That way it's simple,
makes it easy to understand, and you can put the array in a .c file and
access it that way.  That way also all of our existing infrastructure
will work properly (I don't know how you deal with the modinfo sections
here...)


> 
> 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: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Co-developed-by: Danilo Krummrich <dakr@redhat.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/device_id.rs | 336 +++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs       |   2 +
>  2 files changed, 338 insertions(+)
>  create mode 100644 rust/kernel/device_id.rs
> 
> diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
> new file mode 100644
> index 000000000000..c490300f29bb
> --- /dev/null
> +++ b/rust/kernel/device_id.rs

While I see the temptation to attempt to create a generic class for ids,
this is all VERY bus specific.  Please just stick with a simple C array
for now, trying to mess with generic array types of arbitrary structures
and values and fields is a fun exercise in rust macros, but ick, let's
worry about that much later.

Try to do something "simple" if you really must, like just a pci device
id list, and if that works, then try USB ids.  And then, if you think
they look comon, THEN think about maybe combining them into something
that looks crazy like this.

But not now, no, please, keep it simple.  Stick to C arrays.

greg k-h

Re: [PATCH v2 04/10] rust: add rcu abstraction

[Greg KH]

On Wed, Jun 19, 2024 at 01:39:50AM +0200, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> Add a simple abstraction to guard critical code sections with an rcu
> read lock.

Why?  Why not add this when we have code that actually needs it?

The driver model sure doesn't :)

thanks,

greg k-h

Re: [PATCH v2 05/10] rust: add `Revocable` type

[Greg KH]

On Wed, Jun 19, 2024 at 01:39:51AM +0200, Danilo Krummrich wrote:
> 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).

We are getting into the "removed" vs. "unbound" terminology again here
:(

How about this change in the text:
	This is useful, for example, for resources allocated during
	a device probe call; and need to be not accessed after the
	device remove call.

I am guessing this is an attempt to tie into something much like the
devm api, right?  If so, why not call it that?  Why name it something
different?  Revocable seems not tied to a device, and you REALLY want
this to be tied to the lifetime model of a struct device ownership of a
driver.  You don't want it tied to anything else that might come in as
part of another path into that driver (i.e. through a device node
access), right?




> 
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/lib.rs       |   1 +
>  rust/kernel/revocable.rs | 209 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 210 insertions(+)
>  create mode 100644 rust/kernel/revocable.rs
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 98e1a1425d17..601c3d3c9d54 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -43,6 +43,7 @@
>  pub mod net;
>  pub mod prelude;
>  pub mod print;
> +pub mod revocable;
>  mod static_assert;
>  #[doc(hidden)]
>  pub mod std_vendor;
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> new file mode 100644
> index 000000000000..3d13e7b2f2e8
> --- /dev/null
> +++ b/rust/kernel/revocable.rs
> @@ -0,0 +1,209 @@
> +// 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,
> +    init::{self},
> +    prelude::*,
> +    sync::rcu,

Ah, this is why you wanted rcu.  Note that the devm api today does NOT
use rcu, so why use it here?  What is that going to get you?  Why not
keep it simple for now and then, if you REALLY want to use rcu, you can
at a later time, after ensuring that it will be a benefit.


> +};
> +use core::{
> +    cell::UnsafeCell,
> +    marker::PhantomData,
> +    mem::MaybeUninit,
> +    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 = Box::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 = Box::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: MaybeUninit<UnsafeCell<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> {

Don't you want to tie this to a struct device?  If not, why not?  If so,
why not do it here?

thanks,

greg k-h

Re: [PATCH v2 06/10] rust: add `dev_*` print macros.

[Greg KH]

On Wed, Jun 19, 2024 at 01:39:52AM +0200, Danilo Krummrich wrote:
> From: Wedson Almeida Filho <wedsonaf@google.com>
> 
> Implement `dev_*` print macros for `device::Device`.
> 
> They behave like the macros with the same names in C, i.e., they print
> messages to the kernel ring buffer with the given level, prefixing the
> messages with corresponding device information.

Nice, but one issue:

> +    /// Prints a debug-level message (level 7) prefixed with device information.
> +    ///
> +    /// More details are available from [`dev_dbg`].
> +    ///
> +    /// [`dev_dbg`]: crate::dev_dbg
> +    pub fn pr_dbg(&self, args: fmt::Arguments<'_>) {
> +        if cfg!(debug_assertions) {

That should not be an issue here.  debug_assertions is something
independent of dev_dbg() calls.  You made this a Rust-only thing, that
doesn't tie properly into the existing dynamic printk functionality by
having yet-another-way to turn this on/off, right?

So just remove the check please.

And if you want to send this as a single patch after fixing this, I'll
be glad to add it to the tree now, as it's "obviously" correct :)

thanks,

greg k-h

Re: [PATCH v2 07/10] rust: add `io::Io` base type

[Greg KH]

On Wed, Jun 19, 2024 at 01:39:53AM +0200, 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.

Why not?

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

It provides a subset of the existing accessors, one you might want to
trim down for now, see below...

> +/* io.h */
> +u8 rust_helper_readb(const volatile void __iomem *addr)
> +{
> +	return readb(addr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_readb);

<snip>

You provide wrappers for a subset of what io.h provides, why that
specific subset?

Why not just add what you need, when you need it?  I doubt you need all
of these, and odds are you will need more.

> +u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
> +{
> +	return readl_relaxed(addr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_readl_relaxed);

I know everyone complains about wrapper functions around inline
functions, so I'll just say it again, this is horrid.  And it's going to
hurt performance, so any rust code people write is not on a level
playing field here.

Your call, but ick...

> +#ifdef CONFIG_64BIT
> +u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
> +{
> +	return readq_relaxed(addr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_readq_relaxed);
> +#endif

Rust works on 32bit targets in the kernel now?

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

offsets aren't know at compile time for many implementations, as it
could be a dynamically allocated memory range.  How is this going to
work for that?  Heck, how does this work for DT-defined memory ranges
today?

thanks,

greg k-h

Re: [PATCH v2 08/10] rust: add devres abstraction

[Greg KH]

On Wed, Jun 19, 2024 at 01:39:54AM +0200, Danilo Krummrich wrote:
> Add a Rust abstraction for the kernel's devres (device resource
> management) implementation.

Ah, here's the devm stuff.  Why not put it right after the "revokable"
stuff?

And why have revokable at all?  Why not just put it all here in devres?
Who's going to use the generic type?  Especially as it uses rcu when
devres today does NOT use rcu, so you are might get some "interesting"
issues with the interaction of the two.

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

Is this last sentence correct?  Revokes 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().

That's not how a normal driver will use it, right?  It's when the
remove() callback comes into the driver.  That might be well before
Drop() happens, as there might be other things keeping that memory
around (again, think of /dev/ node accesses.)

> --- /dev/null
> +++ b/rust/kernel/devres.rs
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0

One note for all of these new files, no copyright notices?  I'm all for
that, but I know a LOT of lawyers by people who work for companies that
have been working on this code do NOT want to see files without
copyright lines.

So please go check.  Otherwise you might get a "stern talking to" in the
future when someone notices what you all did...

> +
> +//! 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::ffi::c_void;
> +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
> +///
> +/// ```
> +/// # use kernel::{bindings, c_str, device::Device, devres::Devres, io::Io};
> +/// # use core::ops::Deref;
> +///
> +/// // See also [`pci::Bar`] for a real example.
> +/// struct IoMem<const SIZE: usize>(Io<SIZE>);
> +///
> +/// impl<const SIZE: usize> IoMem<SIZE> {
> +///     fn new(paddr: usize) -> Result<Self>{
> +///
> +///         // SAFETY: assert safety for this example
> +///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE.try_into().unwrap()) };
> +///         if addr.is_null() {
> +///             return Err(ENOMEM);
> +///         }
> +///
> +///         // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
> +///         // size `SIZE`.
> +///         let io = unsafe { Io::new(addr as _, SIZE)? };
> +///
> +///         Ok(IoMem(io))
> +///     }
> +/// }
> +///
> +/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
> +///     fn drop(&mut self) {
> +///         // SAFETY: Safe as by the invariant of `Io`.
> +///         unsafe { bindings::iounmap(self.0.base_addr() as _); };
> +///     }
> +/// }
> +///
> +/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
> +///    type Target = Io<SIZE>;
> +///
> +///    fn deref(&self) -> &Self::Target {
> +///        &self.0
> +///    }
> +/// }
> +///
> +/// # // SAFETY: *NOT* safe, just for the example to get an `ARef<Device>` instance
> +/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) };
> +///
> +/// let iomem = IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD).unwrap();
> +/// let devres = Devres::new(&dev, iomem, GFP_KERNEL).unwrap();
> +///
> +/// let res = devres.try_access().ok_or(ENXIO).unwrap();
> +/// res.writel(0x42, 0x0);
> +/// ```
> +///
> +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();
> +        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)
> +    }
> +
> +    unsafe extern "C" fn devres_callback(ptr: *mut 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()`.
> +        self.revoke();
> +    }
> +}

Again, I don't think this can happen at "drop", it needs to happen
earlier sometimes.  Or maybe not, I can't tell when exactly is Drop
called, where would I find that?

thanks,

greg k-h

Re: [PATCH v2 09/10] rust: pci: add basic PCI device / driver abstractions

[Greg KH]

On Wed, Jun 19, 2024 at 01:39:55AM +0200, Danilo Krummrich wrote:
> +/// Abstraction for bindings::pci_device_id.
> +#[derive(Clone, Copy)]
> +pub struct DeviceId {
> +    /// Vendor ID
> +    pub vendor: u32,
> +    /// Device ID
> +    pub device: u32,
> +    /// Subsystem vendor ID
> +    pub subvendor: u32,
> +    /// Subsystem device ID
> +    pub subdevice: u32,
> +    /// Device class and subclass
> +    pub class: u32,
> +    /// Limit which sub-fields of the class
> +    pub class_mask: u32,
> +}

Note, this is exported to userspace, why can't you use the C structure
that's already defined for you?  This is going to get tricky over time
for the different busses, please use what is already there.

And question, how are you guaranteeing the memory layout of this
structure here?  Will rust always do this with no holes and no
re-arranging?

> +impl DeviceId {
> +    const PCI_ANY_ID: u32 = !0;
> +
> +    /// PCI_DEVICE macro.
> +    pub const fn new(vendor: u32, device: u32) -> Self {
> +        Self {
> +            vendor,
> +            device,
> +            subvendor: DeviceId::PCI_ANY_ID,
> +            subdevice: DeviceId::PCI_ANY_ID,
> +            class: 0,
> +            class_mask: 0,
> +        }
> +    }
> +
> +    /// PCI_DEVICE_CLASS macro.
> +    pub const fn with_class(class: u32, class_mask: u32) -> Self {
> +        Self {
> +            vendor: DeviceId::PCI_ANY_ID,
> +            device: DeviceId::PCI_ANY_ID,
> +            subvendor: DeviceId::PCI_ANY_ID,
> +            subdevice: DeviceId::PCI_ANY_ID,
> +            class,
> +            class_mask,
> +        }
> +    }

Why just these 2 subsets of the normal PCI_DEVICE* macros?

> +
> +    /// PCI_DEVICE_ID macro.
> +    pub const fn to_rawid(&self, offset: isize) -> bindings::pci_device_id {

PCI_DEVICE_ID is defined to be "0x02" in pci_regs.h, I don't think you
want that duplication here, right?

> +        bindings::pci_device_id {
> +            vendor: self.vendor,
> +            device: self.device,
> +            subvendor: self.subvendor,
> +            subdevice: self.subdevice,
> +            class: self.class,
> +            class_mask: self.class_mask,
> +            driver_data: offset as _,
> +            override_only: 0,
> +        }
> +    }
> +}
> +
> +// SAFETY: `ZERO` is all zeroed-out and `to_rawid` stores `offset` in `pci_device_id::driver_data`.
> +unsafe impl RawDeviceId for DeviceId {
> +    type RawType = bindings::pci_device_id;
> +
> +    const ZERO: Self::RawType = bindings::pci_device_id {
> +        vendor: 0,
> +        device: 0,
> +        subvendor: 0,
> +        subdevice: 0,
> +        class: 0,
> +        class_mask: 0,
> +        driver_data: 0,
> +        override_only: 0,

This feels rough, why can't the whole memory chunk be set to 0 for any
non-initialized values?  What happens if a new value gets added to the C
side, how will this work here?

> +    };
> +}
> +
> +/// Define a const pci device id table
> +///
> +/// # Examples
> +///
> +/// See [`Driver`]
> +///
> +#[macro_export]
> +macro_rules! define_pci_id_table {
> +    ($data_type:ty, $($t:tt)*) => {
> +        type IdInfo = $data_type;
> +        const ID_TABLE: $crate::device_id::IdTable<'static, $crate::pci::DeviceId, $data_type> = {
> +            $crate::define_id_array!(ARRAY, $crate::pci::DeviceId, $data_type, $($t)* );
> +            ARRAY.as_table()
> +        };
> +    };
> +}
> +pub use define_pci_id_table;
> +
> +/// The PCI driver trait.
> +///
> +/// # Example
> +///
> +///```
> +/// # use kernel::{bindings, define_pci_id_table, pci, sync::Arc};
> +///
> +/// struct MyDriver;
> +/// struct MyDeviceData;
> +///
> +/// impl pci::Driver for MyDriver {
> +///     type Data = Arc<MyDeviceData>;
> +///
> +///     define_pci_id_table! {
> +///         (),
> +///         [ (pci::DeviceId::new(bindings::PCI_VENDOR_ID_REDHAT,
> +///                               bindings::PCI_ANY_ID as u32),
> +///            None)
> +///         ]
> +///     }
> +///
> +///     fn probe(
> +///         _pdev: &mut pci::Device,
> +///         _id_info: Option<&Self::IdInfo>
> +///     ) -> Result<Self::Data> {
> +///         Err(ENODEV)
> +///     }
> +///
> +///     fn remove(_data: &Self::Data) {
> +///     }
> +/// }
> +///```
> +/// 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 {
> +    /// Data stored on device by driver.
> +    ///
> +    /// Corresponds to the data set or retrieved via the kernel's
> +    /// `pci_{set,get}_drvdata()` functions.
> +    ///
> +    /// Require that `Data` implements `ForeignOwnable`. We guarantee to
> +    /// never move the underlying wrapped data structure.
> +    ///
> +    /// TODO: Use associated_type_defaults once stabilized:
> +    ///
> +    /// `type Data: ForeignOwnable = ();`
> +    type Data: ForeignOwnable;

Does this mean this all can be 'static' in memory and never dynamically
allocated?  If so, good, if not, why not?

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

What does static mean here?

> +
> +    /// The table of device ids supported by the driver.
> +    const ID_TABLE: IdTable<'static, DeviceId, Self::IdInfo>;
> +
> +    /// PCI driver probe.
> +    ///
> +    /// Called when a new platform device is added or discovered.

This is not a platform device

> +    /// Implementers should attempt to initialize the device here.
> +    fn probe(dev: &mut Device, id: Option<&Self::IdInfo>) -> Result<Self::Data>;
> +
> +    /// PCI driver remove.
> +    ///
> +    /// Called when a platform device is removed.

This is not a platform device.

> +    /// Implementers should prepare the device for complete removal here.
> +    fn remove(data: &Self::Data);

No suspend?  No resume?  No shutdown?  only probe/remove?  Ok, baby
steps are nice :)


> +}
> +
> +/// 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: Guaranteed by the type invaraints.
> +        unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) as _ }
> +    }
> +
> +    /// Enable the Device's memory.
> +    pub fn enable_device_mem(&self) -> Result {
> +        // SAFETY: Safe by the type invariants.
> +        let ret = unsafe { bindings::pci_enable_device_mem(self.as_raw()) };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Set the Device's master.
> +    pub fn set_master(&self) {
> +        // SAFETY: Safe by the type invariants.
> +        unsafe { bindings::pci_set_master(self.as_raw()) };
> +    }

Why just these 2 functions?  Just an example, or you don't need anything
else yet?

thanks,

greg k-h

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

On Thu, Jun 20, 2024 at 04:19:48PM +0200, Greg KH wrote:
> On Wed, Jun 19, 2024 at 01:39:47AM +0200, Danilo Krummrich wrote:
> > In a subsequent patch we introduce the `Registration` abstraction used
> > to register driver structures. Some subsystems require the module name on
> > driver registration (e.g. PCI in __pci_register_driver()), hence pass
> > the module name to `Module::init`.
> 
> I understand the need/want here, but it feels odd that you have to
> change anything to do it.
> 
> > 
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/kernel/lib.rs           | 14 ++++++++++----
> >  rust/kernel/net/phy.rs       |  2 +-
> >  rust/macros/module.rs        |  3 ++-
> >  samples/rust/rust_minimal.rs |  2 +-
> >  samples/rust/rust_print.rs   |  2 +-
> >  5 files changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index a791702b4fee..5af00e072a58 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
> >      /// should do.
> >      ///
> >      /// Equivalent to the `module_init` macro in the C API.
> > -    fn init(module: &'static ThisModule) -> error::Result<Self>;
> > +    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;
> 
> Why can't the name come directly from the build system?  Why must it be
> passed into the init function of the module "class"?  What is it going
> to do with it?

The name does come from the build system, that's where `Module::init` gets it
from.

> 
> A PCI, or other bus, driver "knows" it's name already by virtue of the
> build system, so it can pass that string into whatever function needs

Let's take pci_register_driver() as example.

```
#define pci_register_driver(driver)		\
	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
```

In C drivers this works because (1) it's a macro and (2) it's called directly
from the driver code.

In Rust, for very good reasons, we have abstractions for C APIs, hence the
actual call to __pci_register_driver() does not come from code within the
module, but from the PCI Rust abstraction `Module::init` calls into instead.

Consequently, we have to pass things through. This also isn't new, please note
that the current code already does the same thing: `Module::init` (without this
patch) is already declared as

`fn init(module: &'static ThisModule) -> error::Result<Self>`

passing through `ThisModule` for the exact same reason.

Please also note that in the most common case (one driver per module) we don't
see any of this anyway.

Just like the C macro module_pci_driver(), Rust drivers can use the
`module_pci_driver!` macro.

Example from Nova:

```
    kernel::module_pci_driver! {
        // The driver type that implements the corresponding probe() and
        // remove() driver callbacks.
        type: NovaDriver,
        name: "Nova",
        author: "Danilo Krummrich",
        description: "Nova GPU driver",
        license: "GPL v2",
    }
```

> that, but the module init function itself does NOT need that.
> 
> So I fail to understand why we need to burden ALL module init functions
> with this, when only a very very very tiny subset of all drivers will
> ever need to know this, and even then, they don't need to know it at
> init module time, they know it at build time and it will be a static
> string at that point, it will not be coming in through an init call.
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Greg KH]

On Thu, Jun 20, 2024 at 06:10:05PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 20, 2024 at 04:19:48PM +0200, Greg KH wrote:
> > On Wed, Jun 19, 2024 at 01:39:47AM +0200, Danilo Krummrich wrote:
> > > In a subsequent patch we introduce the `Registration` abstraction used
> > > to register driver structures. Some subsystems require the module name on
> > > driver registration (e.g. PCI in __pci_register_driver()), hence pass
> > > the module name to `Module::init`.
> > 
> > I understand the need/want here, but it feels odd that you have to
> > change anything to do it.
> > 
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > ---
> > >  rust/kernel/lib.rs           | 14 ++++++++++----
> > >  rust/kernel/net/phy.rs       |  2 +-
> > >  rust/macros/module.rs        |  3 ++-
> > >  samples/rust/rust_minimal.rs |  2 +-
> > >  samples/rust/rust_print.rs   |  2 +-
> > >  5 files changed, 15 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > index a791702b4fee..5af00e072a58 100644
> > > --- a/rust/kernel/lib.rs
> > > +++ b/rust/kernel/lib.rs
> > > @@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
> > >      /// should do.
> > >      ///
> > >      /// Equivalent to the `module_init` macro in the C API.
> > > -    fn init(module: &'static ThisModule) -> error::Result<Self>;
> > > +    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;
> > 
> > Why can't the name come directly from the build system?  Why must it be
> > passed into the init function of the module "class"?  What is it going
> > to do with it?
> 
> The name does come from the build system, that's where `Module::init` gets it
> from.
> 
> > 
> > A PCI, or other bus, driver "knows" it's name already by virtue of the
> > build system, so it can pass that string into whatever function needs
> 
> Let's take pci_register_driver() as example.
> 
> ```
> #define pci_register_driver(driver)		\
> 	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
> ```
> 
> In C drivers this works because (1) it's a macro and (2) it's called directly
> from the driver code.
> 
> In Rust, for very good reasons, we have abstractions for C APIs, hence the
> actual call to __pci_register_driver() does not come from code within the
> module, but from the PCI Rust abstraction `Module::init` calls into instead.

I don't understand those reasons, sorry.

> Consequently, we have to pass things through. This also isn't new, please note
> that the current code already does the same thing: `Module::init` (without this
> patch) is already declared as
> 
> `fn init(module: &'static ThisModule) -> error::Result<Self>`
> passing through `ThisModule` for the exact same reason.

Yeah, and I never quite understood that either :)

> Please also note that in the most common case (one driver per module) we don't
> see any of this anyway.

True, but someone has to review and most importantly, maintain, this
glue code.

> Just like the C macro module_pci_driver(), Rust drivers can use the
> `module_pci_driver!` macro.
> 
> Example from Nova:
> 
> ```
>     kernel::module_pci_driver! {
>         // The driver type that implements the corresponding probe() and
>         // remove() driver callbacks.
>         type: NovaDriver,
>         name: "Nova",
>         author: "Danilo Krummrich",
>         description: "Nova GPU driver",
>         license: "GPL v2",
>     }
> ```

As I said when you implemented this fun macro, don't do this yet.

We added these "helper" macros WAY late in the development cycle of the
driver model, AFTER we were sure we got other parts right.  There is no
need to attempt to implement all of what you can do in C today in Rust,
in fact, I would argue that we don't want to do that, just to make
things simpler to review the glue code, which is the most important part
here to get right!

Changing drivers later, to take advantage of code savings like this
macro can be done then, not just yet.  Let's get this understood and
right first, no need to be tricky or complex.

And, as I hinted at before, I don't think we should be doing this at all
just yet either.  I still think a small "C shim" layer to wrap the
struct pci driver up and pass the calls to the rust portion of the
module is better to start with, it both saves you time and energy so
that you can work on what you really want to do (i.e. a driver in rust)
and not have to worry about the c bindings as that's the "tricky" part
that is stopping you from getting your driver work done.

After all, it's not the pci driver model code that is usually the tricky
bits to verify in C, it's the whole rest of the mess.  Stick with a
small C file, with just the pci driver structure and device ids, and
then instantiate your rust stuff when probe() is called, and clean up
when release() is called.

I can provide an example if needed.

thanks,

greg k-h

Re: [PATCH v2 02/10] rust: implement generic driver registration

[Danilo Krummrich]

On Thu, Jun 20, 2024 at 04:28:23PM +0200, Greg KH wrote:
> On Wed, Jun 19, 2024 at 01:39:48AM +0200, Danilo Krummrich wrote:
> > Implement the generic `Registration` type and the `DriverOps` trait.
> 
> I don't think this is needed, more below...
> 
> > 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.
> 
> But that's not what normally happens, more below...

I can't find below a paragraph that seems related to this, hence I reply here.

The above is just different wording for: A driver is typically registered in
module_init() and unregistered in module_exit().

Isn't that what happens normally?

> 
> > Instead the `DriverOps` trait is provided to subsystems, which have to
> > implement `DriverOps::register` and `DrvierOps::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.
> 
> So you are saying this should be something that a "bus" would do?
> Please be explicit as to what you mean by "subsystem" here.

Yes, I agree it's more precise to say that this should be implemented by a bus
(e.g. PCI). I can reword this one.

> 
> > For instance, the PCI subsystem would call __pci_register_driver() from
> > `DriverOps::register` and pci_unregister_driver() from
> > `DrvierOps::unregister`.
> 
> So this is a BusOps, or more in general, a "subsystem" if it's not a
> bus (i.e. it's a class).  Note, we used to use the term "subsystem" a
> very long time ago but got rid of them in the driver core, let's not
> bring it back unless we REALLY know we want it this time.
> 
> So why isn't this just a BusOps?

I think it's really about perspective. Generally speaking, when a driver is
registered it gets added to a bus through bus_add_driver(). Now, one could argue
that the "register" operation is a bus operation, since something gets
registered on the bus, but one could also argue that it's a driver operation,
since a driver is registered on something.

Consequently, I think it's neither wrong to call this one `BusOps` nor is it
wrong to call it `DriverOps`.

I still think `DriverOps` is more appropriate, since here we're looking at it
from the perspective of the driver.

In the end we call it as `driver.register()` instead of `bus.register()`. For
instance, in the PCI implementation of it, we call __pci_register_driver() from
`DriverOps::register`.

> > This patch is based on previous work from Wedson Almeida Filho.
> > 
> > Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/kernel/driver.rs | 128 ++++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs    |   1 +
> >  2 files changed, 129 insertions(+)
> >  create mode 100644 rust/kernel/driver.rs
> > 
> > diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> > new file mode 100644
> > index 000000000000..e04406b93b56
> > --- /dev/null
> > +++ b/rust/kernel/driver.rs
> > @@ -0,0 +1,128 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.).
> 
> See, you think it's a bus, let's call it a bus!  :)
> 
> > +//!
> > +//! Each bus / subsystem is expected to implement [`DriverOps`], 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 [`DriverOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, Amba,
> > +/// etc.) to privide 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 `DriverOps::register` and
> > +/// `bindings::pci_unregister_driver` from `DriverOps::unregister`.
> > +pub trait DriverOps {
> > +    /// 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.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// `reg` must point to valid, initialised, and writable memory. It may be modified by this
> > +    /// function to hold registration state.
> > +    ///
> > +    /// On success, `reg` must remain pinned and valid until the matching call to
> > +    /// [`DriverOps::unregister`].
> > +    fn register(
> > +        reg: &mut Self::RegType,
> > +        name: &'static CStr,
> > +        module: &'static ThisModule,
> > +    ) -> Result;
> > +
> > +    /// Unregisters a driver previously registered with [`DriverOps::register`].
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// `reg` must point to valid writable memory, initialised by a previous successful call to
> > +    /// [`DriverOps::register`].
> > +    fn unregister(reg: &mut Self::RegType);
> > +}
> 
> So you are getting into what a bus wants/needs to support here, why is
> register/unregister the "big" things to be implemented first?  Why not
> just map the current register/unregister bus functions to a bus-specific
> trait for now?  And then, if you think it really should be generic, we

A bus specific trait would not add any value. The whole point if a trait is to
represent a generic interface. It basically describes the functionality we
expect from a certain category of types.

In this case we know that every driver can be registered and unregistered, hence
we can define a generic trait that every bus specific driver structure, e.g. PCI
driver, has to implement.

> can make it that way then.  I don't see why this needs to be generic now
> as you aren't implementing a bus in rust at this point in time, right?

With the above tait (or interface) we now can have a generic `Registration` that
calls `T::register` and `T::unregister` and works for all driver types (PCI,
platform, etc.). Otherwise we'd need a `pci::Registration`, a
`platform::Registration` etc. and copy-paste the below code for all of them.

> 
> > +
> > +/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
> > +/// `bindings::pci_driver`). Therefore a [`Registration`] is initialized with some type that
> > +/// implements the [`DriverOps`] 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: DriverOps> {
> > +    #[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: DriverOps> 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: DriverOps> Send for Registration<T> {}
> > +
> > +impl<T: DriverOps> Registration<T> {
> > +    /// Creates a new instance of the registration object.
> > +    pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {
> 
> Drivers have modules, not busses.  So you are registering a driver with
> a bus here, it's not something that a driver itself implements as you
> have named here.

We are registering a driver on bus here, see the below `T::register` call, this
one ends up in __pci_register_driver() or __platform_driver_register(), etc. Hence
we need the module and the module name.

Please see the first patch of the series for an explanation why THIS_MODULE and
KBUILD_MODNAME is not in scope here and why we need to pass this through.

> 
> 
> > +        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 { &mut *ptr };
> > +
> > +                T::register(drv, name, module)
> > +            }),
> > +        })
> > +    }
> > +}
> > +
> > +#[pinned_drop]
> > +impl<T: DriverOps> PinnedDrop for Registration<T> {
> > +    fn drop(self: Pin<&mut Self>) {
> > +        let drv = unsafe { &mut *self.reg.get() };
> > +
> > +        T::unregister(drv);
> > +    }
> > +}
> > +
> > +/// A kernel module that only registers the given driver on init.
> > +///
> > +/// This is a helper struct to make it easier to define single-functionality modules, in this case,
> > +/// modules that offer a single driver.
> > +#[pin_data]
> > +pub struct Module<T: DriverOps> {
> > +    #[pin]
> > +    _driver: Registration<T>,
> > +}
> 
> While these are nice, let's not add them just yet, let's keep it simple
> for now until we work out the proper model and see what is, and is not,
> common for drivers to do.

Honestly, even though it seems to be complicated, this is in fact the simple
way. This really is the minimum we can do to register a driver and get it
probed.

Please also consider that all the abstractions I am submitting in this series
are only making use of APIs that regular C drivers use as well. This series
doesn't touch internals of any subsystem.

Hence, we know the model very well. It's the same as in C, we're just
abstracting it into common Rust design patterns.

The only exception might be passing through the module name, but this is just
the consequence of the design and necessity that already has been discussed and
was merged.

> 
> That way we keep the review simpler as well, and saves you time
> rewriting things as we rename / modify stuff.
> 
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -29,6 +29,7 @@
> >  pub mod alloc;
> >  mod build_assert;
> >  pub mod device;
> > +pub mod driver;
> 
> Nope, this is a bus :)
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

On Thu, Jun 20, 2024 at 06:36:08PM +0200, Greg KH wrote:
> On Thu, Jun 20, 2024 at 06:10:05PM +0200, Danilo Krummrich wrote:
> > On Thu, Jun 20, 2024 at 04:19:48PM +0200, Greg KH wrote:
> > > On Wed, Jun 19, 2024 at 01:39:47AM +0200, Danilo Krummrich wrote:
> > > > In a subsequent patch we introduce the `Registration` abstraction used
> > > > to register driver structures. Some subsystems require the module name on
> > > > driver registration (e.g. PCI in __pci_register_driver()), hence pass
> > > > the module name to `Module::init`.
> > > 
> > > I understand the need/want here, but it feels odd that you have to
> > > change anything to do it.
> > > 
> > > > 
> > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > > ---
> > > >  rust/kernel/lib.rs           | 14 ++++++++++----
> > > >  rust/kernel/net/phy.rs       |  2 +-
> > > >  rust/macros/module.rs        |  3 ++-
> > > >  samples/rust/rust_minimal.rs |  2 +-
> > > >  samples/rust/rust_print.rs   |  2 +-
> > > >  5 files changed, 15 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > > index a791702b4fee..5af00e072a58 100644
> > > > --- a/rust/kernel/lib.rs
> > > > +++ b/rust/kernel/lib.rs
> > > > @@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
> > > >      /// should do.
> > > >      ///
> > > >      /// Equivalent to the `module_init` macro in the C API.
> > > > -    fn init(module: &'static ThisModule) -> error::Result<Self>;
> > > > +    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;
> > > 
> > > Why can't the name come directly from the build system?  Why must it be
> > > passed into the init function of the module "class"?  What is it going
> > > to do with it?
> > 
> > The name does come from the build system, that's where `Module::init` gets it
> > from.
> > 
> > > 
> > > A PCI, or other bus, driver "knows" it's name already by virtue of the
> > > build system, so it can pass that string into whatever function needs
> > 
> > Let's take pci_register_driver() as example.
> > 
> > ```
> > #define pci_register_driver(driver)		\
> > 	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
> > ```
> > 
> > In C drivers this works because (1) it's a macro and (2) it's called directly
> > from the driver code.
> > 
> > In Rust, for very good reasons, we have abstractions for C APIs, hence the
> > actual call to __pci_register_driver() does not come from code within the
> > module, but from the PCI Rust abstraction `Module::init` calls into instead.
> 
> I don't understand those reasons, sorry.

Ok, good you point this out. We should definitely discuss this point then and
build some consensus around it.

I propose to focus on this point first and follow up with the discussion of the
rest of the series afterwards.

Let me explain why I am convinced that it's very important to have abstractions
in place in general and from the get-go.

In general, having abstractions for C APIs is the foundation of being able to
make use of a lot of advantages Rust has to offer.

The most obvious one are all the safety aspects. For instance, with an
abstraction we have to get exactly one piece of code right in terms of pointer
validity, lifetimes, type safety, API semantics, etc. and in all other places
(e.g. drivers) we get the compiler to check those things for us through the
abstraction.

Now, the abstraction can be buggy or insufficient and hence there is no absolute
safety guarantee. *But*, if we get this one thing right, there is nothing a
driver can mess up by itself trying to do stupid things anymore.

If we just call the C code instead we have to get it right everywhere instead.

Now, you could approach this top-down instead and argue that we could at least
benefit from Rust for the driver specific parts.

Unfortunately, this doesn't really work out either. Also driver specific code is
typically (very) closely connected to kernel APIs. If you want to use the safety
aspects of Rust for the driver specific parts you inevitably end up writing
abstractions for the C APIs in your driver.

There are basically three options you can end up with:

(1) An abstraction for the C API within your driver that is actually generic
    for every driver, and hence shouldn't be there.
(2) Your driver specific code is full of raw pointers and `unsafe {}` calls,
    which in the end just means that you ended up baking the abstraction into
    your driver specific code.
(3) You ignore everything, put everything in a huge `unsafe {}` block and
    compile C code with the Rust compiler. (Admittedly, maybe slightly
    overstated, but not that far off either.)

The latter is also the reason why it doesn't make sense to only have
abstractions for some things, but not for other.

If an abstraction for B is based on A, but we don't start with A, then B ends up
implementing (at least partially) the abstraction for A as well. For instance,
if we don't implement `driver::Registration` then the PCI abstractions (and
platform, usb, etc.) have to implement it.

It really comes down to the point that it just bubbles up. We really have to do
this bottom-up, otherwise we just end up moving those abstractions up, layer by
layer, where they don't belong to and we re-implement them over and over again.

> 
> > Consequently, we have to pass things through. This also isn't new, please note
> > that the current code already does the same thing: `Module::init` (without this
> > patch) is already declared as
> > 
> > `fn init(module: &'static ThisModule) -> error::Result<Self>`
> > passing through `ThisModule` for the exact same reason.
> 
> Yeah, and I never quite understood that either :)

Since commit 247b365dc8dc ("rust: add `kernel` crate") shows me your RB for
that, am I good to assume that this one isn't a blocker?

> 
> > Please also note that in the most common case (one driver per module) we don't
> > see any of this anyway.
> 
> True, but someone has to review and most importantly, maintain, this
> glue code.
> 
> > Just like the C macro module_pci_driver(), Rust drivers can use the
> > `module_pci_driver!` macro.
> > 
> > Example from Nova:
> > 
> > ```
> >     kernel::module_pci_driver! {
> >         // The driver type that implements the corresponding probe() and
> >         // remove() driver callbacks.
> >         type: NovaDriver,
> >         name: "Nova",
> >         author: "Danilo Krummrich",
> >         description: "Nova GPU driver",
> >         license: "GPL v2",
> >     }
> > ```
> 
> As I said when you implemented this fun macro, don't do this yet.
> 
> We added these "helper" macros WAY late in the development cycle of the
> driver model, AFTER we were sure we got other parts right.  There is no
> need to attempt to implement all of what you can do in C today in Rust,
> in fact, I would argue that we don't want to do that, just to make
> things simpler to review the glue code, which is the most important part
> here to get right!

We're not reinventing the wheel here, we stick to the architecture the kernel
already has.

However, I understand that not starting with this macro directly makes it easier
for you to see what's going on. I can introduce the macro in a separate patch to
make it more obvious what's going on.

> 
> Changing drivers later, to take advantage of code savings like this
> macro can be done then, not just yet.  Let's get this understood and
> right first, no need to be tricky or complex.
> 
> And, as I hinted at before, I don't think we should be doing this at all
> just yet either.  I still think a small "C shim" layer to wrap the
> struct pci driver up and pass the calls to the rust portion of the
> module is better to start with, it both saves you time and energy so
> that you can work on what you really want to do (i.e. a driver in rust)
> and not have to worry about the c bindings as that's the "tricky" part
> that is stopping you from getting your driver work done.

I strongly disagree here. As explained above, it just bubbles up, this approach
would just make me end up having to write a lot of the code from the
abstractions in the driver.

However, it would indeed safe me time and energy to do just that. But that isn't
really what I want. I also don't want to write a driver in Rust *only*.

And I also don't really think that you want people, who commit to work hard to
get things right, to just take the shortcut and create some random mess buried
in a driver. :)

What I actually want is to get to a solid foundation for Rust drivers in
general, since I'm convinced that this provides a lot of value beyond the scope
of a single driver.

Since you've brought the topic up a few times, I am also willing to maintain
those abstractions if this is a concern. Maybe one day the corresponding
maintainers are comfortable enough and this isn't needed anymore, but at least
until then, I'm happy to help out.

> 
> After all, it's not the pci driver model code that is usually the tricky
> bits to verify in C, it's the whole rest of the mess.  Stick with a
> small C file, with just the pci driver structure and device ids, and
> then instantiate your rust stuff when probe() is called, and clean up
> when release() is called.

Again, this really bubbles up.

What do we pass to Rust probe() function? A raw struct pci_dev pointer or the
proper PCI device abstraction?

How do we implement I/O memory mappings for PCI bars without PCI / I/O
abstraction?

How do we perform (boundary checked) I/O memory reads / writes without `Io`
abstraction?

How do we handle the lifetime of resources without `Devres` abstraction?

How do we (properly) implement a DRM device registration abstraction without
`Devres`?

Luckily we already got the `Firmware` and `Device` abstraction in place. :)

> 
> I can provide an example if needed.

If you do so, please don't stop at the probe() boundary, please continue to the
point where the Nova stub driver is. It really does just enough to show / proove
that the abstractions tie together nicely. But it should be enough to see that
you end up with either (1), (2) or (3) from above.

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v2 07/10] rust: add `io::Io` base type

[Philipp Stanner]

On Thu, 2024-06-20 at 16:53 +0200, Greg KH wrote:
> On Wed, Jun 19, 2024 at 01:39:53AM +0200, 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.
> 
> Why not?

Because you'd then up reimplementing all that logic that the C code
already provides. In the worst case that could lead to you effectively
reimplemting the subsystem instead of wrapping it. And that's obviously
uncool because you'd then have two of them (besides, the community in
general rightfully pushes back against reimplementing stuff; see the
attempts to provide redundant Rust drivers in the past).

The C code already takes care of figuring out region ranges and all
that, and it's battle hardened.

The main point of Rust is to make things safer; so if that can be
achieved without rewrite, as is the case with the presented container
solution, that's the way to go.

> 
> > Instead, implement a base type for I/O mapped memory, which
> > generically
> > provides the corresponding accessors, such as `Io::readb` or
> > `Io:try_readb`.
> 
> It provides a subset of the existing accessors, one you might want to
> trim down for now, see below...
> 
> > +/* io.h */
> > +u8 rust_helper_readb(const volatile void __iomem *addr)
> > +{
> > +       return readb(addr);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_readb);
> 
> <snip>
> 
> You provide wrappers for a subset of what io.h provides, why that
> specific subset?
> 
> Why not just add what you need, when you need it?  I doubt you need
> all
> of these, and odds are you will need more.
> 

That was written by me as a first play set to test. Nova itself
currently reads only 8 byte from a PCI BAR, so we could indeed drop
everything but readq() for now and add things subsequently later, as
you suggest.



> > +u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
> > +{
> > +       return readl_relaxed(addr);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_readl_relaxed);
> 
> I know everyone complains about wrapper functions around inline
> functions, so I'll just say it again, this is horrid.  And it's going
> to
> hurt performance, so any rust code people write is not on a level
> playing field here.
> 
> Your call, but ick...

Well, can anyone think of another way to do it?

> 
> > +#ifdef CONFIG_64BIT
> > +u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
> > +{
> > +       return readq_relaxed(addr);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_readq_relaxed);
> > +#endif
> 
> Rust works on 32bit targets in the kernel now?

Ahm, afaik not. That's some relic. Let's address that with your subset
comment from above.

> 
> > +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.
> 
> offsets aren't know at compile time for many implementations, as it
> could be a dynamically allocated memory range.  How is this going to
> work for that?  Heck, how does this work for DT-defined memory ranges
> today?

The macro below will take care of those where it's only knowable at
runtime I think.

Rust has this feature (called "const generic") that can be used for
APIs where ranges which are known at compile time, so the compiler can
check all the parameters at that point. That has been judged to be
positive because errors with the range handling become visible before
the kernel runs and because it gives some performance advantages.


P.

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v2 07/10] rust: add `io::Io` base type

[Danilo Krummrich]

On Fri, Jun 21, 2024 at 11:43:34AM +0200, Philipp Stanner wrote:

Please find a few additions below.

But as mentioned, please let us sort out [1] first.

[1] https://lore.kernel.org/lkml/ZnSeAZu3IMA4fR8P@cassiopeiae/

> On Thu, 2024-06-20 at 16:53 +0200, Greg KH wrote:
> > On Wed, Jun 19, 2024 at 01:39:53AM +0200, 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.
> > 
> > Why not?
> 
> Because you'd then up reimplementing all that logic that the C code
> already provides. In the worst case that could lead to you effectively
> reimplemting the subsystem instead of wrapping it. And that's obviously
> uncool because you'd then have two of them (besides, the community in
> general rightfully pushes back against reimplementing stuff; see the
> attempts to provide redundant Rust drivers in the past).
> 
> The C code already takes care of figuring out region ranges and all
> that, and it's battle hardened.

To add an example, instead of reimplementing things like pci_iomap() we use
`Io` as base type providing the accrssors like readl() and let the resource
implement the mapping parts, such as `pci::Bar`.

> 
> The main point of Rust is to make things safer; so if that can be
> achieved without rewrite, as is the case with the presented container
> solution, that's the way to go.
> 
> > 
> > > Instead, implement a base type for I/O mapped memory, which
> > > generically
> > > provides the corresponding accessors, such as `Io::readb` or
> > > `Io:try_readb`.
> > 
> > It provides a subset of the existing accessors, one you might want to
> > trim down for now, see below...
> > 
> > > +/* io.h */
> > > +u8 rust_helper_readb(const volatile void __iomem *addr)
> > > +{
> > > +       return readb(addr);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_readb);
> > 
> > <snip>
> > 
> > You provide wrappers for a subset of what io.h provides, why that
> > specific subset?
> > 
> > Why not just add what you need, when you need it?  I doubt you need
> > all
> > of these, and odds are you will need more.
> > 
> 
> That was written by me as a first play set to test. Nova itself
> currently reads only 8 byte from a PCI BAR, so we could indeed drop
> everything but readq() for now and add things subsequently later, as
> you suggest.

I think it is reasonable to start with the most common accessors
{read,write}{b,w,l,q and maybe their relaxed variants.

We generate them through the `define_read!` and `define_write!` macros anyways
and the only difference between all the variants is only the size type (u8, u16,
etc.) we pass to the macro.

> 
> 
> 
> > > +u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
> > > +{
> > > +       return readl_relaxed(addr);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_readl_relaxed);
> > 
> > I know everyone complains about wrapper functions around inline
> > functions, so I'll just say it again, this is horrid.  And it's going
> > to
> > hurt performance, so any rust code people write is not on a level
> > playing field here.
> > 
> > Your call, but ick...
> 
> Well, can anyone think of another way to do it?
> 
> > 
> > > +#ifdef CONFIG_64BIT
> > > +u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
> > > +{
> > > +       return readq_relaxed(addr);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_readq_relaxed);
> > > +#endif
> > 
> > Rust works on 32bit targets in the kernel now?
> 
> Ahm, afaik not. That's some relic. Let's address that with your subset
> comment from above.

I think we should keep this guard; readq() implementations in the arch code have
this guard as well.

Should we ever add a 32bit target for Rust we also don't want this to break.

> 
> > 
> > > +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.
> > 
> > offsets aren't know at compile time for many implementations, as it
> > could be a dynamically allocated memory range.  How is this going to
> > work for that?  Heck, how does this work for DT-defined memory ranges
> > today?
> 
> The macro below will take care of those where it's only knowable at
> runtime I think.
> 
> Rust has this feature (called "const generic") that can be used for
> APIs where ranges which are known at compile time, so the compiler can
> check all the parameters at that point. That has been judged to be
> positive because errors with the range handling become visible before
> the kernel runs and because it gives some performance advantages.

Let's add an exammple based on `pci::Bar` here.

As a driver you can optionally map a `pci::Bar` with an additional `SIZE`
constant, e.g.

```
let bar = pdev.iomap_region_sized::<0x1000>(0)?;
```

This call only succeeds of the actual bar size is *at least* 4k. Subsequent
calls to, let's say, `bar.readl(0x10)` can boundary check things on compile
time, such that `bar.readl(0x1000)` would fail on compile time.

This is useful when a driver knows the minum required / expected size of this
memory region.

Alternatively, a driver cann always fall back to a runtime check, e.g.

```
let bar = pdev.iomap_region(0)?;
let val = bar.try_readl(0x1000)?;
```

- Danilo

> 
> 
> P.
> 
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 

Re: [PATCH v2 09/10] rust: pci: add basic PCI device / driver abstractions

[Andreas Hindborg]

Hi Danilo,

Thanks for working on this. I just finished rebasing the Rust NVMe
driver on these patches, and I have just one observation for now.

Danilo Krummrich <dakr@redhat.com> writes:

[...]

> +pub trait Driver {
> +    /// Data stored on device by driver.
> +    ///
> +    /// Corresponds to the data set or retrieved via the kernel's
> +    /// `pci_{set,get}_drvdata()` functions.
> +    ///
> +    /// Require that `Data` implements `ForeignOwnable`. We guarantee to
> +    /// never move the underlying wrapped data structure.
> +    ///
> +    /// TODO: Use associated_type_defaults once stabilized:
> +    ///
> +    /// `type Data: ForeignOwnable = ();`
> +    type Data: ForeignOwnable;
> +
> +    /// 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<'static, DeviceId, 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: Option<&Self::IdInfo>) -> Result<Self::Data>;

Since you changed the `Device` representation to be basically an `ARef`,
the `&mut` makes no sense. I think we should either pass by value or
immutable reference.


Best regards,
Andreas


> +
> +    /// PCI driver remove.
> +    ///
> +    /// Called when a platform device is removed.
> +    /// Implementers should prepare the device for complete removal here.
> +    fn remove(data: &Self::Data);
> +}
> +
> +/// 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>);
> +

Re: [PATCH v2 07/10] rust: add `io::Io` base type

[Andreas Hindborg]

Hi Danilo,

Danilo Krummrich <dakr@redhat.com> writes:

[...]

> +
> +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);
> +
> +            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)?;
> +
> +            unsafe { bindings::$name(value, addr as _) }
> +            Ok(())
> +        }
> +    };
> +}
> +

I am curious why we do not need `&mut self` to write to this memory? Is
it OK to race on these writes?


Best regards,
Andreas

Re: [PATCH v2 07/10] rust: add `io::Io` base type

[Danilo Krummrich]

On Tue, Jun 25, 2024 at 12:59:24PM +0200, Andreas Hindborg wrote:
> Hi Danilo,
> 
> Danilo Krummrich <dakr@redhat.com> writes:
> 
> [...]
> 
> > +
> > +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);
> > +
> > +            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)?;
> > +
> > +            unsafe { bindings::$name(value, addr as _) }
> > +            Ok(())
> > +        }
> > +    };
> > +}
> > +
> 
> I am curious why we do not need `&mut self` to write to this memory? Is
> it OK to race on these writes?

Yes, concurrent writes to the same I/O mapped memory region (within the same
driver) should be totally fine.

In the end it's only the driver that can know about (and has to ensure) the
semantics, concurrency and ordering of those writes.

> 
> 
> Best regards,
> Andreas
> 

Re: [PATCH v2 09/10] rust: pci: add basic PCI device / driver abstractions

[Danilo Krummrich]

On Tue, Jun 25, 2024 at 12:53:48PM +0200, Andreas Hindborg wrote:
> Hi Danilo,
> 
> Thanks for working on this. I just finished rebasing the Rust NVMe
> driver on these patches, and I have just one observation for now.
> 
> Danilo Krummrich <dakr@redhat.com> writes:
> 
> [...]
> 
> > +pub trait Driver {
> > +    /// Data stored on device by driver.
> > +    ///
> > +    /// Corresponds to the data set or retrieved via the kernel's
> > +    /// `pci_{set,get}_drvdata()` functions.
> > +    ///
> > +    /// Require that `Data` implements `ForeignOwnable`. We guarantee to
> > +    /// never move the underlying wrapped data structure.
> > +    ///
> > +    /// TODO: Use associated_type_defaults once stabilized:
> > +    ///
> > +    /// `type Data: ForeignOwnable = ();`
> > +    type Data: ForeignOwnable;
> > +
> > +    /// 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<'static, DeviceId, 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: Option<&Self::IdInfo>) -> Result<Self::Data>;
> 
> Since you changed the `Device` representation to be basically an `ARef`,
> the `&mut` makes no sense. I think we should either pass by value or
> immutable reference.

Agreed, I think we should just pass it by value.

I also noticed that I need to fix `set_master` and `enable_device_mem` to
require a mutable reference.

> 
> 
> Best regards,
> Andreas
> 
> 
> > +
> > +    /// PCI driver remove.
> > +    ///
> > +    /// Called when a platform device is removed.
> > +    /// Implementers should prepare the device for complete removal here.
> > +    fn remove(data: &Self::Data);
> > +}
> > +
> > +/// 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>);
> > +
> 

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

Hi Greg,

On 6/20/24 23:24, Danilo Krummrich wrote:

This is a polite reminder about the below discussion.

> On Thu, Jun 20, 2024 at 06:36:08PM +0200, Greg KH wrote:
>> On Thu, Jun 20, 2024 at 06:10:05PM +0200, Danilo Krummrich wrote:
>>> On Thu, Jun 20, 2024 at 04:19:48PM +0200, Greg KH wrote:
>>>> On Wed, Jun 19, 2024 at 01:39:47AM +0200, Danilo Krummrich wrote:
>>>>> In a subsequent patch we introduce the `Registration` abstraction used
>>>>> to register driver structures. Some subsystems require the module name on
>>>>> driver registration (e.g. PCI in __pci_register_driver()), hence pass
>>>>> the module name to `Module::init`.
>>>>
>>>> I understand the need/want here, but it feels odd that you have to
>>>> change anything to do it.
>>>>
>>>>>
>>>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>>>>> ---
>>>>>   rust/kernel/lib.rs           | 14 ++++++++++----
>>>>>   rust/kernel/net/phy.rs       |  2 +-
>>>>>   rust/macros/module.rs        |  3 ++-
>>>>>   samples/rust/rust_minimal.rs |  2 +-
>>>>>   samples/rust/rust_print.rs   |  2 +-
>>>>>   5 files changed, 15 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>>>> index a791702b4fee..5af00e072a58 100644
>>>>> --- a/rust/kernel/lib.rs
>>>>> +++ b/rust/kernel/lib.rs
>>>>> @@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
>>>>>       /// should do.
>>>>>       ///
>>>>>       /// Equivalent to the `module_init` macro in the C API.
>>>>> -    fn init(module: &'static ThisModule) -> error::Result<Self>;
>>>>> +    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;
>>>>
>>>> Why can't the name come directly from the build system?  Why must it be
>>>> passed into the init function of the module "class"?  What is it going
>>>> to do with it?
>>>
>>> The name does come from the build system, that's where `Module::init` gets it
>>> from.
>>>
>>>>
>>>> A PCI, or other bus, driver "knows" it's name already by virtue of the
>>>> build system, so it can pass that string into whatever function needs
>>>
>>> Let's take pci_register_driver() as example.
>>>
>>> ```
>>> #define pci_register_driver(driver)		\
>>> 	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
>>> ```
>>>
>>> In C drivers this works because (1) it's a macro and (2) it's called directly
>>> from the driver code.
>>>
>>> In Rust, for very good reasons, we have abstractions for C APIs, hence the
>>> actual call to __pci_register_driver() does not come from code within the
>>> module, but from the PCI Rust abstraction `Module::init` calls into instead.
>>
>> I don't understand those reasons, sorry.
> 
> Ok, good you point this out. We should definitely discuss this point then and
> build some consensus around it.
> 
> I propose to focus on this point first and follow up with the discussion of the
> rest of the series afterwards.
> 
> Let me explain why I am convinced that it's very important to have abstractions
> in place in general and from the get-go.
> 
> In general, having abstractions for C APIs is the foundation of being able to
> make use of a lot of advantages Rust has to offer.
> 
> The most obvious one are all the safety aspects. For instance, with an
> abstraction we have to get exactly one piece of code right in terms of pointer
> validity, lifetimes, type safety, API semantics, etc. and in all other places
> (e.g. drivers) we get the compiler to check those things for us through the
> abstraction.
> 
> Now, the abstraction can be buggy or insufficient and hence there is no absolute
> safety guarantee. *But*, if we get this one thing right, there is nothing a
> driver can mess up by itself trying to do stupid things anymore.
> 
> If we just call the C code instead we have to get it right everywhere instead.
> 
> Now, you could approach this top-down instead and argue that we could at least
> benefit from Rust for the driver specific parts.
> 
> Unfortunately, this doesn't really work out either. Also driver specific code is
> typically (very) closely connected to kernel APIs. If you want to use the safety
> aspects of Rust for the driver specific parts you inevitably end up writing
> abstractions for the C APIs in your driver.
> 
> There are basically three options you can end up with:
> 
> (1) An abstraction for the C API within your driver that is actually generic
>      for every driver, and hence shouldn't be there.
> (2) Your driver specific code is full of raw pointers and `unsafe {}` calls,
>      which in the end just means that you ended up baking the abstraction into
>      your driver specific code.
> (3) You ignore everything, put everything in a huge `unsafe {}` block and
>      compile C code with the Rust compiler. (Admittedly, maybe slightly
>      overstated, but not that far off either.)
> 
> The latter is also the reason why it doesn't make sense to only have
> abstractions for some things, but not for other.
> 
> If an abstraction for B is based on A, but we don't start with A, then B ends up
> implementing (at least partially) the abstraction for A as well. For instance,
> if we don't implement `driver::Registration` then the PCI abstractions (and
> platform, usb, etc.) have to implement it.
> 
> It really comes down to the point that it just bubbles up. We really have to do
> this bottom-up, otherwise we just end up moving those abstractions up, layer by
> layer, where they don't belong to and we re-implement them over and over again.
> 
>>
>>> Consequently, we have to pass things through. This also isn't new, please note
>>> that the current code already does the same thing: `Module::init` (without this
>>> patch) is already declared as
>>>
>>> `fn init(module: &'static ThisModule) -> error::Result<Self>`
>>> passing through `ThisModule` for the exact same reason.
>>
>> Yeah, and I never quite understood that either :)
> 
> Since commit 247b365dc8dc ("rust: add `kernel` crate") shows me your RB for
> that, am I good to assume that this one isn't a blocker?
> 
>>
>>> Please also note that in the most common case (one driver per module) we don't
>>> see any of this anyway.
>>
>> True, but someone has to review and most importantly, maintain, this
>> glue code.
>>
>>> Just like the C macro module_pci_driver(), Rust drivers can use the
>>> `module_pci_driver!` macro.
>>>
>>> Example from Nova:
>>>
>>> ```
>>>      kernel::module_pci_driver! {
>>>          // The driver type that implements the corresponding probe() and
>>>          // remove() driver callbacks.
>>>          type: NovaDriver,
>>>          name: "Nova",
>>>          author: "Danilo Krummrich",
>>>          description: "Nova GPU driver",
>>>          license: "GPL v2",
>>>      }
>>> ```
>>
>> As I said when you implemented this fun macro, don't do this yet.
>>
>> We added these "helper" macros WAY late in the development cycle of the
>> driver model, AFTER we were sure we got other parts right.  There is no
>> need to attempt to implement all of what you can do in C today in Rust,
>> in fact, I would argue that we don't want to do that, just to make
>> things simpler to review the glue code, which is the most important part
>> here to get right!
> 
> We're not reinventing the wheel here, we stick to the architecture the kernel
> already has.
> 
> However, I understand that not starting with this macro directly makes it easier
> for you to see what's going on. I can introduce the macro in a separate patch to
> make it more obvious what's going on.
> 
>>
>> Changing drivers later, to take advantage of code savings like this
>> macro can be done then, not just yet.  Let's get this understood and
>> right first, no need to be tricky or complex.
>>
>> And, as I hinted at before, I don't think we should be doing this at all
>> just yet either.  I still think a small "C shim" layer to wrap the
>> struct pci driver up and pass the calls to the rust portion of the
>> module is better to start with, it both saves you time and energy so
>> that you can work on what you really want to do (i.e. a driver in rust)
>> and not have to worry about the c bindings as that's the "tricky" part
>> that is stopping you from getting your driver work done.
> 
> I strongly disagree here. As explained above, it just bubbles up, this approach
> would just make me end up having to write a lot of the code from the
> abstractions in the driver.
> 
> However, it would indeed safe me time and energy to do just that. But that isn't
> really what I want. I also don't want to write a driver in Rust *only*.
> 
> And I also don't really think that you want people, who commit to work hard to
> get things right, to just take the shortcut and create some random mess buried
> in a driver. :)
> 
> What I actually want is to get to a solid foundation for Rust drivers in
> general, since I'm convinced that this provides a lot of value beyond the scope
> of a single driver.
> 
> Since you've brought the topic up a few times, I am also willing to maintain
> those abstractions if this is a concern. Maybe one day the corresponding
> maintainers are comfortable enough and this isn't needed anymore, but at least
> until then, I'm happy to help out.
> 
>>
>> After all, it's not the pci driver model code that is usually the tricky
>> bits to verify in C, it's the whole rest of the mess.  Stick with a
>> small C file, with just the pci driver structure and device ids, and
>> then instantiate your rust stuff when probe() is called, and clean up
>> when release() is called.
> 
> Again, this really bubbles up.
> 
> What do we pass to Rust probe() function? A raw struct pci_dev pointer or the
> proper PCI device abstraction?
> 
> How do we implement I/O memory mappings for PCI bars without PCI / I/O
> abstraction?
> 
> How do we perform (boundary checked) I/O memory reads / writes without `Io`
> abstraction?
> 
> How do we handle the lifetime of resources without `Devres` abstraction?
> 
> How do we (properly) implement a DRM device registration abstraction without
> `Devres`?
> 
> Luckily we already got the `Firmware` and `Device` abstraction in place. :)
> 
>>
>> I can provide an example if needed.
> 
> If you do so, please don't stop at the probe() boundary, please continue to the
> point where the Nova stub driver is. It really does just enough to show / proove
> that the abstractions tie together nicely. But it should be enough to see that
> you end up with either (1), (2) or (3) from above.
> 
>>
>> thanks,
>>
>> greg k-h
>>

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Greg KH]

On Wed, Jun 26, 2024 at 12:29:01PM +0200, Danilo Krummrich wrote:
> Hi Greg,
> 
> On 6/20/24 23:24, Danilo Krummrich wrote:
> 
> This is a polite reminder about the below discussion.

I know, it's on my TODO list, but I'm currently traveling for a
conference and will not have time until next week to even consider
looking at this...

greg k-h

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

On Thu, Jun 27, 2024 at 09:33:39AM +0200, Greg KH wrote:
> On Wed, Jun 26, 2024 at 12:29:01PM +0200, Danilo Krummrich wrote:
> > Hi Greg,
> > 
> > On 6/20/24 23:24, Danilo Krummrich wrote:
> > 
> > This is a polite reminder about the below discussion.
> 
> I know, it's on my TODO list, but I'm currently traveling for a
> conference and will not have time until next week to even consider
> looking at this...

Thanks, Greg, for letting me know. Let's continue this discussion next week
then.

I may send replies on all other mails of this series in order to not leave them
unreplied for too long, but as mentioned, let's get this thread discussed first
once you're able to get back to it.

Have a good trip!

- Danilo

> 
> greg k-h
> 

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

On Thu, Jun 20, 2024 at 11:24:17PM +0200, Danilo Krummrich wrote:

Just another kind reminder on this one. :)

> On Thu, Jun 20, 2024 at 06:36:08PM +0200, Greg KH wrote:
> > On Thu, Jun 20, 2024 at 06:10:05PM +0200, Danilo Krummrich wrote:
> > > On Thu, Jun 20, 2024 at 04:19:48PM +0200, Greg KH wrote:
> > > > On Wed, Jun 19, 2024 at 01:39:47AM +0200, Danilo Krummrich wrote:
> > > > > In a subsequent patch we introduce the `Registration` abstraction used
> > > > > to register driver structures. Some subsystems require the module name on
> > > > > driver registration (e.g. PCI in __pci_register_driver()), hence pass
> > > > > the module name to `Module::init`.
> > > > 
> > > > I understand the need/want here, but it feels odd that you have to
> > > > change anything to do it.
> > > > 
> > > > > 
> > > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > > > ---
> > > > >  rust/kernel/lib.rs           | 14 ++++++++++----
> > > > >  rust/kernel/net/phy.rs       |  2 +-
> > > > >  rust/macros/module.rs        |  3 ++-
> > > > >  samples/rust/rust_minimal.rs |  2 +-
> > > > >  samples/rust/rust_print.rs   |  2 +-
> > > > >  5 files changed, 15 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > > > index a791702b4fee..5af00e072a58 100644
> > > > > --- a/rust/kernel/lib.rs
> > > > > +++ b/rust/kernel/lib.rs
> > > > > @@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
> > > > >      /// should do.
> > > > >      ///
> > > > >      /// Equivalent to the `module_init` macro in the C API.
> > > > > -    fn init(module: &'static ThisModule) -> error::Result<Self>;
> > > > > +    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;
> > > > 
> > > > Why can't the name come directly from the build system?  Why must it be
> > > > passed into the init function of the module "class"?  What is it going
> > > > to do with it?
> > > 
> > > The name does come from the build system, that's where `Module::init` gets it
> > > from.
> > > 
> > > > 
> > > > A PCI, or other bus, driver "knows" it's name already by virtue of the
> > > > build system, so it can pass that string into whatever function needs
> > > 
> > > Let's take pci_register_driver() as example.
> > > 
> > > ```
> > > #define pci_register_driver(driver)		\
> > > 	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
> > > ```
> > > 
> > > In C drivers this works because (1) it's a macro and (2) it's called directly
> > > from the driver code.
> > > 
> > > In Rust, for very good reasons, we have abstractions for C APIs, hence the
> > > actual call to __pci_register_driver() does not come from code within the
> > > module, but from the PCI Rust abstraction `Module::init` calls into instead.
> > 
> > I don't understand those reasons, sorry.
> 
> Ok, good you point this out. We should definitely discuss this point then and
> build some consensus around it.
> 
> I propose to focus on this point first and follow up with the discussion of the
> rest of the series afterwards.
> 
> Let me explain why I am convinced that it's very important to have abstractions
> in place in general and from the get-go.
> 
> In general, having abstractions for C APIs is the foundation of being able to
> make use of a lot of advantages Rust has to offer.
> 
> The most obvious one are all the safety aspects. For instance, with an
> abstraction we have to get exactly one piece of code right in terms of pointer
> validity, lifetimes, type safety, API semantics, etc. and in all other places
> (e.g. drivers) we get the compiler to check those things for us through the
> abstraction.
> 
> Now, the abstraction can be buggy or insufficient and hence there is no absolute
> safety guarantee. *But*, if we get this one thing right, there is nothing a
> driver can mess up by itself trying to do stupid things anymore.
> 
> If we just call the C code instead we have to get it right everywhere instead.
> 
> Now, you could approach this top-down instead and argue that we could at least
> benefit from Rust for the driver specific parts.
> 
> Unfortunately, this doesn't really work out either. Also driver specific code is
> typically (very) closely connected to kernel APIs. If you want to use the safety
> aspects of Rust for the driver specific parts you inevitably end up writing
> abstractions for the C APIs in your driver.
> 
> There are basically three options you can end up with:
> 
> (1) An abstraction for the C API within your driver that is actually generic
>     for every driver, and hence shouldn't be there.
> (2) Your driver specific code is full of raw pointers and `unsafe {}` calls,
>     which in the end just means that you ended up baking the abstraction into
>     your driver specific code.
> (3) You ignore everything, put everything in a huge `unsafe {}` block and
>     compile C code with the Rust compiler. (Admittedly, maybe slightly
>     overstated, but not that far off either.)
> 
> The latter is also the reason why it doesn't make sense to only have
> abstractions for some things, but not for other.
> 
> If an abstraction for B is based on A, but we don't start with A, then B ends up
> implementing (at least partially) the abstraction for A as well. For instance,
> if we don't implement `driver::Registration` then the PCI abstractions (and
> platform, usb, etc.) have to implement it.
> 
> It really comes down to the point that it just bubbles up. We really have to do
> this bottom-up, otherwise we just end up moving those abstractions up, layer by
> layer, where they don't belong to and we re-implement them over and over again.
> 
> > 
> > > Consequently, we have to pass things through. This also isn't new, please note
> > > that the current code already does the same thing: `Module::init` (without this
> > > patch) is already declared as
> > > 
> > > `fn init(module: &'static ThisModule) -> error::Result<Self>`
> > > passing through `ThisModule` for the exact same reason.
> > 
> > Yeah, and I never quite understood that either :)
> 
> Since commit 247b365dc8dc ("rust: add `kernel` crate") shows me your RB for
> that, am I good to assume that this one isn't a blocker?
> 
> > 
> > > Please also note that in the most common case (one driver per module) we don't
> > > see any of this anyway.
> > 
> > True, but someone has to review and most importantly, maintain, this
> > glue code.
> > 
> > > Just like the C macro module_pci_driver(), Rust drivers can use the
> > > `module_pci_driver!` macro.
> > > 
> > > Example from Nova:
> > > 
> > > ```
> > >     kernel::module_pci_driver! {
> > >         // The driver type that implements the corresponding probe() and
> > >         // remove() driver callbacks.
> > >         type: NovaDriver,
> > >         name: "Nova",
> > >         author: "Danilo Krummrich",
> > >         description: "Nova GPU driver",
> > >         license: "GPL v2",
> > >     }
> > > ```
> > 
> > As I said when you implemented this fun macro, don't do this yet.
> > 
> > We added these "helper" macros WAY late in the development cycle of the
> > driver model, AFTER we were sure we got other parts right.  There is no
> > need to attempt to implement all of what you can do in C today in Rust,
> > in fact, I would argue that we don't want to do that, just to make
> > things simpler to review the glue code, which is the most important part
> > here to get right!
> 
> We're not reinventing the wheel here, we stick to the architecture the kernel
> already has.
> 
> However, I understand that not starting with this macro directly makes it easier
> for you to see what's going on. I can introduce the macro in a separate patch to
> make it more obvious what's going on.
> 
> > 
> > Changing drivers later, to take advantage of code savings like this
> > macro can be done then, not just yet.  Let's get this understood and
> > right first, no need to be tricky or complex.
> > 
> > And, as I hinted at before, I don't think we should be doing this at all
> > just yet either.  I still think a small "C shim" layer to wrap the
> > struct pci driver up and pass the calls to the rust portion of the
> > module is better to start with, it both saves you time and energy so
> > that you can work on what you really want to do (i.e. a driver in rust)
> > and not have to worry about the c bindings as that's the "tricky" part
> > that is stopping you from getting your driver work done.
> 
> I strongly disagree here. As explained above, it just bubbles up, this approach
> would just make me end up having to write a lot of the code from the
> abstractions in the driver.
> 
> However, it would indeed safe me time and energy to do just that. But that isn't
> really what I want. I also don't want to write a driver in Rust *only*.
> 
> And I also don't really think that you want people, who commit to work hard to
> get things right, to just take the shortcut and create some random mess buried
> in a driver. :)
> 
> What I actually want is to get to a solid foundation for Rust drivers in
> general, since I'm convinced that this provides a lot of value beyond the scope
> of a single driver.
> 
> Since you've brought the topic up a few times, I am also willing to maintain
> those abstractions if this is a concern. Maybe one day the corresponding
> maintainers are comfortable enough and this isn't needed anymore, but at least
> until then, I'm happy to help out.
> 
> > 
> > After all, it's not the pci driver model code that is usually the tricky
> > bits to verify in C, it's the whole rest of the mess.  Stick with a
> > small C file, with just the pci driver structure and device ids, and
> > then instantiate your rust stuff when probe() is called, and clean up
> > when release() is called.
> 
> Again, this really bubbles up.
> 
> What do we pass to Rust probe() function? A raw struct pci_dev pointer or the
> proper PCI device abstraction?
> 
> How do we implement I/O memory mappings for PCI bars without PCI / I/O
> abstraction?
> 
> How do we perform (boundary checked) I/O memory reads / writes without `Io`
> abstraction?
> 
> How do we handle the lifetime of resources without `Devres` abstraction?
> 
> How do we (properly) implement a DRM device registration abstraction without
> `Devres`?
> 
> Luckily we already got the `Firmware` and `Device` abstraction in place. :)
> 
> > 
> > I can provide an example if needed.
> 
> If you do so, please don't stop at the probe() boundary, please continue to the
> point where the Nova stub driver is. It really does just enough to show / proove
> that the abstractions tie together nicely. But it should be enough to see that
> you end up with either (1), (2) or (3) from above.
> 
> > 
> > thanks,
> > 
> > greg k-h
> > 

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Greg KH]

On Thu, Jun 20, 2024 at 11:24:17PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 20, 2024 at 06:36:08PM +0200, Greg KH wrote:
> > On Thu, Jun 20, 2024 at 06:10:05PM +0200, Danilo Krummrich wrote:
> > > On Thu, Jun 20, 2024 at 04:19:48PM +0200, Greg KH wrote:
> > > > On Wed, Jun 19, 2024 at 01:39:47AM +0200, Danilo Krummrich wrote:
> > > > > In a subsequent patch we introduce the `Registration` abstraction used
> > > > > to register driver structures. Some subsystems require the module name on
> > > > > driver registration (e.g. PCI in __pci_register_driver()), hence pass
> > > > > the module name to `Module::init`.
> > > > 
> > > > I understand the need/want here, but it feels odd that you have to
> > > > change anything to do it.
> > > > 
> > > > > 
> > > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > > > ---
> > > > >  rust/kernel/lib.rs           | 14 ++++++++++----
> > > > >  rust/kernel/net/phy.rs       |  2 +-
> > > > >  rust/macros/module.rs        |  3 ++-
> > > > >  samples/rust/rust_minimal.rs |  2 +-
> > > > >  samples/rust/rust_print.rs   |  2 +-
> > > > >  5 files changed, 15 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > > > index a791702b4fee..5af00e072a58 100644
> > > > > --- a/rust/kernel/lib.rs
> > > > > +++ b/rust/kernel/lib.rs
> > > > > @@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
> > > > >      /// should do.
> > > > >      ///
> > > > >      /// Equivalent to the `module_init` macro in the C API.
> > > > > -    fn init(module: &'static ThisModule) -> error::Result<Self>;
> > > > > +    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;
> > > > 
> > > > Why can't the name come directly from the build system?  Why must it be
> > > > passed into the init function of the module "class"?  What is it going
> > > > to do with it?
> > > 
> > > The name does come from the build system, that's where `Module::init` gets it
> > > from.
> > > 
> > > > 
> > > > A PCI, or other bus, driver "knows" it's name already by virtue of the
> > > > build system, so it can pass that string into whatever function needs
> > > 
> > > Let's take pci_register_driver() as example.
> > > 
> > > ```
> > > #define pci_register_driver(driver)		\
> > > 	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
> > > ```
> > > 
> > > In C drivers this works because (1) it's a macro and (2) it's called directly
> > > from the driver code.
> > > 
> > > In Rust, for very good reasons, we have abstractions for C APIs, hence the
> > > actual call to __pci_register_driver() does not come from code within the
> > > module, but from the PCI Rust abstraction `Module::init` calls into instead.
> > 
> > I don't understand those reasons, sorry.
> 
> Ok, good you point this out. We should definitely discuss this point then and
> build some consensus around it.
> 
> I propose to focus on this point first and follow up with the discussion of the
> rest of the series afterwards.
> 
> Let me explain why I am convinced that it's very important to have abstractions
> in place in general and from the get-go.
> 
> In general, having abstractions for C APIs is the foundation of being able to
> make use of a lot of advantages Rust has to offer.
> 
> The most obvious one are all the safety aspects. For instance, with an
> abstraction we have to get exactly one piece of code right in terms of pointer
> validity, lifetimes, type safety, API semantics, etc. and in all other places
> (e.g. drivers) we get the compiler to check those things for us through the
> abstraction.
> 
> Now, the abstraction can be buggy or insufficient and hence there is no absolute
> safety guarantee. *But*, if we get this one thing right, there is nothing a
> driver can mess up by itself trying to do stupid things anymore.
> 
> If we just call the C code instead we have to get it right everywhere instead.

I too want a pony.  But unfortunatly you are shaving a yak here instead :)

I'm not saying to call C code from rust, I'm saying call rust code from
C.

Have a "normal" pci_driver structure with C functions that THEN call
into the rust code that you want to implement them, with a pointer to
the proper structure.  That gives you everything that you really want
here, EXCEPT you don't have to build the whole tower of drivers and
busses and the like.

> Now, you could approach this top-down instead and argue that we could at least
> benefit from Rust for the driver specific parts.
> 
> Unfortunately, this doesn't really work out either. Also driver specific code is
> typically (very) closely connected to kernel APIs. If you want to use the safety
> aspects of Rust for the driver specific parts you inevitably end up writing
> abstractions for the C APIs in your driver.
> 
> There are basically three options you can end up with:
> 
> (1) An abstraction for the C API within your driver that is actually generic
>     for every driver, and hence shouldn't be there.
> (2) Your driver specific code is full of raw pointers and `unsafe {}` calls,
>     which in the end just means that you ended up baking the abstraction into
>     your driver specific code.
> (3) You ignore everything, put everything in a huge `unsafe {}` block and
>     compile C code with the Rust compiler. (Admittedly, maybe slightly
>     overstated, but not that far off either.)
> 
> The latter is also the reason why it doesn't make sense to only have
> abstractions for some things, but not for other.
> 
> If an abstraction for B is based on A, but we don't start with A, then B ends up
> implementing (at least partially) the abstraction for A as well. For instance,
> if we don't implement `driver::Registration` then the PCI abstractions (and
> platform, usb, etc.) have to implement it.
> 
> It really comes down to the point that it just bubbles up. We really have to do
> this bottom-up, otherwise we just end up moving those abstractions up, layer by
> layer, where they don't belong to and we re-implement them over and over again.

I think we are talking past each other.

Here is an example .c file that you can use today for your "implement a
PCI driver in rust" wish in a mere 34 lines of .c code:

------------------------------------------------------------------------
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/module.h>
#include <linux/pci.h>
#include "my_pci_rust_bindings.h"

#define PCI_DEVICE_ID_FOO		0x0f00
#define PCI_VENDOR_ID_FOO		0x0f00

static int probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
	return my_rust_probe(pdev);
}

static void remove(struct pci_dev *pdev)
{
	my_rust_remove(pdev);
}

static const struct pci_device_id pci_ids[] = {
	{PCI_DEVICE(PCI_VENDOR_ID_FOO, PCI_DEVICE_ID_FOO)},
	{}
};
MODULE_DEVICE_TABLE(pci, pci_ids);

static struct pci_driver my_rust_pci_driver = {
	.name = "my_pci_rust_driver",
	.id_table = pci_ids,
	.probe = probe,
	.remove = remove,
};
module_pci_driver(my_rust_pci_driver);

MODULE_DESCRIPTION("Driver for my fancy PCI device");
MODULE_LICENSE("GPL v2");
------------------------------------------------------------------------

Now, all you have to do is provide a my_rust_probe() and
my_rust_remove() call in your rust code, and handle the conversion of a
struct pci_dev to whatever you want to use (which you had to do anyway),
and you are set!

That .c code above is "obviously" correct, and much simpler and worlds
easier for all of us to understand instead of the huge .rs files that
this patch series was attempting to implement.

You have to draw the c/rust line somewhere, you all seem to be wanting
to draw that line at "no .c in my module at all!" and I'm saying "put 34
lines of .c in your module and you will save a LOT of headache now."

All of the "real" work you want to do for your driver is behind the
probe/remove callbacks (ok, add a suspend/resume as well, forgot that),
and stop worrying about all of the bindings and other mess to tie into
the driver model for a specific bus type and the lunacy that the device
id mess was.

In short, KEEP IT SIMPLE!

Then, after we are comfortable with stuff like the above, slowly think
about moving the line back a bit more, if you really even need to.  But
why would you need to?  Only reason I can think of is if you wanted to
write an entire bus in rust, and for right now, I would strongly suggest
anyone not attempt that.

The "joy" of writing a driver in rust is that a driver consumes from
EVERYWHERE in the kernel (as you well know.)  Having to write bindings
and mappings for EVERYTHING all at once, when all you really want to do
is implement the logic between probe/remove is rough, I'm sorry.  I'm
suggesting that you stop at the above line for now, which should make
your life easier as the .c code is obviously correct, and anything you
do in the rust side is your problem, not mine as a bus maintainer :)

> > Changing drivers later, to take advantage of code savings like this
> > macro can be done then, not just yet.  Let's get this understood and
> > right first, no need to be tricky or complex.
> > 
> > And, as I hinted at before, I don't think we should be doing this at all
> > just yet either.  I still think a small "C shim" layer to wrap the
> > struct pci driver up and pass the calls to the rust portion of the
> > module is better to start with, it both saves you time and energy so
> > that you can work on what you really want to do (i.e. a driver in rust)
> > and not have to worry about the c bindings as that's the "tricky" part
> > that is stopping you from getting your driver work done.
> 
> I strongly disagree here. As explained above, it just bubbles up, this approach
> would just make me end up having to write a lot of the code from the
> abstractions in the driver.

See above, you cut out all of the .rs files you submitted in this series
and can probably just add one to map a pci_device and away you go.

> What I actually want is to get to a solid foundation for Rust drivers in
> general, since I'm convinced that this provides a lot of value beyond the scope
> of a single driver.

I don't see what not needing the above 35+ lines of .c code provides a
value for at all, EXCEPT we now need to maintain a lot more code
overall.

> What do we pass to Rust probe() function? A raw struct pci_dev pointer or the
> proper PCI device abstraction?

See above.

> How do we implement I/O memory mappings for PCI bars without PCI / I/O
> abstraction?

Call the .c functions you have today.

> How do we perform (boundary checked) I/O memory reads / writes without `Io`
> abstraction?

Do you really need/want that?  If so, ok, but I think you are going to
run into big issues with dynamic ranges that I didn't see answered on
those patches, but I could be totally wrong.

Again, one step at a time please, not the whole yak.

> How do we handle the lifetime of resources without `Devres` abstraction?

We have this question on the .c side as well.  Please work with the
people who are working to solve that there as it hits everyone.

> How do we (properly) implement a DRM device registration abstraction without
> `Devres`?

No idea, drm is huge :)

That being said, I think you can implement it above as the lifetime of
your device lives between probe and remove.  After remove is gone, all
of your resources HAVE to be freed, so do that in your driver.

> Luckily we already got the `Firmware` and `Device` abstraction in place. :)

Baby steps :)

thanks,

greg k-h

Re: [PATCH v2 02/10] rust: implement generic driver registration

[Greg KH]

On Thu, Jun 20, 2024 at 07:12:42PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 20, 2024 at 04:28:23PM +0200, Greg KH wrote:
> > On Wed, Jun 19, 2024 at 01:39:48AM +0200, Danilo Krummrich wrote:
> > > Implement the generic `Registration` type and the `DriverOps` trait.
> > 
> > I don't think this is needed, more below...
> > 
> > > 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.
> > 
> > But that's not what normally happens, more below...
> 
> I can't find below a paragraph that seems related to this, hence I reply here.
> 
> The above is just different wording for: A driver is typically registered in
> module_init() and unregistered in module_exit().
> 
> Isn't that what happens normally?

Yes, but it's nothing we have ever used in the kernel before.  You are
defining new terms in some places, and renaming existing ones in others,
which is going to do nothing but confuse us all.

I don't see why you need a "registration" structure here when no .c
driver ever does.  You just have a module init/exit call and go from
there.  Why not stick with that?

> > > Instead the `DriverOps` trait is provided to subsystems, which have to
> > > implement `DriverOps::register` and `DrvierOps::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.
> > 
> > So you are saying this should be something that a "bus" would do?
> > Please be explicit as to what you mean by "subsystem" here.
> 
> Yes, I agree it's more precise to say that this should be implemented by a bus
> (e.g. PCI). I can reword this one.

Wording matters.

> > > For instance, the PCI subsystem would call __pci_register_driver() from
> > > `DriverOps::register` and pci_unregister_driver() from
> > > `DrvierOps::unregister`.
> > 
> > So this is a BusOps, or more in general, a "subsystem" if it's not a
> > bus (i.e. it's a class).  Note, we used to use the term "subsystem" a
> > very long time ago but got rid of them in the driver core, let's not
> > bring it back unless we REALLY know we want it this time.
> > 
> > So why isn't this just a BusOps?
> 
> I think it's really about perspective. Generally speaking, when a driver is
> registered it gets added to a bus through bus_add_driver(). Now, one could argue
> that the "register" operation is a bus operation, since something gets
> registered on the bus, but one could also argue that it's a driver operation,
> since a driver is registered on something.
> 
> Consequently, I think it's neither wrong to call this one `BusOps` nor is it
> wrong to call it `DriverOps`.
> 
> I still think `DriverOps` is more appropriate, since here we're looking at it
> from the perspective of the driver.

Stick with the same terms we have today please.  These are specific bus
operations.  Some drivers implement multiple bus operations within them
as they can handle multiple bus types.  Keep the same names we have
today, it will save maintaining this for the next 40+ years easier.

> In the end we call it as `driver.register()` instead of `bus.register()`. For
> instance, in the PCI implementation of it, we call __pci_register_driver() from
> `DriverOps::register`.

You are registering with a bus, stick with that term please.

> > > +/// The [`DriverOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, Amba,
> > > +/// etc.) to privide 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 `DriverOps::register` and
> > > +/// `bindings::pci_unregister_driver` from `DriverOps::unregister`.
> > > +pub trait DriverOps {
> > > +    /// 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.
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// `reg` must point to valid, initialised, and writable memory. It may be modified by this
> > > +    /// function to hold registration state.
> > > +    ///
> > > +    /// On success, `reg` must remain pinned and valid until the matching call to
> > > +    /// [`DriverOps::unregister`].
> > > +    fn register(
> > > +        reg: &mut Self::RegType,
> > > +        name: &'static CStr,
> > > +        module: &'static ThisModule,
> > > +    ) -> Result;
> > > +
> > > +    /// Unregisters a driver previously registered with [`DriverOps::register`].
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// `reg` must point to valid writable memory, initialised by a previous successful call to
> > > +    /// [`DriverOps::register`].
> > > +    fn unregister(reg: &mut Self::RegType);
> > > +}
> > 
> > So you are getting into what a bus wants/needs to support here, why is
> > register/unregister the "big" things to be implemented first?  Why not
> > just map the current register/unregister bus functions to a bus-specific
> > trait for now?  And then, if you think it really should be generic, we
> 
> A bus specific trait would not add any value. The whole point if a trait is to
> represent a generic interface. It basically describes the functionality we
> expect from a certain category of types.
> 
> In this case we know that every driver can be registered and unregistered, hence
> we can define a generic trait that every bus specific driver structure, e.g. PCI
> driver, has to implement.

So this is a generic trait that all drivers of all bus types must do?
Ick, no, don't make this so generic it's impossible to unwind.

Make things specific FIRST.  Then implement a second one.  Then a third
one.  And THEN see what you can make generic, before then it's going to
be hard and a mess.

Stick with specifics first, you can always make them more generic later.

> > can make it that way then.  I don't see why this needs to be generic now
> > as you aren't implementing a bus in rust at this point in time, right?
> 
> With the above tait (or interface) we now can have a generic `Registration` that
> calls `T::register` and `T::unregister` and works for all driver types (PCI,
> platform, etc.). Otherwise we'd need a `pci::Registration`, a
> `platform::Registration` etc. and copy-paste the below code for all of them.

Good, copy/paste to start with and then if it gets messy on the third
implementation, THEN we can think about unifying them.

I'm going to argue that registering with a pci bus is VERY different
than registering with the platform bus, or a USB bus, which, if you look
at the kernel today, is why those are different functions!  Only in the
driver core is the unified portions.  Don't attempt to make the rust
code generic and then have it split back out, and THEN have it become
generic afterward.  That's an odd way to do things, please don't.

> > > +
> > > +/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
> > > +/// `bindings::pci_driver`). Therefore a [`Registration`] is initialized with some type that
> > > +/// implements the [`DriverOps`] 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: DriverOps> {
> > > +    #[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: DriverOps> 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: DriverOps> Send for Registration<T> {}
> > > +
> > > +impl<T: DriverOps> Registration<T> {
> > > +    /// Creates a new instance of the registration object.
> > > +    pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {
> > 
> > Drivers have modules, not busses.  So you are registering a driver with
> > a bus here, it's not something that a driver itself implements as you
> > have named here.
> 
> We are registering a driver on bus here, see the below `T::register` call, this
> one ends up in __pci_register_driver() or __platform_driver_register(), etc. Hence
> we need the module and the module name.

Again, let's be specific first, make things generic later.  Why do you
need/want a trait at all when you have only 1 bus type in the whole
kernel?

Keep it simple, make it so obvious that I would be foolish to reject it.
Right now, it's so complex and generic that I feel foolish accepting it.

thanks,

greg k-h

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

On Wed, Jul 10, 2024 at 04:02:04PM +0200, Greg KH wrote:
> On Thu, Jun 20, 2024 at 11:24:17PM +0200, Danilo Krummrich wrote:
> > Ok, good you point this out. We should definitely discuss this point then and
> > build some consensus around it.
> > 
> > I propose to focus on this point first and follow up with the discussion of the
> > rest of the series afterwards.
> > 
> > Let me explain why I am convinced that it's very important to have abstractions
> > in place in general and from the get-go.
> > 
> > In general, having abstractions for C APIs is the foundation of being able to
> > make use of a lot of advantages Rust has to offer.
> > 
> > The most obvious one are all the safety aspects. For instance, with an
> > abstraction we have to get exactly one piece of code right in terms of pointer
> > validity, lifetimes, type safety, API semantics, etc. and in all other places
> > (e.g. drivers) we get the compiler to check those things for us through the
> > abstraction.
> > 
> > Now, the abstraction can be buggy or insufficient and hence there is no absolute
> > safety guarantee. *But*, if we get this one thing right, there is nothing a
> > driver can mess up by itself trying to do stupid things anymore.
> > 
> > If we just call the C code instead we have to get it right everywhere instead.
> 
> I too want a pony.  But unfortunatly you are shaving a yak here instead :)
> 
> I'm not saying to call C code from rust, I'm saying call rust code from
> C.
> 
> Have a "normal" pci_driver structure with C functions that THEN call
> into the rust code that you want to implement them, with a pointer to
> the proper structure.  That gives you everything that you really want
> here, EXCEPT you don't have to build the whole tower of drivers and
> busses and the like.

Please find my reply below where you expand this point.

> 
> > Now, you could approach this top-down instead and argue that we could at least
> > benefit from Rust for the driver specific parts.
> > 
> > Unfortunately, this doesn't really work out either. Also driver specific code is
> > typically (very) closely connected to kernel APIs. If you want to use the safety
> > aspects of Rust for the driver specific parts you inevitably end up writing
> > abstractions for the C APIs in your driver.
> > 
> > There are basically three options you can end up with:
> > 
> > (1) An abstraction for the C API within your driver that is actually generic
> >     for every driver, and hence shouldn't be there.
> > (2) Your driver specific code is full of raw pointers and `unsafe {}` calls,
> >     which in the end just means that you ended up baking the abstraction into
> >     your driver specific code.
> > (3) You ignore everything, put everything in a huge `unsafe {}` block and
> >     compile C code with the Rust compiler. (Admittedly, maybe slightly
> >     overstated, but not that far off either.)
> > 
> > The latter is also the reason why it doesn't make sense to only have
> > abstractions for some things, but not for other.
> > 
> > If an abstraction for B is based on A, but we don't start with A, then B ends up
> > implementing (at least partially) the abstraction for A as well. For instance,
> > if we don't implement `driver::Registration` then the PCI abstractions (and
> > platform, usb, etc.) have to implement it.
> > 
> > It really comes down to the point that it just bubbles up. We really have to do
> > this bottom-up, otherwise we just end up moving those abstractions up, layer by
> > layer, where they don't belong to and we re-implement them over and over again.
> 
> I think we are talking past each other.

Given your proposal below, that is correct.

I read your previous mail as if you question having abstractions for C APIs at
all. And some parts of your latest reply still read like that to me, but for the
rest of my reply I will assume that's not the case.

> 
> Here is an example .c file that you can use today for your "implement a
> PCI driver in rust" wish in a mere 34 lines of .c code:

Your proposal below actually only discards a rather small amount of the proposed
abstractions, in particular:

  (1) the code to create a struct pci_driver and the code to call
      pci_register_driver() and pci_unregister_driver() (implemented in Patch 2)
  (2) the generic code to create the struct pci_device_id table (or any other ID
      table) (implemented in Patch 3)

As I understand you, the concern here really seems to be about the complexity
vs. what we get from it and that someone has to maintain this.

I got the point and I take it seriously. As already mentioned, I'm also willing
to take responsibility for the code and offer to maintain it.

But back to the technical details.

Let's start with (2):

I agree that this seems a bit complicated. I'd propose to remove the
abstractions in device_id.rs (i.e. Patch 3) for now and let the PCI abstraction
simply create a struct pci_device_id table directly.

As for (1):

This just makes a pretty small part of the abstractions and, it's really only
about creating the struct pci_driver (or another driver type) instance and
call the corresponding register() / unregister() functions, since we already
have the Rust module support upstream.

As in C, we really just call register() from module_init() and unregister() from
module_exit() and the code for that, without comments, is around 50 lines of
code.

As mentioned this is implemented in Patch 2; Hence, please see my reply on Patch
2, where I put a rather long and detailed explanation which hopefully clarifies
things.

Now, what do we get from that compared to the proposal below?

  - The driver's probe() and remove() functions get called with the
    corresponding abstraction types directly instead of raw pointers; this moves
    the required  `unsafe {}` blocks to a cental place which otherwise *every*
    driver would need to implement itself (obviously it'd be the same for future
    suspend, resume, shutdown, etc. callbacks).

  - More complex drivers can do the work required to be done in module_init()
    and module_exit() in Rust directly, which allows them to attach the lifetime
    of structures to the lifetime of the `Module` structure in Rust which avoids
    the need for explicit cleanup in module_exit() since they can just implement
    in Rust's drop() trait.

The latter may sound a bit less important than it actually is, since it can break
the design, safety and soundness of Rust types. Let me explain:

In Rust a type instance should cleanup after itself when it goes out of scope.

Let's make up an example:

```
// DISCLAIMER: Obviously, this is not how we actually handle memory allocations
// in Rust, it's just an example.
struct Buffer {
	ptr: *const u8,
}

impl Buffer {
	fn alloc(size: u8) -> Result<Self> {
		let ptr = Kmalloc::alloc(size, GFP_KERNEL)?;

		// Return an instance of `Buffer` initialized with a pointer to
		// the above memory allocation.
		Ok(Self {
			ptr,
		})
	}
}

impl Drop for Buffer {
	fn drop(&mut self) {
		// SAFETY: `Self` is always holding a pointer to valid memory
		// allocated with `Kmalloc`.
		unsafe { Kmalloc::free(self.ptr) };
	}
}
```

(Side note: `Kmalloc::free` is unsafe since it has no control on whether the
pointer passed to it is valid and was allocated with `Kmalloc` previously.)

In Rust's module_init() you could now attach an instance of `Buffer` to the
`Module` instance, which lives until module_exit(), which looks like this:

```
struct MyModule {
	buffer: Buffer,
}

impl kernel::Moduke for MyModule {
	fn init(module: &'static ThisModule) -> Result<Self> {
		Ok(MyModule {
			buffer: Buffer::alloc(0x100)?,
		})
	}
}
```

Note that we don't need to implement module_exit() here, since `MyModule` lives
until module_exit() and hence `buffer` has the same lifetime. Once `buffer` goes
out of scope it frees itself due to the drop() trait. In fact, buffer can never
leak it's memory.

With the proposed approach below we can't do this, we'd need to play dirty and
unsafe tricks in order to not entirely break the design of `Buffer`, or any
other more complex data structure that the module needs.

> 
> ------------------------------------------------------------------------
> // SPDX-License-Identifier: GPL-2.0-only
> #include <linux/module.h>
> #include <linux/pci.h>
> #include "my_pci_rust_bindings.h"
> 
> #define PCI_DEVICE_ID_FOO		0x0f00
> #define PCI_VENDOR_ID_FOO		0x0f00
> 
> static int probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> 	return my_rust_probe(pdev);
> }
> 
> static void remove(struct pci_dev *pdev)
> {
> 	my_rust_remove(pdev);
> }
> 
> static const struct pci_device_id pci_ids[] = {
> 	{PCI_DEVICE(PCI_VENDOR_ID_FOO, PCI_DEVICE_ID_FOO)},
> 	{}
> };
> MODULE_DEVICE_TABLE(pci, pci_ids);
> 
> static struct pci_driver my_rust_pci_driver = {
> 	.name = "my_pci_rust_driver",
> 	.id_table = pci_ids,
> 	.probe = probe,
> 	.remove = remove,
> };
> module_pci_driver(my_rust_pci_driver);
> 
> MODULE_DESCRIPTION("Driver for my fancy PCI device");
> MODULE_LICENSE("GPL v2");
> ------------------------------------------------------------------------
> 
> Now, all you have to do is provide a my_rust_probe() and
> my_rust_remove() call in your rust code, and handle the conversion of a
> struct pci_dev to whatever you want to use (which you had to do anyway),
> and you are set!
> 
> That .c code above is "obviously" correct, and much simpler and worlds
> easier for all of us to understand instead of the huge .rs files that
> this patch series was attempting to implement.
> 
> You have to draw the c/rust line somewhere, you all seem to be wanting
> to draw that line at "no .c in my module at all!" and I'm saying "put 34
> lines of .c in your module and you will save a LOT of headache now."
> 
> All of the "real" work you want to do for your driver is behind the
> probe/remove callbacks (ok, add a suspend/resume as well, forgot that),
> and stop worrying about all of the bindings and other mess to tie into
> the driver model for a specific bus type and the lunacy that the device
> id mess was.
> 
> In short, KEEP IT SIMPLE!
> 
> Then, after we are comfortable with stuff like the above, slowly think
> about moving the line back a bit more, if you really even need to.  But
> why would you need to?  Only reason I can think of is if you wanted to
> write an entire bus in rust, and for right now, I would strongly suggest
> anyone not attempt that.

We're not trying to do that. Also, please note that all the abstractions are
*only* making use of APIs that C drivers use directly, just abstracting them
in a way, such that we can actually use the strength of Rust.

> 
> The "joy" of writing a driver in rust is that a driver consumes from
> EVERYWHERE in the kernel (as you well know.)  Having to write bindings
> and mappings for EVERYTHING all at once, when all you really want to do
> is implement the logic between probe/remove is rough, I'm sorry.  I'm
> suggesting that you stop at the above line for now, which should make
> your life easier as the .c code is obviously correct, and anything you
> do in the rust side is your problem, not mine as a bus maintainer :)

As explained above, we already have the module abstraction in place. Really all
that we're left with is creating the struct pci_driver and call register() /
remove() from it.

Now, this could be fully done in the PCI abstraction. The only reason we have
the `Registration` and `DriverOps` (again there are a few misunderstandings
around that, that I try to clarify in the corresponding thread) in "driver.rs"
is to not duplicate code for maintainability.

What I'm trying to say, the stuff in "driver.rs" is not even abstracting things
from drivers/base/, but is Rust helper code for subsystems to abstract their
stuff (e.g. struct pci_driver).

And again, I'm willing to take responsibility for the code and offer to maintain
it - I really want to do this the proper way.

- Danilo

(cutting of the rest since it was based on the misunderstanding mentioned above)

Re: [PATCH v2 02/10] rust: implement generic driver registration

[Danilo Krummrich]

(Please read my reply to Patch 1 first)

On Wed, Jul 10, 2024 at 04:10:40PM +0200, Greg KH wrote:
> On Thu, Jun 20, 2024 at 07:12:42PM +0200, Danilo Krummrich wrote:
> > On Thu, Jun 20, 2024 at 04:28:23PM +0200, Greg KH wrote:
> > > On Wed, Jun 19, 2024 at 01:39:48AM +0200, Danilo Krummrich wrote:
> > > > Implement the generic `Registration` type and the `DriverOps` trait.
> > > 
> > > I don't think this is needed, more below...
> > > 
> > > > 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.
> > > 
> > > But that's not what normally happens, more below...
> > 
> > I can't find below a paragraph that seems related to this, hence I reply here.
> > 
> > The above is just different wording for: A driver is typically registered in
> > module_init() and unregistered in module_exit().
> > 
> > Isn't that what happens normally?
> 
> Yes, but it's nothing we have ever used in the kernel before.  You are
> defining new terms in some places, and renaming existing ones in others,
> which is going to do nothing but confuse us all.

We're not renaming anything, but...

New terms, yes, because it's new structures that aren't needed in C, but in
Rust. Why do we need those things in Rust, but not in C you may ask.

Let me try to explain it while trying to clarify what the `Registration` and
`DriverOps` types are actually used for, as promised in my reply to Patch 1.

The first misunderstanding may be that they abstract something in drivers/base/,
but that's not the case. In fact, those are not abstractions around C
structures themselfes. Think of them as small helpers to implement driver
abstractions in general (e.g. PCI, platform, etc.), which is why they are in a
file named driver.rs.

Now, what are `DriverOps`? It's just an interface that asks the implementer of
the interface to implement a register() and an unregister() function. PCI
obviously does implement this as pci_register_driver() and
pci_unregister_driver().

Having that said, I agree with you that `DriverOps` is a bad name, I think it
should be `RegistrationOps` instead - it represents the operations to register()
and unregister() a driver. I will use this name in the following instead, it is
less confusing.

In terms of what a `Registration` does and why we need this in Rust, but not in
C it is easiest to see from an example with some inline comments:

```
struct MyDriver;

impl pci::Driver for MyDriver {
	define_pci_id_table! {
		bindings::PCI_VENDOR_ID_FOO, bindings::PCI_ANY_ID,
		None,
	}

	fn probe(dev: ARef<pci::Device>) {}
	fn remove() {}
}

struct MyModule {
	// `pci::RegOps` is the PCI implementation of `RegistrationOps`, i.e.
	// `pci::Ops::register()` calls pci_register_driver() and 
	// `pci::Ops::unregister()` calls pci_unregister_driver().
	//
	// `pci::RegOps` also creates the `struct pci_dev` setting probe() to
	// `MyDriver::probe` and remove() to `MyDriver::remove()`.
	reg: Registration<pci::RegOps<MyDriver>>,
}

impl kernel::Moduke for MyModule {
fn init(name: &'static CStr, module: &'static ThisModule) -> Result<Self> {
		Ok(MyModule {
			reg: Registration::<pci::RegOps<MyDriver>>::new(name, module),
		})
	}
}
```

This code is equivalent to the following C code:

```
static int probe(struct pci_dev *pdev, const struct pci_device_id *ent) {}

static void remove(struct pci_dev *pdev) {}

static struct pci_driver my_pci_driver {
	.name = "my_driver",
	.id_table = pci_ids,
	.probe = probe,
	.remove = remove,
};

static int __init my_module_init(void)
{
	pci_register_driver(my_pci_driver);
}
module_init(my_module_init);

static void __exit my_module_exit(void)
{
	pci_unregister_driver(my_pci_driver();
}
module_exit(my_module_exit);
```

You may have noticed that the Rust code doesn't need `Module::exit` at all. And
the reason is the `Registration` type.

`Registration` is implemented as:

```
struct Registration<T: RegistrationOps> {
	// In the example above `T::DriverType` is struct pci_dev.
	drv: T::DriverType,
}

impl<T: RegistrationOps> Registration<T> {
	pub fn new(name: &'static Cstr, module &'static ThisModule) -> Self {
		// SAFETY: `T::DriverType` is a C type (e.g. struct pci_dev) and
		// can be zero initialized.
		// This is a bit simplified, to not bloat the example with
		// pinning.
		let drv: T::DriverType = unsafe { core::mem::zeroed() };

		// In this example, this calls `pci::RegOps::register`, which
		// initializes the struct pci_dev and calls
		// pci_register_driver().
		T::register(drv, name, module);
	}
}

impl<T: RegistrationOps> Drop for Registration<T> {
	fn drop(&mut self) {
		// This calls pci_unregister_driver() on the struct pci_dev
		// stored in `self.drv`.
		T::unregister(self.drv);
	}
}
```

As you can see, once the `Registration` goes out of scope the driver is
automatically unregistered due to the drop() implementation, which is why we
don't need `Module::exit`. 

This also answers why we need a `Registration` structure in Rust, but not in C.
Rust uses different programming paradigms than C, and it uses type
representations with `Drop` traits to clean things up, rather than relying on
the user of the API doing it manually.

I really hope this explanation and example helps and contributes to progress.
As you can see I really put a lot of effort and dedication into this work.

- Danilo

--

Just for completeness, please find the relevant parts of `pci::RegOps` below.

```
impl<T: Driver> driver::DriverOps for Adapter<T> {
	type DriverType = bindings::pci_driver;

	fn register(
		pdrv: &mut bindings::pci_driver,
		name: &'static CStr,
		module: &'static ThisModule,
	) -> Result {
		pdrv.name = name.as_char_ptr();
		pdrv.probe = Some(Self::probe_callback);
		pdrv.remove = Some(Self::remove_callback);
		pdrv.id_table = T::ID_TABLE.as_ref();

		// SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
		to_result(unsafe {
			bindings::__pci_register_driver(pdrv as _, module.0, name.as_char_ptr())
		})
	}

	fn unregister(pdrv: &mut Self::RegType) {
		// SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
		unsafe { bindings::pci_unregister_driver(pdrv) }
	}
}
```

(cutting the rest of the mail, since everything else is covered already)

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

On Thu, Jul 11, 2024 at 04:06:25AM +0200, Danilo Krummrich wrote:

Gentle reminder.

> On Wed, Jul 10, 2024 at 04:02:04PM +0200, Greg KH wrote:
> > On Thu, Jun 20, 2024 at 11:24:17PM +0200, Danilo Krummrich wrote:
> > > Ok, good you point this out. We should definitely discuss this point then and
> > > build some consensus around it.
> > > 
> > > I propose to focus on this point first and follow up with the discussion of the
> > > rest of the series afterwards.
> > > 
> > > Let me explain why I am convinced that it's very important to have abstractions
> > > in place in general and from the get-go.
> > > 
> > > In general, having abstractions for C APIs is the foundation of being able to
> > > make use of a lot of advantages Rust has to offer.
> > > 
> > > The most obvious one are all the safety aspects. For instance, with an
> > > abstraction we have to get exactly one piece of code right in terms of pointer
> > > validity, lifetimes, type safety, API semantics, etc. and in all other places
> > > (e.g. drivers) we get the compiler to check those things for us through the
> > > abstraction.
> > > 
> > > Now, the abstraction can be buggy or insufficient and hence there is no absolute
> > > safety guarantee. *But*, if we get this one thing right, there is nothing a
> > > driver can mess up by itself trying to do stupid things anymore.
> > > 
> > > If we just call the C code instead we have to get it right everywhere instead.
> > 
> > I too want a pony.  But unfortunatly you are shaving a yak here instead :)
> > 
> > I'm not saying to call C code from rust, I'm saying call rust code from
> > C.
> > 
> > Have a "normal" pci_driver structure with C functions that THEN call
> > into the rust code that you want to implement them, with a pointer to
> > the proper structure.  That gives you everything that you really want
> > here, EXCEPT you don't have to build the whole tower of drivers and
> > busses and the like.
> 
> Please find my reply below where you expand this point.
> 
> > 
> > > Now, you could approach this top-down instead and argue that we could at least
> > > benefit from Rust for the driver specific parts.
> > > 
> > > Unfortunately, this doesn't really work out either. Also driver specific code is
> > > typically (very) closely connected to kernel APIs. If you want to use the safety
> > > aspects of Rust for the driver specific parts you inevitably end up writing
> > > abstractions for the C APIs in your driver.
> > > 
> > > There are basically three options you can end up with:
> > > 
> > > (1) An abstraction for the C API within your driver that is actually generic
> > >     for every driver, and hence shouldn't be there.
> > > (2) Your driver specific code is full of raw pointers and `unsafe {}` calls,
> > >     which in the end just means that you ended up baking the abstraction into
> > >     your driver specific code.
> > > (3) You ignore everything, put everything in a huge `unsafe {}` block and
> > >     compile C code with the Rust compiler. (Admittedly, maybe slightly
> > >     overstated, but not that far off either.)
> > > 
> > > The latter is also the reason why it doesn't make sense to only have
> > > abstractions for some things, but not for other.
> > > 
> > > If an abstraction for B is based on A, but we don't start with A, then B ends up
> > > implementing (at least partially) the abstraction for A as well. For instance,
> > > if we don't implement `driver::Registration` then the PCI abstractions (and
> > > platform, usb, etc.) have to implement it.
> > > 
> > > It really comes down to the point that it just bubbles up. We really have to do
> > > this bottom-up, otherwise we just end up moving those abstractions up, layer by
> > > layer, where they don't belong to and we re-implement them over and over again.
> > 
> > I think we are talking past each other.
> 
> Given your proposal below, that is correct.
> 
> I read your previous mail as if you question having abstractions for C APIs at
> all. And some parts of your latest reply still read like that to me, but for the
> rest of my reply I will assume that's not the case.
> 
> > 
> > Here is an example .c file that you can use today for your "implement a
> > PCI driver in rust" wish in a mere 34 lines of .c code:
> 
> Your proposal below actually only discards a rather small amount of the proposed
> abstractions, in particular:
> 
>   (1) the code to create a struct pci_driver and the code to call
>       pci_register_driver() and pci_unregister_driver() (implemented in Patch 2)
>   (2) the generic code to create the struct pci_device_id table (or any other ID
>       table) (implemented in Patch 3)
> 
> As I understand you, the concern here really seems to be about the complexity
> vs. what we get from it and that someone has to maintain this.
> 
> I got the point and I take it seriously. As already mentioned, I'm also willing
> to take responsibility for the code and offer to maintain it.
> 
> But back to the technical details.
> 
> Let's start with (2):
> 
> I agree that this seems a bit complicated. I'd propose to remove the
> abstractions in device_id.rs (i.e. Patch 3) for now and let the PCI abstraction
> simply create a struct pci_device_id table directly.
> 
> As for (1):
> 
> This just makes a pretty small part of the abstractions and, it's really only
> about creating the struct pci_driver (or another driver type) instance and
> call the corresponding register() / unregister() functions, since we already
> have the Rust module support upstream.
> 
> As in C, we really just call register() from module_init() and unregister() from
> module_exit() and the code for that, without comments, is around 50 lines of
> code.
> 
> As mentioned this is implemented in Patch 2; Hence, please see my reply on Patch
> 2, where I put a rather long and detailed explanation which hopefully clarifies
> things.
> 
> Now, what do we get from that compared to the proposal below?
> 
>   - The driver's probe() and remove() functions get called with the
>     corresponding abstraction types directly instead of raw pointers; this moves
>     the required  `unsafe {}` blocks to a cental place which otherwise *every*
>     driver would need to implement itself (obviously it'd be the same for future
>     suspend, resume, shutdown, etc. callbacks).
> 
>   - More complex drivers can do the work required to be done in module_init()
>     and module_exit() in Rust directly, which allows them to attach the lifetime
>     of structures to the lifetime of the `Module` structure in Rust which avoids
>     the need for explicit cleanup in module_exit() since they can just implement
>     in Rust's drop() trait.
> 
> The latter may sound a bit less important than it actually is, since it can break
> the design, safety and soundness of Rust types. Let me explain:
> 
> In Rust a type instance should cleanup after itself when it goes out of scope.
> 
> Let's make up an example:
> 
> ```
> // DISCLAIMER: Obviously, this is not how we actually handle memory allocations
> // in Rust, it's just an example.
> struct Buffer {
> 	ptr: *const u8,
> }
> 
> impl Buffer {
> 	fn alloc(size: u8) -> Result<Self> {
> 		let ptr = Kmalloc::alloc(size, GFP_KERNEL)?;
> 
> 		// Return an instance of `Buffer` initialized with a pointer to
> 		// the above memory allocation.
> 		Ok(Self {
> 			ptr,
> 		})
> 	}
> }
> 
> impl Drop for Buffer {
> 	fn drop(&mut self) {
> 		// SAFETY: `Self` is always holding a pointer to valid memory
> 		// allocated with `Kmalloc`.
> 		unsafe { Kmalloc::free(self.ptr) };
> 	}
> }
> ```
> 
> (Side note: `Kmalloc::free` is unsafe since it has no control on whether the
> pointer passed to it is valid and was allocated with `Kmalloc` previously.)
> 
> In Rust's module_init() you could now attach an instance of `Buffer` to the
> `Module` instance, which lives until module_exit(), which looks like this:
> 
> ```
> struct MyModule {
> 	buffer: Buffer,
> }
> 
> impl kernel::Moduke for MyModule {
> 	fn init(module: &'static ThisModule) -> Result<Self> {
> 		Ok(MyModule {
> 			buffer: Buffer::alloc(0x100)?,
> 		})
> 	}
> }
> ```
> 
> Note that we don't need to implement module_exit() here, since `MyModule` lives
> until module_exit() and hence `buffer` has the same lifetime. Once `buffer` goes
> out of scope it frees itself due to the drop() trait. In fact, buffer can never
> leak it's memory.
> 
> With the proposed approach below we can't do this, we'd need to play dirty and
> unsafe tricks in order to not entirely break the design of `Buffer`, or any
> other more complex data structure that the module needs.
> 
> > 
> > ------------------------------------------------------------------------
> > // SPDX-License-Identifier: GPL-2.0-only
> > #include <linux/module.h>
> > #include <linux/pci.h>
> > #include "my_pci_rust_bindings.h"
> > 
> > #define PCI_DEVICE_ID_FOO		0x0f00
> > #define PCI_VENDOR_ID_FOO		0x0f00
> > 
> > static int probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > {
> > 	return my_rust_probe(pdev);
> > }
> > 
> > static void remove(struct pci_dev *pdev)
> > {
> > 	my_rust_remove(pdev);
> > }
> > 
> > static const struct pci_device_id pci_ids[] = {
> > 	{PCI_DEVICE(PCI_VENDOR_ID_FOO, PCI_DEVICE_ID_FOO)},
> > 	{}
> > };
> > MODULE_DEVICE_TABLE(pci, pci_ids);
> > 
> > static struct pci_driver my_rust_pci_driver = {
> > 	.name = "my_pci_rust_driver",
> > 	.id_table = pci_ids,
> > 	.probe = probe,
> > 	.remove = remove,
> > };
> > module_pci_driver(my_rust_pci_driver);
> > 
> > MODULE_DESCRIPTION("Driver for my fancy PCI device");
> > MODULE_LICENSE("GPL v2");
> > ------------------------------------------------------------------------
> > 
> > Now, all you have to do is provide a my_rust_probe() and
> > my_rust_remove() call in your rust code, and handle the conversion of a
> > struct pci_dev to whatever you want to use (which you had to do anyway),
> > and you are set!
> > 
> > That .c code above is "obviously" correct, and much simpler and worlds
> > easier for all of us to understand instead of the huge .rs files that
> > this patch series was attempting to implement.
> > 
> > You have to draw the c/rust line somewhere, you all seem to be wanting
> > to draw that line at "no .c in my module at all!" and I'm saying "put 34
> > lines of .c in your module and you will save a LOT of headache now."
> > 
> > All of the "real" work you want to do for your driver is behind the
> > probe/remove callbacks (ok, add a suspend/resume as well, forgot that),
> > and stop worrying about all of the bindings and other mess to tie into
> > the driver model for a specific bus type and the lunacy that the device
> > id mess was.
> > 
> > In short, KEEP IT SIMPLE!
> > 
> > Then, after we are comfortable with stuff like the above, slowly think
> > about moving the line back a bit more, if you really even need to.  But
> > why would you need to?  Only reason I can think of is if you wanted to
> > write an entire bus in rust, and for right now, I would strongly suggest
> > anyone not attempt that.
> 
> We're not trying to do that. Also, please note that all the abstractions are
> *only* making use of APIs that C drivers use directly, just abstracting them
> in a way, such that we can actually use the strength of Rust.
> 
> > 
> > The "joy" of writing a driver in rust is that a driver consumes from
> > EVERYWHERE in the kernel (as you well know.)  Having to write bindings
> > and mappings for EVERYTHING all at once, when all you really want to do
> > is implement the logic between probe/remove is rough, I'm sorry.  I'm
> > suggesting that you stop at the above line for now, which should make
> > your life easier as the .c code is obviously correct, and anything you
> > do in the rust side is your problem, not mine as a bus maintainer :)
> 
> As explained above, we already have the module abstraction in place. Really all
> that we're left with is creating the struct pci_driver and call register() /
> remove() from it.
> 
> Now, this could be fully done in the PCI abstraction. The only reason we have
> the `Registration` and `DriverOps` (again there are a few misunderstandings
> around that, that I try to clarify in the corresponding thread) in "driver.rs"
> is to not duplicate code for maintainability.
> 
> What I'm trying to say, the stuff in "driver.rs" is not even abstracting things
> from drivers/base/, but is Rust helper code for subsystems to abstract their
> stuff (e.g. struct pci_driver).
> 
> And again, I'm willing to take responsibility for the code and offer to maintain
> it - I really want to do this the proper way.
> 
> - Danilo
> 
> (cutting of the rest since it was based on the misunderstanding mentioned above)

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Greg KH]

On Mon, Jul 22, 2024 at 01:23:25PM +0200, Danilo Krummrich wrote:
> On Thu, Jul 11, 2024 at 04:06:25AM +0200, Danilo Krummrich wrote:
> 
> Gentle reminder.

It's the merge window, sorry, will be back after -rc1 is out...

Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

On Thu, Jul 11, 2024 at 04:06:25AM +0200, Danilo Krummrich wrote:

Gentle reminder on this and patch 2 of this series.

> On Wed, Jul 10, 2024 at 04:02:04PM +0200, Greg KH wrote:
> > On Thu, Jun 20, 2024 at 11:24:17PM +0200, Danilo Krummrich wrote:
> > > Ok, good you point this out. We should definitely discuss this point then and
> > > build some consensus around it.
> > > 
> > > I propose to focus on this point first and follow up with the discussion of the
> > > rest of the series afterwards.
> > > 
> > > Let me explain why I am convinced that it's very important to have abstractions
> > > in place in general and from the get-go.
> > > 
> > > In general, having abstractions for C APIs is the foundation of being able to
> > > make use of a lot of advantages Rust has to offer.
> > > 
> > > The most obvious one are all the safety aspects. For instance, with an
> > > abstraction we have to get exactly one piece of code right in terms of pointer
> > > validity, lifetimes, type safety, API semantics, etc. and in all other places
> > > (e.g. drivers) we get the compiler to check those things for us through the
> > > abstraction.
> > > 
> > > Now, the abstraction can be buggy or insufficient and hence there is no absolute
> > > safety guarantee. *But*, if we get this one thing right, there is nothing a
> > > driver can mess up by itself trying to do stupid things anymore.
> > > 
> > > If we just call the C code instead we have to get it right everywhere instead.
> > 
> > I too want a pony.  But unfortunatly you are shaving a yak here instead :)
> > 
> > I'm not saying to call C code from rust, I'm saying call rust code from
> > C.
> > 
> > Have a "normal" pci_driver structure with C functions that THEN call
> > into the rust code that you want to implement them, with a pointer to
> > the proper structure.  That gives you everything that you really want
> > here, EXCEPT you don't have to build the whole tower of drivers and
> > busses and the like.
> 
> Please find my reply below where you expand this point.
> 
> > 
> > > Now, you could approach this top-down instead and argue that we could at least
> > > benefit from Rust for the driver specific parts.
> > > 
> > > Unfortunately, this doesn't really work out either. Also driver specific code is
> > > typically (very) closely connected to kernel APIs. If you want to use the safety
> > > aspects of Rust for the driver specific parts you inevitably end up writing
> > > abstractions for the C APIs in your driver.
> > > 
> > > There are basically three options you can end up with:
> > > 
> > > (1) An abstraction for the C API within your driver that is actually generic
> > >     for every driver, and hence shouldn't be there.
> > > (2) Your driver specific code is full of raw pointers and `unsafe {}` calls,
> > >     which in the end just means that you ended up baking the abstraction into
> > >     your driver specific code.
> > > (3) You ignore everything, put everything in a huge `unsafe {}` block and
> > >     compile C code with the Rust compiler. (Admittedly, maybe slightly
> > >     overstated, but not that far off either.)
> > > 
> > > The latter is also the reason why it doesn't make sense to only have
> > > abstractions for some things, but not for other.
> > > 
> > > If an abstraction for B is based on A, but we don't start with A, then B ends up
> > > implementing (at least partially) the abstraction for A as well. For instance,
> > > if we don't implement `driver::Registration` then the PCI abstractions (and
> > > platform, usb, etc.) have to implement it.
> > > 
> > > It really comes down to the point that it just bubbles up. We really have to do
> > > this bottom-up, otherwise we just end up moving those abstractions up, layer by
> > > layer, where they don't belong to and we re-implement them over and over again.
> > 
> > I think we are talking past each other.
> 
> Given your proposal below, that is correct.
> 
> I read your previous mail as if you question having abstractions for C APIs at
> all. And some parts of your latest reply still read like that to me, but for the
> rest of my reply I will assume that's not the case.
> 
> > 
> > Here is an example .c file that you can use today for your "implement a
> > PCI driver in rust" wish in a mere 34 lines of .c code:
> 
> Your proposal below actually only discards a rather small amount of the proposed
> abstractions, in particular:
> 
>   (1) the code to create a struct pci_driver and the code to call
>       pci_register_driver() and pci_unregister_driver() (implemented in Patch 2)
>   (2) the generic code to create the struct pci_device_id table (or any other ID
>       table) (implemented in Patch 3)
> 
> As I understand you, the concern here really seems to be about the complexity
> vs. what we get from it and that someone has to maintain this.
> 
> I got the point and I take it seriously. As already mentioned, I'm also willing
> to take responsibility for the code and offer to maintain it.
> 
> But back to the technical details.
> 
> Let's start with (2):
> 
> I agree that this seems a bit complicated. I'd propose to remove the
> abstractions in device_id.rs (i.e. Patch 3) for now and let the PCI abstraction
> simply create a struct pci_device_id table directly.
> 
> As for (1):
> 
> This just makes a pretty small part of the abstractions and, it's really only
> about creating the struct pci_driver (or another driver type) instance and
> call the corresponding register() / unregister() functions, since we already
> have the Rust module support upstream.
> 
> As in C, we really just call register() from module_init() and unregister() from
> module_exit() and the code for that, without comments, is around 50 lines of
> code.
> 
> As mentioned this is implemented in Patch 2; Hence, please see my reply on Patch
> 2, where I put a rather long and detailed explanation which hopefully clarifies
> things.
> 
> Now, what do we get from that compared to the proposal below?
> 
>   - The driver's probe() and remove() functions get called with the
>     corresponding abstraction types directly instead of raw pointers; this moves
>     the required  `unsafe {}` blocks to a cental place which otherwise *every*
>     driver would need to implement itself (obviously it'd be the same for future
>     suspend, resume, shutdown, etc. callbacks).
> 
>   - More complex drivers can do the work required to be done in module_init()
>     and module_exit() in Rust directly, which allows them to attach the lifetime
>     of structures to the lifetime of the `Module` structure in Rust which avoids
>     the need for explicit cleanup in module_exit() since they can just implement
>     in Rust's drop() trait.
> 
> The latter may sound a bit less important than it actually is, since it can break
> the design, safety and soundness of Rust types. Let me explain:
> 
> In Rust a type instance should cleanup after itself when it goes out of scope.
> 
> Let's make up an example:
> 
> ```
> // DISCLAIMER: Obviously, this is not how we actually handle memory allocations
> // in Rust, it's just an example.
> struct Buffer {
> 	ptr: *const u8,
> }
> 
> impl Buffer {
> 	fn alloc(size: u8) -> Result<Self> {
> 		let ptr = Kmalloc::alloc(size, GFP_KERNEL)?;
> 
> 		// Return an instance of `Buffer` initialized with a pointer to
> 		// the above memory allocation.
> 		Ok(Self {
> 			ptr,
> 		})
> 	}
> }
> 
> impl Drop for Buffer {
> 	fn drop(&mut self) {
> 		// SAFETY: `Self` is always holding a pointer to valid memory
> 		// allocated with `Kmalloc`.
> 		unsafe { Kmalloc::free(self.ptr) };
> 	}
> }
> ```
> 
> (Side note: `Kmalloc::free` is unsafe since it has no control on whether the
> pointer passed to it is valid and was allocated with `Kmalloc` previously.)
> 
> In Rust's module_init() you could now attach an instance of `Buffer` to the
> `Module` instance, which lives until module_exit(), which looks like this:
> 
> ```
> struct MyModule {
> 	buffer: Buffer,
> }
> 
> impl kernel::Moduke for MyModule {
> 	fn init(module: &'static ThisModule) -> Result<Self> {
> 		Ok(MyModule {
> 			buffer: Buffer::alloc(0x100)?,
> 		})
> 	}
> }
> ```
> 
> Note that we don't need to implement module_exit() here, since `MyModule` lives
> until module_exit() and hence `buffer` has the same lifetime. Once `buffer` goes
> out of scope it frees itself due to the drop() trait. In fact, buffer can never
> leak it's memory.
> 
> With the proposed approach below we can't do this, we'd need to play dirty and
> unsafe tricks in order to not entirely break the design of `Buffer`, or any
> other more complex data structure that the module needs.
> 
> > 
> > ------------------------------------------------------------------------
> > // SPDX-License-Identifier: GPL-2.0-only
> > #include <linux/module.h>
> > #include <linux/pci.h>
> > #include "my_pci_rust_bindings.h"
> > 
> > #define PCI_DEVICE_ID_FOO		0x0f00
> > #define PCI_VENDOR_ID_FOO		0x0f00
> > 
> > static int probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > {
> > 	return my_rust_probe(pdev);
> > }
> > 
> > static void remove(struct pci_dev *pdev)
> > {
> > 	my_rust_remove(pdev);
> > }
> > 
> > static const struct pci_device_id pci_ids[] = {
> > 	{PCI_DEVICE(PCI_VENDOR_ID_FOO, PCI_DEVICE_ID_FOO)},
> > 	{}
> > };
> > MODULE_DEVICE_TABLE(pci, pci_ids);
> > 
> > static struct pci_driver my_rust_pci_driver = {
> > 	.name = "my_pci_rust_driver",
> > 	.id_table = pci_ids,
> > 	.probe = probe,
> > 	.remove = remove,
> > };
> > module_pci_driver(my_rust_pci_driver);
> > 
> > MODULE_DESCRIPTION("Driver for my fancy PCI device");
> > MODULE_LICENSE("GPL v2");
> > ------------------------------------------------------------------------
> > 
> > Now, all you have to do is provide a my_rust_probe() and
> > my_rust_remove() call in your rust code, and handle the conversion of a
> > struct pci_dev to whatever you want to use (which you had to do anyway),
> > and you are set!
> > 
> > That .c code above is "obviously" correct, and much simpler and worlds
> > easier for all of us to understand instead of the huge .rs files that
> > this patch series was attempting to implement.
> > 
> > You have to draw the c/rust line somewhere, you all seem to be wanting
> > to draw that line at "no .c in my module at all!" and I'm saying "put 34
> > lines of .c in your module and you will save a LOT of headache now."
> > 
> > All of the "real" work you want to do for your driver is behind the
> > probe/remove callbacks (ok, add a suspend/resume as well, forgot that),
> > and stop worrying about all of the bindings and other mess to tie into
> > the driver model for a specific bus type and the lunacy that the device
> > id mess was.
> > 
> > In short, KEEP IT SIMPLE!
> > 
> > Then, after we are comfortable with stuff like the above, slowly think
> > about moving the line back a bit more, if you really even need to.  But
> > why would you need to?  Only reason I can think of is if you wanted to
> > write an entire bus in rust, and for right now, I would strongly suggest
> > anyone not attempt that.
> 
> We're not trying to do that. Also, please note that all the abstractions are
> *only* making use of APIs that C drivers use directly, just abstracting them
> in a way, such that we can actually use the strength of Rust.
> 
> > 
> > The "joy" of writing a driver in rust is that a driver consumes from
> > EVERYWHERE in the kernel (as you well know.)  Having to write bindings
> > and mappings for EVERYTHING all at once, when all you really want to do
> > is implement the logic between probe/remove is rough, I'm sorry.  I'm
> > suggesting that you stop at the above line for now, which should make
> > your life easier as the .c code is obviously correct, and anything you
> > do in the rust side is your problem, not mine as a bus maintainer :)
> 
> As explained above, we already have the module abstraction in place. Really all
> that we're left with is creating the struct pci_driver and call register() /
> remove() from it.
> 
> Now, this could be fully done in the PCI abstraction. The only reason we have
> the `Registration` and `DriverOps` (again there are a few misunderstandings
> around that, that I try to clarify in the corresponding thread) in "driver.rs"
> is to not duplicate code for maintainability.
> 
> What I'm trying to say, the stuff in "driver.rs" is not even abstracting things
> from drivers/base/, but is Rust helper code for subsystems to abstract their
> stuff (e.g. struct pci_driver).
> 
> And again, I'm willing to take responsibility for the code and offer to maintain
> it - I really want to do this the proper way.
> 
> - Danilo
> 
> (cutting of the rest since it was based on the misunderstanding mentioned above)

Re: [PATCH v2 07/10] rust: add `io::Io` base type

[Daniel Almeida]

Hi Danilo,

From a Rust API point of view, this looks good to me.

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Cheers,

— Daniel