Date: Fri, 20 Oct 2023 00:32:19 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: benno.lossin@...ton.me
Cc: fujita.tomonori@...il.com, 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 Thu, 19 Oct 2023 15:20:51 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

> 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) })?;
> 
>>                  })?;

The above solves DriverVTable.0 but still the macro can't access to
kernel::ThisModule.0. I got the following error:

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
	   |
	      = note: this error originates in the macro
	      `kernel::module_phy_driver` (in Nightly builds, run with
	      -Z macro-backtrace for more info)


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

It's worse than having public unsafe function (phy_drivers_unregister)?

Powered by blists - more mailing lists