Message-ID: <2024062010-change-clubhouse-b16c@gregkh>
Date: Thu, 20 Jun 2024 18:36:08 +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, robh@...nel.org,
	daniel.almeida@...labora.com, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

On Thu, Jun 20, 2024 at 06:10:05PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 20, 2024 at 04:19:48PM +0200, Greg KH wrote:
> > On Wed, Jun 19, 2024 at 01:39:47AM +0200, Danilo Krummrich wrote:
> > > In a subsequent patch we introduce the `Registration` abstraction used
> > > to register driver structures. Some subsystems require the module name on
> > > driver registration (e.g. PCI in __pci_register_driver()), hence pass
> > > the module name to `Module::init`.
> > 
> > I understand the need/want here, but it feels odd that you have to
> > change anything to do it.
> > 
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@...hat.com>
> > > ---
> > >  rust/kernel/lib.rs           | 14 ++++++++++----
> > >  rust/kernel/net/phy.rs       |  2 +-
> > >  rust/macros/module.rs        |  3 ++-
> > >  samples/rust/rust_minimal.rs |  2 +-
> > >  samples/rust/rust_print.rs   |  2 +-
> > >  5 files changed, 15 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > index a791702b4fee..5af00e072a58 100644
> > > --- a/rust/kernel/lib.rs
> > > +++ b/rust/kernel/lib.rs
> > > @@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
> > >      /// 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>;
> > 
> > Why can't the name come directly from the build system?  Why must it be
> > passed into the init function of the module "class"?  What is it going
> > to do with it?
> 
> The name does come from the build system, that's where `Module::init` gets it
> from.
> 
> > 
> > A PCI, or other bus, driver "knows" it's name already by virtue of the
> > build system, so it can pass that string into whatever function needs
> 
> Let's take pci_register_driver() as example.
> 
> ```
> #define pci_register_driver(driver)		\
> 	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
> ```
> 
> In C drivers this works because (1) it's a macro and (2) it's called directly
> from the driver code.
> 
> In Rust, for very good reasons, we have abstractions for C APIs, hence the
> actual call to __pci_register_driver() does not come from code within the
> module, but from the PCI Rust abstraction `Module::init` calls into instead.

I don't understand those reasons, sorry.

> Consequently, we have to pass things through. This also isn't new, please note
> that the current code already does the same thing: `Module::init` (without this
> patch) is already declared as
> 
> `fn init(module: &'static ThisModule) -> error::Result<Self>`
> passing through `ThisModule` for the exact same reason.

Yeah, and I never quite understood that either :)

> Please also note that in the most common case (one driver per module) we don't
> see any of this anyway.

True, but someone has to review and most importantly, maintain, this
glue code.

> Just like the C macro module_pci_driver(), Rust drivers can use the
> `module_pci_driver!` macro.
> 
> Example from Nova:
> 
> ```
>     kernel::module_pci_driver! {
>         // The driver type that implements the corresponding probe() and
>         // remove() driver callbacks.
>         type: NovaDriver,
>         name: "Nova",
>         author: "Danilo Krummrich",
>         description: "Nova GPU driver",
>         license: "GPL v2",
>     }
> ```

As I said when you implemented this fun macro, don't do this yet.

We added these "helper" macros WAY late in the development cycle of the
driver model, AFTER we were sure we got other parts right.  There is no
need to attempt to implement all of what you can do in C today in Rust,
in fact, I would argue that we don't want to do that, just to make
things simpler to review the glue code, which is the most important part
here to get right!

Changing drivers later, to take advantage of code savings like this
macro can be done then, not just yet.  Let's get this understood and
right first, no need to be tricky or complex.

And, as I hinted at before, I don't think we should be doing this at all
just yet either.  I still think a small "C shim" layer to wrap the
struct pci driver up and pass the calls to the rust portion of the
module is better to start with, it both saves you time and energy so
that you can work on what you really want to do (i.e. a driver in rust)
and not have to worry about the c bindings as that's the "tricky" part
that is stopping you from getting your driver work done.

After all, it's not the pci driver model code that is usually the tricky
bits to verify in C, it's the whole rest of the mess.  Stick with a
small C file, with just the pci driver structure and device ids, and
then instantiate your rust stuff when probe() is called, and clean up
when release() is called.

I can provide an example if needed.

thanks,

greg k-h

Powered by blists - more mailing lists