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

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

On 21.10.23 13:36, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 11:21:12 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> On 21.10.23 12:27, FUJITA Tomonori wrote:
>>> On Sat, 21 Oct 2023 08:37:08 +0000
>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>
>>>> On 21.10.23 09:30, FUJITA Tomonori wrote:
>>>>> 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.
>>>>
>>>> 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.

[...]

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

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

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

-- 
Cheers,
Benno