| Message ID | 20240520172554.182094-3-dakr@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Device / Driver and PCI Rust abstractions | expand |
On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > This defines general functionality related to registering drivers with > their respective subsystems, and registering modules that implement > drivers. > > Co-developed-by: Asahi Lina <lina@asahilina.net> > Signed-off-by: Asahi Lina <lina@asahilina.net> > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 4 +- > rust/macros/module.rs | 2 +- > samples/rust/rust_minimal.rs | 2 +- > samples/rust/rust_print.rs | 2 +- > 5 files changed, 498 insertions(+), 4 deletions(-) > 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..e0cfc36d47ff > --- /dev/null > +++ b/rust/kernel/driver.rs > @@ -0,0 +1,492 @@ > +// 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. Why are you creating new "names" here? "DriverOps" is part of a 'struct device_driver' why are you separating it out here? And what is 'Registration'? That's a bus/class thing, not a driver thing. And be very careful of the use of the word 'class' here, remember, there is 'struct class' as part of the driver model :) > +use crate::{ > + alloc::{box_ext::BoxExt, flags::*}, > + error::code::*, > + error::Result, > + str::CStr, > + sync::Arc, > + ThisModule, > +}; > +use alloc::boxed::Box; > +use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin}; > + > +/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it. > +pub trait DriverOps { Again, why is this not called DeviceDriver? > + /// 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`]. > + unsafe 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`]. > + unsafe fn unregister(reg: *mut Self::RegType); > +} > + > +/// The registration of a driver. > +pub struct Registration<T: DriverOps> { > + is_registered: bool, Why does a driver need to know if it is registered or not? Only the driver core cares about that, please do not expose that, it's racy and should not be relied on. > + concrete_reg: UnsafeCell<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> {} > + > +impl<T: DriverOps> Registration<T> { > + /// Creates a new instance of the registration object. > + pub fn new() -> Self { > + Self { > + is_registered: false, > + concrete_reg: UnsafeCell::new(T::RegType::default()), > + } > + } > + > + /// Allocates a pinned registration object and registers it. > + /// > + /// Returns a pinned heap-allocated representation of the registration. > + pub fn new_pinned(name: &'static CStr, module: &'static ThisModule) -> Result<Pin<Box<Self>>> { > + let mut reg = Pin::from(Box::new(Self::new(), GFP_KERNEL)?); > + reg.as_mut().register(name, module)?; > + Ok(reg) > + } > + > + /// Registers a driver with its subsystem. > + /// > + /// It must be pinned because the memory block that represents the registration is potentially > + /// self-referential. > + pub fn register( > + self: Pin<&mut Self>, > + name: &'static CStr, > + module: &'static ThisModule, > + ) -> Result { > + // SAFETY: We never move out of `this`. > + let this = unsafe { self.get_unchecked_mut() }; > + if this.is_registered { > + // Already registered. > + return Err(EINVAL); > + } > + > + // SAFETY: `concrete_reg` was initialised via its default constructor. It is only freed > + // after `Self::drop` is called, which first calls `T::unregister`. > + unsafe { T::register(this.concrete_reg.get(), name, module) }?; > + > + this.is_registered = true; Again, the driver core knows if it is registered or not, that's all that matters, the rust side should never care. > + Ok(()) > + } > +} > + > +impl<T: DriverOps> Default for Registration<T> { > + fn default() -> Self { > + Self::new() > + } > +} > + > +impl<T: DriverOps> Drop for Registration<T> { > + fn drop(&mut self) { > + if self.is_registered { > + // SAFETY: This path only runs if a previous call to `T::register` completed > + // successfully. > + unsafe { T::unregister(self.concrete_reg.get()) }; Can't the rust code ensure that this isn't run if register didn't succeed? Having a boolean feels really wrong here (can't that race?) > + } > + } > +} > + > +/// 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. > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > +/// that buses can recover the pointer to the data. > +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; All busses have their own way of creating "ids" and that is limited to the bus code itself, why is any of this in the rust side? What needs this? A bus will create the id for the devices it manages, and can use it as part of the name it gives the device (but not required), so all of this belongs to the bus, NOT to a driver, or a device. > +} > + > +/// A zero-terminated device id array, followed by context data. Why? What is this for? > +#[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 as a parameter the concrete ID type 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. Again, what are these id lists for? Busses work on individual ids that they create dynamically. You aren't thinking this is an id that could be used to match devices to drivers, are you? That's VERY bus specific, and also specified already in .c code for those busses and passed to userspace. That doesn't belong here. If this isn't that list, what exactly is this? > +#[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::driver::IdArray<$id_type, U, N> > + where > + $id_type: $crate::driver::RawDeviceId + Copy, > + <$id_type as $crate::driver::RawDeviceId>::RawType: Copy + Clone, > + { > + let mut raw_ids = > + [<$id_type as $crate::driver::RawDeviceId>::ZERO; N]; > + let mut i = 0usize; > + while i < N { > + let offset: isize = $crate::driver::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::driver::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>, > +} All busses have different ways of matching drivers to devices, and they might be called a "device id table" and it might not. The driver core doesn't care, and neither should this rust code. That's all bus-specific and unique to each and every one of them. None of this should be needed here at all. > + > +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 }; > +} Shouldn't this go in some common header somewhere? Why is this here? > + > +/// 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,)*] }; > +} Again, why here? > + > +/// 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,)*] }; > +} Again, why here? > + > +/// 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 > +/// > +// TODO: Exported but not usable by kernel modules (requires `const_trait_impl`). > +/// ```ignore > +/// #![feature(const_trait_impl)] > +/// # use kernel::{define_id_array, driver::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 const RawDeviceId for Id { > +/// type RawType = (u64, isize); > +/// const ZERO: Self::RawType = (0, 0); > +/// fn to_rawid(&self, offset: isize) -> Self::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), ]); > +/// ``` Again, busses define this, put this in the bus-specific code that you wish to define/match with, it does not belong here as every bus does it differently. Not all the world is PCI :) > +#[macro_export] > +macro_rules! define_id_array { > + ($table_name:ident, $id_type:ty, $data_type:ty, [ $($t:tt)* ]) => { > + const $table_name: > + $crate::driver::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 > +/// > +// TODO: Exported but not usable by kernel modules (requires `const_trait_impl`). > +/// ```ignore > +/// #![feature(const_trait_impl)] > +/// # use kernel::{define_id_table, driver::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 const RawDeviceId for Id { > +/// type RawType = (u64, isize); > +/// const ZERO: Self::RawType = (0, 0); > +/// fn to_rawid(&self, offset: isize) -> Self::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::driver::IdTable<'static, $id_type, $data_type>> = { > + $crate::define_id_array!(ARRAY, $id_type, $data_type, [ $($t)* ]); > + Some(ARRAY.as_table()) > + }; > + }; > +} Again, see above, does not belong here. > + > +/// Custom code within device removal. You better define the heck out of "device removal" as specified last time this all came up. From what I can see here, this is totally wrong and confusing and will be a mess. Do it right, name it properly. I'm not reviewingn beyond here, sorry. It's the merge window and I shouldn't have even looked at this until next week anyway. But I was hoping that the whole long rant I gave last time would be addressed at least a little bit. I don't see that it has :( > +pub trait DeviceRemoval { > + /// Cleans resources up when the device is removed. > + /// > + /// This is called when a device is removed and offers implementers the chance to run some code > + /// that cleans state up. > + fn device_remove(&self); > +} > + > +impl DeviceRemoval for () { > + fn device_remove(&self) {} > +} > + > +impl<T: DeviceRemoval> DeviceRemoval for Arc<T> { > + fn device_remove(&self) { > + self.deref().device_remove(); > + } > +} > + > +impl<T: DeviceRemoval> DeviceRemoval for Box<T> { > + fn device_remove(&self) { > + self.deref().device_remove(); > + } > +} > + > +/// 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. > +pub struct Module<T: DriverOps> { > + _driver: Pin<Box<Registration<T>>>, > +} > + > +impl<T: DriverOps> crate::Module for Module<T> { > + fn init(name: &'static CStr, module: &'static ThisModule) -> Result<Self> { > + Ok(Self { > + _driver: Registration::new_pinned(name, module)?, > + }) > + } > +} > + > +/// Declares a kernel module that exposes a single driver. There is no requirement that a kernel module only exposes a single driver, so what exactly is this used for? Is this some attempt to make the module_driver() macro in rust? Why? Only add something like that if you really need it (i.e. you have a bus in rust and you want the benifit of having that macro to save you some lines of code.) I don't see that happening here... greg k-h
On Mon, May 20, 2024 at 08:14:18PM +0200, Greg KH wrote: > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > This defines general functionality related to registering drivers with > > their respective subsystems, and registering modules that implement > > drivers. > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > rust/kernel/lib.rs | 4 +- > > rust/macros/module.rs | 2 +- > > samples/rust/rust_minimal.rs | 2 +- > > samples/rust/rust_print.rs | 2 +- > > 5 files changed, 498 insertions(+), 4 deletions(-) > > 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..e0cfc36d47ff > > --- /dev/null > > +++ b/rust/kernel/driver.rs > > @@ -0,0 +1,492 @@ > > +// 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. > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > device_driver' why are you separating it out here? And what is DriverOps is a trait which abstracts a subsystems register() and unregister() functions to (un)register drivers. It exists such that a generic Registration implementation calls the correct one for the subsystem. For instance, PCI would implement DriverOps::register() by calling into bindings::__pci_register_driver(). We can discuss whether DriverOps is a good name for the trait, but it's not a (different) name for something that already exists and already has a name. > 'Registration'? That's a bus/class thing, not a driver thing. A Registration is an object representation of the driver's registered state. Having an object representation for that is basically the Rust way to manage the lifetime of this state. Once the Registration is dropped, the driver is unregistered. For instance, a PCI driver Registration can be bound to a module, such that the PCI driver is unregistered when the module is unloaded (the Registration is dropped in the module_exit() callback). > > And be very careful of the use of the word 'class' here, remember, there > is 'struct class' as part of the driver model :) I think in this context "class" might be meant as something like "struct with methods", not sure what would be the best term to describe this. > > > +use crate::{ > > + alloc::{box_ext::BoxExt, flags::*}, > > + error::code::*, > > + error::Result, > > + str::CStr, > > + sync::Arc, > > + ThisModule, > > +}; > > +use alloc::boxed::Box; > > +use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin}; > > + > > +/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it. > > +pub trait DriverOps { > > Again, why is this not called DeviceDriver? See above. > > > + /// 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`]. > > + unsafe 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`]. > > + unsafe fn unregister(reg: *mut Self::RegType); > > +} > > + > > +/// The registration of a driver. > > +pub struct Registration<T: DriverOps> { > > + is_registered: bool, > > Why does a driver need to know if it is registered or not? Only the > driver core cares about that, please do not expose that, it's racy and > should not be relied on. We need it to ensure we do not try to register the same thing twice, some subsystems might just catch fire otherwise. Another reason is the one you mention below. > > > + concrete_reg: UnsafeCell<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> {} > > + > > +impl<T: DriverOps> Registration<T> { > > + /// Creates a new instance of the registration object. > > + pub fn new() -> Self { > > + Self { > > + is_registered: false, > > + concrete_reg: UnsafeCell::new(T::RegType::default()), > > + } > > + } > > + > > + /// Allocates a pinned registration object and registers it. > > + /// > > + /// Returns a pinned heap-allocated representation of the registration. > > + pub fn new_pinned(name: &'static CStr, module: &'static ThisModule) -> Result<Pin<Box<Self>>> { > > + let mut reg = Pin::from(Box::new(Self::new(), GFP_KERNEL)?); > > + reg.as_mut().register(name, module)?; > > + Ok(reg) > > + } > > + > > + /// Registers a driver with its subsystem. > > + /// > > + /// It must be pinned because the memory block that represents the registration is potentially > > + /// self-referential. > > + pub fn register( > > + self: Pin<&mut Self>, > > + name: &'static CStr, > > + module: &'static ThisModule, > > + ) -> Result { > > + // SAFETY: We never move out of `this`. > > + let this = unsafe { self.get_unchecked_mut() }; > > + if this.is_registered { > > + // Already registered. > > + return Err(EINVAL); > > + } > > + > > + // SAFETY: `concrete_reg` was initialised via its default constructor. It is only freed > > + // after `Self::drop` is called, which first calls `T::unregister`. > > + unsafe { T::register(this.concrete_reg.get(), name, module) }?; > > + > > + this.is_registered = true; > > Again, the driver core knows if it is registered or not, that's all that > matters, the rust side should never care. > > > + Ok(()) > > + } > > +} > > + > > +impl<T: DriverOps> Default for Registration<T> { > > + fn default() -> Self { > > + Self::new() > > + } > > +} > > + > > +impl<T: DriverOps> Drop for Registration<T> { > > + fn drop(&mut self) { > > + if self.is_registered { > > + // SAFETY: This path only runs if a previous call to `T::register` completed > > + // successfully. > > + unsafe { T::unregister(self.concrete_reg.get()) }; > > Can't the rust code ensure that this isn't run if register didn't > succeed? Having a boolean feels really wrong here (can't that race?) No, we want to automatically unregister once this object is destroyed, but not if it was never registered in the first place. This shouldn't be racy, we only ever (un)register things in places like probe() / remove() or module_init() / module_exit(). > > > + } > > + } > > +} > > + > > +/// 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. > > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > > +/// that buses can recover the pointer to the data. > > +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; > > All busses have their own way of creating "ids" and that is limited to > the bus code itself, why is any of this in the rust side? What needs > this? A bus will create the id for the devices it manages, and can use > it as part of the name it gives the device (but not required), so all of > this belongs to the bus, NOT to a driver, or a device. This is just a generalized interface which can be used by subsystems to implement the subsystem / bus specific ID type. > > > +} > > + > > +/// A zero-terminated device id array, followed by context data. > > Why? What is this for? An array of generic ID types implementing the RawDeviceId interface. It's used as generic implementation for the pattern of having some (generic) ID type that ends up in an array with some sentinel marking the end of it. > > +#[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 as a parameter the concrete ID type 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. > > Again, what are these id lists for? Busses work on individual ids that > they create dynamically. > > You aren't thinking this is an id that could be used to match devices to > drivers, are you? That's VERY bus specific, and also specified already > in .c code for those busses and passed to userspace. That doesn't > belong here. It is indeed a *generic* representation of IDs to match drivers to a device. It's up the the corresponding subsystem to fill it with an ID type that matches the subsystem. The reason we need this in Rust is that also Rust drivers need to provide those IDs on registration with the subsystem. This is just the abstraction for that. In this patch series this is implemented for PCI [1], but, for instance, it'd also work for OF [1] with 'compatible' strings. [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-10-dakr@redhat.com/ [2] https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/of.rs > > If this isn't that list, what exactly is this? > > > +#[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::driver::IdArray<$id_type, U, N> > > + where > > + $id_type: $crate::driver::RawDeviceId + Copy, > > + <$id_type as $crate::driver::RawDeviceId>::RawType: Copy + Clone, > > + { > > + let mut raw_ids = > > + [<$id_type as $crate::driver::RawDeviceId>::ZERO; N]; > > + let mut i = 0usize; > > + while i < N { > > + let offset: isize = $crate::driver::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::driver::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>, > > +} > > All busses have different ways of matching drivers to devices, and they > might be called a "device id table" and it might not. The driver core > doesn't care, and neither should this rust code. That's all > bus-specific and unique to each and every one of them. None of this > should be needed here at all. Again, this is just a generic abstraction that needs to be filled with subsystem / bus specific ID types by the subsystem code. It's also not matching anything on the Rust side. It's just a helper to let drivers provide IDs codes / types that can be passed to the C side subsystem to do the match. > > > + > > +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 }; > > +} > > Shouldn't this go in some common header somewhere? Why is this here? There are no headers in Rust. This seems to be the correct place for this macro (and the ones below). > > > + > > +/// 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,)*] }; > > +} > > Again, why here? > > > + > > +/// 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,)*] }; > > +} > > Again, why here? > > > + > > +/// 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 > > +/// > > +// TODO: Exported but not usable by kernel modules (requires `const_trait_impl`). > > +/// ```ignore > > +/// #![feature(const_trait_impl)] > > +/// # use kernel::{define_id_array, driver::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 const RawDeviceId for Id { > > +/// type RawType = (u64, isize); > > +/// const ZERO: Self::RawType = (0, 0); > > +/// fn to_rawid(&self, offset: isize) -> Self::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), ]); > > +/// ``` > > Again, busses define this, put this in the bus-specific code that you > wish to define/match with, it does not belong here as every bus does it > differently. Not all the world is PCI :) The macro below is just a helper macro for the subsystem / bus specific code to define its own macro that can be used by drivers to provide the ID table to match. In [1] PCI uses this macro to define its own one (define_pci_id_table() and in [2] Nova uses define_pci_id_table() to define the matching PCI devices. [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-10-dakr@redhat.com/ [2] https://lore.kernel.org/rust-for-linux/20240520172422.181763-5-dakr@redhat.com/ > > > +#[macro_export] > > +macro_rules! define_id_array { > > + ($table_name:ident, $id_type:ty, $data_type:ty, [ $($t:tt)* ]) => { > > + const $table_name: > > + $crate::driver::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 > > +/// > > +// TODO: Exported but not usable by kernel modules (requires `const_trait_impl`). > > +/// ```ignore > > +/// #![feature(const_trait_impl)] > > +/// # use kernel::{define_id_table, driver::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 const RawDeviceId for Id { > > +/// type RawType = (u64, isize); > > +/// const ZERO: Self::RawType = (0, 0); > > +/// fn to_rawid(&self, offset: isize) -> Self::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::driver::IdTable<'static, $id_type, $data_type>> = { > > + $crate::define_id_array!(ARRAY, $id_type, $data_type, [ $($t)* ]); > > + Some(ARRAY.as_table()) > > + }; > > + }; > > +} > > Again, see above, does not belong here. > > > + > > +/// Custom code within device removal. > > You better define the heck out of "device removal" as specified last > time this all came up. From what I can see here, this is totally wrong > and confusing and will be a mess. > > Do it right, name it properly. Please also see my reply [1] on your reply to another patch of this series. Otherwise, what's wrong with this name? device_remove() is supposed to be called once the driver's remove() callback is called. It doesn't seem uncommon to use this terminology. For instance, in the documentation of struct pci_driver [2] it says: "The remove() function gets called whenever a device being handled by this driver is removed [...]". In the documentation of struct device_driver [3] it says: "Called when the device is removed from the system to unbind a device from this driver.". Don't get me wrong, I'm happy to change it, but I think you should make me understand why this terminology is widely used in the kernel, but here it's totally wrong. [1] https://lore.kernel.org/rust-for-linux/ZkupaTM+xjCiBbb4@pollux.localdomain/ [2] https://elixir.bootlin.com/linux/latest/source/include/linux/pci.h#L905 [3] https://elixir.bootlin.com/linux/latest/source/include/linux/device/driver.h#L71 > > I'm not reviewingn beyond here, sorry. It's the merge window and I > shouldn't have even looked at this until next week anyway. > > But I was hoping that the whole long rant I gave last time would be > addressed at least a little bit. I don't see that it has :( I think it's not really fair to expect this documentation to be rewritten when you just back out of the discussion before getting to an agreement. Especially when the situation at least looks contradictive as presented above. I take your concerns serious and spend a lot of time in answering your questions and giving explanations - let's keep the discussion up! > > > > +pub trait DeviceRemoval { > > + /// Cleans resources up when the device is removed. > > + /// > > + /// This is called when a device is removed and offers implementers the chance to run some code > > + /// that cleans state up. > > + fn device_remove(&self); > > +} > > + > > +impl DeviceRemoval for () { > > + fn device_remove(&self) {} > > +} > > + > > +impl<T: DeviceRemoval> DeviceRemoval for Arc<T> { > > + fn device_remove(&self) { > > + self.deref().device_remove(); > > + } > > +} > > + > > +impl<T: DeviceRemoval> DeviceRemoval for Box<T> { > > + fn device_remove(&self) { > > + self.deref().device_remove(); > > + } > > +} > > + > > +/// 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. > > +pub struct Module<T: DriverOps> { > > + _driver: Pin<Box<Registration<T>>>, > > +} > > + > > +impl<T: DriverOps> crate::Module for Module<T> { > > + fn init(name: &'static CStr, module: &'static ThisModule) -> Result<Self> { > > + Ok(Self { > > + _driver: Registration::new_pinned(name, module)?, > > + }) > > + } > > +} > > + > > +/// Declares a kernel module that exposes a single driver. > > There is no requirement that a kernel module only exposes a single > driver, so what exactly is this used for? Is this some attempt to make > the module_driver() macro in rust? Why? Only add something like that > if you really need it (i.e. you have a bus in rust and you want the > benifit of having that macro to save you some lines of code.) I don't > see that happening here... It's just a macro helper for subsystems to cover the common case of a single driver in a module. Kinda the generic Rust version of e.g. module_pci_driver() or module_usb_driver(). There is no general limitation of a single driver per module. > > greg k-h >
On Tue, 21 May 2024 at 04:14, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > This defines general functionality related to registering drivers with > > their respective subsystems, and registering modules that implement > > drivers. > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > rust/kernel/lib.rs | 4 +- > > rust/macros/module.rs | 2 +- > > samples/rust/rust_minimal.rs | 2 +- > > samples/rust/rust_print.rs | 2 +- > > 5 files changed, 498 insertions(+), 4 deletions(-) > > 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..e0cfc36d47ff > > --- /dev/null > > +++ b/rust/kernel/driver.rs > > @@ -0,0 +1,492 @@ > > +// 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. > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > device_driver' why are you separating it out here? And what is > 'Registration'? That's a bus/class thing, not a driver thing. > > And be very careful of the use of the word 'class' here, remember, there > is 'struct class' as part of the driver model :) > > > +use crate::{ > > + alloc::{box_ext::BoxExt, flags::*}, > > + error::code::*, > > + error::Result, > > + str::CStr, > > + sync::Arc, > > + ThisModule, > > +}; > > +use alloc::boxed::Box; > > +use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin}; > > + > > +/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it. > > +pub trait DriverOps { > > Again, why is this not called DeviceDriver? This is not the same as the C device_driver, it might mildly align in concept with it, but I'm not sure it shares enough to align it name wise with the C one. > > + /// 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`]. > > + unsafe 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`]. > > + unsafe fn unregister(reg: *mut Self::RegType); > > +} > > + > > +/// The registration of a driver. > > +pub struct Registration<T: DriverOps> { > > + is_registered: bool, > > Why does a driver need to know if it is registered or not? Only the > driver core cares about that, please do not expose that, it's racy and > should not be relied on. From the C side this does look unusual because on the C side something like struct pci_driver is statically allocated everywhere. In this rust abstraction, these are allocated dynamically, so instead of just it always being safe to just call register/unregister with static memory, a flag is kept around to say if the unregister should happen at all, as the memory may have been allocated but never registered. This is all the Registration is for, it's tracking the bus _driver structure allocation, and whether the bus register/unregister have been called since the object was allocated. I'm not sure it makes sense (or if you can at all), have a static like pci_driver object here to match how C does things. > > +impl<T: DriverOps> Drop for Registration<T> { > > + fn drop(&mut self) { > > + if self.is_registered { > > + // SAFETY: This path only runs if a previous call to `T::register` completed > > + // successfully. > > + unsafe { T::unregister(self.concrete_reg.get()) }; > > Can't the rust code ensure that this isn't run if register didn't > succeed? Having a boolean feels really wrong here (can't that race?) There might be a way of using Option<> here but I don't think it adds anything over and above using an explicit bool. > > +/// > > +/// 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. > > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > > +/// that buses can recover the pointer to the data. > > +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; > > All busses have their own way of creating "ids" and that is limited to > the bus code itself, why is any of this in the rust side? What needs > this? A bus will create the id for the devices it manages, and can use > it as part of the name it gives the device (but not required), so all of > this belongs to the bus, NOT to a driver, or a device. Consider this a base class (Trait) for bus specific IDs. > > +} > > + > > +// Creates a new ID array. This is a macro so it can take as a parameter the concrete ID type 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. > > Again, what are these id lists for? Busses work on individual ids that > they create dynamically. > > You aren't thinking this is an id that could be used to match devices to > drivers, are you? That's VERY bus specific, and also specified already > in .c code for those busses and passed to userspace. That doesn't > belong here. > > If this isn't that list, what exactly is this? Rust traits have to be specified somewhere, a RawDeviceId is the basis for other DeviceId Traits to be implemented on top off, those then talk to the C side ones. All the pci and nova code is posted as well for you to work this out, like you asked for. The use cases are all there to be reviewed, and should help answer these sorts of questions. > > > +#[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::driver::IdArray<$id_type, U, N> > > + where > > + $id_type: $crate::driver::RawDeviceId + Copy, > > + <$id_type as $crate::driver::RawDeviceId>::RawType: Copy + Clone, > > + { > > + let mut raw_ids = > > + [<$id_type as $crate::driver::RawDeviceId>::ZERO; N]; > > + let mut i = 0usize; > > + while i < N { > > + let offset: isize = $crate::driver::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::driver::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>, > > +} > > All busses have different ways of matching drivers to devices, and they > might be called a "device id table" and it might not. The driver core > doesn't care, and neither should this rust code. That's all > bus-specific and unique to each and every one of them. None of this > should be needed here at all. This is just a base Trait, for the bus specifics to implement. Consider it a bunch of C macros or a bunch of C++ vfuncs, whatever mental model helps more. > Shouldn't this go in some common header somewhere? Why is this here? Again to reiterate what Danilo said, rust has no header files. Things that in C we would put in header files (macros, struct definitions, inlines) go into rust code and other rust code references it. Now there might be some sense in splitting things further into separate rust files if this concept is going to confuse people, and what is "core" device code, and what is "template" device code. But I'm not sure how much sense that makes really. Maybe the device table stuff should be in a separate file? > > +/// Custom code within device removal. > > You better define the heck out of "device removal" as specified last > time this all came up. From what I can see here, this is totally wrong > and confusing and will be a mess. > > Do it right, name it properly. > > I'm not reviewingn beyond here, sorry. It's the merge window and I > shouldn't have even looked at this until next week anyway. > > But I was hoping that the whole long rant I gave last time would be > addressed at least a little bit. I don't see that it has :( I won't comment too much on the specifics here, but you've twice got to this stage, said something is wrong, change it, and given no actionable feedback on what is wrong, or what to change. I've looked at this code for a few hours today with the device docs and core code and can't spot what you are seeing that is wrong here, which means I don't expect anyone else is going to unless you can help educate us more. Dave.
On Tue, May 21, 2024 at 03:42:50PM +1000, Dave Airlie wrote: > On Tue, 21 May 2024 at 04:14, Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > > > This defines general functionality related to registering drivers with > > > their respective subsystems, and registering modules that implement > > > drivers. > > > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > > rust/kernel/lib.rs | 4 +- > > > rust/macros/module.rs | 2 +- > > > samples/rust/rust_minimal.rs | 2 +- > > > samples/rust/rust_print.rs | 2 +- > > > 5 files changed, 498 insertions(+), 4 deletions(-) > > > 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..e0cfc36d47ff > > > --- /dev/null > > > +++ b/rust/kernel/driver.rs > > > @@ -0,0 +1,492 @@ > > > +// 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. > > > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > > device_driver' why are you separating it out here? And what is > > 'Registration'? That's a bus/class thing, not a driver thing. > > > > And be very careful of the use of the word 'class' here, remember, there > > is 'struct class' as part of the driver model :) > > > > > +use crate::{ > > > + alloc::{box_ext::BoxExt, flags::*}, > > > + error::code::*, > > > + error::Result, > > > + str::CStr, > > > + sync::Arc, > > > + ThisModule, > > > +}; > > > +use alloc::boxed::Box; > > > +use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin}; > > > + > > > +/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it. > > > +pub trait DriverOps { > > > > Again, why is this not called DeviceDriver? > > This is not the same as the C device_driver, it might mildly align in > concept with it, but I'm not sure it shares enough to align it name > wise with the C one. Why not use the same terms and design decisions that the C code has? To differ needs to have a good reason, otherwise it's just going to cause us all confusion as we have to learn two different terms for the same thing. > > > + /// 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`]. > > > + unsafe 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`]. > > > + unsafe fn unregister(reg: *mut Self::RegType); > > > +} > > > + > > > +/// The registration of a driver. > > > +pub struct Registration<T: DriverOps> { > > > + is_registered: bool, > > > > Why does a driver need to know if it is registered or not? Only the > > driver core cares about that, please do not expose that, it's racy and > > should not be relied on. > > >From the C side this does look unusual because on the C side something > like struct pci_driver is statically allocated everywhere. > In this rust abstraction, these are allocated dynamically, so instead > of just it always being safe to just call register/unregister > with static memory, a flag is kept around to say if the unregister > should happen at all, as the memory may have > been allocated but never registered. This is all the Registration is > for, it's tracking the bus _driver structure allocation, and > whether the bus register/unregister have been called since the object > was allocated. > > I'm not sure it makes sense (or if you can at all), have a static like > pci_driver object here to match how C does things. Wait, why can't you have a static "rust_driver" type thing? Having it be in ram feels like a waste of memory. We are working hard to move more and more of these driver model structures into read-only memory for good reasons (security, reduce bugs, etc.), moving backwards to having them all be dynamically created/copied around feels wrong. We are one stage away from being able to mark all 'driver_*()' calls as using a const * to struct driver, please don't make that work pointless if you want to write a driver in rust instead. And again, a driver should never know/care/wonder if it has been registered or not, that is up to the driver core to handle, not the rust code, as it is the only thing that can know this, and the only thing that should need to know it. A driver structure should not have any dynamic memory associated with it to require knowing its "state" in the system, so let's not move backwards here to require that just because we are using Rust. > > > +impl<T: DriverOps> Drop for Registration<T> { > > > + fn drop(&mut self) { > > > + if self.is_registered { > > > + // SAFETY: This path only runs if a previous call to `T::register` completed > > > + // successfully. > > > + unsafe { T::unregister(self.concrete_reg.get()) }; > > > > Can't the rust code ensure that this isn't run if register didn't > > succeed? Having a boolean feels really wrong here (can't that race?) > > There might be a way of using Option<> here but I don't think it adds > anything over and above using an explicit bool. Again, this should not be needed. If so, something is designed incorrectly with the bindings. > > > +/// > > > +/// 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. > > > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > > > +/// that buses can recover the pointer to the data. > > > +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; > > > > All busses have their own way of creating "ids" and that is limited to > > the bus code itself, why is any of this in the rust side? What needs > > this? A bus will create the id for the devices it manages, and can use > > it as part of the name it gives the device (but not required), so all of > > this belongs to the bus, NOT to a driver, or a device. > > Consider this a base class (Trait) for bus specific IDs. Then all of that should be in a separate file as they belong to a "bus" not a driver, as each and every bus will have a different way of expressing the list of devices a driver can bind to. And again, the core "struct device_driver" does not have a list of ids, and neither should the rust bindings, that belongs to the bus logic. > > > +/// Custom code within device removal. > > > > You better define the heck out of "device removal" as specified last > > time this all came up. From what I can see here, this is totally wrong > > and confusing and will be a mess. > > > > Do it right, name it properly. > > > > I'm not reviewingn beyond here, sorry. It's the merge window and I > > shouldn't have even looked at this until next week anyway. > > > > But I was hoping that the whole long rant I gave last time would be > > addressed at least a little bit. I don't see that it has :( > > I won't comment too much on the specifics here, but you've twice got > to this stage, said something is wrong, change it, and given no > actionable feedback on what is wrong, or what to change. > > I've looked at this code for a few hours today with the device docs > and core code and can't spot what you are seeing that is wrong here, > which means I don't expect anyone else is going to unless you can help > educate us more. A lifecycle of a device is: device create by the bus device register with driver core device bind to a driver (i.e. probe() callback in the driver) device unbind from a driver (i.e. remove() callback in the driver, can be triggered many ways) device destroy by the bus "remove" can happen for a driver where it needs to clean up everything it has done for a specific device, but that does not mean the device is actually gone from the system, that's up to the bus to decide, as it owns the device lifecycle and gets to decide when it thinks it is really gone. And then, when the bus tells the driver core that it wants to mark the device as "destroyed", it's up to the driver core to eventually call back into the bus to really clean that device up from the system at some later point in time. Try splitting this file out into driver and bus and device logic, like the driver core has, and see if that helps in explaining and making all of this more understandable, and to keep with the names/design of the model we currently have. Note, all of this is my biggest worry about writing a driver in rust, getting the lifecycle of the driver core and it's logic of how it handles memory manged in C, to align up with how rust is going to access it with its different rules and requirements, is not going to be simple as you have two different models intersecting at the same place. I've been working over the past year to make the rust side happen easier by marking more and more of the C structures as "not mutable", i.e. const *, so that the Rust side doesn't have to worry that the driver core really will change things it passes to the C side, but there is more to be done (see above about making struct device_driver * const). I want to see this happen, but it's going to be a hard slog due to the different expectations of these two systems. Keeping naming identical where possible is one way to make it simpler for everyone involved, as would splitting the logic out the same way and not mixing it all up into one big hairy file that combines different things into a single chunk. thanks, greg k-h
On Tue, May 21, 2024 at 12:30:37AM +0200, Danilo Krummrich wrote: > On Mon, May 20, 2024 at 08:14:18PM +0200, Greg KH wrote: > > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > > > This defines general functionality related to registering drivers with > > > their respective subsystems, and registering modules that implement > > > drivers. > > > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > > rust/kernel/lib.rs | 4 +- > > > rust/macros/module.rs | 2 +- > > > samples/rust/rust_minimal.rs | 2 +- > > > samples/rust/rust_print.rs | 2 +- > > > 5 files changed, 498 insertions(+), 4 deletions(-) > > > 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..e0cfc36d47ff > > > --- /dev/null > > > +++ b/rust/kernel/driver.rs > > > @@ -0,0 +1,492 @@ > > > +// 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. > > > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > > device_driver' why are you separating it out here? And what is > > DriverOps is a trait which abstracts a subsystems register() and unregister() > functions to (un)register drivers. It exists such that a generic Registration > implementation calls the correct one for the subsystem. > > For instance, PCI would implement DriverOps::register() by calling into > bindings::__pci_register_driver(). > > We can discuss whether DriverOps is a good name for the trait, but it's not a > (different) name for something that already exists and already has a name. It's a name we don't have in the C code as the design of the driver core does not need or provide it. It's just the section of 'struct device_driver' that provides function callbacks, why does it need to be separate at all? > > 'Registration'? That's a bus/class thing, not a driver thing. > > A Registration is an object representation of the driver's registered state. And that representation should not ever need to be tracked by the driver, that's up to the driver core to track that. > Having an object representation for that is basically the Rust way to manage the > lifetime of this state. This all should be a static chunk of read-only memory that should never have a lifetime, why does it need to be managed at all? > Once the Registration is dropped, the driver is > unregistered. For instance, a PCI driver Registration can be bound to a module, > such that the PCI driver is unregistered when the module is unloaded (the > Registration is dropped in the module_exit() callback). Ok, that's fine, but note that your module_exit() function will never be called if your module_init() callback fails, so why do you need to track this state? Again, C drivers never need to track this state, why is rust requiring more logic here for something that is NOT a dynamic chunk of memory (or should not be a dynamic chunk of memory, let's get it correct from the start and not require us to change things later on to make it more secure). > > And be very careful of the use of the word 'class' here, remember, there > > is 'struct class' as part of the driver model :) > > I think in this context "class" might be meant as something like "struct with > methods", not sure what would be the best term to describe this. "struct with methods" is nice :) Again, 'class' means something different here in the driver model, so be careful with terms, language matters, especially when many of our developers do not have English as their native language. > > > +/// The registration of a driver. > > > +pub struct Registration<T: DriverOps> { > > > + is_registered: bool, > > > > Why does a driver need to know if it is registered or not? Only the > > driver core cares about that, please do not expose that, it's racy and > > should not be relied on. > > We need it to ensure we do not try to register the same thing twice Your logic in your code is wrong if you attempt to register it twice, AND the driver core will return an error if you do so, so a driver should not need to care about this. > , some subsystems might just catch fire otherwise. Which ones? > > > +impl<T: DriverOps> Drop for Registration<T> { > > > + fn drop(&mut self) { > > > + if self.is_registered { > > > + // SAFETY: This path only runs if a previous call to `T::register` completed > > > + // successfully. > > > + unsafe { T::unregister(self.concrete_reg.get()) }; > > > > Can't the rust code ensure that this isn't run if register didn't > > succeed? Having a boolean feels really wrong here (can't that race?) > > No, we want to automatically unregister once this object is destroyed, but not > if it was never registered in the first place. > > This shouldn't be racy, we only ever (un)register things in places like probe() > / remove() or module_init() / module_exit(). probe/remove never calls driver_register/unregister on itself, so that's not an issue. module_init/exit() does not race with itself and again, module_exit() is not called if module_init() fails. Again, you are adding logic here that is not needed. Or if it really is needed, please explain why the C code does not need this today and let's work to fix that. > > > + } > > > + } > > > +} > > > + > > > +/// 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. > > > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > > > +/// that buses can recover the pointer to the data. > > > +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; > > > > All busses have their own way of creating "ids" and that is limited to > > the bus code itself, why is any of this in the rust side? What needs > > this? A bus will create the id for the devices it manages, and can use > > it as part of the name it gives the device (but not required), so all of > > this belongs to the bus, NOT to a driver, or a device. > > This is just a generalized interface which can be used by subsystems to > implement the subsystem / bus specific ID type. Please move this all to a different file as it has nothing to do with the driver core bindings with the include/device/driver.h api. Let's keep it simple and obvious please, and separate out things into logical chunks, hopefully in the same way that the C apis are separated out into. That way we can properly review, understand, and most importantly for all of us, maintain the code over the next 40+ years. thanks, greg k-h
On Tue, May 21, 2024 at 11:35:43AM +0200, Greg KH wrote: > On Tue, May 21, 2024 at 12:30:37AM +0200, Danilo Krummrich wrote: > > > > +impl<T: DriverOps> Drop for Registration<T> { > > > > + fn drop(&mut self) { > > > > + if self.is_registered { > > > > + // SAFETY: This path only runs if a previous call to `T::register` completed > > > > + // successfully. > > > > + unsafe { T::unregister(self.concrete_reg.get()) }; > > > > > > Can't the rust code ensure that this isn't run if register didn't > > > succeed? Having a boolean feels really wrong here (can't that race?) > > > > No, we want to automatically unregister once this object is destroyed, but not > > if it was never registered in the first place. > > > > This shouldn't be racy, we only ever (un)register things in places like probe() > > / remove() or module_init() / module_exit(). > > probe/remove never calls driver_register/unregister on itself, so that's > not an issue. module_init/exit() does not race with itself and again, > module_exit() is not called if module_init() fails. > > Again, you are adding logic here that is not needed. Or if it really is > needed, please explain why the C code does not need this today and let's > work to fix that. And, to be more explicit, a driver should not have any state of its own, any "internal state" should always be bound to the device that it is controlling, NOT generic to the driver itself, as a driver can, and should, be able to control multiple devices all at the same time without ever knowing anything is going on. A driver is just code, not data or state. Yes, we have bad examples in the kernel where drivers do keep independent state, but those are the exceptions, never the rule, and I would argue, should be fixed to not do that. Most were due to being created before the driver model existed, or programmers being lazy :) thanks, greg k-h
On Tue, May 21, 2024 at 11:35:43AM +0200, Greg KH wrote: > On Tue, May 21, 2024 at 12:30:37AM +0200, Danilo Krummrich wrote: > > On Mon, May 20, 2024 at 08:14:18PM +0200, Greg KH wrote: > > > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > > > > > This defines general functionality related to registering drivers with > > > > their respective subsystems, and registering modules that implement > > > > drivers. > > > > > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > > > rust/kernel/lib.rs | 4 +- > > > > rust/macros/module.rs | 2 +- > > > > samples/rust/rust_minimal.rs | 2 +- > > > > samples/rust/rust_print.rs | 2 +- > > > > 5 files changed, 498 insertions(+), 4 deletions(-) > > > > 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..e0cfc36d47ff > > > > --- /dev/null > > > > +++ b/rust/kernel/driver.rs > > > > @@ -0,0 +1,492 @@ > > > > +// 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. > > > > > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > > > device_driver' why are you separating it out here? And what is > > > > DriverOps is a trait which abstracts a subsystems register() and unregister() > > functions to (un)register drivers. It exists such that a generic Registration > > implementation calls the correct one for the subsystem. > > > > For instance, PCI would implement DriverOps::register() by calling into > > bindings::__pci_register_driver(). > > > > We can discuss whether DriverOps is a good name for the trait, but it's not a > > (different) name for something that already exists and already has a name. > > It's a name we don't have in the C code as the design of the driver core > does not need or provide it. It's just the section of 'struct > device_driver' that provides function callbacks, why does it need to be > separate at all? I'm confused by the relationship to `struct device_driver` you seem to imply. How is it related? Again, this is just a trait for subsystems to provide their corresponding register and unregister implementation, e.g. pci_register_driver() and pci_unregister_driver(), such that they can be called from the generic Registration code below. See [1] for an example implementation in PCI. Please also consider that some structures might be a 1:1 representation of C structures, some C structures are not required at the Rust side at all, and then there might be additional structures and traits that abstract things C has no data structure for. This is due to the fact that 1. we're not replicating the functionality on the C side, but only make use of it, and 2. we're trying to abstract existing code to make it work with a conceptually different language. Which means, if you find something that's not on the C side, it doesn't (necessarily) mean we're making something up that should either not exist or be on the C side as well. I'm not saying don't question it, I appreciate that, and it's even the whole reason for sending this RFC, but I ask you to consider this fact. :) [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-10-dakr@redhat.com/ > > > > 'Registration'? That's a bus/class thing, not a driver thing. > > > > A Registration is an object representation of the driver's registered state. > > And that representation should not ever need to be tracked by the > driver, that's up to the driver core to track that. The driver doesn't need it, the Registration abstraction does need it. Please see my comments below. > > > Having an object representation for that is basically the Rust way to manage the > > lifetime of this state. > > This all should be a static chunk of read-only memory that should never > have a lifetime, why does it need to be managed at all? What I meant here is that if a driver was registered, we need to make sure it's going to be unregistered eventually, e.g. when the module is removed or when something fails after registration and we need to unwind. When the Registration structure goes out of scope, which would happen in both the cases above, it will automatically unregister the driver, due to the automatic call to `drop()`. > > > Once the Registration is dropped, the driver is > > unregistered. For instance, a PCI driver Registration can be bound to a module, > > such that the PCI driver is unregistered when the module is unloaded (the > > Registration is dropped in the module_exit() callback). > > Ok, that's fine, but note that your module_exit() function will never be > called if your module_init() callback fails, so why do you need to track > this state? Again, C drivers never need to track this state, why is > rust requiring more logic here for something that is NOT a dynamic chunk > of memory (or should not be a dynamic chunk of memory, let's get it > correct from the start and not require us to change things later on to > make it more secure). That's fine, if module_init() would fail, the Registration would be dropped as well. As for why doesn't C need this is a good example of what I wrote above. Because it is useful for Rust, but not for C. In Rust we get drop() automatically called when a structure is destroyed. This means that if we let drivers put the Registration structure (e.g. representing that a PCI driver was registered) into its `Module` representation structure (already upstream) then this Registration is automatically destroyed once the module representation is destroyed (which happens on module_exit()). This leads to `drop()` of the `Registration` structure being called, which unregisteres the (e.g. PCI) driver. This way the driver does not need to take care of unregistering the PCI driver explicitly. The driver can also place multiple registrations into the `Module` structure. All of them would be unregistered automatically in module_exit(). > > > > And be very careful of the use of the word 'class' here, remember, there > > > is 'struct class' as part of the driver model :) > > > > I think in this context "class" might be meant as something like "struct with > > methods", not sure what would be the best term to describe this. > > "struct with methods" is nice :) Great, I will change that. > > Again, 'class' means something different here in the driver model, so be > careful with terms, language matters, especially when many of our > developers do not have English as their native language. > > > > > +/// The registration of a driver. > > > > +pub struct Registration<T: DriverOps> { > > > > + is_registered: bool, > > > > > > Why does a driver need to know if it is registered or not? Only the > > > driver core cares about that, please do not expose that, it's racy and > > > should not be relied on. > > > > We need it to ensure we do not try to register the same thing twice > > Your logic in your code is wrong if you attempt to register it twice, > AND the driver core will return an error if you do so, so a driver > should not need to care about this. We want it to be safe, if the driver logic is wrong and registers it twice, we don't want it to blow up. The driver core takes care, but I think there are subsystems that do initializations that could make things blow up when registering the driver twice. > > > , some subsystems might just catch fire otherwise. > > Which ones? Let's take the one we provide abstractons for, PCI. In __pci_register_driver() we call spin_lock_init() and INIT_LIST_HEAD() before driver_register() could bail out [1]. What if this driver is already registered and in use and we're randomly altering the list pointers or call spin_lock_init() on a spin lock that's currently being held? [1] https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-driver.c#L1447 > > > > > +impl<T: DriverOps> Drop for Registration<T> { > > > > + fn drop(&mut self) { > > > > + if self.is_registered { > > > > + // SAFETY: This path only runs if a previous call to `T::register` completed > > > > + // successfully. > > > > + unsafe { T::unregister(self.concrete_reg.get()) }; > > > > > > Can't the rust code ensure that this isn't run if register didn't > > > succeed? Having a boolean feels really wrong here (can't that race?) > > > > No, we want to automatically unregister once this object is destroyed, but not > > if it was never registered in the first place. > > > > This shouldn't be racy, we only ever (un)register things in places like probe() > > / remove() or module_init() / module_exit(). > > probe/remove never calls driver_register/unregister on itself, so that's > not an issue. module_init/exit() does not race with itself and again, > module_exit() is not called if module_init() fails. That's clear, I should mention that we can use the Registration abstraction also for things that are typically registered in probe(), a DRM device for instance. As mentioned above I'm aware that module_exit() is not called when module_init() fails, in this case the Registration structure is dropped in module_init(), hence it's fine. > > Again, you are adding logic here that is not needed. Or if it really is > needed, please explain why the C code does not need this today and let's > work to fix that. Please see above, where I start with "As for why doesn't C need this..". > > > > > + } > > > > + } > > > > +} > > > > + > > > > +/// 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. > > > > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > > > > +/// that buses can recover the pointer to the data. > > > > +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; > > > > > > All busses have their own way of creating "ids" and that is limited to > > > the bus code itself, why is any of this in the rust side? What needs > > > this? A bus will create the id for the devices it manages, and can use > > > it as part of the name it gives the device (but not required), so all of > > > this belongs to the bus, NOT to a driver, or a device. > > > > This is just a generalized interface which can be used by subsystems to > > implement the subsystem / bus specific ID type. > > Please move this all to a different file as it has nothing to do with > the driver core bindings with the include/device/driver.h api. I don't think driver.rs in an unreasonable place for a generic device ID representation that is required by every driver structure. But I'm also not overly resistant to move it out. What do you think would be a good name? Please consider that this name will also be the namespace for this trait. Currently you can reference it with `kernel::driver::RawDeviceId`. If you move it to foo.rs, it'd be `kernel::foo::RawDeviceId`. > > Let's keep it simple and obvious please, and separate out things into > logical chunks, hopefully in the same way that the C apis are separated > out into. That way we can properly review, understand, and most > importantly for all of us, maintain the code over the next 40+ years. > > thanks, > > greg k-h >
On Tue, May 21, 2024 at 10:04:02AM +0200, Greg KH wrote: > On Tue, May 21, 2024 at 03:42:50PM +1000, Dave Airlie wrote: > > On Tue, 21 May 2024 at 04:14, Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > > > > > This defines general functionality related to registering drivers with > > > > their respective subsystems, and registering modules that implement > > > > drivers. > > > > > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > > > rust/kernel/lib.rs | 4 +- > > > > rust/macros/module.rs | 2 +- > > > > samples/rust/rust_minimal.rs | 2 +- > > > > samples/rust/rust_print.rs | 2 +- > > > > 5 files changed, 498 insertions(+), 4 deletions(-) > > > > 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..e0cfc36d47ff > > > > --- /dev/null > > > > +++ b/rust/kernel/driver.rs > > > > @@ -0,0 +1,492 @@ > > > > +// 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. > > > > > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > > > device_driver' why are you separating it out here? And what is > > > 'Registration'? That's a bus/class thing, not a driver thing. > > > > > > And be very careful of the use of the word 'class' here, remember, there > > > is 'struct class' as part of the driver model :) > > > > > > > +use crate::{ > > > > + alloc::{box_ext::BoxExt, flags::*}, > > > > + error::code::*, > > > > + error::Result, > > > > + str::CStr, > > > > + sync::Arc, > > > > + ThisModule, > > > > +}; > > > > +use alloc::boxed::Box; > > > > +use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin}; > > > > + > > > > +/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it. > > > > +pub trait DriverOps { > > > > > > Again, why is this not called DeviceDriver? > > > > This is not the same as the C device_driver, it might mildly align in > > concept with it, but I'm not sure it shares enough to align it name > > wise with the C one. > > Why not use the same terms and design decisions that the C code has? To > differ needs to have a good reason, otherwise it's just going to cause > us all confusion as we have to learn two different terms for the same > thing. Please see my reply [1] for that one. Let's continue this in the other thread. [1] https://lore.kernel.org/rust-for-linux/Zk0egew_AxvNpUG-@pollux/ > > > > > + /// 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`]. > > > > + unsafe 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`]. > > > > + unsafe fn unregister(reg: *mut Self::RegType); > > > > +} > > > > + > > > > +/// The registration of a driver. > > > > +pub struct Registration<T: DriverOps> { > > > > + is_registered: bool, > > > > > > Why does a driver need to know if it is registered or not? Only the > > > driver core cares about that, please do not expose that, it's racy and > > > should not be relied on. > > > > >From the C side this does look unusual because on the C side something > > like struct pci_driver is statically allocated everywhere. > > In this rust abstraction, these are allocated dynamically, so instead > > of just it always being safe to just call register/unregister > > with static memory, a flag is kept around to say if the unregister > > should happen at all, as the memory may have > > been allocated but never registered. This is all the Registration is > > for, it's tracking the bus _driver structure allocation, and > > whether the bus register/unregister have been called since the object > > was allocated. > > > > I'm not sure it makes sense (or if you can at all), have a static like > > pci_driver object here to match how C does things. > > Wait, why can't you have a static "rust_driver" type thing? Having it > be in ram feels like a waste of memory. We are working hard to move > more and more of these driver model structures into read-only memory for > good reasons (security, reduce bugs, etc.), moving backwards to having > them all be dynamically created/copied around feels wrong. I think this would be possible, though it might cause some inconvenience and a and maybe a few trade offs to do that on the Rust side. However, I agree that we should explore how to do it in a static way. I will dig in a bit more on the options we have. > > We are one stage away from being able to mark all 'driver_*()' calls as > using a const * to struct driver, please don't make that work pointless > if you want to write a driver in rust instead. > > And again, a driver should never know/care/wonder if it has been > registered or not, that is up to the driver core to handle, not the rust > code, as it is the only thing that can know this, and the only thing > that should need to know it. A driver structure should not have any > dynamic memory associated with it to require knowing its "state" in the > system, so let's not move backwards here to require that just because we > are using Rust. I answered on this in [1], let's continue there. This also accounts for all points below I don't address. :) [1] https://lore.kernel.org/rust-for-linux/Zk0egew_AxvNpUG-@pollux/ > > > > > +impl<T: DriverOps> Drop for Registration<T> { > > > > + fn drop(&mut self) { > > > > + if self.is_registered { > > > > + // SAFETY: This path only runs if a previous call to `T::register` completed > > > > + // successfully. > > > > + unsafe { T::unregister(self.concrete_reg.get()) }; > > > > > > Can't the rust code ensure that this isn't run if register didn't > > > succeed? Having a boolean feels really wrong here (can't that race?) > > > > There might be a way of using Option<> here but I don't think it adds > > anything over and above using an explicit bool. > > Again, this should not be needed. If so, something is designed > incorrectly with the bindings. > > > > > +/// > > > > +/// 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. > > > > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > > > > +/// that buses can recover the pointer to the data. > > > > +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; > > > > > > All busses have their own way of creating "ids" and that is limited to > > > the bus code itself, why is any of this in the rust side? What needs > > > this? A bus will create the id for the devices it manages, and can use > > > it as part of the name it gives the device (but not required), so all of > > > this belongs to the bus, NOT to a driver, or a device. > > > > Consider this a base class (Trait) for bus specific IDs. > > Then all of that should be in a separate file as they belong to a "bus" > not a driver, as each and every bus will have a different way of > expressing the list of devices a driver can bind to. > > And again, the core "struct device_driver" does not have a list of ids, > and neither should the rust bindings, that belongs to the bus logic. > > > > > +/// Custom code within device removal. > > > > > > You better define the heck out of "device removal" as specified last > > > time this all came up. From what I can see here, this is totally wrong > > > and confusing and will be a mess. > > > > > > Do it right, name it properly. > > > > > > I'm not reviewingn beyond here, sorry. It's the merge window and I > > > shouldn't have even looked at this until next week anyway. > > > > > > But I was hoping that the whole long rant I gave last time would be > > > addressed at least a little bit. I don't see that it has :( > > > > I won't comment too much on the specifics here, but you've twice got > > to this stage, said something is wrong, change it, and given no > > actionable feedback on what is wrong, or what to change. > > > > I've looked at this code for a few hours today with the device docs > > and core code and can't spot what you are seeing that is wrong here, > > which means I don't expect anyone else is going to unless you can help > > educate us more. > > A lifecycle of a device is: > device create by the bus > device register with driver core > device bind to a driver (i.e. probe() callback in the driver) > device unbind from a driver (i.e. remove() callback in the driver, can be triggered many ways) > device destroy by the bus > > "remove" can happen for a driver where it needs to clean up everything > it has done for a specific device, but that does not mean the device is > actually gone from the system, that's up to the bus to decide, as it > owns the device lifecycle and gets to decide when it thinks it is really > gone. And then, when the bus tells the driver core that it wants to > mark the device as "destroyed", it's up to the driver core to eventually > call back into the bus to really clean that device up from the system at > some later point in time. That matches at least my understanding and I don't see how the name of the `DeviceRemoval` trait for instance is contradictive to that. Again, the C side uses the exact same terminology in struct device_driver and struct pci_driver, e.g. [1][2]. If it's wrong there as well, how would you phrase it differently? I'm happy to fix those places as well. If it's correct there, why is it wrong here? Where is the difference? May I ask you for a proposal on how to name the `DeviceRemoval` trait instead? Maybe something like `DeviceUnbind`? Again, I'm not generally resisting to change it, but I think we should clarify the above. [1] https://elixir.bootlin.com/linux/latest/source/include/linux/device/driver.h#L71 [2] https://elixir.bootlin.com/linux/latest/source/include/linux/pci.h#L905 > > Try splitting this file out into driver and bus and device logic, like > the driver core has, and see if that helps in explaining and making all > of this more understandable, and to keep with the names/design of the > model we currently have. > > Note, all of this is my biggest worry about writing a driver in rust, > getting the lifecycle of the driver core and it's logic of how it > handles memory manged in C, to align up with how rust is going to access > it with its different rules and requirements, is not going to be simple > as you have two different models intersecting at the same place. I've > been working over the past year to make the rust side happen easier by > marking more and more of the C structures as "not mutable", i.e. const > *, so that the Rust side doesn't have to worry that the driver core > really will change things it passes to the C side, but there is more to > be done (see above about making struct device_driver * const). > > I want to see this happen, but it's going to be a hard slog due to the > different expectations of these two systems. Keeping naming identical > where possible is one way to make it simpler for everyone involved, as > would splitting the logic out the same way and not mixing it all up into > one big hairy file that combines different things into a single chunk. > > thanks, > > greg k-h >
On 20.05.2024 19:25, Danilo Krummrich wrote: > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > This defines general functionality related to registering drivers with > their respective subsystems, and registering modules that implement > drivers. > > Co-developed-by: Asahi Lina <lina@asahilina.net> > Signed-off-by: Asahi Lina <lina@asahilina.net> > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 4 +- > rust/macros/module.rs | 2 +- > samples/rust/rust_minimal.rs | 2 +- > samples/rust/rust_print.rs | 2 +- > 5 files changed, 498 insertions(+), 4 deletions(-) > 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..e0cfc36d47ff > --- /dev/null > +++ b/rust/kernel/driver.rs > @@ -0,0 +1,492 @@ > +// 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::{ > + alloc::{box_ext::BoxExt, flags::*}, > + error::code::*, > + error::Result, > + str::CStr, > + sync::Arc, > + ThisModule, > +}; > +use alloc::boxed::Box; > +use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin}; > + > +/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it. > +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`]. > + unsafe 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`]. > + unsafe fn unregister(reg: *mut Self::RegType); > +} > + > +/// The registration of a driver. > +pub struct Registration<T: DriverOps> { > + is_registered: bool, > + concrete_reg: UnsafeCell<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> {} You might want to check if we additionally need 'Send' due to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=323617f649c0966ad5e741e47e27e06d3a680d8f here? + unsafe impl<T: DriverOps> Send for Registration<T> {} This was found re-basing https://github.com/Rust-for-Linux/linux/commits/staging/rust-device/ to v6.10-rc1. Sorry if I missed anything ;) Best regards Dirk
On 20.05.2024 19:25, Danilo Krummrich wrote: > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > This defines general functionality related to registering drivers with > their respective subsystems, and registering modules that implement > drivers. > > Co-developed-by: Asahi Lina <lina@asahilina.net> > Signed-off-by: Asahi Lina <lina@asahilina.net> > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 4 +- > rust/macros/module.rs | 2 +- > samples/rust/rust_minimal.rs | 2 +- > samples/rust/rust_print.rs | 2 +- > 5 files changed, 498 insertions(+), 4 deletions(-) > 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..e0cfc36d47ff > --- /dev/null > +++ b/rust/kernel/driver.rs > @@ -0,0 +1,492 @@ > +// 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::{ > + alloc::{box_ext::BoxExt, flags::*}, > + error::code::*, > + error::Result, > + str::CStr, > + sync::Arc, > + ThisModule, > +}; > +use alloc::boxed::Box; > +use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin}; > + > +/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it. > +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`]. > + unsafe 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`]. > + unsafe fn unregister(reg: *mut Self::RegType); > +} > + > +/// The registration of a driver. > +pub struct Registration<T: DriverOps> { > + is_registered: bool, > + concrete_reg: UnsafeCell<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> {} > + > +impl<T: DriverOps> Registration<T> { > + /// Creates a new instance of the registration object. > + pub fn new() -> Self { > + Self { > + is_registered: false, > + concrete_reg: UnsafeCell::new(T::RegType::default()), > + } > + } > + > + /// Allocates a pinned registration object and registers it. > + /// > + /// Returns a pinned heap-allocated representation of the registration. > + pub fn new_pinned(name: &'static CStr, module: &'static ThisModule) -> Result<Pin<Box<Self>>> { > + let mut reg = Pin::from(Box::new(Self::new(), GFP_KERNEL)?); > + reg.as_mut().register(name, module)?; > + Ok(reg) > + } > + > + /// Registers a driver with its subsystem. > + /// > + /// It must be pinned because the memory block that represents the registration is potentially > + /// self-referential. > + pub fn register( > + self: Pin<&mut Self>, > + name: &'static CStr, > + module: &'static ThisModule, > + ) -> Result { > + // SAFETY: We never move out of `this`. > + let this = unsafe { self.get_unchecked_mut() }; > + if this.is_registered { > + // Already registered. > + return Err(EINVAL); > + } > + > + // SAFETY: `concrete_reg` was initialised via its default constructor. It is only freed > + // after `Self::drop` is called, which first calls `T::unregister`. > + unsafe { T::register(this.concrete_reg.get(), name, module) }?; > + > + this.is_registered = true; > + Ok(()) > + } > +} > + > +impl<T: DriverOps> Default for Registration<T> { > + fn default() -> Self { > + Self::new() > + } > +} > + > +impl<T: DriverOps> Drop for Registration<T> { > + fn drop(&mut self) { > + if self.is_registered { > + // SAFETY: This path only runs if a previous call to `T::register` completed > + // successfully. > + unsafe { T::unregister(self.concrete_reg.get()) }; > + } > + } > +} > + > +/// 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. > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > +/// that buses can recover the pointer to the data. In his I2C branch Fabien has a patch [1] [2] to remove the RawDeviceId::to_rawid part above. Maybe it could be aligned that that patch isn't required any more? Best regards Dirk [1] https://github.com/Fabo/linux/commit/4b65b8d7ffe07057672b8eb89d376759d67bf060 [2] From 4b65b8d7ffe07057672b8eb89d376759d67bf060 Mon Sep 17 00:00:00 2001 From: Fabien Parent <fabien.parent@linaro.org> Date: Sun, 28 Apr 2024 11:12:46 -0700 Subject: [PATCH] fixup! rust: add driver abstraction RawDeviceId::to_rawid is not part of the RawDeviceId trait. Nonetheless this function must be defined by the type that will implement RawDeviceId, but to keep `rustdoc` from throwing a warning, let's just remove it from the docs. warning: unresolved link to `RawDeviceId::to_rawid` --> rust/kernel/driver.rs:120:11 | 120 | /// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so | ^^^^^^^^^^^^^^^^^^^^^ the trait `RawDeviceId` has no associated item named `to_rawid` | = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default warning: 1 warning emitted --- rust/kernel/driver.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs index c1258ce0d041af..d141e23224d3db 100644 --- a/rust/kernel/driver.rs +++ b/rust/kernel/driver.rs @@ -128,8 +128,6 @@ impl<T: DriverOps> Drop for Registration<T> { /// /// Implementers must ensure that: /// - [`RawDeviceId::ZERO`] is actually a zeroed-out version of the raw device id. -/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so -/// that buses can recover the pointer to the data. pub unsafe trait RawDeviceId { /// The raw type that holds the device id. ///
On Wed, May 22, 2024 at 12:21:53AM +0200, Danilo Krummrich wrote: > On Tue, May 21, 2024 at 11:35:43AM +0200, Greg KH wrote: > > On Tue, May 21, 2024 at 12:30:37AM +0200, Danilo Krummrich wrote: > > > On Mon, May 20, 2024 at 08:14:18PM +0200, Greg KH wrote: > > > > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > > > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > > > > > > > This defines general functionality related to registering drivers with > > > > > their respective subsystems, and registering modules that implement > > > > > drivers. > > > > > > > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > > > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > > > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > > > > rust/kernel/lib.rs | 4 +- > > > > > rust/macros/module.rs | 2 +- > > > > > samples/rust/rust_minimal.rs | 2 +- > > > > > samples/rust/rust_print.rs | 2 +- > > > > > 5 files changed, 498 insertions(+), 4 deletions(-) > > > > > 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..e0cfc36d47ff > > > > > --- /dev/null > > > > > +++ b/rust/kernel/driver.rs > > > > > @@ -0,0 +1,492 @@ > > > > > +// 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. > > > > > > > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > > > > device_driver' why are you separating it out here? And what is > > > > > > DriverOps is a trait which abstracts a subsystems register() and unregister() > > > functions to (un)register drivers. It exists such that a generic Registration > > > implementation calls the correct one for the subsystem. > > > > > > For instance, PCI would implement DriverOps::register() by calling into > > > bindings::__pci_register_driver(). > > > > > > We can discuss whether DriverOps is a good name for the trait, but it's not a > > > (different) name for something that already exists and already has a name. > > > > It's a name we don't have in the C code as the design of the driver core > > does not need or provide it. It's just the section of 'struct > > device_driver' that provides function callbacks, why does it need to be > > separate at all? > > I'm confused by the relationship to `struct device_driver` you seem to imply. > How is it related? > > Again, this is just a trait for subsystems to provide their corresponding > register and unregister implementation, e.g. pci_register_driver() and > pci_unregister_driver(), such that they can be called from the generic > Registration code below. > > See [1] for an example implementation in PCI. registering and unregistering drivers belongs in the bus code, NOT in the driver code. I think lots of the objections I had here will be fixed up when you move the bus logic out to it's own file, it does not belong here in a driver file (device ids, etc.) > Please also consider that some structures might be a 1:1 representation of C > structures, some C structures are not required at the Rust side at all, and > then there might be additional structures and traits that abstract things C has > no data structure for. That's fine, but let's keep the separate of what we have today at the very least and not try to lump it all into one file, that makes it harder to review and maintain over time. > > > > 'Registration'? That's a bus/class thing, not a driver thing. > > > > > > A Registration is an object representation of the driver's registered state. > > > > And that representation should not ever need to be tracked by the > > driver, that's up to the driver core to track that. > > The driver doesn't need it, the Registration abstraction does need it. Please > see my comments below. Great, put it elsewhere please, it does not belong in driver.rs. > > > Having an object representation for that is basically the Rust way to manage the > > > lifetime of this state. > > > > This all should be a static chunk of read-only memory that should never > > have a lifetime, why does it need to be managed at all? > > What I meant here is that if a driver was registered, we need to make sure it's > going to be unregistered eventually, e.g. when the module is removed or when > something fails after registration and we need to unwind. > > When the Registration structure goes out of scope, which would happen in both > the cases above, it will automatically unregister the driver, due to the > automatic call to `drop()`. That's fine, but again, this all should just be static code, not dynamic. > > > Once the Registration is dropped, the driver is > > > unregistered. For instance, a PCI driver Registration can be bound to a module, > > > such that the PCI driver is unregistered when the module is unloaded (the > > > Registration is dropped in the module_exit() callback). > > > > Ok, that's fine, but note that your module_exit() function will never be > > called if your module_init() callback fails, so why do you need to track > > this state? Again, C drivers never need to track this state, why is > > rust requiring more logic here for something that is NOT a dynamic chunk > > of memory (or should not be a dynamic chunk of memory, let's get it > > correct from the start and not require us to change things later on to > > make it more secure). > > That's fine, if module_init() would fail, the Registration would be dropped as > well. > > As for why doesn't C need this is a good example of what I wrote above. Because > it is useful for Rust, but not for C. > > In Rust we get drop() automatically called when a structure is destroyed. This > means that if we let drivers put the Registration structure (e.g. representing > that a PCI driver was registered) into its `Module` representation structure > (already upstream) then this Registration is automatically destroyed once the > module representation is destroyed (which happens on module_exit()). This leads > to `drop()` of the `Registration` structure being called, which unregisteres the > (e.g. PCI) driver. > > This way the driver does not need to take care of unregistering the PCI driver > explicitly. The driver can also place multiple registrations into the `Module` > structure. All of them would be unregistered automatically in module_exit(). Ok, I think we are agreeing here, except that you do not need a "am I registered" flag, as the existance of the "object" defines if it is registered or not (i.e. if it exists and the "destructor" is called, it's been registered, otherwise it hasn't been and the check is pointless.) > > Again, 'class' means something different here in the driver model, so be > > careful with terms, language matters, especially when many of our > > developers do not have English as their native language. > > > > > > > +/// The registration of a driver. > > > > > +pub struct Registration<T: DriverOps> { > > > > > + is_registered: bool, > > > > > > > > Why does a driver need to know if it is registered or not? Only the > > > > driver core cares about that, please do not expose that, it's racy and > > > > should not be relied on. > > > > > > We need it to ensure we do not try to register the same thing twice > > > > Your logic in your code is wrong if you attempt to register it twice, > > AND the driver core will return an error if you do so, so a driver > > should not need to care about this. > > We want it to be safe, if the driver logic is wrong and registers it twice, we > don't want it to blow up. How could that happen? > The driver core takes care, but I think there are subsystems that do > initializations that could make things blow up when registering the driver > twice. Nope, should not be needed, see above. Rust should make this _easier_ not harder, than C code here :) > > > , some subsystems might just catch fire otherwise. > > > > Which ones? > > Let's take the one we provide abstractons for, PCI. > > In __pci_register_driver() we call spin_lock_init() and INIT_LIST_HEAD() before > driver_register() could bail out [1]. > > What if this driver is already registered and in use and we're randomly altering > the list pointers or call spin_lock_init() on a spin lock that's currently being > held? I don't understand, why would you ever call "register driver" BEFORE the driver was properly set up to actually be registered? PCI works properly here, you don't register unless everything is set up. Which is why it doesn't have a "hey, am I registered or not?" type flag, it's not needed. > > > > > > > + } > > > > > + } > > > > > +} > > > > > + > > > > > +/// 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. > > > > > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > > > > > +/// that buses can recover the pointer to the data. > > > > > +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; > > > > > > > > All busses have their own way of creating "ids" and that is limited to > > > > the bus code itself, why is any of this in the rust side? What needs > > > > this? A bus will create the id for the devices it manages, and can use > > > > it as part of the name it gives the device (but not required), so all of > > > > this belongs to the bus, NOT to a driver, or a device. > > > > > > This is just a generalized interface which can be used by subsystems to > > > implement the subsystem / bus specific ID type. > > > > Please move this all to a different file as it has nothing to do with > > the driver core bindings with the include/device/driver.h api. > > I don't think driver.rs in an unreasonable place for a generic device ID > representation that is required by every driver structure. It has nothing to do with drivers on their own, it's a bus attribute, please put that in bus.rs at the least. Or it's own file if you need lots of code for simple arrays like this :) > But I'm also not overly resistant to move it out. What do you think would be a > good name? > > Please consider that this name will also be the namespace for this trait. > Currently you can reference it with `kernel::driver::RawDeviceId`. If you move > it to foo.rs, it'd be `kernel::foo::RawDeviceId`. I don't see why this isn't just unique to each bus type, it is not a driver attribute, but a bus-specific-driver type. Please put it there and don't attempt to make it "generic". thanks, greg k-h
On Tue, Jun 04, 2024 at 04:27:31PM +0200, Greg KH wrote: > On Wed, May 22, 2024 at 12:21:53AM +0200, Danilo Krummrich wrote: > > On Tue, May 21, 2024 at 11:35:43AM +0200, Greg KH wrote: > > > On Tue, May 21, 2024 at 12:30:37AM +0200, Danilo Krummrich wrote: > > > > On Mon, May 20, 2024 at 08:14:18PM +0200, Greg KH wrote: > > > > > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > > > > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > > > > > > > > > This defines general functionality related to registering drivers with > > > > > > their respective subsystems, and registering modules that implement > > > > > > drivers. > > > > > > > > > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > > > > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > > > > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > > > > > rust/kernel/lib.rs | 4 +- > > > > > > rust/macros/module.rs | 2 +- > > > > > > samples/rust/rust_minimal.rs | 2 +- > > > > > > samples/rust/rust_print.rs | 2 +- > > > > > > 5 files changed, 498 insertions(+), 4 deletions(-) > > > > > > 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..e0cfc36d47ff > > > > > > --- /dev/null > > > > > > +++ b/rust/kernel/driver.rs > > > > > > @@ -0,0 +1,492 @@ > > > > > > +// 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. > > > > > > > > > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > > > > > device_driver' why are you separating it out here? And what is > > > > > > > > DriverOps is a trait which abstracts a subsystems register() and unregister() > > > > functions to (un)register drivers. It exists such that a generic Registration > > > > implementation calls the correct one for the subsystem. > > > > > > > > For instance, PCI would implement DriverOps::register() by calling into > > > > bindings::__pci_register_driver(). > > > > > > > > We can discuss whether DriverOps is a good name for the trait, but it's not a > > > > (different) name for something that already exists and already has a name. > > > > > > It's a name we don't have in the C code as the design of the driver core > > > does not need or provide it. It's just the section of 'struct > > > device_driver' that provides function callbacks, why does it need to be > > > separate at all? > > > > I'm confused by the relationship to `struct device_driver` you seem to imply. > > How is it related? > > > > Again, this is just a trait for subsystems to provide their corresponding > > register and unregister implementation, e.g. pci_register_driver() and > > pci_unregister_driver(), such that they can be called from the generic > > Registration code below. > > > > See [1] for an example implementation in PCI. > > registering and unregistering drivers belongs in the bus code, NOT in > the driver code. Why? We're not (re-)implementing a bus here. Again, those are just abstractions to call the C functions to register a driver. The corresponding C functions are e.g. driver_register() or __pci_register_driver(). Those are defined in drivers/base/driver.c and drivers/pci/pci-driver.c respectively. Why wouldn't we follow the same scheme in Rust abstractions? > > I think lots of the objections I had here will be fixed up when you move > the bus logic out to it's own file, it does not belong here in a driver > file (device ids, etc.) > > > Please also consider that some structures might be a 1:1 representation of C > > structures, some C structures are not required at the Rust side at all, and > > then there might be additional structures and traits that abstract things C has > > no data structure for. > > That's fine, but let's keep the separate of what we have today at the > very least and not try to lump it all into one file, that makes it > harder to review and maintain over time. > > > > > > 'Registration'? That's a bus/class thing, not a driver thing. > > > > > > > > A Registration is an object representation of the driver's registered state. > > > > > > And that representation should not ever need to be tracked by the > > > driver, that's up to the driver core to track that. > > > > The driver doesn't need it, the Registration abstraction does need it. Please > > see my comments below. > > Great, put it elsewhere please, it does not belong in driver.rs. This `Registration` structure is a generic abstraction to call some $SUBSYSTEM_driver_register() function (e.g. pci_register_driver()). Why would it belong somewhere else? Again, the corresponding C functions are in some driver.c file as well. > > > > > Having an object representation for that is basically the Rust way to manage the > > > > lifetime of this state. > > > > > > This all should be a static chunk of read-only memory that should never > > > have a lifetime, why does it need to be managed at all? > > > > What I meant here is that if a driver was registered, we need to make sure it's > > going to be unregistered eventually, e.g. when the module is removed or when > > something fails after registration and we need to unwind. > > > > When the Registration structure goes out of scope, which would happen in both > > the cases above, it will automatically unregister the driver, due to the > > automatic call to `drop()`. > > That's fine, but again, this all should just be static code, not > dynamic. I agree. As already mentioned in another thread, this will be static in v2. I got the code for that in place already. :) > > > > > Once the Registration is dropped, the driver is > > > > unregistered. For instance, a PCI driver Registration can be bound to a module, > > > > such that the PCI driver is unregistered when the module is unloaded (the > > > > Registration is dropped in the module_exit() callback). > > > > > > Ok, that's fine, but note that your module_exit() function will never be > > > called if your module_init() callback fails, so why do you need to track > > > this state? Again, C drivers never need to track this state, why is > > > rust requiring more logic here for something that is NOT a dynamic chunk > > > of memory (or should not be a dynamic chunk of memory, let's get it > > > correct from the start and not require us to change things later on to > > > make it more secure). > > > > That's fine, if module_init() would fail, the Registration would be dropped as > > well. > > > > As for why doesn't C need this is a good example of what I wrote above. Because > > it is useful for Rust, but not for C. > > > > In Rust we get drop() automatically called when a structure is destroyed. This > > means that if we let drivers put the Registration structure (e.g. representing > > that a PCI driver was registered) into its `Module` representation structure > > (already upstream) then this Registration is automatically destroyed once the > > module representation is destroyed (which happens on module_exit()). This leads > > to `drop()` of the `Registration` structure being called, which unregisteres the > > (e.g. PCI) driver. > > > > This way the driver does not need to take care of unregistering the PCI driver > > explicitly. The driver can also place multiple registrations into the `Module` > > structure. All of them would be unregistered automatically in module_exit(). > > Ok, I think we are agreeing here, except that you do not need a "am I > registered" flag, as the existance of the "object" defines if it is > registered or not (i.e. if it exists and the "destructor" is called, > it's been registered, otherwise it hasn't been and the check is > pointless.) The static implementation does not need this anymore, since there is no separate register() function anymore that we need to protect. > > > > Again, 'class' means something different here in the driver model, so be > > > careful with terms, language matters, especially when many of our > > > developers do not have English as their native language. > > > > > > > > > +/// The registration of a driver. > > > > > > +pub struct Registration<T: DriverOps> { > > > > > > + is_registered: bool, > > > > > > > > > > Why does a driver need to know if it is registered or not? Only the > > > > > driver core cares about that, please do not expose that, it's racy and > > > > > should not be relied on. > > > > > > > > We need it to ensure we do not try to register the same thing twice > > > > > > Your logic in your code is wrong if you attempt to register it twice, > > > AND the driver core will return an error if you do so, so a driver > > > should not need to care about this. > > > > We want it to be safe, if the driver logic is wrong and registers it twice, we > > don't want it to blow up. > > How could that happen? > > > The driver core takes care, but I think there are subsystems that do > > initializations that could make things blow up when registering the driver > > twice. > > Nope, should not be needed, see above. Rust should make this _easier_ > not harder, than C code here :) Agree, and as mentioned above, with v2 Rust drivers can't register the same thing twice anymore. > > > > > , some subsystems might just catch fire otherwise. > > > > > > Which ones? > > > > Let's take the one we provide abstractons for, PCI. > > > > In __pci_register_driver() we call spin_lock_init() and INIT_LIST_HEAD() before > > driver_register() could bail out [1]. > > > > What if this driver is already registered and in use and we're randomly altering > > the list pointers or call spin_lock_init() on a spin lock that's currently being > > held? > > I don't understand, why would you ever call "register driver" BEFORE the > driver was properly set up to actually be registered? > > PCI works properly here, you don't register unless everything is set up. > Which is why it doesn't have a "hey, am I registered or not?" type flag, > it's not needed. That's not what I meant, but I think we can drop this specific part of the discussion anyways, since with v2 we can't hit this anymore. :) > > > > > > > > > > + } > > > > > > + } > > > > > > +} > > > > > > + > > > > > > +/// 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. > > > > > > +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so > > > > > > +/// that buses can recover the pointer to the data. > > > > > > +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; > > > > > > > > > > All busses have their own way of creating "ids" and that is limited to > > > > > the bus code itself, why is any of this in the rust side? What needs > > > > > this? A bus will create the id for the devices it manages, and can use > > > > > it as part of the name it gives the device (but not required), so all of > > > > > this belongs to the bus, NOT to a driver, or a device. > > > > > > > > This is just a generalized interface which can be used by subsystems to > > > > implement the subsystem / bus specific ID type. > > > > > > Please move this all to a different file as it has nothing to do with > > > the driver core bindings with the include/device/driver.h api. > > > > I don't think driver.rs in an unreasonable place for a generic device ID > > representation that is required by every driver structure. > > It has nothing to do with drivers on their own, it's a bus attribute, > please put that in bus.rs at the least. Or it's own file if you need > lots of code for simple arrays like this :) It's not a lot of code actually, probably less than 100 lines, there is a lot of documentation / examples though. :) The corresponding C structure definitions are in include/linux/mod_devicetable.h. Maybe we can move it to a separte device_id.rs if you prefer that? > > > But I'm also not overly resistant to move it out. What do you think would be a > > good name? > > > > Please consider that this name will also be the namespace for this trait. > > Currently you can reference it with `kernel::driver::RawDeviceId`. If you move > > it to foo.rs, it'd be `kernel::foo::RawDeviceId`. > > I don't see why this isn't just unique to each bus type, it is not a > driver attribute, but a bus-specific-driver type. Please put it there > and don't attempt to make it "generic". If we don't make it generic we end up with a lot of duplicate code in every subsystem that has some kind of device ID, e.g. OF, PCI, etc. Why would we want to do that? I think moving the generic abstraction into a separate device_id.rs seems to be the better option. > > thanks, > > greg k-h >
On Tue, Jun 04, 2024 at 05:41:21PM +0200, Danilo Krummrich wrote: > On Tue, Jun 04, 2024 at 04:27:31PM +0200, Greg KH wrote: > > On Wed, May 22, 2024 at 12:21:53AM +0200, Danilo Krummrich wrote: > > > On Tue, May 21, 2024 at 11:35:43AM +0200, Greg KH wrote: > > > > On Tue, May 21, 2024 at 12:30:37AM +0200, Danilo Krummrich wrote: > > > > > On Mon, May 20, 2024 at 08:14:18PM +0200, Greg KH wrote: > > > > > > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > > > > > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > > > > > > > > > > > This defines general functionality related to registering drivers with > > > > > > > their respective subsystems, and registering modules that implement > > > > > > > drivers. > > > > > > > > > > > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > > > > > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > > > > > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > > > > > > rust/kernel/lib.rs | 4 +- > > > > > > > rust/macros/module.rs | 2 +- > > > > > > > samples/rust/rust_minimal.rs | 2 +- > > > > > > > samples/rust/rust_print.rs | 2 +- > > > > > > > 5 files changed, 498 insertions(+), 4 deletions(-) > > > > > > > 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..e0cfc36d47ff > > > > > > > --- /dev/null > > > > > > > +++ b/rust/kernel/driver.rs > > > > > > > @@ -0,0 +1,492 @@ > > > > > > > +// 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. > > > > > > > > > > > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > > > > > > device_driver' why are you separating it out here? And what is > > > > > > > > > > DriverOps is a trait which abstracts a subsystems register() and unregister() > > > > > functions to (un)register drivers. It exists such that a generic Registration > > > > > implementation calls the correct one for the subsystem. > > > > > > > > > > For instance, PCI would implement DriverOps::register() by calling into > > > > > bindings::__pci_register_driver(). > > > > > > > > > > We can discuss whether DriverOps is a good name for the trait, but it's not a > > > > > (different) name for something that already exists and already has a name. > > > > > > > > It's a name we don't have in the C code as the design of the driver core > > > > does not need or provide it. It's just the section of 'struct > > > > device_driver' that provides function callbacks, why does it need to be > > > > separate at all? > > > > > > I'm confused by the relationship to `struct device_driver` you seem to imply. > > > How is it related? > > > > > > Again, this is just a trait for subsystems to provide their corresponding > > > register and unregister implementation, e.g. pci_register_driver() and > > > pci_unregister_driver(), such that they can be called from the generic > > > Registration code below. > > > > > > See [1] for an example implementation in PCI. > > > > registering and unregistering drivers belongs in the bus code, NOT in > > the driver code. > > Why? We're not (re-)implementing a bus here. Again, those are just abstractions > to call the C functions to register a driver. The corresponding C functions are > e.g. driver_register() or __pci_register_driver(). Those are defined in > drivers/base/driver.c and drivers/pci/pci-driver.c respectively. > > Why wouldn't we follow the same scheme in Rust abstractions? It's the bus that does the registering, so yeah, don't put it here at all as it's not going to be needed (i.e. unless you write a bus in rust you will never call driver_register()) So this can just be a wrapper for the pci bus logic, keeping it simpler. So you might be able to delete a lot of code here, only deal with a "dumb" struct device wrapper to handle reference counts, and then do the rest for the specific bus bindings? Or is that too much to dream? You aren't writing a "raw" driver here, no one does that, it's a bus that handles that logic for you, and you should not have to expose any "raw" driver attributes. Yes, for some busses, they like to force a driver to set the "raw" driver attribute, but I don't think that's a good idea and for the pci driver layer, that shouldn't be necessary now, right? If not, what fields are you wanting to get direct access to? thanks, greg k-h
On Tue, Jun 04, 2024 at 06:00:11PM +0200, Greg KH wrote: > On Tue, Jun 04, 2024 at 05:41:21PM +0200, Danilo Krummrich wrote: > > On Tue, Jun 04, 2024 at 04:27:31PM +0200, Greg KH wrote: > > > On Wed, May 22, 2024 at 12:21:53AM +0200, Danilo Krummrich wrote: > > > > On Tue, May 21, 2024 at 11:35:43AM +0200, Greg KH wrote: > > > > > On Tue, May 21, 2024 at 12:30:37AM +0200, Danilo Krummrich wrote: > > > > > > On Mon, May 20, 2024 at 08:14:18PM +0200, Greg KH wrote: > > > > > > > On Mon, May 20, 2024 at 07:25:39PM +0200, Danilo Krummrich wrote: > > > > > > > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > > > > > > > > > > > > > This defines general functionality related to registering drivers with > > > > > > > > their respective subsystems, and registering modules that implement > > > > > > > > drivers. > > > > > > > > > > > > > > > > Co-developed-by: Asahi Lina <lina@asahilina.net> > > > > > > > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > > > > > > > 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/kernel/driver.rs | 492 +++++++++++++++++++++++++++++++++++ > > > > > > > > rust/kernel/lib.rs | 4 +- > > > > > > > > rust/macros/module.rs | 2 +- > > > > > > > > samples/rust/rust_minimal.rs | 2 +- > > > > > > > > samples/rust/rust_print.rs | 2 +- > > > > > > > > 5 files changed, 498 insertions(+), 4 deletions(-) > > > > > > > > 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..e0cfc36d47ff > > > > > > > > --- /dev/null > > > > > > > > +++ b/rust/kernel/driver.rs > > > > > > > > @@ -0,0 +1,492 @@ > > > > > > > > +// 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. > > > > > > > > > > > > > > Why are you creating new "names" here? "DriverOps" is part of a 'struct > > > > > > > device_driver' why are you separating it out here? And what is > > > > > > > > > > > > DriverOps is a trait which abstracts a subsystems register() and unregister() > > > > > > functions to (un)register drivers. It exists such that a generic Registration > > > > > > implementation calls the correct one for the subsystem. > > > > > > > > > > > > For instance, PCI would implement DriverOps::register() by calling into > > > > > > bindings::__pci_register_driver(). > > > > > > > > > > > > We can discuss whether DriverOps is a good name for the trait, but it's not a > > > > > > (different) name for something that already exists and already has a name. > > > > > > > > > > It's a name we don't have in the C code as the design of the driver core > > > > > does not need or provide it. It's just the section of 'struct > > > > > device_driver' that provides function callbacks, why does it need to be > > > > > separate at all? > > > > > > > > I'm confused by the relationship to `struct device_driver` you seem to imply. > > > > How is it related? > > > > > > > > Again, this is just a trait for subsystems to provide their corresponding > > > > register and unregister implementation, e.g. pci_register_driver() and > > > > pci_unregister_driver(), such that they can be called from the generic > > > > Registration code below. > > > > > > > > See [1] for an example implementation in PCI. > > > > > > registering and unregistering drivers belongs in the bus code, NOT in > > > the driver code. > > > > Why? We're not (re-)implementing a bus here. Again, those are just abstractions > > to call the C functions to register a driver. The corresponding C functions are > > e.g. driver_register() or __pci_register_driver(). Those are defined in > > drivers/base/driver.c and drivers/pci/pci-driver.c respectively. > > > > Why wouldn't we follow the same scheme in Rust abstractions? > > It's the bus that does the registering, so yeah, don't put it here at > all as it's not going to be needed (i.e. unless you write a bus in rust > you will never call driver_register()) So this can just be a wrapper > for the pci bus logic, keeping it simpler. We never call driver_register() of course, I gave this example for another reason above. Sorry if that was confusing. > > So you might be able to delete a lot of code here, only deal with a > "dumb" struct device wrapper to handle reference counts, and then do the > rest for the specific bus bindings? Or is that too much to dream? Again, this is a generalization such that we do not have to replicate code for every subsystem / bus. Please see the full explanation below. > > You aren't writing a "raw" driver here, no one does that, it's a bus > that handles that logic for you, and you should not have to expose any > "raw" driver attributes. Indeed - we're not doing that here. > > Yes, for some busses, they like to force a driver to set the "raw" > driver attribute, but I don't think that's a good idea and for the pci > driver layer, that shouldn't be necessary now, right? If not, what > fields are you wanting to get direct access to? Honestly, this all reads as if you did not (carefully) read the code we're discussing about in the first place, did you? It reads more as if you take assumptions based on my previous explanations, and since communication is difficult, it looks like we're talking past each other. Maybe also my explanations were just not good enough. :( Either way, I suggest to focus more on the actual code. In particular let's have a look at the `Registration` and `DriverOps` struct from this patch and how they're used in the PCI code and in an actual driver. Please have a look at how the PCI code implements the `DriverOps` trait (or interface as many other languages would call it) [1]. In `DriverOps::register` and `DriverOps::unregister` the PCI code simply calls the C functions __pci_register_driver() and pci_unregister_driver(). A driver can use the `module_pci_driver!` macro [2] to declare a kernel module that registers a single PCI driver. This is equivalent to C's module_pci_driver() macro. The `module_pci_driver!` macro calls the generic `module_driver!` macro [3] and passes the `pci::Adapter` [1] (the PCI thing that actually calls the C pci_{un}register_driver() functions). The `module_driver!` macro creates a new `Module` structure [4] that holds the `Registration` structure. This `Registration` structure has a generic argument which is the `pci::Adapter` [1]. Which means that once the `Registration` is created C's __pci_register_driver() is called and once it is destroyed C's pci_unregister_driver() is called. The `Registration` is created in module_init() and destroyed in module_exit() accordingly. So, as you can see a `Registration` is really just the parts generalized that otherwise every subsystem would need to implement and `DriverOps` is the glue between a driver `Registration` and the subsystem (e.g. PCI) defining which function to call on driver register() or driver unregister(). My argument above is simply that since all this is just the abstraction to declare a driver structure and register it, it belongs into driver.rs. At least if we want to go along with where the C side places the correspong functions on the C side. (The links already point to the new code that allocates the driver statically, but this should not matter, since conceptually it's the same.) [1] https://gitlab.freedesktop.org/drm/nova/-/blob/989338f129146af9952304c2cc6b33fbd90e8909/rust/kernel/pci.rs#L24 [2] https://gitlab.freedesktop.org/drm/nova/-/blob/989338f129146af9952304c2cc6b33fbd90e8909/rust/kernel/pci.rs#L135 [3] https://gitlab.freedesktop.org/drm/nova/-/blob/989338f129146af9952304c2cc6b33fbd90e8909/rust/kernel/driver.rs#L453 [4] https://gitlab.freedesktop.org/drm/nova/-/blob/989338f129146af9952304c2cc6b33fbd90e8909/rust/kernel/driver.rs#L435 > > thanks, > > greg k-h >
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs new file mode 100644 index 000000000000..e0cfc36d47ff --- /dev/null +++ b/rust/kernel/driver.rs @@ -0,0 +1,492 @@ +// 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::{ + alloc::{box_ext::BoxExt, flags::*}, + error::code::*, + error::Result, + str::CStr, + sync::Arc, + ThisModule, +}; +use alloc::boxed::Box; +use core::{cell::UnsafeCell, marker::PhantomData, ops::Deref, pin::Pin}; + +/// A subsystem (e.g., PCI, Platform, Amba, etc.) that allows drivers to be written for it. +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`]. + unsafe 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`]. + unsafe fn unregister(reg: *mut Self::RegType); +} + +/// The registration of a driver. +pub struct Registration<T: DriverOps> { + is_registered: bool, + concrete_reg: UnsafeCell<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> {} + +impl<T: DriverOps> Registration<T> { + /// Creates a new instance of the registration object. + pub fn new() -> Self { + Self { + is_registered: false, + concrete_reg: UnsafeCell::new(T::RegType::default()), + } + } + + /// Allocates a pinned registration object and registers it. + /// + /// Returns a pinned heap-allocated representation of the registration. + pub fn new_pinned(name: &'static CStr, module: &'static ThisModule) -> Result<Pin<Box<Self>>> { + let mut reg = Pin::from(Box::new(Self::new(), GFP_KERNEL)?); + reg.as_mut().register(name, module)?; + Ok(reg) + } + + /// Registers a driver with its subsystem. + /// + /// It must be pinned because the memory block that represents the registration is potentially + /// self-referential. + pub fn register( + self: Pin<&mut Self>, + name: &'static CStr, + module: &'static ThisModule, + ) -> Result { + // SAFETY: We never move out of `this`. + let this = unsafe { self.get_unchecked_mut() }; + if this.is_registered { + // Already registered. + return Err(EINVAL); + } + + // SAFETY: `concrete_reg` was initialised via its default constructor. It is only freed + // after `Self::drop` is called, which first calls `T::unregister`. + unsafe { T::register(this.concrete_reg.get(), name, module) }?; + + this.is_registered = true; + Ok(()) + } +} + +impl<T: DriverOps> Default for Registration<T> { + fn default() -> Self { + Self::new() + } +} + +impl<T: DriverOps> Drop for Registration<T> { + fn drop(&mut self) { + if self.is_registered { + // SAFETY: This path only runs if a previous call to `T::register` completed + // successfully. + unsafe { T::unregister(self.concrete_reg.get()) }; + } + } +} + +/// 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. +/// - [`RawDeviceId::to_rawid`] stores `offset` in the context/data field of the raw device id so +/// that buses can recover the pointer to the data. +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 as a parameter the concrete ID type 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::driver::IdArray<$id_type, U, N> + where + $id_type: $crate::driver::RawDeviceId + Copy, + <$id_type as $crate::driver::RawDeviceId>::RawType: Copy + Clone, + { + let mut raw_ids = + [<$id_type as $crate::driver::RawDeviceId>::ZERO; N]; + let mut i = 0usize; + while i < N { + let offset: isize = $crate::driver::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::driver::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 +/// +// TODO: Exported but not usable by kernel modules (requires `const_trait_impl`). +/// ```ignore +/// #![feature(const_trait_impl)] +/// # use kernel::{define_id_array, driver::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 const RawDeviceId for Id { +/// type RawType = (u64, isize); +/// const ZERO: Self::RawType = (0, 0); +/// fn to_rawid(&self, offset: isize) -> Self::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::driver::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 +/// +// TODO: Exported but not usable by kernel modules (requires `const_trait_impl`). +/// ```ignore +/// #![feature(const_trait_impl)] +/// # use kernel::{define_id_table, driver::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 const RawDeviceId for Id { +/// type RawType = (u64, isize); +/// const ZERO: Self::RawType = (0, 0); +/// fn to_rawid(&self, offset: isize) -> Self::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::driver::IdTable<'static, $id_type, $data_type>> = { + $crate::define_id_array!(ARRAY, $id_type, $data_type, [ $($t)* ]); + Some(ARRAY.as_table()) + }; + }; +} + +/// Custom code within device removal. +pub trait DeviceRemoval { + /// Cleans resources up when the device is removed. + /// + /// This is called when a device is removed and offers implementers the chance to run some code + /// that cleans state up. + fn device_remove(&self); +} + +impl DeviceRemoval for () { + fn device_remove(&self) {} +} + +impl<T: DeviceRemoval> DeviceRemoval for Arc<T> { + fn device_remove(&self) { + self.deref().device_remove(); + } +} + +impl<T: DeviceRemoval> DeviceRemoval for Box<T> { + fn device_remove(&self) { + self.deref().device_remove(); + } +} + +/// 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. +pub struct Module<T: DriverOps> { + _driver: Pin<Box<Registration<T>>>, +} + +impl<T: DriverOps> crate::Module for Module<T> { + fn init(name: &'static CStr, module: &'static ThisModule) -> Result<Self> { + Ok(Self { + _driver: Registration::new_pinned(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 4ba3d4a49e9c..698121c925f3 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 driver; pub mod error; pub mod init; pub mod ioctl; @@ -69,7 +71,7 @@ pub trait Module: Sized + Sync { /// 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>; } /// Equivalent to `THIS_MODULE` in the C API. diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 27979e582e4b..3e7a6a8560f5 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -275,7 +275,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { }} fn __init() -> core::ffi::c_int {{ - match <{type_} as kernel::Module>::init(&THIS_MODULE) {{ + match <{type_} as kernel::Module>::init(kernel::c_str!(\"{name}\"), &THIS_MODULE) {{ Ok(m) => {{ unsafe {{ __MOD = Some(m); 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");