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

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

On 21.10.23 15:31, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 13:05:59 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> On 21.10.23 15:00, FUJITA Tomonori wrote:
>>> On Sat, 21 Oct 2023 12:50:10 +0000
>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>>>>>> I think this is very weird, do you have any idea why this
>>>>>>>> could happen?
>>>>>>>
>>>>>>> DriverVtable is created on kernel stack, I guess.
>>>>>>
>>>>>> But how does that invalidate the function pointers?
>>>>>
>>>>> Not only funciton pointers. You can't store something on stack for
>>>>> later use.
>>>>
>>>> It is not stored on the stack, it is only created on the stack and
>>>> moved to a global static later on. The `module!` macro creates a
>>>> `static mut __MOD: Option<Module>` where the module data is stored in.
>>>
>>> I know. The problem is that we call phy_drivers_register() with
>>> DriverVTable on stack. Then it was moved.
>>
>> I see, what exactly is the problem with that? In other words:
>> why does PHYLIB need `phy_driver` to stay at the same address?
> 
> phy_driver_register stores addresses that you passed.

But the function pointers don't change?

>> This is an important requirement in Rust. Rust can ensure that
>> types are not moved by means of pinning them. In this case, Wedson's
>> patch below should fix the issue completely.
>>
>> But we should also fix this in the abstractions, the `DriverVTable`
>> type should only be constructible in a pinned state. For this purpose
>> we have the `pin-init` API [2].
> 
> You can create DriverVTable freely. The restriction is what
> phy_driver_register takes. 

Sure you can also keep it this way, I just thought only allowing
pinned creation makes things simpler.

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

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

-- 
Cheers,
Benno