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


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