| Message ID | 20241205141533.111830-3-dakr@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device / Driver PCI / Platform Rust abstractions | expand |
On Thu, Dec 5, 2024 at 3:16 PM Danilo Krummrich <dakr@kernel.org> wrote: > > Implement the generic `Registration` type and the `DriverOps` trait. > > The `Registration` structure is the common type that represents a driver > registration and is typically bound to the lifetime of a module. However, > it doesn't implement actual calls to the kernel's driver core to register > drivers itself. > > Instead the `DriverOps` trait is provided to subsystems, which have to > implement `DriverOps::register` and `DrvierOps::unregister`. Subsystems typo > have to provide an implementation for both of those methods where the > subsystem specific variants to register / unregister a driver have to > implemented. > > For instance, the PCI subsystem would call __pci_register_driver() from > `DriverOps::register` and pci_unregister_driver() from > `DrvierOps::unregister`. > > Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Signed-off-by: Danilo Krummrich <dakr@kernel.org> [...] > +/// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, > +/// Amba, etc.) to provide the corresponding subsystem specific implementation to register / > +/// unregister a driver of the particular type (`RegType`). > +/// > +/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call > +/// `bindings::__pci_register_driver` from `RegistrationOps::register` and > +/// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`. > +pub trait RegistrationOps { > + /// The type that holds information about the registration. This is typically a struct defined > + /// by the C portion of the kernel. > + type RegType: Default; This Default implementation doesn't seem useful. You initialize it and then `register` calls a C function to initialize it. Having `register` return an `impl PinInit` seems like it would work better here. > + /// Registers a driver. > + /// > + /// On success, `reg` must remain pinned and valid until the matching call to > + /// [`RegistrationOps::unregister`]. > + fn register( > + reg: &mut Self::RegType, If the intent is that RegType is going to be the raw bindings:: type, then this isn't going to work because you're creating &mut references to the raw type without a Opaque wrapper in between. > + name: &'static CStr, > + module: &'static ThisModule, > + ) -> Result; > + > + /// Unregisters a driver previously registered with [`RegistrationOps::register`]. > + fn unregister(reg: &mut Self::RegType); I believe this handles pinning incorrectly. You can't hand out &mut references to pinned values. Alice
On Fri, Dec 06, 2024 at 02:57:19PM +0100, Alice Ryhl wrote: > On Thu, Dec 5, 2024 at 3:16 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > Implement the generic `Registration` type and the `DriverOps` trait. > > > > The `Registration` structure is the common type that represents a driver > > registration and is typically bound to the lifetime of a module. However, > > it doesn't implement actual calls to the kernel's driver core to register > > drivers itself. > > > > Instead the `DriverOps` trait is provided to subsystems, which have to > > implement `DriverOps::register` and `DrvierOps::unregister`. Subsystems > > typo > > > have to provide an implementation for both of those methods where the > > subsystem specific variants to register / unregister a driver have to > > implemented. > > > > For instance, the PCI subsystem would call __pci_register_driver() from > > `DriverOps::register` and pci_unregister_driver() from > > `DrvierOps::unregister`. > > > > Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com> > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > [...] > > > +/// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, > > +/// Amba, etc.) to provide the corresponding subsystem specific implementation to register / > > +/// unregister a driver of the particular type (`RegType`). > > +/// > > +/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call > > +/// `bindings::__pci_register_driver` from `RegistrationOps::register` and > > +/// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`. > > +pub trait RegistrationOps { > > + /// The type that holds information about the registration. This is typically a struct defined > > + /// by the C portion of the kernel. > > + type RegType: Default; > > This Default implementation doesn't seem useful. You initialize it and I think it is -- `RegType` is always the raw bindings:: type and in `Registration::new` in `Opaque::try_ffi_init` we call `ptr.write(T::RegType::default())` for - since `RegType` is a raw bindings:: type - zero initialization. > then `register` calls a C function to initialize it. Having `register` > return an `impl PinInit` seems like it would work better here. This would work as well, but it would effectively move the common code from `Registration::new` to the bus specific type. I think it's quite nice that the bus specific code does not need to care about messing with `try_pin_init`, `Opaque::try_ffi_init`, zero initialization, etc., but just needs to assign the relevant fields and call register. > > > + /// Registers a driver. > > + /// > > + /// On success, `reg` must remain pinned and valid until the matching call to > > + /// [`RegistrationOps::unregister`]. > > + fn register( > > + reg: &mut Self::RegType, > > If the intent is that RegType is going to be the raw bindings:: type, > then this isn't going to work because you're creating &mut references > to the raw type without a Opaque wrapper in between. True, that seems unsound. Since this is called from when the corresponding `Opaque` wrapper is created, I think we need to fall back to a raw pointer then and make `register` and `unregister` unsafe. I don't think that too big of a deal though, since those two should never be called from anywhere else than `Registration:new` or `Registration::drop`. > > > + name: &'static CStr, > > + module: &'static ThisModule, > > + ) -> Result; > > + > > + /// Unregisters a driver previously registered with [`RegistrationOps::register`]. > > + fn unregister(reg: &mut Self::RegType); > > I believe this handles pinning incorrectly. You can't hand out &mut > references to pinned values. Same as above. > > Alice
diff --git a/MAINTAINERS b/MAINTAINERS index 1e930c7a58b1..085b20dc5c0b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7031,6 +7031,7 @@ F: include/linux/kobj* F: include/linux/property.h F: lib/kobj* F: rust/kernel/device.rs +F: rust/kernel/driver.rs DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS) M: Nishanth Menon <nm@ti.com> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs new file mode 100644 index 000000000000..3ec0ba0556a1 --- /dev/null +++ b/rust/kernel/driver.rs @@ -0,0 +1,120 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.). +//! +//! Each bus / subsystem is expected to implement [`RegistrationOps`], which allows drivers to +//! register using the [`Registration`] class. + +use crate::error::{Error, Result}; +use crate::{init::PinInit, str::CStr, try_pin_init, types::Opaque, ThisModule}; +use core::pin::Pin; +use macros::{pin_data, pinned_drop}; + +/// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, +/// Amba, etc.) to provide the corresponding subsystem specific implementation to register / +/// unregister a driver of the particular type (`RegType`). +/// +/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call +/// `bindings::__pci_register_driver` from `RegistrationOps::register` and +/// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`. +pub trait RegistrationOps { + /// The type that holds information about the registration. This is typically a struct defined + /// by the C portion of the kernel. + type RegType: Default; + + /// Registers a driver. + /// + /// On success, `reg` must remain pinned and valid until the matching call to + /// [`RegistrationOps::unregister`]. + fn register( + reg: &mut Self::RegType, + name: &'static CStr, + module: &'static ThisModule, + ) -> Result; + + /// Unregisters a driver previously registered with [`RegistrationOps::register`]. + fn unregister(reg: &mut Self::RegType); +} + +/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g. +/// `bindings::pci_driver`). Therefore a [`Registration`] must be initialized with a type that +/// implements the [`RegistrationOps`] trait, such that the generic `T::register` and +/// `T::unregister` calls result in the subsystem specific registration calls. +/// +///Once the `Registration` structure is dropped, the driver is unregistered. +#[pin_data(PinnedDrop)] +pub struct Registration<T: RegistrationOps> { + #[pin] + reg: Opaque<T::RegType>, +} + +// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to +// share references to it with multiple threads as nothing can be done. +unsafe impl<T: RegistrationOps> Sync for Registration<T> {} + +// SAFETY: Both registration and unregistration are implemented in C and safe to be performed from +// any thread, so `Registration` is `Send`. +unsafe impl<T: RegistrationOps> Send for Registration<T> {} + +impl<T: RegistrationOps> Registration<T> { + /// Creates a new instance of the registration object. + pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> { + try_pin_init!(Self { + reg <- Opaque::try_ffi_init(|ptr: *mut T::RegType| { + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write. + unsafe { ptr.write(T::RegType::default()) }; + + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write, and it has + // just been initialised above, so it's also valid for read. + let drv = unsafe { &mut *ptr }; + + T::register(drv, name, module) + }), + }) + } +} + +#[pinned_drop] +impl<T: RegistrationOps> PinnedDrop for Registration<T> { + fn drop(self: Pin<&mut Self>) { + // SAFETY: The existence of the `Registration` guarantees that `self.reg.get()` is properly + // aligned and points to a valid value. + let drv = unsafe { &mut *self.reg.get() }; + + T::unregister(drv); + } +} + +/// A kernel module that only registers the given driver on init. +/// +/// This is a helper struct to make it easier to define single-functionality modules, in this case, +/// modules that offer a single driver. +#[pin_data] +pub struct Module<T: RegistrationOps> { + #[pin] + _driver: Registration<T>, +} + +impl<T: RegistrationOps + Sync + Send> crate::InPlaceModule for Module<T> { + fn init(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> { + try_pin_init!(Self { + _driver <- Registration::<T>::new(name, module), + }) + } +} + +/// Declares a kernel module that exposes a single driver. +/// +/// It is meant to be used as a helper by other subsystems so they can more easily expose their own +/// macros. +#[macro_export] +macro_rules! module_driver { + (<$gen_type:ident>, $driver_ops:ty, { type: $type:ty, $($f:tt)* }) => { + type Ops<$gen_type> = $driver_ops; + type ModuleType = $crate::driver::Module<Ops<$type>>; + $crate::prelude::module! { + type: ModuleType, + $($f)* + } + } +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 686db6aa3323..0a719396256f 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -35,6 +35,7 @@ mod build_assert; pub mod cred; pub mod device; +pub mod driver; pub mod error; #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)] pub mod firmware;