Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

On Sat, 21 Oct 2023 13:35:57 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>> Currently, it needs &'static DriverVTable
>> array so it works.
> 
> That is actually also incorrect. As the C side is going to modify
> the `DriverVTable`, you should actually use `&'static mut DriverVTable`.
> But since it is not allowed to be moved you have to use
> `Pin<&'static mut DriverVTable>`.

I updated Registration::register(). Needs to add comments on requirement?

impl Registration {
    /// Registers a PHY driver.
    pub fn register(
        module: &'static crate::ThisModule,
        drivers: Pin<&'static mut [DriverVTable]>,
    ) -> Result<Self> {
        // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
        // are initialized properly. So an FFI call with a valid pointer.
        to_result(unsafe {
            bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
        })?;
        // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
        Ok(Registration { drivers })
    }
}


>> The C side uses static allocation too. If someone asks for, we could
>> loosen the restriction with a complicated implentation. But I doubt
>> that someone would ask for such.
> 
> With Wedson's patch you also would be using the static allocation
> from `module!`. What my problem is, is that you are using a `static mut`
> which is `unsafe` and you do not actually have to use it (with
> Wedson's patch of course).

Like your vtable patch, I improve the code when something useful is
available.