Date: Thu, 19 Oct 2023 13:45:27 +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 02:24, FUJITA Tomonori wrote:
> On Wed, 18 Oct 2023 15:07:52 +0000
> Benno Lossin <benno.lossin@...ton.me> wrote:
> 
>>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>>> new file mode 100644
>>> index 000000000000..7d4927ece32f
>>> --- /dev/null
>>> +++ b/rust/kernel/net/phy.rs
>>> @@ -0,0 +1,701 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@...il.com>
>>> +
>>> +//! Network PHY device.
>>> +//!
>>> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
>>> +//!
>>
>> Add a new section "# Abstraction Overview" or similar.
> 
> With the rest of comments on this secsion addressed, how about the following?
> 
> //! Network PHY device.
> //!
> //! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> //!
> //! # Abstraction Overview
> //!
> //! "During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is unique

Please remove the quotes ", they were intended to separate my comments
from my suggestion.

> //! for every instance of [`Device`]". `PHYLIB` uses a different serialization technique for
> //! [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with the lock hold,

hold -> held

> //! to guarantee that [`Driver::resume`] accesses to the instance exclusively. [`Driver::resume`] and

to guarantee -> thus guaranteeing
accesses to the instance exclusively. -> has exclusive access to the instance.

> //! [`Driver::suspend`] also are called where only one thread can access to the instance.
> //!
> //! All the PHYLIB helper functions for [`Device`] modify some members in [`Device`]. Except for

PHYLIB -> `PHYLIB`

> //! getter functions, [`Device`] methods take `&mut self`. This also applied to `[Device::read]`,

`[Device::read]` -> [`Device::read`]

> //! which reads a hardware register and updates the stats.

Otherwise this looks good.

[...]

>>> +impl Device {
>>> +    /// Creates a new [`Device`] instance from a raw pointer.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// This function must only be called from the callbacks in `phy_driver`. PHYLIB guarantees
>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>
>> I would not put the second sentence in the `# Safety` section. Just move it
>> above. The reason behind this is the following: the second sentence is not
>> a precondition needed to call the function.
> 
> Where is the `above`? You meant the following?
> 
> impl Device {
>      /// Creates a new [`Device`] instance from a raw pointer.
>      ///
>      /// `PHYLIB` guarantees the exclusive access for the duration of the lifetime `'a`.
>      ///
>      /// # Safety
>      ///
>      /// This function must only be called from the callbacks in `phy_driver`.
>      unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a  mut Self {

Yes this is what I had in mind. Although now that I see it in code,
I am not so sure that this comment is needed. If you feel the same
way, just remove it.

That being said, I am not too happy with the safety requirement of this
function. It does not really match with the safety comment in the function
body. Since I have not yet finished my safety standardization, I think we
can defer that problem until it is finished. Unless some other reviewer
wants to change this, you can keep it as is.

>>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>>> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
>>> +        let ptr = ptr.cast::<Self>();
>>> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
>>> +        // the duration of `'a`.
>>> +        unsafe { &mut *ptr }
>>> +    }
>>> +
>>> +    /// Gets the id of the PHY.
>>> +    pub fn phy_id(&self) -> u32 {
>>> +        let phydev = self.0.get();
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        unsafe { (*phydev).phy_id }
>>> +    }
>>> +
>>> +    /// Gets the state of the PHY.
>>> +    pub fn state(&self) -> DeviceState {
>>> +        let phydev = self.0.get();
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        let state = unsafe { (*phydev).state };
>>> +        // TODO: this conversion code will be replaced with automatically generated code by bindgen
>>> +        // when it becomes possible.
>>> +        // better to call WARN_ONCE() when the state is out-of-range (needs to add WARN_ONCE support).
>>> +        match state {
>>> +            bindings::phy_state_PHY_DOWN => DeviceState::Down,
>>> +            bindings::phy_state_PHY_READY => DeviceState::Ready,
>>> +            bindings::phy_state_PHY_HALTED => DeviceState::Halted,
>>> +            bindings::phy_state_PHY_ERROR => DeviceState::Error,
>>> +            bindings::phy_state_PHY_UP => DeviceState::Up,
>>> +            bindings::phy_state_PHY_RUNNING => DeviceState::Running,
>>> +            bindings::phy_state_PHY_NOLINK => DeviceState::NoLink,
>>> +            bindings::phy_state_PHY_CABLETEST => DeviceState::CableTest,
>>> +            _ => DeviceState::Error,
>>> +        }
>>> +    }
>>> +
>>> +    /// Returns true if the link is up.
>>> +    pub fn get_link(&self) -> bool {
>>
>> I still think this name should be changed. My response at [1] has not yet
>> been replied to. This has already been discussed before:
>> - https://lore.kernel.org/rust-for-linux/2023100237-satirical-prance-bd57@gregkh/
>> - https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/
>> - https://lore.kernel.org/rust-for-linux/CALNs47syMxiZBUwKLk3vKxzmCbX0FS5A37FjwUzZO9Fn-iPaoA@mail.gmail.com/
>>
>> And I want to suggest to change it to `is_link_up`.
>>
>> Reasons why I do not like the name:
>> - `get_`/`put_` are used for ref counting on the C side, I would like to
>>     avoid confusion.
>> - `get` in Rust is often used to fetch a value from e.g. a datastructure
>>     such as a hashmap, so I expect the call to do some computation.
>> - getters in Rust usually are not prefixed with `get_`, but rather are
>>     just the name of the field.
>> - in this case I like the name `is_link_up` much better, since code becomes
>>     a lot more readable with that.
>> - I do not want this pattern as an example for other drivers.
>>
>> [1]: https://lore.kernel.org/rust-for-linux/f5878806-5ba2-d932-858d-dda3f55ceb67@proton.me/
> 
> IIRC, Andrew suggested the current name. If the change is fine by him,
> I'll rename.
> 
> 
>>> +/// An instance of a PHY driver.
>>> +///
>>> +/// Wraps the kernel's `struct phy_driver`.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// `self.0` is always in a valid state.
>>> +#[repr(transparent)]
>>> +pub struct DriverType(Opaque<bindings::phy_driver>);
>>
>> I think `DriverVTable` is a better name.
> 
> Renamed.
> 
>>> +/// Creates the kernel's `phy_driver` instance.
>>
>> This function returns a `DriverType`, not a `phy_driver`.
> 
> How about?
> 
> /// Creates the [`DriverVTable`] instance from [`Driver`] trait.

Sounds good, but to this sounds a bit more natural:

     /// Creates a [`DriverVTable`] instance from a [`Driver`].

>>> +///
>>> +/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`.
>>> +///
>>> +/// [`module_phy_driver`]: crate::module_phy_driver
>>> +pub const fn create_phy_driver<T: Driver>() -> DriverType {
>>> +    // All the fields of `struct phy_driver` are initialized properly.
>>> +    // It ensures the invariants.
>>
>> Use `// INVARIANT: `.
> 
> Oops,
> 
> // INVARIANT: All the fields of `struct phy_driver` are initialized properly.
> DriverVTable(Opaque::new(bindings::phy_driver {
>      name: T::NAME.as_char_ptr().cast_mut(),

Seems good.

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

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

-- 
Cheers,
Benno

Powered by blists - more mailing lists