Date: Tue, 4 Jun 2024 18:00:11 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...hat.com>
Cc: rafael@...nel.org, bhelgaas@...gle.com, ojeda@...nel.org,
	alex.gaynor@...il.com, wedsonaf@...il.com, boqun.feng@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...sung.com, aliceryhl@...gle.com, airlied@...il.com,
	fujita.tomonori@...il.com, lina@...hilina.net, pstanner@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: [RFC PATCH 02/11] rust: add driver abstraction

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@...il.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@...hilina.net>
> > > > > > > Signed-off-by: Asahi Lina <lina@...hilina.net>
> > > > > > > Co-developed-by: Andreas Hindborg <a.hindborg@...sung.com>
> > > > > > > Signed-off-by: Andreas Hindborg <a.hindborg@...sung.com>
> > > > > > > Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
> > > > > > > Signed-off-by: Danilo Krummrich <dakr@...hat.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

Powered by blists - more mailing lists