Message-ID: <Zo8-K7vKECn0X9cR@pollux>
Date: Thu, 11 Jul 2024 04:06:35 +0200
From: Danilo Krummrich <dakr@...hat.com>
To: Greg KH <gregkh@...uxfoundation.org>
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 02/10] rust: implement generic driver registration

(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)

Powered by blists - more mailing lists