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

[Benno Lossin <benno.lossin@proton.me>]

On 21.10.23 23:45, FUJITA Tomonori wrote:
> 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.

This SAFETY comment needs to mention that `drivers[0].0.get()` are
pinned and will not change address.

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

Otherwise this looks good.

> 
> 
>>> 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.

Sure. If you have the time though, it would be helpful to know
if the patch actually fixes the issue. I am pretty sure it will,
but you never know unless you try.

-- 
Cheers,
Benno