Received-SPF: pass (google.com: domain of linux-kernel+bounces-201330-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 4 Jun 2024 22:06:33 +0200
From: Danilo Krummrich <dakr@redhat.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: rafael@kernel.org, bhelgaas@google.com, ojeda@kernel.org,
	alex.gaynor@gmail.com, wedsonaf@gmail.com, boqun.feng@gmail.com,
	gary@garyguo.net, bjorn3_gh@protonmail.com, benno.lossin@proton.me,
	a.hindborg@samsung.com, aliceryhl@google.com, airlied@gmail.com,
	fujita.tomonori@gmail.com, lina@asahilina.net, pstanner@redhat.com,
	ajanulgu@redhat.com, lyude@redhat.com,
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-pci@vger.kernel.org
Subject: Re: [RFC PATCH 02/11] rust: add driver abstraction
Message-ID: <Zl9zybWIeZqt8tWc@pollux>
References: <20240520172554.182094-1-dakr@redhat.com>
 <20240520172554.182094-3-dakr@redhat.com>
 <2024052045-lived-retiree-d8b9@gregkh>
 <ZkvPDbAQLo2/7acY@pollux.localdomain>
 <2024052155-pulverize-feeble-49bb@gregkh>
 <Zk0egew_AxvNpUG-@pollux>
 <2024060432-chloride-grappling-cf95@gregkh>
 <Zl81oUmNO5TX063x@cassiopeiae>
 <2024060404-figment-resolute-7c6c@gregkh>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2024060404-figment-resolute-7c6c@gregkh>

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
>