Message-ID: <e361ef91-607d-400b-a721-f846c21e2400@proton.me>
Date: Wed, 18 Oct 2023 15:07:52 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org
Cc: 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 17.10.23 13:30, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
> 
> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
> 
> This patch enables unstable const_maybe_uninit_zeroed feature for
> kernel crate to enable unsafe code to handle a constant value with
> uninitialized data. With the feature, the abstractions can initialize
> a phy_driver structure with zero easily; instead of initializing all
> the members by hand. It's supposed to be stable in the not so distant
> future.
> 
> Link: https://github.com/rust-lang/rust/pull/116218
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> ---
>   drivers/net/phy/Kconfig         |   8 +
>   rust/bindings/bindings_helper.h |   3 +
>   rust/kernel/lib.rs              |   3 +
>   rust/kernel/net.rs              |   6 +
>   rust/kernel/net/phy.rs          | 701 ++++++++++++++++++++++++++++++++
>   5 files changed, 721 insertions(+)
>   create mode 100644 rust/kernel/net.rs
>   create mode 100644 rust/kernel/net/phy.rs
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 421d2b62918f..0faebdb184ca 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -60,6 +60,14 @@ config FIXED_PHY
> 
>   	  Currently tested with mpc866ads and mpc8349e-mitx.
> 
> +config RUST_PHYLIB_ABSTRACTIONS
> +        bool "PHYLIB abstractions support"
> +        depends on RUST
> +        depends on PHYLIB=y
> +        help
> +          Adds support needed for PHY drivers written in Rust. It provides
> +          a wrapper around the C phylib core.
> +
>   config SFP
>   	tristate "SFP cage support"
>   	depends on I2C && PHYLINK
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c91a3c24f607..ec4ee09a34ad 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,9 @@
> 
>   #include <kunit/test.h>
>   #include <linux/errname.h>
> +#include <linux/ethtool.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
>   #include <linux/slab.h>
>   #include <linux/refcount.h>
>   #include <linux/wait.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e8811700239a..0588422e273c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -14,6 +14,7 @@
>   #![no_std]
>   #![feature(allocator_api)]
>   #![feature(coerce_unsized)]
> +#![feature(const_maybe_uninit_zeroed)]
>   #![feature(dispatch_from_dyn)]
>   #![feature(new_uninit)]
>   #![feature(receiver_trait)]
> @@ -36,6 +37,8 @@
>   pub mod ioctl;
>   #[cfg(CONFIG_KUNIT)]
>   pub mod kunit;
> +#[cfg(CONFIG_NET)]
> +pub mod net;
>   pub mod prelude;
>   pub mod print;
>   mod static_assert;
> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
> new file mode 100644
> index 000000000000..fe415cb369d3
> --- /dev/null
> +++ b/rust/kernel/net.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Networking.
> +
> +#[cfg(CONFIG_RUST_PHYLIB_ABSTRACTIONS)]
> +pub mod phy;
> 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.

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

> +//! 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;
> +
> +/// Corresponds to the kernel's `enum phy_state`.
> +#[derive(PartialEq)]
> +pub enum DeviceState {
> +    /// PHY device and driver are not ready for anything.
> +    Down,
> +    /// PHY is ready to send and receive packets.
> +    Ready,
> +    /// PHY is up, but no polling or interrupts are done.
> +    Halted,
> +    /// PHY is up, but is in an error state.
> +    Error,
> +    /// PHY and attached device are ready to do work.
> +    Up,
> +    /// PHY is currently running.
> +    Running,
> +    /// PHY is up, but not currently plugged in.
> +    NoLink,
> +    /// PHY is performing a cable test.
> +    CableTest,
> +}
> +
> +/// Represents duplex mode.
> +pub enum DuplexMode {
> +    /// PHY is in full-duplex mode.
> +    Full,
> +    /// PHY is in half-duplex mode.
> +    Half,
> +    /// PHY is in unknown duplex mode.
> +    Unknown,
> +}
> +
> +/// 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.

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

> +        const LINK_IS_UP: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.link() == LINK_IS_UP
> +    }

[...]

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

> +/// Creates the kernel's `phy_driver` instance.

This function returns a `DriverType`, not a `phy_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: `.

> +    DriverType(Opaque::new(bindings::phy_driver {

[...]

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

-- 
Cheers,
Benno

> +
> +impl Registration {
> +    /// Registers a PHY driver.
> +    pub fn register(
> +        module: &'static crate::ThisModule,
> +        drivers: &'static [DriverType],
> +    ) -> Result<Self> {
> +        if drivers.is_empty() {
> +            return Err(code::EINVAL);
> +        }
> +        // SAFETY: The type invariants of [`DriverType`] 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(drivers[0].0.get(), drivers.len().try_into()?, module.0)
> +        })?;
> +        // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
> +        Ok(Registration { drivers })
> +    }
> +}

[...]

Powered by blists - more mailing lists