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

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

On 21.10.23 14:38, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 12:13:32 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>>>>>> Can you please share your setup and the error? For me it booted
>>>>>> fine.
>>>>>
>>>>> You use ASIX PHY hardware?
>>>>
>>>> It seems I have configured something wrong. Can you share your testing
>>>> setup? Do you use a virtual PHY device in qemu, or do you boot it from
>>>> real hardware with a real ASIX PHY device?
>>>
>>> real hardware with real ASIX PHY device.
>>
>> I see.
>>
>>> Qemu supports a virtual PHY device?
>>
>> I have no idea.
> 
> When I had a look at Qemu several months ago, it didn't support such.
> 
>> [...]
>>
>>>> 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.

It seems that constructing the driver table not at that location
is somehow interfering with something?

Wedson has a patch [1] to create in-place initialized modules, but
it probably is not completely finished, as he has not yet begun to
post it to the list. But I am sure that it is mature enough for
you to test this hypothesis.

[1]: https://github.com/wedsonaf/linux/commit/484ec70025ff9887d9ca228ec631264039cee355

-- 
Cheers,
Benno

>>>> If you don't mind, could you try if the following changes
>>>> anything?
>>>
>>> I don't think it works. If you use const for DriverTable, DriverTable
>>> is placed on read-only pages. The C side modifies DriverVTable array
>>> so it does't work.
>>
>> Did you try it? Note that I copy the `DriverVTable` into the Module
>> struct, so it will not be placed on a read-only page.
> 
> Ah, I misunderstood code. It doesn't work. DriverVTable on stack.
> 
> 
>>>>        (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>>>>            const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
>>>>            struct Module {
>>>>                _drivers: [::kernel::net::phy::DriverVTable; N],
>>>>            }
>>>>
>>>>            $crate::prelude::module! {
>>>>                type: Module,
>>>>                $($f)*
>>>>            }
>>>>
>>>>            unsafe impl Sync for Module {}
>>>>
>>>>            impl ::kernel::Module for Module {
>>>>                fn init(module: &'static ThisModule) -> Result<Self> {
>>>> 		const DRIVERS: [::kernel::net::phy::DriverVTable; N] = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+];
>>>>                    let mut m = Module {
>>>>                        _drivers: unsafe { core::ptr::read(&DRIVERS) },
>>>>                    };
>>>>                    let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
>>>>                    ::kernel::error::to_result(unsafe {
>>>>                        kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
>>>>                    })?;
>>>>                    Ok(m)
>>>>                }
>>>>            }
>>>>
>>>> and also the variation where you replace `const DRIVERS` with
>>>> `static DRIVERS`.
>>>
>>> Probably works. But looks like similar with the current code? This is
>>> simpler?
>>
>> Just curious if it has to do with using `static` vs `const`.
> 
> static doesn't work too due to the same reason.