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 07:25:17 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 20.10.23 14:54, FUJITA Tomonori wrote:
>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>> 
>>> On Thu, 19 Oct 2023 15:20:51 +0000
>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>
>>>> I would like to remove the mutable static variable and simplify
>>>> the macro.
>>>
>>> How about adding DriverVTable array to Registration?
>>>
>>> /// Registration structure for a PHY driver.
>>> ///
>>> /// # Invariants
>>> ///
>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>> pub struct Registration<const N: usize> {
>>>      drivers: [DriverVTable; N],
>>> }
>>>
>>> impl<const N: usize> Registration<{ N }> {
>>>      /// Registers a PHY driver.
>>>      pub fn register(
>>>          module: &'static crate::ThisModule,
>>>          drivers: [DriverVTable; N],
>>>      ) -> Result<Self> {
>>>          let mut reg = Registration { drivers };
>>>          let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>>          // 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(ptr, reg.drivers.len().try_into()?, module.0)
>>>          })?;
>>>          // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>>          Ok(reg)
>>>      }
>>> }
>> 
>> Scratch this.
>> 
>> This doesn't work. Also simply putting slice of DriverVTable into
>> Module strcut doesn't work.
> 
> Why does it not work? I tried it and it compiled fine for me.

You can compile but the kernel crashes. The addresses of the callback
functions are invalid.


>> We cannot move a slice of DriverVTable. Except for static allocation,
>> is there a simple way?
> 
> I do not know what you are referring to, you can certainly move an array
> of `DriverVTable`s.
> 
> -- 
> Cheers,
> Benno
> 
> 
>