| Message ID | 20240618234025.15036-3-dakr@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Device / Driver and PCI Rust abstractions | expand |
On Wed, Jun 19, 2024 at 01:39:48AM +0200, Danilo Krummrich wrote: > Implement the generic `Registration` type and the `DriverOps` trait. I don't think this is needed, more below... > The `Registration` structure is the common type that represents a driver > registration and is typically bound to the lifetime of a module. However, > it doesn't implement actual calls to the kernel's driver core to register > drivers itself. But that's not what normally happens, more below... > Instead the `DriverOps` trait is provided to subsystems, which have to > implement `DriverOps::register` and `DrvierOps::unregister`. Subsystems > have to provide an implementation for both of those methods where the > subsystem specific variants to register / unregister a driver have to > implemented. So you are saying this should be something that a "bus" would do? Please be explicit as to what you mean by "subsystem" here. > For instance, the PCI subsystem would call __pci_register_driver() from > `DriverOps::register` and pci_unregister_driver() from > `DrvierOps::unregister`. So this is a BusOps, or more in general, a "subsystem" if it's not a bus (i.e. it's a class). Note, we used to use the term "subsystem" a very long time ago but got rid of them in the driver core, let's not bring it back unless we REALLY know we want it this time. So why isn't this just a BusOps? > This patch is based on previous work from Wedson Almeida Filho. > > Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > --- > rust/kernel/driver.rs | 128 ++++++++++++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 2 files changed, 129 insertions(+) > create mode 100644 rust/kernel/driver.rs > > diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs > new file mode 100644 > index 000000000000..e04406b93b56 > --- /dev/null > +++ b/rust/kernel/driver.rs > @@ -0,0 +1,128 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.). See, you think it's a bus, let's call it a bus! :) > +//! > +//! Each bus / subsystem is expected to implement [`DriverOps`], which allows drivers to register > +//! using the [`Registration`] class. > + > +use crate::error::{Error, Result}; > +use crate::{init::PinInit, str::CStr, try_pin_init, types::Opaque, ThisModule}; > +use core::pin::Pin; > +use macros::{pin_data, pinned_drop}; > + > +/// The [`DriverOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, Amba, > +/// etc.) to privide the corresponding subsystem specific implementation to register / unregister a > +/// driver of the particular type (`RegType`). > +/// > +/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call > +/// `bindings::__pci_register_driver` from `DriverOps::register` and > +/// `bindings::pci_unregister_driver` from `DriverOps::unregister`. > +pub trait DriverOps { > + /// The type that holds information about the registration. This is typically a struct defined > + /// by the C portion of the kernel. > + type RegType: Default; > + > + /// Registers a driver. > + /// > + /// # Safety > + /// > + /// `reg` must point to valid, initialised, and writable memory. It may be modified by this > + /// function to hold registration state. > + /// > + /// On success, `reg` must remain pinned and valid until the matching call to > + /// [`DriverOps::unregister`]. > + fn register( > + reg: &mut Self::RegType, > + name: &'static CStr, > + module: &'static ThisModule, > + ) -> Result; > + > + /// Unregisters a driver previously registered with [`DriverOps::register`]. > + /// > + /// # Safety > + /// > + /// `reg` must point to valid writable memory, initialised by a previous successful call to > + /// [`DriverOps::register`]. > + fn unregister(reg: &mut Self::RegType); > +} So you are getting into what a bus wants/needs to support here, why is register/unregister the "big" things to be implemented first? Why not just map the current register/unregister bus functions to a bus-specific trait for now? And then, if you think it really should be generic, we can make it that way then. I don't see why this needs to be generic now as you aren't implementing a bus in rust at this point in time, right? > + > +/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g. > +/// `bindings::pci_driver`). Therefore a [`Registration`] is initialized with some type that > +/// implements the [`DriverOps`] trait, such that the generic `T::register` and `T::unregister` > +/// calls result in the subsystem specific registration calls. > +/// > +///Once the `Registration` structure is dropped, the driver is unregistered. > +#[pin_data(PinnedDrop)] > +pub struct Registration<T: DriverOps> { > + #[pin] > + reg: Opaque<T::RegType>, > +} > + > +// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to > +// share references to it with multiple threads as nothing can be done. > +unsafe impl<T: DriverOps> Sync for Registration<T> {} > + > +// SAFETY: Both registration and unregistration are implemented in C and safe to be performed from > +// any thread, so `Registration` is `Send`. > +unsafe impl<T: DriverOps> Send for Registration<T> {} > + > +impl<T: DriverOps> Registration<T> { > + /// Creates a new instance of the registration object. > + pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> { Drivers have modules, not busses. So you are registering a driver with a bus here, it's not something that a driver itself implements as you have named here. > + try_pin_init!(Self { > + reg <- Opaque::try_ffi_init(|ptr: *mut T::RegType| { > + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write. > + unsafe { ptr.write(T::RegType::default()) }; > + > + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write, and it has > + // just been initialised above, so it's also valid for read. > + let drv = unsafe { &mut *ptr }; > + > + T::register(drv, name, module) > + }), > + }) > + } > +} > + > +#[pinned_drop] > +impl<T: DriverOps> PinnedDrop for Registration<T> { > + fn drop(self: Pin<&mut Self>) { > + let drv = unsafe { &mut *self.reg.get() }; > + > + T::unregister(drv); > + } > +} > + > +/// A kernel module that only registers the given driver on init. > +/// > +/// This is a helper struct to make it easier to define single-functionality modules, in this case, > +/// modules that offer a single driver. > +#[pin_data] > +pub struct Module<T: DriverOps> { > + #[pin] > + _driver: Registration<T>, > +} While these are nice, let's not add them just yet, let's keep it simple for now until we work out the proper model and see what is, and is not, common for drivers to do. That way we keep the review simpler as well, and saves you time rewriting things as we rename / modify stuff. > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -29,6 +29,7 @@ > pub mod alloc; > mod build_assert; > pub mod device; > +pub mod driver; Nope, this is a bus :) thanks, greg k-h
On Thu, Jun 20, 2024 at 04:28:23PM +0200, Greg KH wrote: > On Wed, Jun 19, 2024 at 01:39:48AM +0200, Danilo Krummrich wrote: > > Implement the generic `Registration` type and the `DriverOps` trait. > > I don't think this is needed, more below... > > > The `Registration` structure is the common type that represents a driver > > registration and is typically bound to the lifetime of a module. However, > > it doesn't implement actual calls to the kernel's driver core to register > > drivers itself. > > But that's not what normally happens, more below... I can't find below a paragraph that seems related to this, hence I reply here. The above is just different wording for: A driver is typically registered in module_init() and unregistered in module_exit(). Isn't that what happens normally? > > > Instead the `DriverOps` trait is provided to subsystems, which have to > > implement `DriverOps::register` and `DrvierOps::unregister`. Subsystems > > have to provide an implementation for both of those methods where the > > subsystem specific variants to register / unregister a driver have to > > implemented. > > So you are saying this should be something that a "bus" would do? > Please be explicit as to what you mean by "subsystem" here. Yes, I agree it's more precise to say that this should be implemented by a bus (e.g. PCI). I can reword this one. > > > For instance, the PCI subsystem would call __pci_register_driver() from > > `DriverOps::register` and pci_unregister_driver() from > > `DrvierOps::unregister`. > > So this is a BusOps, or more in general, a "subsystem" if it's not a > bus (i.e. it's a class). Note, we used to use the term "subsystem" a > very long time ago but got rid of them in the driver core, let's not > bring it back unless we REALLY know we want it this time. > > So why isn't this just a BusOps? I think it's really about perspective. Generally speaking, when a driver is registered it gets added to a bus through bus_add_driver(). Now, one could argue that the "register" operation is a bus operation, since something gets registered on the bus, but one could also argue that it's a driver operation, since a driver is registered on something. Consequently, I think it's neither wrong to call this one `BusOps` nor is it wrong to call it `DriverOps`. I still think `DriverOps` is more appropriate, since here we're looking at it from the perspective of the driver. In the end we call it as `driver.register()` instead of `bus.register()`. For instance, in the PCI implementation of it, we call __pci_register_driver() from `DriverOps::register`. > > This patch is based on previous work from Wedson Almeida Filho. > > > > Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com> > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > > --- > > rust/kernel/driver.rs | 128 ++++++++++++++++++++++++++++++++++++++++++ > > rust/kernel/lib.rs | 1 + > > 2 files changed, 129 insertions(+) > > create mode 100644 rust/kernel/driver.rs > > > > diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs > > new file mode 100644 > > index 000000000000..e04406b93b56 > > --- /dev/null > > +++ b/rust/kernel/driver.rs > > @@ -0,0 +1,128 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.). > > See, you think it's a bus, let's call it a bus! :) > > > +//! > > +//! Each bus / subsystem is expected to implement [`DriverOps`], which allows drivers to register > > +//! using the [`Registration`] class. > > + > > +use crate::error::{Error, Result}; > > +use crate::{init::PinInit, str::CStr, try_pin_init, types::Opaque, ThisModule}; > > +use core::pin::Pin; > > +use macros::{pin_data, pinned_drop}; > > + > > +/// The [`DriverOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, Amba, > > +/// etc.) to privide the corresponding subsystem specific implementation to register / unregister a > > +/// driver of the particular type (`RegType`). > > +/// > > +/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call > > +/// `bindings::__pci_register_driver` from `DriverOps::register` and > > +/// `bindings::pci_unregister_driver` from `DriverOps::unregister`. > > +pub trait DriverOps { > > + /// The type that holds information about the registration. This is typically a struct defined > > + /// by the C portion of the kernel. > > + type RegType: Default; > > + > > + /// Registers a driver. > > + /// > > + /// # Safety > > + /// > > + /// `reg` must point to valid, initialised, and writable memory. It may be modified by this > > + /// function to hold registration state. > > + /// > > + /// On success, `reg` must remain pinned and valid until the matching call to > > + /// [`DriverOps::unregister`]. > > + fn register( > > + reg: &mut Self::RegType, > > + name: &'static CStr, > > + module: &'static ThisModule, > > + ) -> Result; > > + > > + /// Unregisters a driver previously registered with [`DriverOps::register`]. > > + /// > > + /// # Safety > > + /// > > + /// `reg` must point to valid writable memory, initialised by a previous successful call to > > + /// [`DriverOps::register`]. > > + fn unregister(reg: &mut Self::RegType); > > +} > > So you are getting into what a bus wants/needs to support here, why is > register/unregister the "big" things to be implemented first? Why not > just map the current register/unregister bus functions to a bus-specific > trait for now? And then, if you think it really should be generic, we A bus specific trait would not add any value. The whole point if a trait is to represent a generic interface. It basically describes the functionality we expect from a certain category of types. In this case we know that every driver can be registered and unregistered, hence we can define a generic trait that every bus specific driver structure, e.g. PCI driver, has to implement. > can make it that way then. I don't see why this needs to be generic now > as you aren't implementing a bus in rust at this point in time, right? With the above tait (or interface) we now can have a generic `Registration` that calls `T::register` and `T::unregister` and works for all driver types (PCI, platform, etc.). Otherwise we'd need a `pci::Registration`, a `platform::Registration` etc. and copy-paste the below code for all of them. > > > + > > +/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g. > > +/// `bindings::pci_driver`). Therefore a [`Registration`] is initialized with some type that > > +/// implements the [`DriverOps`] trait, such that the generic `T::register` and `T::unregister` > > +/// calls result in the subsystem specific registration calls. > > +/// > > +///Once the `Registration` structure is dropped, the driver is unregistered. > > +#[pin_data(PinnedDrop)] > > +pub struct Registration<T: DriverOps> { > > + #[pin] > > + reg: Opaque<T::RegType>, > > +} > > + > > +// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to > > +// share references to it with multiple threads as nothing can be done. > > +unsafe impl<T: DriverOps> Sync for Registration<T> {} > > + > > +// SAFETY: Both registration and unregistration are implemented in C and safe to be performed from > > +// any thread, so `Registration` is `Send`. > > +unsafe impl<T: DriverOps> Send for Registration<T> {} > > + > > +impl<T: DriverOps> Registration<T> { > > + /// Creates a new instance of the registration object. > > + pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> { > > Drivers have modules, not busses. So you are registering a driver with > a bus here, it's not something that a driver itself implements as you > have named here. We are registering a driver on bus here, see the below `T::register` call, this one ends up in __pci_register_driver() or __platform_driver_register(), etc. Hence we need the module and the module name. Please see the first patch of the series for an explanation why THIS_MODULE and KBUILD_MODNAME is not in scope here and why we need to pass this through. > > > > + try_pin_init!(Self { > > + reg <- Opaque::try_ffi_init(|ptr: *mut T::RegType| { > > + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write. > > + unsafe { ptr.write(T::RegType::default()) }; > > + > > + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write, and it has > > + // just been initialised above, so it's also valid for read. > > + let drv = unsafe { &mut *ptr }; > > + > > + T::register(drv, name, module) > > + }), > > + }) > > + } > > +} > > + > > +#[pinned_drop] > > +impl<T: DriverOps> PinnedDrop for Registration<T> { > > + fn drop(self: Pin<&mut Self>) { > > + let drv = unsafe { &mut *self.reg.get() }; > > + > > + T::unregister(drv); > > + } > > +} > > + > > +/// A kernel module that only registers the given driver on init. > > +/// > > +/// This is a helper struct to make it easier to define single-functionality modules, in this case, > > +/// modules that offer a single driver. > > +#[pin_data] > > +pub struct Module<T: DriverOps> { > > + #[pin] > > + _driver: Registration<T>, > > +} > > While these are nice, let's not add them just yet, let's keep it simple > for now until we work out the proper model and see what is, and is not, > common for drivers to do. Honestly, even though it seems to be complicated, this is in fact the simple way. This really is the minimum we can do to register a driver and get it probed. Please also consider that all the abstractions I am submitting in this series are only making use of APIs that regular C drivers use as well. This series doesn't touch internals of any subsystem. Hence, we know the model very well. It's the same as in C, we're just abstracting it into common Rust design patterns. The only exception might be passing through the module name, but this is just the consequence of the design and necessity that already has been discussed and was merged. > > That way we keep the review simpler as well, and saves you time > rewriting things as we rename / modify stuff. > > > --- a/rust/kernel/lib.rs > > +++ b/rust/kernel/lib.rs > > @@ -29,6 +29,7 @@ > > pub mod alloc; > > mod build_assert; > > pub mod device; > > +pub mod driver; > > Nope, this is a bus :) > > thanks, > > greg k-h >
On Thu, Jun 20, 2024 at 07:12:42PM +0200, Danilo Krummrich wrote: > On Thu, Jun 20, 2024 at 04:28:23PM +0200, Greg KH wrote: > > On Wed, Jun 19, 2024 at 01:39:48AM +0200, Danilo Krummrich wrote: > > > Implement the generic `Registration` type and the `DriverOps` trait. > > > > I don't think this is needed, more below... > > > > > The `Registration` structure is the common type that represents a driver > > > registration and is typically bound to the lifetime of a module. However, > > > it doesn't implement actual calls to the kernel's driver core to register > > > drivers itself. > > > > But that's not what normally happens, more below... > > I can't find below a paragraph that seems related to this, hence I reply here. > > The above is just different wording for: A driver is typically registered in > module_init() and unregistered in module_exit(). > > Isn't that what happens normally? Yes, but it's nothing we have ever used in the kernel before. You are defining new terms in some places, and renaming existing ones in others, which is going to do nothing but confuse us all. I don't see why you need a "registration" structure here when no .c driver ever does. You just have a module init/exit call and go from there. Why not stick with that? > > > Instead the `DriverOps` trait is provided to subsystems, which have to > > > implement `DriverOps::register` and `DrvierOps::unregister`. Subsystems > > > have to provide an implementation for both of those methods where the > > > subsystem specific variants to register / unregister a driver have to > > > implemented. > > > > So you are saying this should be something that a "bus" would do? > > Please be explicit as to what you mean by "subsystem" here. > > Yes, I agree it's more precise to say that this should be implemented by a bus > (e.g. PCI). I can reword this one. Wording matters. > > > For instance, the PCI subsystem would call __pci_register_driver() from > > > `DriverOps::register` and pci_unregister_driver() from > > > `DrvierOps::unregister`. > > > > So this is a BusOps, or more in general, a "subsystem" if it's not a > > bus (i.e. it's a class). Note, we used to use the term "subsystem" a > > very long time ago but got rid of them in the driver core, let's not > > bring it back unless we REALLY know we want it this time. > > > > So why isn't this just a BusOps? > > I think it's really about perspective. Generally speaking, when a driver is > registered it gets added to a bus through bus_add_driver(). Now, one could argue > that the "register" operation is a bus operation, since something gets > registered on the bus, but one could also argue that it's a driver operation, > since a driver is registered on something. > > Consequently, I think it's neither wrong to call this one `BusOps` nor is it > wrong to call it `DriverOps`. > > I still think `DriverOps` is more appropriate, since here we're looking at it > from the perspective of the driver. Stick with the same terms we have today please. These are specific bus operations. Some drivers implement multiple bus operations within them as they can handle multiple bus types. Keep the same names we have today, it will save maintaining this for the next 40+ years easier. > In the end we call it as `driver.register()` instead of `bus.register()`. For > instance, in the PCI implementation of it, we call __pci_register_driver() from > `DriverOps::register`. You are registering with a bus, stick with that term please. > > > +/// The [`DriverOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, Amba, > > > +/// etc.) to privide the corresponding subsystem specific implementation to register / unregister a > > > +/// driver of the particular type (`RegType`). > > > +/// > > > +/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call > > > +/// `bindings::__pci_register_driver` from `DriverOps::register` and > > > +/// `bindings::pci_unregister_driver` from `DriverOps::unregister`. > > > +pub trait DriverOps { > > > + /// The type that holds information about the registration. This is typically a struct defined > > > + /// by the C portion of the kernel. > > > + type RegType: Default; > > > + > > > + /// Registers a driver. > > > + /// > > > + /// # Safety > > > + /// > > > + /// `reg` must point to valid, initialised, and writable memory. It may be modified by this > > > + /// function to hold registration state. > > > + /// > > > + /// On success, `reg` must remain pinned and valid until the matching call to > > > + /// [`DriverOps::unregister`]. > > > + fn register( > > > + reg: &mut Self::RegType, > > > + name: &'static CStr, > > > + module: &'static ThisModule, > > > + ) -> Result; > > > + > > > + /// Unregisters a driver previously registered with [`DriverOps::register`]. > > > + /// > > > + /// # Safety > > > + /// > > > + /// `reg` must point to valid writable memory, initialised by a previous successful call to > > > + /// [`DriverOps::register`]. > > > + fn unregister(reg: &mut Self::RegType); > > > +} > > > > So you are getting into what a bus wants/needs to support here, why is > > register/unregister the "big" things to be implemented first? Why not > > just map the current register/unregister bus functions to a bus-specific > > trait for now? And then, if you think it really should be generic, we > > A bus specific trait would not add any value. The whole point if a trait is to > represent a generic interface. It basically describes the functionality we > expect from a certain category of types. > > In this case we know that every driver can be registered and unregistered, hence > we can define a generic trait that every bus specific driver structure, e.g. PCI > driver, has to implement. So this is a generic trait that all drivers of all bus types must do? Ick, no, don't make this so generic it's impossible to unwind. Make things specific FIRST. Then implement a second one. Then a third one. And THEN see what you can make generic, before then it's going to be hard and a mess. Stick with specifics first, you can always make them more generic later. > > can make it that way then. I don't see why this needs to be generic now > > as you aren't implementing a bus in rust at this point in time, right? > > With the above tait (or interface) we now can have a generic `Registration` that > calls `T::register` and `T::unregister` and works for all driver types (PCI, > platform, etc.). Otherwise we'd need a `pci::Registration`, a > `platform::Registration` etc. and copy-paste the below code for all of them. Good, copy/paste to start with and then if it gets messy on the third implementation, THEN we can think about unifying them. I'm going to argue that registering with a pci bus is VERY different than registering with the platform bus, or a USB bus, which, if you look at the kernel today, is why those are different functions! Only in the driver core is the unified portions. Don't attempt to make the rust code generic and then have it split back out, and THEN have it become generic afterward. That's an odd way to do things, please don't. > > > + > > > +/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g. > > > +/// `bindings::pci_driver`). Therefore a [`Registration`] is initialized with some type that > > > +/// implements the [`DriverOps`] trait, such that the generic `T::register` and `T::unregister` > > > +/// calls result in the subsystem specific registration calls. > > > +/// > > > +///Once the `Registration` structure is dropped, the driver is unregistered. > > > +#[pin_data(PinnedDrop)] > > > +pub struct Registration<T: DriverOps> { > > > + #[pin] > > > + reg: Opaque<T::RegType>, > > > +} > > > + > > > +// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to > > > +// share references to it with multiple threads as nothing can be done. > > > +unsafe impl<T: DriverOps> Sync for Registration<T> {} > > > + > > > +// SAFETY: Both registration and unregistration are implemented in C and safe to be performed from > > > +// any thread, so `Registration` is `Send`. > > > +unsafe impl<T: DriverOps> Send for Registration<T> {} > > > + > > > +impl<T: DriverOps> Registration<T> { > > > + /// Creates a new instance of the registration object. > > > + pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> { > > > > Drivers have modules, not busses. So you are registering a driver with > > a bus here, it's not something that a driver itself implements as you > > have named here. > > We are registering a driver on bus here, see the below `T::register` call, this > one ends up in __pci_register_driver() or __platform_driver_register(), etc. Hence > we need the module and the module name. Again, let's be specific first, make things generic later. Why do you need/want a trait at all when you have only 1 bus type in the whole kernel? Keep it simple, make it so obvious that I would be foolish to reject it. Right now, it's so complex and generic that I feel foolish accepting it. thanks, greg k-h
(Please read my reply to Patch 1 first) On Wed, Jul 10, 2024 at 04:10:40PM +0200, Greg KH wrote: > On Thu, Jun 20, 2024 at 07:12:42PM +0200, Danilo Krummrich wrote: > > On Thu, Jun 20, 2024 at 04:28:23PM +0200, Greg KH wrote: > > > On Wed, Jun 19, 2024 at 01:39:48AM +0200, Danilo Krummrich wrote: > > > > Implement the generic `Registration` type and the `DriverOps` trait. > > > > > > I don't think this is needed, more below... > > > > > > > The `Registration` structure is the common type that represents a driver > > > > registration and is typically bound to the lifetime of a module. However, > > > > it doesn't implement actual calls to the kernel's driver core to register > > > > drivers itself. > > > > > > But that's not what normally happens, more below... > > > > I can't find below a paragraph that seems related to this, hence I reply here. > > > > The above is just different wording for: A driver is typically registered in > > module_init() and unregistered in module_exit(). > > > > Isn't that what happens normally? > > Yes, but it's nothing we have ever used in the kernel before. You are > defining new terms in some places, and renaming existing ones in others, > which is going to do nothing but confuse us all. We're not renaming anything, but... New terms, yes, because it's new structures that aren't needed in C, but in Rust. Why do we need those things in Rust, but not in C you may ask. Let me try to explain it while trying to clarify what the `Registration` and `DriverOps` types are actually used for, as promised in my reply to Patch 1. The first misunderstanding may be that they abstract something in drivers/base/, but that's not the case. In fact, those are not abstractions around C structures themselfes. Think of them as small helpers to implement driver abstractions in general (e.g. PCI, platform, etc.), which is why they are in a file named driver.rs. Now, what are `DriverOps`? It's just an interface that asks the implementer of the interface to implement a register() and an unregister() function. PCI obviously does implement this as pci_register_driver() and pci_unregister_driver(). Having that said, I agree with you that `DriverOps` is a bad name, I think it should be `RegistrationOps` instead - it represents the operations to register() and unregister() a driver. I will use this name in the following instead, it is less confusing. In terms of what a `Registration` does and why we need this in Rust, but not in C it is easiest to see from an example with some inline comments: ``` struct MyDriver; impl pci::Driver for MyDriver { define_pci_id_table! { bindings::PCI_VENDOR_ID_FOO, bindings::PCI_ANY_ID, None, } fn probe(dev: ARef<pci::Device>) {} fn remove() {} } struct MyModule { // `pci::RegOps` is the PCI implementation of `RegistrationOps`, i.e. // `pci::Ops::register()` calls pci_register_driver() and // `pci::Ops::unregister()` calls pci_unregister_driver(). // // `pci::RegOps` also creates the `struct pci_dev` setting probe() to // `MyDriver::probe` and remove() to `MyDriver::remove()`. reg: Registration<pci::RegOps<MyDriver>>, } impl kernel::Moduke for MyModule { fn init(name: &'static CStr, module: &'static ThisModule) -> Result<Self> { Ok(MyModule { reg: Registration::<pci::RegOps<MyDriver>>::new(name, module), }) } } ``` This code is equivalent to the following C code: ``` static int probe(struct pci_dev *pdev, const struct pci_device_id *ent) {} static void remove(struct pci_dev *pdev) {} static struct pci_driver my_pci_driver { .name = "my_driver", .id_table = pci_ids, .probe = probe, .remove = remove, }; static int __init my_module_init(void) { pci_register_driver(my_pci_driver); } module_init(my_module_init); static void __exit my_module_exit(void) { pci_unregister_driver(my_pci_driver(); } module_exit(my_module_exit); ``` You may have noticed that the Rust code doesn't need `Module::exit` at all. And the reason is the `Registration` type. `Registration` is implemented as: ``` struct Registration<T: RegistrationOps> { // In the example above `T::DriverType` is struct pci_dev. drv: T::DriverType, } impl<T: RegistrationOps> Registration<T> { pub fn new(name: &'static Cstr, module &'static ThisModule) -> Self { // SAFETY: `T::DriverType` is a C type (e.g. struct pci_dev) and // can be zero initialized. // This is a bit simplified, to not bloat the example with // pinning. let drv: T::DriverType = unsafe { core::mem::zeroed() }; // In this example, this calls `pci::RegOps::register`, which // initializes the struct pci_dev and calls // pci_register_driver(). T::register(drv, name, module); } } impl<T: RegistrationOps> Drop for Registration<T> { fn drop(&mut self) { // This calls pci_unregister_driver() on the struct pci_dev // stored in `self.drv`. T::unregister(self.drv); } } ``` As you can see, once the `Registration` goes out of scope the driver is automatically unregistered due to the drop() implementation, which is why we don't need `Module::exit`. This also answers why we need a `Registration` structure in Rust, but not in C. Rust uses different programming paradigms than C, and it uses type representations with `Drop` traits to clean things up, rather than relying on the user of the API doing it manually. I really hope this explanation and example helps and contributes to progress. As you can see I really put a lot of effort and dedication into this work. - Danilo -- Just for completeness, please find the relevant parts of `pci::RegOps` below. ``` impl<T: Driver> driver::DriverOps for Adapter<T> { type DriverType = bindings::pci_driver; fn register( pdrv: &mut bindings::pci_driver, name: &'static CStr, module: &'static ThisModule, ) -> Result { pdrv.name = name.as_char_ptr(); pdrv.probe = Some(Self::probe_callback); pdrv.remove = Some(Self::remove_callback); pdrv.id_table = T::ID_TABLE.as_ref(); // SAFETY: `pdrv` is guaranteed to be a valid `RegType`. to_result(unsafe { bindings::__pci_register_driver(pdrv as _, module.0, name.as_char_ptr()) }) } fn unregister(pdrv: &mut Self::RegType) { // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`. unsafe { bindings::pci_unregister_driver(pdrv) } } } ``` (cutting the rest of the mail, since everything else is covered already)
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs new file mode 100644 index 000000000000..e04406b93b56 --- /dev/null +++ b/rust/kernel/driver.rs @@ -0,0 +1,128 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.). +//! +//! Each bus / subsystem is expected to implement [`DriverOps`], which allows drivers to register +//! using the [`Registration`] class. + +use crate::error::{Error, Result}; +use crate::{init::PinInit, str::CStr, try_pin_init, types::Opaque, ThisModule}; +use core::pin::Pin; +use macros::{pin_data, pinned_drop}; + +/// The [`DriverOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, Amba, +/// etc.) to privide the corresponding subsystem specific implementation to register / unregister a +/// driver of the particular type (`RegType`). +/// +/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call +/// `bindings::__pci_register_driver` from `DriverOps::register` and +/// `bindings::pci_unregister_driver` from `DriverOps::unregister`. +pub trait DriverOps { + /// The type that holds information about the registration. This is typically a struct defined + /// by the C portion of the kernel. + type RegType: Default; + + /// Registers a driver. + /// + /// # Safety + /// + /// `reg` must point to valid, initialised, and writable memory. It may be modified by this + /// function to hold registration state. + /// + /// On success, `reg` must remain pinned and valid until the matching call to + /// [`DriverOps::unregister`]. + fn register( + reg: &mut Self::RegType, + name: &'static CStr, + module: &'static ThisModule, + ) -> Result; + + /// Unregisters a driver previously registered with [`DriverOps::register`]. + /// + /// # Safety + /// + /// `reg` must point to valid writable memory, initialised by a previous successful call to + /// [`DriverOps::register`]. + fn unregister(reg: &mut Self::RegType); +} + +/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g. +/// `bindings::pci_driver`). Therefore a [`Registration`] is initialized with some type that +/// implements the [`DriverOps`] trait, such that the generic `T::register` and `T::unregister` +/// calls result in the subsystem specific registration calls. +/// +///Once the `Registration` structure is dropped, the driver is unregistered. +#[pin_data(PinnedDrop)] +pub struct Registration<T: DriverOps> { + #[pin] + reg: Opaque<T::RegType>, +} + +// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to +// share references to it with multiple threads as nothing can be done. +unsafe impl<T: DriverOps> Sync for Registration<T> {} + +// SAFETY: Both registration and unregistration are implemented in C and safe to be performed from +// any thread, so `Registration` is `Send`. +unsafe impl<T: DriverOps> Send for Registration<T> {} + +impl<T: DriverOps> Registration<T> { + /// Creates a new instance of the registration object. + pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> { + try_pin_init!(Self { + reg <- Opaque::try_ffi_init(|ptr: *mut T::RegType| { + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write. + unsafe { ptr.write(T::RegType::default()) }; + + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write, and it has + // just been initialised above, so it's also valid for read. + let drv = unsafe { &mut *ptr }; + + T::register(drv, name, module) + }), + }) + } +} + +#[pinned_drop] +impl<T: DriverOps> PinnedDrop for Registration<T> { + fn drop(self: Pin<&mut Self>) { + let drv = unsafe { &mut *self.reg.get() }; + + T::unregister(drv); + } +} + +/// A kernel module that only registers the given driver on init. +/// +/// This is a helper struct to make it easier to define single-functionality modules, in this case, +/// modules that offer a single driver. +#[pin_data] +pub struct Module<T: DriverOps> { + #[pin] + _driver: Registration<T>, +} + +impl<T: DriverOps + Sync + Send> crate::InPlaceModule for Module<T> { + fn init(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> { + try_pin_init!(Self { + _driver <- Registration::<T>::new(name, module), + }) + } +} + +/// Declares a kernel module that exposes a single driver. +/// +/// It is meant to be used as a helper by other subsystems so they can more easily expose their own +/// macros. +#[macro_export] +macro_rules! module_driver { + (<$gen_type:ident>, $driver_ops:ty, { type: $type:ty, $($f:tt)* }) => { + type Ops<$gen_type> = $driver_ops; + type ModuleType = $crate::driver::Module<Ops<$type>>; + $crate::prelude::module! { + type: ModuleType, + $($f)* + } + } +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 5af00e072a58..5382402cd3db 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -29,6 +29,7 @@ pub mod alloc; mod build_assert; pub mod device; +pub mod driver; pub mod error; #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)] pub mod firmware;