Date: Thu, 19 Oct 2023 09:24:36 +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 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
//! 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,
//! to guarantee that [`Driver::resume`] accesses to the instance exclusively. [`Driver::resume`] and
//! [`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
//! getter functions, [`Device`] methods take `&mut self`. This also applied to `[Device::read]`,
//! which reads a hardware register and updates the stats.


>> +//! The C side (PHYLIB) guarantees that there is only one thread accessing to one `phy_device` instance
>> +//! during a callback in `phy_driver`. The callback safely accesses to the instance exclusively.
>> +//! Except for `resume()` and `suspend()`, PHYLIB holds `phy_device`'s lock during a callback.
> 
> I would start with "During the calls to most functions in [`Driver`],
> the C side (`PHYLIB`) holds a lock that is unique for every instance of
> [`Device`]". Then you can note the exception for `resume` and `suspend`
> (and also link them e.g. [`Driver::resume`]).
> 
>> +//! PHYLIB doesn't hold `phy_device`'s lock in both `resume()` and `suspend()`. Instead, PHYLIB
>> +//! updates `phy_device`'s state with `phy_device`'s lock hold, to guarantee that resume() accesses
>> +//! to the instance exclusively. Note that `resume()` and `suspend()` also are called where only
>> +//! one thread can access to the instance.
> 
> I would just write "`PHYLIB` uses a different serialization technique for
> [`Driver::resume`] and [`Driver::suspend`]: <use the above explanation>".
> 
>> +//! This abstractions creates [`Device`] instance only during a callback so it's guaranteed that
>> +//! only one reference exists. All its methods can accesses to the instance exclusively.
> 
> Not really sure if this is needed, what are you trying to explain here?

I tried to add an explanation that Device::from_raw() return &mut. But
I guess that the description of Device is enough.


>> +//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for
>> +//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads
>> +//! a hardware register and updates the stats.
> 
> I would use [`Device`] instead of `phy_device`, since the Rust reader
> might not be aware what wraps `phy_device`.
> 
>> +use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
>> +use core::marker::PhantomData;

(snip)


>> +/// An instance of a PHY device.
>> +///
>> +/// Wraps the kernel's `struct phy_device`.
>> +///
>> +/// # Invariants
>> +///
>> +/// `self.0` is always in a valid state.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
>> +
>> +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 {


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

>> +///
>> +/// 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(),


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

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.

Also Module trait support exit()?

Powered by blists - more mailing lists