Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich <dakr@redhat.com>]

Hi Greg,

On 6/20/24 23:24, Danilo Krummrich wrote:

This is a polite reminder about the below discussion.

> On Thu, Jun 20, 2024 at 06:36:08PM +0200, Greg KH wrote:
>> 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@redhat.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.
> 
> Ok, good you point this out. We should definitely discuss this point then and
> build some consensus around it.
> 
> I propose to focus on this point first and follow up with the discussion of the
> rest of the series afterwards.
> 
> Let me explain why I am convinced that it's very important to have abstractions
> in place in general and from the get-go.
> 
> In general, having abstractions for C APIs is the foundation of being able to
> make use of a lot of advantages Rust has to offer.
> 
> The most obvious one are all the safety aspects. For instance, with an
> abstraction we have to get exactly one piece of code right in terms of pointer
> validity, lifetimes, type safety, API semantics, etc. and in all other places
> (e.g. drivers) we get the compiler to check those things for us through the
> abstraction.
> 
> Now, the abstraction can be buggy or insufficient and hence there is no absolute
> safety guarantee. *But*, if we get this one thing right, there is nothing a
> driver can mess up by itself trying to do stupid things anymore.
> 
> If we just call the C code instead we have to get it right everywhere instead.
> 
> Now, you could approach this top-down instead and argue that we could at least
> benefit from Rust for the driver specific parts.
> 
> Unfortunately, this doesn't really work out either. Also driver specific code is
> typically (very) closely connected to kernel APIs. If you want to use the safety
> aspects of Rust for the driver specific parts you inevitably end up writing
> abstractions for the C APIs in your driver.
> 
> There are basically three options you can end up with:
> 
> (1) An abstraction for the C API within your driver that is actually generic
>      for every driver, and hence shouldn't be there.
> (2) Your driver specific code is full of raw pointers and `unsafe {}` calls,
>      which in the end just means that you ended up baking the abstraction into
>      your driver specific code.
> (3) You ignore everything, put everything in a huge `unsafe {}` block and
>      compile C code with the Rust compiler. (Admittedly, maybe slightly
>      overstated, but not that far off either.)
> 
> The latter is also the reason why it doesn't make sense to only have
> abstractions for some things, but not for other.
> 
> If an abstraction for B is based on A, but we don't start with A, then B ends up
> implementing (at least partially) the abstraction for A as well. For instance,
> if we don't implement `driver::Registration` then the PCI abstractions (and
> platform, usb, etc.) have to implement it.
> 
> It really comes down to the point that it just bubbles up. We really have to do
> this bottom-up, otherwise we just end up moving those abstractions up, layer by
> layer, where they don't belong to and we re-implement them over and over again.
> 
>>
>>> 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 :)
> 
> Since commit 247b365dc8dc ("rust: add `kernel` crate") shows me your RB for
> that, am I good to assume that this one isn't a blocker?
> 
>>
>>> 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!
> 
> We're not reinventing the wheel here, we stick to the architecture the kernel
> already has.
> 
> However, I understand that not starting with this macro directly makes it easier
> for you to see what's going on. I can introduce the macro in a separate patch to
> make it more obvious what's going on.
> 
>>
>> 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.
> 
> I strongly disagree here. As explained above, it just bubbles up, this approach
> would just make me end up having to write a lot of the code from the
> abstractions in the driver.
> 
> However, it would indeed safe me time and energy to do just that. But that isn't
> really what I want. I also don't want to write a driver in Rust *only*.
> 
> And I also don't really think that you want people, who commit to work hard to
> get things right, to just take the shortcut and create some random mess buried
> in a driver. :)
> 
> What I actually want is to get to a solid foundation for Rust drivers in
> general, since I'm convinced that this provides a lot of value beyond the scope
> of a single driver.
> 
> Since you've brought the topic up a few times, I am also willing to maintain
> those abstractions if this is a concern. Maybe one day the corresponding
> maintainers are comfortable enough and this isn't needed anymore, but at least
> until then, I'm happy to help out.
> 
>>
>> 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.
> 
> Again, this really bubbles up.
> 
> What do we pass to Rust probe() function? A raw struct pci_dev pointer or the
> proper PCI device abstraction?
> 
> How do we implement I/O memory mappings for PCI bars without PCI / I/O
> abstraction?
> 
> How do we perform (boundary checked) I/O memory reads / writes without `Io`
> abstraction?
> 
> How do we handle the lifetime of resources without `Devres` abstraction?
> 
> How do we (properly) implement a DRM device registration abstraction without
> `Devres`?
> 
> Luckily we already got the `Firmware` and `Device` abstraction in place. :)
> 
>>
>> I can provide an example if needed.
> 
> If you do so, please don't stop at the probe() boundary, please continue to the
> point where the Nova stub driver is. It really does just enough to show / proove
> that the abstractions tie together nicely. But it should be enough to see that
> you end up with either (1), (2) or (3) from above.
> 
>>
>> thanks,
>>
>> greg k-h
>>