Message-ID: <64db34c0-a50a-4321-a3d8-b692e26899d9@proton.me>
Date: Thu, 19 Oct 2023 15:20:51 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

On 19.10.23 16:42, FUJITA Tomonori wrote:
>>>>> +/// Registration structure for a PHY driver.
>>>>> +///
>>>>> +/// # Invariants
>>>>> +///
>>>>> +/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>>> +pub struct Registration {
>>>>> +    drivers: &'static [DriverType],
>>>>> +}
>>>>
>>>> You did not reply to my suggestion [2] to remove this type,
>>>> what do you think?
>>>>
>>>> [2]: https://lore.kernel.org/rust-for-linux/85d5c498-efbc-4c1a-8d12-f1eca63c45cf@proton.me/
>>>
>>> I tried before but I'm not sure it simplifies the implementation.
>>>
>>> Firstly, instead of Reservation, we need a public function like
>>>
>>> pub fn phy_drivers_register(module: &'static crate::ThisModule, drivers: &[DriverVTable]) -> Result {
>>>       to_result(unsafe {
>>>           bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
>>>       })
>>> }
>>>
>>> This is because module.0 is private.
>>
>> Why can't this be part of the macro?
> 
> I'm not sure I correctly understand what you suggest so you meant the following?
> 
>      (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>          struct Module {
>               _drv:  [
>                  ::kernel::net::phy::DriverVTable;
>                  $crate::module_phy_driver!(@count_devices $($driver),+)
>              ],
>          }
>          unsafe impl Sync for Module {}
> 
>          $crate::prelude::module! {
>              type: Module,
>              $($f)*
>          }
> 
>          impl ::kernel::Module for Module {
>              fn init(module: &'static ThisModule) -> Result<Self> {
>                  let drv = [
>                      $(::kernel::net::phy::create_phy_driver::<$driver>()),+
>                  ];
>                  ::kernel::error::to_result(unsafe {
>                      ::kernel::bindings::phy_drivers_register(drv[0].0.get(), drv.len().try_into()?, module.0)

You can just do this (I omitted the `::kernel::` prefix for
readability, if you add this in the macro, please include it):

     // CAST: `DriverVTable` is `repr(transparent)` and wrapping `bindings::phy_driver`.
     let ptr = drv.as_mut_ptr().cast::<bindings::phy_driver>();
     let len = drv.len().try_into()?;
     // SAFETY: ...
     to_result(unsafe { bindings::phy_drivers_register(ptr, len, module.0) })?;

>                  })?;
> 
>                  Ok(Module {
>                      _drv: drv,
>                  })
>              }
>          }
> 
> Then we got the following error:
> 
> error[E0616]: field `0` of struct `DriverVTable` is private
>    --> drivers/net/phy/ax88796b_rust.rs:12:1
>       |
>       12 | / kernel::module_phy_driver! {
>       13 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>       14 | |     device_table: [
>       15 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
>       ...  |
>       22 | |     license: "GPL",
>       23 | | }
>          | |_^ private field
> 	   |
> 	      = note: this error originates in the macro
> 	      `kernel::module_phy_driver` (in Nightly builds, run with
> 	      -Z macro-backtrace for more info)
> 
> error[E0616]: field `0` of struct `kernel::ThisModule` is private
>    --> drivers/net/phy/ax88796b_rust.rs:12:1
>       |
>       12 | / kernel::module_phy_driver! {
>       13 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>       14 | |     device_table: [
>       15 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
>       ...  |
>       22 | |     license: "GPL",
>       23 | | }
>          | |_^ private field
> 
> 
>>> Also if we keep DriverVtable.0 private, we need another public function.
>>>
>>> pub unsafe fn phy_drivers_unregister(drivers: &'static [DriverVTable])
>>> {
>>>       unsafe {
>>>           bindings::phy_drivers_unregister(drivers[0].0.get(), drivers.len() as i32)
>>>       };
>>> }
>>>
>>> DriverVTable isn't guaranteed to be registered to the kernel so needs
>>> to be unsafe, I guesss.
>>
>> In one of the options I suggest to make that an invariant of `DriverVTable`.
>>
>>>
>>> Also Module trait support exit()?
>>
>> Yes, just implement `Drop` and do the cleanup there.
>>
>> In the two options that I suggested there is a trade off. I do not know
>> which option is better, I hoped that you or Andrew would know more:
>> Option 1:
>> * advantages:
>>     - manual creation of a phy driver module becomes possible.
>>     - less complex `module_phy_driver` macro.
>>     - no static variable needed.
>> * disadvantages:
>>     - calls `phy_drivers_register` for every driver on module
>>       initialization.
>>     - calls `phy_drivers_unregister` for every driver on module
>>       exit.
>>
>> Option 2:
>> * advantages:
>>     - less complex `module_phy_driver` macro.
>>     - no static variable needed.
>>     - only a single call to
>>       `phy_drivers_register`/`phy_drivers_unregister`.
>> * disadvantages:
>>     - no safe manual creation of phy drivers possible, the only safe
>>       way is to use the `module_phy_driver` macro.
>>
>> I suppose that it would be ok to call the register function multiple
>> times, since it only is on module startup/shutdown and it is not
>> performance critical.
> 
> I think that we can use the current implantation using Reservation
> struct until someone requests manual creation. I doubt that we will
> need to support such.

I would like to remove the mutable static variable and simplify
the macro.

-- 
Cheers,
Benno

Powered by blists - more mailing lists